[tracing] Fix MemoryDumpManager to support startup tracing

[tracing] Fix MemoryDumpManager to support startup tracing

This CL simplifies a lot the design of the MemoryDumpManager (MDM)
initialization (\o/). Before this change the MDM had a two-stage
initialization: 1. Initialize() that was registering to the
TraceLog enabled stateobserver; 2. SetDelegate() that was setting
the inversion-of-control delegate.
This design was fragile as it required to think about the two possible
sequences of events (1 -> 2 or 2 -> 1) and not really necessary.
The simplification herein introduced consists in:
 - Drop the IsCoordinatorProcess() method from the delegate. That
   information is not supposed to change over time and is known
   at MDM initialization time.
 - Merge the SetDelegate() into the Initialize() call. There is no
   point allowing to initialize the MDM without a delegate, so
   why bother? Moved all the initialization to the point where the
   delegate (concretelly the TracingControllerImpl) is ready.

As a side effect, startup tracing now works properly (tests are
coming in a separate CL).

BUG=524057
TEST=--trace-startup=-*,disabled-by-default-memory-infra --trace-startup-file=/tmp/startup.json

Committed: https://crrev.com/ba4ca8421832350dd3335a8bd7abea9f0e1311b6
Cr-Commit-Position: refs/heads/master@{#348168}

[Primiano Tucci (use gerrit), 2015-09-10 11:30:13]

primiano@chromium.org changed reviewers:
+ dsinclair@chromium.org

[Primiano Tucci (use gerrit), 2015-09-10 11:34:10]

Dan, could you PTAL to this?
Test is coming separately in crrev.com/1335503003
I'm doing that in a separate CL as these tests are tricky to write and in the
past some took some iterations to get them right. If that is the case I don't
want to get this entire refactor to be reverted as I have other stuff piled on
top.
Thanks

[Primiano Tucci (use gerrit), 2015-09-10 14:09:37]

primiano@chromium.org changed reviewers:
+ sievers@chromium.org

[Primiano Tucci (use gerrit), 2015-09-10 14:09:38]

In the meantime +sievers for 2 lines changes in:
content/browser/browser_main_loop.cc
content/child/child_thread_impl.cc

I hope you don't mind if I remove some code from the browser+child startup loop
:)

[dsinclair, 2015-09-10 14:16:28]

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:153: bool is_tracing_already_enabled =
TraceLog::GetInstance()->IsEnabled();
Move the bool and the comment down to right before the if()? They aren't useful
to the other two bits in the middle.

Don't necessarily need to store the bool, can do the IsEnabled() check right in
the if, no?

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:154: TRACE_EVENT0(kTraceCategory,
"init");  // Add to trace-viewer category list.
Is this early enough? Do we always Initialize the MDM or does it only happen in
certain cases?

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:52: //  deleagate: inversion-of-control
interface for embedder-specific behaviors
nit: s/deleagate/delegate

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:55: void Initialize(bool is_coordinator,
MemoryDumpManagerDelegate* delegate);
ramble: Initialize(MemoryDumpManagerDelegate, bool)?

Just looks odd to me with the bool first, ignore as you wish.

https://codereview.chromium.org/1333873002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/1333873002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:55: void Initialize(bool is_coordinator,
MemoryDumpManagerDelegate* delegate);
What about making is_coordinator an enum? I see in most uses you marked it as /*
is_coordinator */ anyway.

MemoryDumpManagerActor,
MemoryDumpManagerCoordinator

Or something?

[Primiano Tucci (use gerrit), 2015-09-10 14:48:12]

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:153: bool is_tracing_already_enabled =
TraceLog::GetInstance()->IsEnabled();
On 2015/09/10 14:16:28, dsinclair wrote:
> Move the bool and the comment down to right before the if()? They aren't
useful
> to the other two bits in the middle.
> 
> Don't necessarily need to store the bool, can do the IsEnabled() check right
in
> the if, no?

THis is just to avoid the following super-edge case:
- MDM gets added to the enabled state observer
- tracing gets enabled right now -> 1st genuine call to OnTraceLogEnabled() from
the observer.
- We do the check now and decide that "oh damn the party started" -> 2nd
spurious call to OnTraceLogEnabled().

It is probably fine right now that OnTraceLogEnabled() gets called twice given
the current implementation, but I don't really want to have to think to that.

