Teach the mojo_shell --trace-startup flag to gather data from services

Teach the mojo_shell --trace-startup flag to gather data from services

This makes passing --trace-startup to mojo_shell (or MojoShell.apk)
collect and save data from all apps/services that support tracing, not
just the shell itself.

R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/6997fa253c58bea69e94903a718bc382a577173b

[jamesr, 2015-04-23 23:35:53]

This makes startup tracing much more useful, although it still doesn't capture
data on shutdown.

[viettrungluu, 2015-04-27 17:04:11]

Seems to cause some test crashes?

https://codereview.chromium.org/1105773002/diff/1/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/1105773002/diff/1/shell/context.cc#newcode293
shell/context.cc:293: new TracingServiceProvider(tracer_,
GetProxy(&tracing_exposed_services));
nit: mojo::GetProxy(), since you probably don't want to rely on ADL (and below).

https://codereview.chromium.org/1105773002/diff/1/shell/context.h
File shell/context.h (right):

https://codereview.chromium.org/1105773002/diff/1/shell/context.h#newcode81
shell/context.h:81: Tracer* tracer_;
Tracer* const?

https://codereview.chromium.org/1105773002/diff/1/shell/desktop/main.cc
File shell/desktop/main.cc (right):

https://codereview.chromium.org/1105773002/diff/1/shell/desktop/main.cc#newco...
shell/desktop/main.cc:104: if (!shell_context.Init()) {
I don't know if the sketchy failure case is compatible with the post task above.
(I suppose it probably is, strictly speaking, but it only becomes even more
sketchy.)

https://codereview.chromium.org/1105773002/diff/1/shell/tracer.h
File shell/tracer.h (right):

https://codereview.chromium.org/1105773002/diff/1/shell/tracer.h#newcode72
shell/tracer.h:72: // Pointer to the tracing service.
I'm confused by this comment (and/or its placement).

[jamesr, 2015-04-27 23:46:57]

Now less crashy, PTAL

https://codereview.chromium.org/1105773002/diff/1/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/1105773002/diff/1/shell/context.cc#newcode293
shell/context.cc:293: new TracingServiceProvider(tracer_,
GetProxy(&tracing_exposed_services));
On 2015/04/27 17:04:11, viettrungluu wrote:
> nit: mojo::GetProxy(), since you probably don't want to rely on ADL (and
below).

I actually am intending to rely on ADL here - mojo::GetProxy is designed for ADL
to work.

https://codereview.chromium.org/1105773002/diff/1/shell/desktop/main.cc
File shell/desktop/main.cc (right):

