Add tracing support to NaCl

Add tracing support to NaCl

Add Ppapi IPC messages and a handler inside of the NaCl IRT to enable, disable, and send the result of tracing.

BUG=93839
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171911

[jbauman, 2012-11-21 09:53:13]

Not sure if this is the best approach, but it seems to work.

[bbudge, 2012-11-21 18:34:52]

The general approach looks OK. I don't think moving the messages from content to
ppapi is though. Is this a proof-of-concept patch?

[jbauman, 2012-11-21 19:08:51]

On 2012/11/21 18:34:52, bbudge1 wrote:
> The general approach looks OK. I don't think moving the messages from content
to
> ppapi is though. Is this a proof-of-concept patch?

Pretty much, yeah. I don't know if it would be better to duplicate the messages
(and trace_message_filter) or instead to find someplace common to put the
messages. I can't think of where that common place would be, as this seems
rather unique.

[bbudge, 2012-11-21 19:18:03]

We should probably discuss how to expose what we need in 'content' with Brett
and John (AbdelMalek).

[jbauman, 2012-11-26 18:33:32]

jam@, brettw@, we'll need to send some of the content child process messages
(like ChildProcessMsg_BeginTracing) to NaCl processes as well with this patch. I
hacked this up by moving those messages to ppapi/proxy/ppapi_messages.h, but
that's not a good solution.

Is there a place where we could put these messages that are shared between
content/ and ppapi/? It looks like this situation is unique. Or should I just
duplicate the messages and the code to handle them in the browser?

[brettw, 2012-11-27 23:53:32]

I don't have a better idea for where to put these. Maybe John does?

I think duplicating them is better than the moving them to PPAPI.

Brett

On Mon, Nov 26, 2012 at 10:33 AM,  <jbauman@chromium.org> wrote:
> jam@, brettw@, we'll need to send some of the content child process messages
> (like ChildProcessMsg_BeginTracing) to NaCl processes as well with this
> patch. I
> hacked this up by moving those messages to ppapi/proxy/ppapi_messages.h, but
> that's not a good solution.
>
> Is there a place where we could put these messages that are shared between
> content/ and ppapi/? It looks like this situation is unique. Or should I
> just
> duplicate the messages and the code to handle them in the browser?
>
> https://codereview.chromium.org/11411118/

[jbauman, 2012-11-28 03:34:20]

Ok, I've duplicated all of the messages. Better?

[bbudge, 2012-11-28 22:45:57]

It seems like you could factor a bunch of duplicate code from
TraceMessageFilterImpl and PepperTraceMessageFilterImpl. Am I missing something?

[jbauman, 2012-12-05 00:20:09]

On 2012/11/28 22:45:57, bbudge1 wrote:
> It seems like you could factor a bunch of duplicate code from
> TraceMessageFilterImpl and PepperTraceMessageFilterImpl. Am I missing
something?

Ok, I moved the tracing messages to content/components to reduce duplication.

[bbudge, 2012-12-05 02:22:28]

Component-izing trace messages seems good. One issue we have is the size of the
IRT nexes (nacl_irt_x86_32.nexe, etc.) Not that you have any control over it,
but do you have an idea of how much bigger the nexes get after this change? They
should be in your build product directory.

I think the .gyp file should be split up but the rest looks reasonable to me.

https://codereview.chromium.org/11411118/diff/16001/content/content_component...
File content/content_components_tracing.gyp (right):

https://codereview.chromium.org/11411118/diff/16001/content/content_component...
content/content_components_tracing.gyp:5: 
The usual pattern when supporting a NaCl (untrusted) build is to create a .gypi
file with common stuff, and then .gyp files for normal and untrusted builds.
base/base.gypi and ipc/ipc.gypi and associated .gyp files are good examples. You
can also look in ppapi/ for more.

https://codereview.chromium.org/11411118/diff/16001/content/content_component...
content/content_components_tracing.gyp:8: '../build/common_untrusted.gypi',
Specifically it worries me to include this in the same .gyp file as the trusted
target. It may work today but break tomorrow.

https://codereview.chromium.org/11411118/diff/16001/ppapi/proxy/plugin_main_n...
File ppapi/proxy/plugin_main_nacl.cc (right):