In essence what that boolean is capturing is:
did_tracing_get_enabled_before_we_registered_the_enabled_state_observer, which
is probably a bit too long :)

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:154: TRACE_EVENT0(kTraceCategory,
"init");  // Add to trace-viewer category list.
On 2015/09/10 14:16:28, dsinclair wrote:
> Is this early enough? Do we always Initialize the MDM or does it only happen
in
> certain cases?

If we don't initialize the MDM (shouldn never happen) it won't produce any dump,
so not worth showing a non-working category.
Technically the MDM is initialized by:
the first TracingControllerImpl()::GetInstance() in the browser
the ChildTrace...MessageFilter in the child
So, to answer your question: yes MDM should be always get initialized

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:52: //  deleagate: inversion-of-control
interface for embedder-specific behaviors
On 2015/09/10 14:16:28, dsinclair wrote:
> nit: s/deleagate/delegate

Done.

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:55: void Initialize(bool is_coordinator,
MemoryDumpManagerDelegate* delegate);
On 2015/09/10 14:16:28, dsinclair wrote:
> ramble: Initialize(MemoryDumpManagerDelegate, bool)?
> 
> Just looks odd to me with the bool first, ignore as you wish.

Makes sense. Done

https://codereview.chromium.org/1333873002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/1333873002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:55: void Initialize(bool is_coordinator,
MemoryDumpManagerDelegate* delegate);
On 2015/09/10 14:16:28, dsinclair wrote:
> What about making is_coordinator an enum? I see in most uses you marked it as
/*
> is_coordinator */ anyway.
> 
> MemoryDumpManagerActor,
> MemoryDumpManagerCoordinator
> 
> Or something?

Hmm my personal take in this case is that it makes the code look more complex
than what it actually is.
If I see a line that does
MDM::INitialize(Role::kMemoryDumpManagerCoordinator) then I start wondering: how
many other roles could there be, what do they mean?
Instead a bool with the comment IMHO gives to the reader a bit more information
(this is either a coordinator or not)

[dsinclair, 2015-09-10 14:53:49]

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:153: bool is_tracing_already_enabled =
TraceLog::GetInstance()->IsEnabled();
On 2015/09/10 14:48:12, Primiano Tucci wrote:
> On 2015/09/10 14:16:28, dsinclair wrote:
> > Move the bool and the comment down to right before the if()? They aren't
> useful
> > to the other two bits in the middle.
> > 
> > Don't necessarily need to store the bool, can do the IsEnabled() check right
> in
> > the if, no?
> 
> THis is just to avoid the following super-edge case:
> - MDM gets added to the enabled state observer
> - tracing gets enabled right now -> 1st genuine call to OnTraceLogEnabled()
from
> the observer.
> - We do the check now and decide that "oh damn the party started" -> 2nd
> spurious call to OnTraceLogEnabled().
> 
> It is probably fine right now that OnTraceLogEnabled() gets called twice given
> the current implementation, but I don't really want to have to think to that.
> 
> In essence what that boolean is capturing is:
> did_tracing_get_enabled_before_we_registered_the_enabled_state_observer, which
> is probably a bit too long :)


In that case, doesn't the if below need to be:

if (is_tracing_already_enabled && !TraceLog::GetInstance()->IsEnabled())?

So, tracing was enabled before we set the observer, but is not enabled after we
set the observer? Otherwise you can OnTraceLogEnabled() twice?

https://codereview.chromium.org/1333873002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/1333873002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:55: void Initialize(bool is_coordinator,
MemoryDumpManagerDelegate* delegate);
On 2015/09/10 14:48:12, Primiano Tucci wrote:
> On 2015/09/10 14:16:28, dsinclair wrote:
> > What about making is_coordinator an enum? I see in most uses you marked it
as
> /*
> > is_coordinator */ anyway.
> > 
> > MemoryDumpManagerActor,
> > MemoryDumpManagerCoordinator
> > 
> > Or something?
> 
> Hmm my personal take in this case is that it makes the code look more complex
> than what it actually is.
> If I see a line that does
> MDM::INitialize(Role::kMemoryDumpManagerCoordinator) then I start wondering:
how
> many other roles could there be, what do they mean?
> Instead a bool with the comment IMHO gives to the reader a bit more
information
> (this is either a coordinator or not)

Fair enough.

[dsinclair, 2015-09-10 15:01:47]