https://codereview.chromium.org/1105773002/diff/1/shell/desktop/main.cc#newco...
shell/desktop/main.cc:104: if (!shell_context.Init()) {
On 2015/04/27 17:04:11, viettrungluu wrote:
> I don't know if the sketchy failure case is compatible with the post task
above.
> (I suppose it probably is, strictly speaking, but it only becomes even more
> sketchy.)

I think it's OK - even here the message_loop goes away before the shell_context
so if the task were to run somehow it wouldn't see any dead objects

https://codereview.chromium.org/1105773002/diff/1/shell/tracer.h
File shell/tracer.h (right):

https://codereview.chromium.org/1105773002/diff/1/shell/tracer.h#newcode72
shell/tracer.h:72: // Pointer to the tracing service.
On 2015/04/27 17:04:11, viettrungluu wrote:
> I'm confused by this comment (and/or its placement).

Took another crack

[viettrungluu, 2015-04-28 00:42:18]

https://codereview.chromium.org/1105773002/diff/40001/shell/tracer.cc
File shell/tracer.cc (right):

https://codereview.chromium.org/1105773002/diff/40001/shell/tracer.cc#newcode56
shell/tracer.cc:56: PCHECK(trace_file_);
nit: move this to be immediately after the fopen()

https://codereview.chromium.org/1105773002/diff/40001/shell/tracer.cc#newcode121
shell/tracer.cc:121: void Tracer::OnDataAvailable(const void* data, size_t
num_bytes) {
Hmmm, this seems sketchy to me -- you may not get entire chunks in
|OnDataAvailable()|, no?

https://codereview.chromium.org/1105773002/diff/40001/sky/tools/debugger/debu...
File sky/tools/debugger/debugger.cc (right):

https://codereview.chromium.org/1105773002/diff/40001/sky/tools/debugger/debu...
sky/tools/debugger/debugger.cc:31: SkyDebugger() : is_tracing_(false),
handler_binding_(this) {}
nit: Initialize |app_| (either |app_()| or |app_(nullptr)| is fine).

[jamesr, 2015-04-28 18:18:52]

On 2015/04/28 00:42:18, viettrungluu wrote:
>
https://codereview.chromium.org/1105773002/diff/40001/shell/tracer.cc#newcode121
> shell/tracer.cc:121: void Tracer::OnDataAvailable(const void* data, size_t
> num_bytes) {
> Hmmm, this seems sketchy to me -- you may not get entire chunks in
> |OnDataAvailable()|, no?
> 

Yeah, this is le sketch.  Will change this from a data pipe to a message pipe.

[jamesr, 2015-04-29 00:56:38]

On 2015/04/28 18:18:52, jamesr wrote:
> On 2015/04/28 00:42:18, viettrungluu wrote:
> >
>
https://codereview.chromium.org/1105773002/diff/40001/shell/tracer.cc#newcode121
> > shell/tracer.cc:121: void Tracer::OnDataAvailable(const void* data, size_t
> > num_bytes) {
> > Hmmm, this seems sketchy to me -- you may not get entire chunks in
> > |OnDataAvailable()|, no?
> > 
> 
> Yeah, this is le sketch.  Will change this from a data pipe to a message pipe.

Actually no, that's a pain.  This treats the data coming from the data pipe as
opaque chunks but assumes that the whole thing is a sequence of json objects and
concatenates it with a comma with the data gathered locally (by just buffering
the thing).  Buffering the whole thing in memory is not ideal but until we have
a way to transfer around big arrays I think it's necessary.

[viettrungluu, 2015-05-01 17:29:42]

LGTM

[jamesr, 2015-05-01 18:03:15]

Committed patchset #4 (id:60001) manually as
6997fa253c58bea69e94903a718bc382a577173b (presubmit successful).

[etiennej, 2015-09-11 09:43:15]

etiennej@chromium.org changed reviewers:
+ etiennej@chromium.org

[etiennej, 2015-09-11 09:43:16]

https://codereview.chromium.org/1105773002/diff/60001/mojo/common/trace_contr...
File mojo/common/trace_controller_impl.cc (right):

https://codereview.chromium.org/1105773002/diff/60001/mojo/common/trace_contr...
mojo/common/trace_controller_impl.cc:16: : tracing_already_started_(false),
binding_(this, request.Pass()) {
I am currently looking at this code. Was there a reason to use this field
instead of base::trace_event::TraceLog::GetInstance()->IsEnabled(), or instead
of doing nothing and calling SetEnabled every time?

[jamesr, 2015-09-11 22:37:32]

On 2015/09/11 at 09:43:16, etiennej wrote:
>
https://codereview.chromium.org/1105773002/diff/60001/mojo/common/trace_contr...
> File mojo/common/trace_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/1105773002/diff/60001/mojo/common/trace_contr...
> mojo/common/trace_controller_impl.cc:16: : tracing_already_started_(false),
binding_(this, request.Pass()) {
> I am currently looking at this code. Was there a reason to use this field
instead of base::trace_event::TraceLog::GetInstance()->IsEnabled(), or instead
of doing nothing and calling SetEnabled every time?

Calling SetEnabled() requires knowing the configuration (specifically the set of
categories) and is not idempotent.  It will silently merge, but that's not
necessarily what you want to happen.  IsEnabled() queries TraceLog::mode_
without taking its lock and thus isn't safe to call unless the code knows what
every other thread is doing at the time of call.