https://codereview.chromium.org/11411118/diff/16001/ppapi/proxy/plugin_main_n...
ppapi/proxy/plugin_main_nacl.cc:57: // This class sends and receives trace
messages on child processes.
This file has a bunch of different things already. Could you split this into a
separate file, say, ppapi/ppapi_trace_message_filter_nacl?

[jbauman, 2012-12-05 05:38:56]

Ok, I split up the .gyp and plugin_main_nacl.cc files.
nacl_irt_x86_64.nexe grew from 5,981,188 bytes to 6,057,817 bytes.

On 2012/12/05 02:22:28, bbudge1 wrote:
> Component-izing trace messages seems good. One issue we have is the size of
the
> IRT nexes (nacl_irt_x86_32.nexe, etc.) Not that you have any control over it,
> but do you have an idea of how much bigger the nexes get after this change?
They
> should be in your build product directory.
> 
> I think the .gyp file should be split up but the rest looks reasonable to me.
> 
>
https://codereview.chromium.org/11411118/diff/16001/content/content_component...
> File content/content_components_tracing.gyp (right):
> 
>
https://codereview.chromium.org/11411118/diff/16001/content/content_component...
> content/content_components_tracing.gyp:5: 
> The usual pattern when supporting a NaCl (untrusted) build is to create a
.gypi
> file with common stuff, and then .gyp files for normal and untrusted builds.
> base/base.gypi and ipc/ipc.gypi and associated .gyp files are good examples.
You
> can also look in ppapi/ for more.
> 
>
https://codereview.chromium.org/11411118/diff/16001/content/content_component...
> content/content_components_tracing.gyp:8: '../build/common_untrusted.gypi',
> Specifically it worries me to include this in the same .gyp file as the
trusted
> target. It may work today but break tomorrow.
> 
>
https://codereview.chromium.org/11411118/diff/16001/ppapi/proxy/plugin_main_n...
> File ppapi/proxy/plugin_main_nacl.cc (right):
> 
>
https://codereview.chromium.org/11411118/diff/16001/ppapi/proxy/plugin_main_n...
> ppapi/proxy/plugin_main_nacl.cc:57: // This class sends and receives trace
> messages on child processes.
> This file has a bunch of different things already. Could you split this into a
> separate file, say, ppapi/ppapi_trace_message_filter_nacl?

[bbudge, 2012-12-05 22:25:54]

LGTM w/nit
FYI I am not an OWNER on most of this code.

https://codereview.chromium.org/11411118/diff/19002/ppapi/proxy/ppapi_trace_m...
File ppapi/proxy/ppapi_trace_message_filter.h (right):

https://codereview.chromium.org/11411118/diff/19002/ppapi/proxy/ppapi_trace_m...
ppapi/proxy/ppapi_trace_message_filter.h:20:
PpapiTraceMessageFilter(base::MessageLoopProxy* ipc_message_loop);
explicit PpapiTraceMessageFilter ...

[jam, 2012-12-05 22:58:12]

can the trace_messge_filter.* be shared? i.e. have one implementation in
content/components

[jbauman, 2012-12-06 00:45:47]

On 2012/12/05 22:58:12, John Abd-El-Malek wrote:
> can the trace_messge_filter.* be shared? i.e. have one implementation in
> content/components

Yeah, I've combined them into
content/components/tracing/child_trace_message_filter.cc now.

[jam, 2012-12-06 01:36:16]

lgtm with nits

https://codereview.chromium.org/11411118/diff/25001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/11411118/diff/25001/content/DEPS#newcode12
content/DEPS:12: "+content/components/tracing",
put this in content/browser instead

https://codereview.chromium.org/11411118/diff/25001/content/components/tracin...
File content/components/tracing/child_trace_message_filter.h (right):