lgtm

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:153: bool is_tracing_already_enabled =
TraceLog::GetInstance()->IsEnabled();
On 2015/09/10 14:53:49, dsinclair wrote:
> On 2015/09/10 14:48:12, Primiano Tucci wrote:
> > On 2015/09/10 14:16:28, dsinclair wrote:
> > > Move the bool and the comment down to right before the if()? They aren't
> > useful
> > > to the other two bits in the middle.
> > > 
> > > Don't necessarily need to store the bool, can do the IsEnabled() check
right
> > in
> > > the if, no?
> > 
> > THis is just to avoid the following super-edge case:
> > - MDM gets added to the enabled state observer
> > - tracing gets enabled right now -> 1st genuine call to OnTraceLogEnabled()
> from
> > the observer.
> > - We do the check now and decide that "oh damn the party started" -> 2nd
> > spurious call to OnTraceLogEnabled().
> > 
> > It is probably fine right now that OnTraceLogEnabled() gets called twice
given
> > the current implementation, but I don't really want to have to think to
that.
> > 
> > In essence what that boolean is capturing is:
> > did_tracing_get_enabled_before_we_registered_the_enabled_state_observer,
which
> > is probably a bit too long :)
> 
> 
> In that case, doesn't the if below need to be:
> 
> if (is_tracing_already_enabled && !TraceLog::GetInstance()->IsEnabled())?
> 
> So, tracing was enabled before we set the observer, but is not enabled after
we
> set the observer? Otherwise you can OnTraceLogEnabled() twice?

Ignore me.

[Primiano Tucci (use gerrit), 2015-09-10 15:03:15]

Thanks.
Tested startup tracing, seems to work even on Windows \o/

CC: +bratell so he has a happy weekend :P (obviously joking)

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1333873002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:153: bool is_tracing_already_enabled =
TraceLog::GetInstance()->IsEnabled();
On 2015/09/10 14:53:49, dsinclair wrote:
> On 2015/09/10 14:48:12, Primiano Tucci wrote:
> > On 2015/09/10 14:16:28, dsinclair wrote:
> > > Move the bool and the comment down to right before the if()? They aren't
> > useful
> > > to the other two bits in the middle.
> > > 
> > > Don't necessarily need to store the bool, can do the IsEnabled() check
right
> > in
> > > the if, no?
> > 
> > THis is just to avoid the following super-edge case:
> > - MDM gets added to the enabled state observer
> > - tracing gets enabled right now -> 1st genuine call to OnTraceLogEnabled()
> from
> > the observer.
> > - We do the check now and decide that "oh damn the party started" -> 2nd
> > spurious call to OnTraceLogEnabled().
> > 
> > It is probably fine right now that OnTraceLogEnabled() gets called twice
given
> > the current implementation, but I don't really want to have to think to
that.
> > 
> > In essence what that boolean is capturing is:
> > did_tracing_get_enabled_before_we_registered_the_enabled_state_observer,
which
> > is probably a bit too long :)
> 
> 
> In that case, doesn't the if below need to be:
> 
> if (is_tracing_already_enabled && !TraceLog::GetInstance()->IsEnabled())?
> 
> So, tracing was enabled before we set the observer, but is not enabled after
we
> set the observer? Otherwise you can OnTraceLogEnabled() twice?

Discussed offline. THe main problem is that the trace log observer doesn't
notify if you register after tracing has been started (has a very simple impl)

[Primiano Tucci (use gerrit), 2015-09-10 15:37:15]

primiano@chromium.org changed reviewers:
+ jochen@chromium.org

[Primiano Tucci (use gerrit), 2015-09-10 15:37:15]

+jochen for removing 2 lines in:
content/browser/browser_main_loop.cc
content/child/child_thread_impl.cc

[jochen (gone - plz use gerrit), 2015-09-10 15:57:37]

lgtm

[Primiano Tucci (use gerrit), 2015-09-10 15:57:54]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2015-09-10 15:58:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1333873002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1333873002/40001

[commit-bot: I haz the power, 2015-09-10 16:51:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-10 16:51:55]

Patchset 3 (id:??) landed as
https://crrev.com/ba4ca8421832350dd3335a8bd7abea9f0e1311b6
Cr-Commit-Position: refs/heads/master@{#348168}

[commit-bot: I haz the power, 2015-09-23 12:11:22]

Patchset 3 (id:??) landed as
https://crrev.com/ba4ca8421832350dd3335a8bd7abea9f0e1311b6
Cr-Commit-Position: refs/heads/master@{#348168}