Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1308403002:
[Tracing] Disable registration of regular dump providers during tests (Closed)
|
Created:
5 years, 4 months ago by Ruud van Asseldonk Modified:
5 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, ssid, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@reland-content-browsertest Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Tracing] Disable registration of regular dump providers during tests
This adds a testing-only property to |MemoryDumpManager| which disables
registration of memory dump providers. This is to ensure that in tests,
the memory dump providers that are registered are only the providers
which were registered explicitly for the test.
The motivation for doing this is https://crbug.com/522009, flakiness of
some of the tests that was caused by an unrelated memory dump provider
unregistering during a dump. The flakiness has been resolved in 535f087
by allowing unregistering while dumping, but to avoid such problems in
the future, this CL allows ignoring all registrations except the ones
intended for testing.
BUG=522009, 522165
Committed: https://crrev.com/16ca6b63afbb3f1c6f95c1fc3f592459b7060292
Cr-Commit-Position: refs/heads/master@{#355071}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address review issues #
Total comments: 13
Patch Set 3 : Rebase on top of master #Patch Set 4 : Rename variable #
Total comments: 4
Patch Set 5 : Address primiano comments #Patch Set 6 : Fix RegisterDumperWhileDumping test #
Total comments: 10
Patch Set 7 : Address primiano nits #
Total comments: 2
Patch Set 8 : Fix indentation #
Messages
Total messages: 29 (8 generated)
ruuda@google.com changed reviewers: + primiano@chromium.org
Thanks for doing this. Some comments below https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.h:21: class MemoryTracingTest; Hmm no, base should never know anything about content, we have to find another way to achieve this. https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.h:216: mutable Lock lock_; See comment below, you don't need to use the lock at all. https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.h:233: bool ignore_registrations_for_testing_; I'd name this (and the setter) ignore_dumper_registrations_for_testing_ https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.h:236: AutoLock lock(lock_); If this was production code would have been fine. But for testing code we can just safely assume that the setter is called early enough and does not race. Also, I think you just want a setter here. Are you ever going to use the getter? https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:88: void EnableRegistrations() { Nit: EnableMemoryDumpProviderRegistrations (Same below)
https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.h:21: class MemoryTracingTest; On 2015/08/24 at 17:23:34, Primiano Tucci wrote: > Hmm no, base should never know anything about content, we have to find another way to achieve this. I made |ignore_dumper_registrations_for_testing()| public. The for_testing suffix should prevent people from using this outside of tests anyway. https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.h:233: bool ignore_registrations_for_testing_; On 2015/08/24 at 17:23:34, Primiano Tucci wrote: > I'd name this (and the setter) ignore_dumper_registrations_for_testing_ Done. https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.h:236: AutoLock lock(lock_); On 2015/08/24 at 17:23:34, Primiano Tucci wrote: > If this was production code would have been fine. > But for testing code we can just safely assume that the setter is called early enough and does not race. > Also, I think you just want a setter here. Are you ever going to use the getter? It is used in |MemoryDumpManagerTest::EnableTracing| to enable tracing without registering any extra dump provider, and without changing the state of whether registrations are ignored (because the tests do register providers, and it would be tedious to wrap every registration into a set ignore/unset ignore pair). I think you are right about the lock, setting is only done from one thread so even if a test starts up new threads this is not a problem. https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1308403002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:88: void EnableRegistrations() { On 2015/08/24 at 17:23:34, Primiano Tucci wrote: > Nit: EnableMemoryDumpProviderRegistrations (Same below) Done.
Let's have a syncup with ssid later this afternoon on this. All these magicForTesting methods start to confuse me (it's not really your fault, I asked for them) and might not be required anymore in the light of the recent TraceConfig changes.
Sure. Semi-related: I failed to reproduce the trybot failure for that CL on the Windows machine ... On 25 August 2015 at 12:28, <primiano@chromium.org> wrote: > Let's have a syncup with ssid later this afternoon on this. > All these magicForTesting methods start to confuse me (it's not really your > fault, I asked for them) and might not be required anymore in the light of > the > recent TraceConfig changes. > > > https://codereview.chromium.org/1308403002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/08/25 at 11:59:04, chromium-reviews wrote: > Sure. Semi-related: I failed to reproduce the trybot failure for that CL on > the Windows machine ... > > On 25 August 2015 at 12:28, <primiano@chromium.org> wrote: > > > Let's have a syncup with ssid later this afternoon on this. > > All these magicForTesting methods start to confuse me (it's not really your > > fault, I asked for them) and might not be required anymore in the light of > > the > > recent TraceConfig changes. > > > > > > https://codereview.chromium.org/1308403002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. > Primiano, has this become obsolete? It conflicts with master everywhere; it appears that a similar mechanism is now in place?
I made lot of clanups in the previous weeks, so yes I guess this has lot of conflicts. However, the logic of this CL still holds. I just think this could be super-simplifier (easier to replay the diff manually). the unittest should be left unchanged I think. you need the disable...fortesting logic only in the browsertest. https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:153: MemoryDumpProviderInfo mdp_info(mdp, task_runner); Keep this before the lock. No need to call this ctor while locked https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:182: EnableMemoryDumpProviderRegistrations(); Hmm this is a unittest. Why do you need all these enable/disable registrations here? Nothing else should happen (as opposite to a browser test, where random things can register and unregister)
picksi@google.com changed reviewers: + picksi@google.com
Some drive by thoughts. I'll happily defer to Primiano if I'm making bad suggestions! https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:57: mdm_->SetDelegate(&delegate_); Random drive-by thought. For other users (i.e. outside of these tests) does the order of these calls need to be maintained for correct execution? If so can this be enforced somehow (at compile time), e.g. Add and InitialzizeWithDelegate() function or make Initilize return a delegate-handler (?) that is used to set a delegate... etc. https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:80: mdm_->ignore_dumper_registrations_for_testing(); Nit: This function name makes it sound like you are ignoring dumper registration, rather than getting it. Should this be renamed to something like are_dumper_registrations_ignored()? https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:93: void DisableMemoryDumpProviderRegistrations() { nit: Should you be using this function in EnableTracing() instead of directly calling the setter? https://codereview.chromium.org/1308403002/diff/20001/content/browser/tracing... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/1308403002/diff/20001/content/browser/tracing... content/browser/tracing/memory_tracing_browsertest.cc:75: ->set_ignore_dumper_registrations_for_testing(true); nit: Should you be calling DisableMemoryDumpProviderRegistrations() here?
https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:153: MemoryDumpProviderInfo mdp_info(mdp, task_runner); On 2015/09/22 at 08:07:02, Primiano Tucci wrote: > Keep this before the lock. No need to call this ctor while locked Done. https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:57: mdm_->SetDelegate(&delegate_); On 2015/09/22 at 10:53:38, picksi wrote: > Random drive-by thought. For other users (i.e. outside of these tests) does the order of these calls need to be maintained for correct execution? If so can this be enforced somehow (at compile time), e.g. Add and InitialzizeWithDelegate() function or make Initilize return a delegate-handler (?) that is used to set a delegate... etc. The |SetDelegate| method did not survive Primiano’s Great Refactor™. In fact, the new |Initialize| method takes a delegate. https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:80: mdm_->ignore_dumper_registrations_for_testing(); On 2015/09/22 at 10:53:38, picksi wrote: > Nit: This function name makes it sound like you are ignoring dumper registration, rather than getting it. Should this be renamed to something like are_dumper_registrations_ignored()? Not according to the style guide (unless I also rename the variable) which says: > Accessors and mutators (get and set functions) should match the name of the variable they are getting and setting Do you think the variable should be renamed? https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:93: void DisableMemoryDumpProviderRegistrations() { On 2015/09/22 at 10:53:38, picksi wrote: > nit: Should you be using this function in EnableTracing() instead of directly calling the setter? In |UnregisterDumperFromThreadWhileDumping|, registrations need to be enabled/disabled at a different point, so a method is necessary either way. https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:182: EnableMemoryDumpProviderRegistrations(); On 2015/09/22 at 08:07:02, Primiano Tucci wrote: > Hmm this is a unittest. Why do you need all these enable/disable registrations here? Nothing else should happen (as opposite to a browser test, where random things can register and unregister) I removed |skip_core_dumpers_auto_registration_for_testing_| in favour of |ignore_dumper_registrations_for_testing_|, so this is required to prevent the core dumpers from registering. https://codereview.chromium.org/1308403002/diff/20001/content/browser/tracing... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/1308403002/diff/20001/content/browser/tracing... content/browser/tracing/memory_tracing_browsertest.cc:75: ->set_ignore_dumper_registrations_for_testing(true); On 2015/09/22 at 10:53:38, picksi wrote: > nit: Should you be calling DisableMemoryDumpProviderRegistrations() here? It is not declared in this test. (This |MemoryTracingTest| in content_browsertests, not |MemoryDumpManagerTest| in base_unittests.)
https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:80: mdm_->ignore_dumper_registrations_for_testing(); You could rename the variable 'dumper_registrations_ignored_for_testing' which would be less confusing and is in line with your local variable 'registrations_ignored'. FWIW Are you also able to drop 'dumper_' as a prefix? (I think) the context of the call, e.g. mdm->registrations_ignored_for_testing(), makes it obvious that it's a dumper. Then you'd end up with 'registrations_ignored_for_testing'. WDYT?
On 2015/09/29 at 10:55:56, picksi wrote: > https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... > File base/trace_event/memory_dump_manager_unittest.cc (right): > > https://codereview.chromium.org/1308403002/diff/20001/base/trace_event/memory... > base/trace_event/memory_dump_manager_unittest.cc:80: mdm_->ignore_dumper_registrations_for_testing(); > You could rename the variable 'dumper_registrations_ignored_for_testing' which would be less confusing and is in line with your local variable 'registrations_ignored'. Yes, that is a good name. > FWIW Are you also able to drop 'dumper_' as a prefix? (I think) the context of the call, e.g. mdm->registrations_ignored_for_testing(), makes it obvious that it's a dumper. Then you'd end up with 'registrations_ignored_for_testing'. WDYT? I agree but see comment #3.
https://codereview.chromium.org/1308403002/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1308403002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:101: mdm_->dumper_registrations_ignored_for_testing(); Why is this get and re-set needed? https://codereview.chromium.org/1308403002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:117: void EnableMemoryDumpProviderRegistrations() { I'd like that we used a slightly different approach here. Instead of explicitlyh enabling and disabling the registrations in the unittest what I would do is: Introducing a protected method here: void RegisterDumpProvider(mdp) { mdm_->set_dumper_registrations_ignored_for_testing(false); mdm->Register(mdp); mdm_->set_dumper_registrations_ignored_for_testing(true); } and avoid touch all the rest below.
https://codereview.chromium.org/1308403002/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1308403002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:101: mdm_->dumper_registrations_ignored_for_testing(); On 2015/09/29 at 17:19:09, Primiano Tucci wrote: > Why is this get and re-set needed? There should have been an additional disable there; |SetEnabled| subscribes the trace log dump provider. It is not required any more now. https://codereview.chromium.org/1308403002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:117: void EnableMemoryDumpProviderRegistrations() { On 2015/09/29 at 17:19:09, Primiano Tucci wrote: > I'd like that we used a slightly different approach here. > Instead of explicitlyh enabling and disabling the registrations in the unittest what I would do is: > > Introducing a protected method here: void RegisterDumpProvider(mdp) { > mdm_->set_dumper_registrations_ignored_for_testing(false); > mdm->Register(mdp); > mdm_->set_dumper_registrations_ignored_for_testing(true); > } > > and avoid touch all the rest below. That is much cleaner, great idea.
LGTM with some comments https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:108: bool dumper_registrations_ignored_for_testing() const { there doesn't seem to be any use of this getter. plz remove it https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:112: void set_dumper_registrations_ignored_for_testing(bool ignore) { s/ignore/ignored/ in the arg name https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:111: void RegisterDumpProvider(MemoryDumpProvider* mdp) { This could be made a standalone method in the anonymous namespace (use getinstance) as the set_dumpers_...ignored is public. https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:112: mdm_->set_dumper_registrations_ignored_for_testing(false); you can just call RegisterDumpProvider(mdp, nullptr) and avoid duplicating the boilerplate. https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:452: [this, &mdp2](const MemoryDumpArgs&, ProcessMemoryDump*) -> bool { if you move RegisterDumpProvider to the anonymous namespace there is no need to keep capturing |this| in the lambda
The CQ bit was checked by ruuda@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1308403002/#ps120001 (title: "Address primiano nits")
The CQ bit was unchecked by ruuda@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308403002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308403002/120001
ruuda@google.com changed reviewers: + dsinclair@chromium.org
+dsinclair for content/browser https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:108: bool dumper_registrations_ignored_for_testing() const { On 2015/10/13 at 13:35:50, Primiano Tucci wrote: > there doesn't seem to be any use of this getter. plz remove it Done. https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:112: void set_dumper_registrations_ignored_for_testing(bool ignore) { On 2015/10/13 at 13:35:50, Primiano Tucci wrote: > s/ignore/ignored/ in the arg name Done. https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:111: void RegisterDumpProvider(MemoryDumpProvider* mdp) { On 2015/10/13 at 13:35:50, Primiano Tucci wrote: > This could be made a standalone method in the anonymous namespace (use getinstance) as the set_dumpers_...ignored is public. As you wish. https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:112: mdm_->set_dumper_registrations_ignored_for_testing(false); On 2015/10/13 at 13:35:50, Primiano Tucci wrote: > you can just call RegisterDumpProvider(mdp, nullptr) and avoid duplicating the boilerplate. Done. https://codereview.chromium.org/1308403002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:452: [this, &mdp2](const MemoryDumpArgs&, ProcessMemoryDump*) -> bool { On 2015/10/13 at 13:35:50, Primiano Tucci wrote: > if you move RegisterDumpProvider to the anonymous namespace there is no need to keep capturing |this| in the lambda Done.
lgtm w/ nit https://codereview.chromium.org/1308403002/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1308403002/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:135: RegisterDumpProvider(ProcessMemoryTotalsDumpProvider::GetInstance()); Indenting
https://codereview.chromium.org/1308403002/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1308403002/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:135: RegisterDumpProvider(ProcessMemoryTotalsDumpProvider::GetInstance()); On 2015/10/20 14:31:39, dsinclair wrote: > Indenting Done.
The CQ bit was checked by ruuda@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dsinclair@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1308403002/#ps140001 (title: "Fix indentation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308403002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308403002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/16ca6b63afbb3f1c6f95c1fc3f592459b7060292 Cr-Commit-Position: refs/heads/master@{#355071} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