https://codereview.chromium.org/11411118/diff/25001/content/components/tracin...
content/components/tracing/child_trace_message_filter.h:14: namespace tracing {
nit: everything under src/content is in the content namespace, so please move
this to that

[jbauman, 2012-12-06 03:27:30]

Ok, fixed those things. Adding jschuh@ to review exposing these messages to
NaCl, brettw@ for base/ and ppapi/ OWNERS review, and piman@ for ppapi/proxy and
gpu/ OWNERS review

[piman, 2012-12-06 04:02:44]

lgtm

[jschuh, 2012-12-07 18:46:47]

ipc lgtm.

[brettw, 2012-12-07 21:23:37]

lgtm

https://codereview.chromium.org/11411118/diff/21010/content/components/tracin...
File content/components/tracing/DEPS (right):

https://codereview.chromium.org/11411118/diff/21010/content/components/tracin...
content/components/tracing/DEPS:3: "+ipc"
Can you add a "-content" rule here, with a comment that this component must not
depend on other parts of content since it's compiled in other contexts like
nacl?

[tfarina, 2013-02-27 15:55:39]

https://codereview.chromium.org/11411118/diff/29002/content/components/tracin...
File content/components/tracing/DEPS (right):

https://codereview.chromium.org/11411118/diff/29002/content/components/tracin...
content/components/tracing/DEPS:2: "+base",
was this and +ipc necessary? They are allowed by the toplevel src/DEPS file, see
line 600 in that file.

[jschuh, 2013-02-27 18:13:09]

On 2013/02/27 15:55:39, tfarina wrote:
>
https://codereview.chromium.org/11411118/diff/29002/content/components/tracin...
> File content/components/tracing/DEPS (right):
> 
>
https://codereview.chromium.org/11411118/diff/29002/content/components/tracin...
> content/components/tracing/DEPS:2: "+base",
> was this and +ipc necessary? They are allowed by the toplevel src/DEPS file,
see
> line 600 in that file.

All IPC changes must be reviewed by the security team due to the high risk of
introducing sandbox escapes via IPC. Now, my question for you is why you're
commenting on a closed CL that doesn't involve an area you work on? Just
curiosity, or was there something specific you were trying to do?

[tfarina, 2013-02-27 18:16:39]

On 2013/02/27 18:13:09, Justin Schuh wrote:
> All IPC changes must be reviewed by the security team due to the high risk of
> introducing sandbox escapes via IPC.
But that does not require you to introduce a DEPS rule for ipc. I think that is
not the way to enforce that IPC changes should be reviewed by the security team.

Now, my question for you is why you're
> commenting on a closed CL that doesn't involve an area you work on? Just
> curiosity, or was there something specific you were trying to do?
I was just auditing the usage of DEPS through components/ and saw this as
suspicious.

[jbauman, 2013-02-27 18:19:32]

On 2013/02/27 15:55:39, tfarina wrote:
>
https://codereview.chromium.org/11411118/diff/29002/content/components/tracin...
> File content/components/tracing/DEPS (right):
> 
>
https://codereview.chromium.org/11411118/diff/29002/content/components/tracin...
> content/components/tracing/DEPS:2: "+base",
> was this and +ipc necessary? They are allowed by the toplevel src/DEPS file,
see
> line 600 in that file.

Actually, you're probably right. I doubt we need those.

[jschuh, 2013-02-27 18:29:55]

On 2013/02/27 18:16:39, tfarina wrote:
> On 2013/02/27 18:13:09, Justin Schuh wrote:
> > All IPC changes must be reviewed by the security team due to the high risk
of
> > introducing sandbox escapes via IPC.
> But that does not require you to introduce a DEPS rule for ipc. I think that
is
> not the way to enforce that IPC changes should be reviewed by the security
team.

I'm sorry, but your understanding here is wrong. Owner reviews are an ideal
mechanism for enforcing this policy. Regardless, this CL isn't a good place for
such a discussion. If you are unconvinced please raise your concern on
chromium-dev and CC security@chromium.org.

[jschuh, 2013-02-27 22:17:22]

On 2013/02/27 18:29:55, Justin Schuh wrote:
> On 2013/02/27 18:16:39, tfarina wrote:
> > On 2013/02/27 18:13:09, Justin Schuh wrote:
> > > All IPC changes must be reviewed by the security team due to the high risk
> of
> > > introducing sandbox escapes via IPC.
> > But that does not require you to introduce a DEPS rule for ipc. I think that
> is
> > not the way to enforce that IPC changes should be reviewed by the security
> team.
> 
> I'm sorry, but your understanding here is wrong. Owner reviews are an ideal
> mechanism for enforcing this policy. Regardless, this CL isn't a good place
for
> such a discussion. If you are unconvinced please raise your concern on
> chromium-dev and CC mailto:security@chromium.org.

Okay, it appears I totally misread the context here. Please ignore. You're right
that this dependency has nothing to do with IPC reviews.