Pepper: Add VLOG support for NaCl plugins.

Pepper: Add VLOG support for NaCl plugins.

Previously, VLOG statements would never log anything to the console, since
there was no way for state from the "v" and "vmodule" flags to be passed to the
untrusted NaCl process.

This change passes the values of the "v" and "vmodule" flags to the untrusted
process as part of channel creation, and then re-initializes the logging
subsystem to parse the new flag values.

BUG=181607
R=dmichael

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

[teravest, 2013-04-10 18:37:27]

I'll add reviewers for base/ and chrome/browser/nacl_host/ if we agree this
approach makes sense.

[dmichael (off chromium), 2013-04-10 23:10:22]

https://codereview.chromium.org/13866034/diff/1/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/13866034/diff/1/base/logging.cc#newcode353
base/logging.cc:353: g_dcheck_state = dcheck_state;
Is it worth checking that LoggingDestination is either LOG_NONE or
LOG_ONLY_TO_SYSTEM_DEBUG_LOG for NaCl, since the file stuff will crap out later?

https://codereview.chromium.org/13866034/diff/1/chrome/browser/nacl_host/nacl...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/13866034/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:850: std::string v_flag =
cmdline->GetSwitchValueASCII(switches::kV);
Is it worth doing:
std::string flagWhitelist[] = [switches::kV, switches::kVModule];
for (size_t i = 0; i < arraysize(flagWhitelist); ++i) {
  //...
}
?

Feels like we might end up adding more.

https://codereview.chromium.org/13866034/diff/1/ppapi/proxy/plugin_main_nacl.cc
File ppapi/proxy/plugin_main_nacl.cc (right):

https://codereview.chromium.org/13866034/diff/1/ppapi/proxy/plugin_main_nacl....
ppapi/proxy/plugin_main_nacl.cc:188: if (!cmdline_and_logging_initialized) {
Does this ever happen more than once? If it really does, we should probably have
a separate message for the 1-time initialization stuff (which would the
permissions and incognito stuff)

https://codereview.chromium.org/13866034/diff/1/ppapi/shared_impl/ppapi_nacl_...
File ppapi/shared_impl/ppapi_nacl_channel_args.h (right):

https://codereview.chromium.org/13866034/diff/1/ppapi/shared_impl/ppapi_nacl_...
ppapi/shared_impl/ppapi_nacl_channel_args.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: 2013 for new files

[teravest, 2013-04-11 16:31:36]

On Wed, Apr 10, 2013 at 5:10 PM,  <dmichael@chromium.org> wrote:
>
> https://codereview.chromium.org/13866034/diff/1/base/logging.cc
> File base/logging.cc (right):
>
> https://codereview.chromium.org/13866034/diff/1/base/logging.cc#newcode353
> base/logging.cc:353: g_dcheck_state = dcheck_state;
> Is it worth checking that LoggingDestination is either LOG_NONE or
> LOG_ONLY_TO_SYSTEM_DEBUG_LOG for NaCl, since the file stuff will crap
> out later?
>

Good call.
Done.

>
https://codereview.chromium.org/13866034/diff/1/chrome/browser/nacl_host/nacl...
> File chrome/browser/nacl_host/nacl_process_host.cc (right):
>
>
https://codereview.chromium.org/13866034/diff/1/chrome/browser/nacl_host/nacl...
> chrome/browser/nacl_host/nacl_process_host.cc:850: std::string v_flag =
> cmdline->GetSwitchValueASCII(switches::kV);
> Is it worth doing:
> std::string flagWhitelist[] = [switches::kV, switches::kVModule];
> for (size_t i = 0; i < arraysize(flagWhitelist); ++i) {
>   //...
> }
> ?
>
> Feels like we might end up adding more.

Done.

>
>
https://codereview.chromium.org/13866034/diff/1/ppapi/proxy/plugin_main_nacl.cc
> File ppapi/proxy/plugin_main_nacl.cc (right):
>
>
https://codereview.chromium.org/13866034/diff/1/ppapi/proxy/plugin_main_nacl....
> ppapi/proxy/plugin_main_nacl.cc:188: if
> (!cmdline_and_logging_initialized) {
> Does this ever happen more than once? If it really does, we should
> probably have a separate message for the 1-time initialization stuff
> (which would the permissions and incognito stuff)

I don't think it can. I added a defensive DCHECK here just in case
there's a way it can happen I don't understand.

>
>
https://codereview.chromium.org/13866034/diff/1/ppapi/shared_impl/ppapi_nacl_...
> File ppapi/shared_impl/ppapi_nacl_channel_args.h (right):
>
>
https://codereview.chromium.org/13866034/diff/1/ppapi/shared_impl/ppapi_nacl_...
> ppapi/shared_impl/ppapi_nacl_channel_args.h:1: // Copyright (c) 2012 The
>
> Chromium Authors. All rights reserved.
> nit: 2013 for new files

Done.

>
> https://codereview.chromium.org/13866034/

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[teravest, 2013-04-11 16:31:42]

On Wed, Apr 10, 2013 at 5:10 PM,  <dmichael@chromium.org> wrote:
>
> https://codereview.chromium.org/13866034/diff/1/base/logging.cc
> File base/logging.cc (right):
>
> https://codereview.chromium.org/13866034/diff/1/base/logging.cc#newcode353
> base/logging.cc:353: g_dcheck_state = dcheck_state;
> Is it worth checking that LoggingDestination is either LOG_NONE or
> LOG_ONLY_TO_SYSTEM_DEBUG_LOG for NaCl, since the file stuff will crap
> out later?
>

Good call.
Done.

>
https://codereview.chromium.org/13866034/diff/1/chrome/browser/nacl_host/nacl...
> File chrome/browser/nacl_host/nacl_process_host.cc (right):
>
>
https://codereview.chromium.org/13866034/diff/1/chrome/browser/nacl_host/nacl...
> chrome/browser/nacl_host/nacl_process_host.cc:850: std::string v_flag =
> cmdline->GetSwitchValueASCII(switches::kV);
> Is it worth doing:
> std::string flagWhitelist[] = [switches::kV, switches::kVModule];
> for (size_t i = 0; i < arraysize(flagWhitelist); ++i) {
>   //...
> }
> ?
>
> Feels like we might end up adding more.

Done.

>
>
https://codereview.chromium.org/13866034/diff/1/ppapi/proxy/plugin_main_nacl.cc
> File ppapi/proxy/plugin_main_nacl.cc (right):
>
>
https://codereview.chromium.org/13866034/diff/1/ppapi/proxy/plugin_main_nacl....
> ppapi/proxy/plugin_main_nacl.cc:188: if
> (!cmdline_and_logging_initialized) {
> Does this ever happen more than once? If it really does, we should
> probably have a separate message for the 1-time initialization stuff
> (which would the permissions and incognito stuff)

I don't think it can. I added a defensive DCHECK here just in case
there's a way it can happen I don't understand.

>
>
https://codereview.chromium.org/13866034/diff/1/ppapi/shared_impl/ppapi_nacl_...
> File ppapi/shared_impl/ppapi_nacl_channel_args.h (right):
>
>
https://codereview.chromium.org/13866034/diff/1/ppapi/shared_impl/ppapi_nacl_...
> ppapi/shared_impl/ppapi_nacl_channel_args.h:1: // Copyright (c) 2012 The
>
> Chromium Authors. All rights reserved.
> nit: 2013 for new files

Done.

>
> https://codereview.chromium.org/13866034/

[dmichael (off chromium), 2013-04-11 16:39:06]

lgtm

+bbudge, since he knows that part best, to make sure we're not doing anything
crazy.

[dmichael (off chromium), 2013-04-11 16:39:53]

Also +brettw for base/ OWNERS

[bbudge, 2013-04-11 17:14:45]

It's great that you're adding this - it would have been very helpful when we
were debugging apps for the proxy switch.

I don't think we want to set up the CommandLine and logging stuff on creating a
NaCl renderer channel though. While we only do that once per process right now,
the code is designed to someday allow multiple renderer channels, and it seems
wrong to keep re-initializing this.

My first thought is to pass this information via the nacl::NaClStartParams
structure, but this would require somehow plumbing the command line flags
through NaCl startup code (see chrome/nacl/nacl_listener.cc) so it could be
picked up in our PpapiPluginMain in plugin_main_nacl.cc.

[dmichael (off chromium), 2013-04-11 17:30:22]

On Thu, Apr 11, 2013 at 11:14 AM, <bbudge@chromium.org> wrote:

> It's great that you're adding this - it would have been very helpful when
> we
> were debugging apps for the proxy switch.
>
> I don't think we want to set up the CommandLine and logging stuff on
> creating a
> NaCl renderer channel though. While we only do that once per process right
> now,
> the code is designed to someday allow multiple renderer channels, and it
> seems
> wrong to keep re-initializing this.
>
We really shouldn't be doing permissions and "off the record" in that
message then either, right?

Maybe a 2nd message would suffice?


>
> My first thought is to pass this information via the nacl::NaClStartParams
> structure, but this would require somehow plumbing the command line flags
> through NaCl startup code (see chrome/nacl/nacl_listener.cc) so it could be
> picked up in our PpapiPluginMain in plugin_main_nacl.cc.
>
We thought about that, but dismissed it mostly because it would be harder
:-)

Maybe it's worth looking into.


>
>
https://codereview.chromium.**org/13866034/<https://codereview.chromium.org/1...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[dmichael (off chromium), 2013-04-11 17:30:23]

On Thu, Apr 11, 2013 at 11:14 AM, <bbudge@chromium.org> wrote:

> It's great that you're adding this - it would have been very helpful when
> we
> were debugging apps for the proxy switch.
>
> I don't think we want to set up the CommandLine and logging stuff on
> creating a
> NaCl renderer channel though. While we only do that once per process right
> now,
> the code is designed to someday allow multiple renderer channels, and it
> seems
> wrong to keep re-initializing this.
>
We really shouldn't be doing permissions and "off the record" in that
message then either, right?

Maybe a 2nd message would suffice?


>
> My first thought is to pass this information via the nacl::NaClStartParams
> structure, but this would require somehow plumbing the command line flags
> through NaCl startup code (see chrome/nacl/nacl_listener.cc) so it could be
> picked up in our PpapiPluginMain in plugin_main_nacl.cc.
>
We thought about that, but dismissed it mostly because it would be harder
:-)

Maybe it's worth looking into.


>
>
https://codereview.chromium.**org/13866034/<https://codereview.chromium.org/1...
>

[bbudge, 2013-04-11 17:43:49]

It *is* hard to imagine that 'permissions' and 'off the record' could be
per-channel, but each PluginDispatcher *could* have different values.
Permissions are really checked in the browser process, and 'off the record' just
affects behavior.

I'm not sure about the difficulty of passing them as start up parameters. It
does mean working in less familiar NaCl sel_ldr code.

[bbudge, 2013-04-11 18:22:56]

On 2013/04/11 17:43:49, bbudge1 wrote:
> It *is* hard to imagine that 'permissions' and 'off the record' could be
> per-channel, but each PluginDispatcher *could* have different values.
> Permissions are really checked in the browser process, and 'off the record'
just
> affects behavior.
> 
> I'm not sure about the difficulty of passing them as start up parameters. It
> does mean working in less familiar NaCl sel_ldr code.

I talked with some NaCl folks and it doesn't sound easy to hook into the IRT
argc and argv stuff. It's OK with me if we take this approach, perhaps changing
the DCHECK to an if so we don't perform startup stuff more than once.
LGTM

[brettw, 2013-04-11 22:42:09]

lgtm

https://codereview.chromium.org/13866034/diff/8001/ppapi/proxy/plugin_main_na...
File ppapi/proxy/plugin_main_nacl.cc (right):

https://codereview.chromium.org/13866034/diff/8001/ppapi/proxy/plugin_main_na...
ppapi/proxy/plugin_main_nacl.cc:182: static bool create_channel_ran = false;
I'd move this static to be inside the function you need it in (I think this
avoids exporting it form the .o file, etc.).

[teravest, 2013-04-15 17:47:51]

+jschuh for ppapi/proxy/ppapi_messages.h

This change modifies PpapiMsg_CreateNaClChannel so that it also contains command
line arguments passed to chrome.

This is so that we can pass the "v" and "vmodule" flags to the untrusted nacl
process, so that VLOG operations will work there.

[teravest, 2013-04-17 22:23:53]

-jschuh, +jln for ppapi/proxy/ppapi_messages.h

This change modifies PpapiMsg_CreateNaClChannel so that it also contains command
line arguments passed to chrome.

This is so that we can pass the "v" and "vmodule" flags to the untrusted nacl
process, so that VLOG operations will work there.

[jln (very slow on Chromium), 2013-04-18 22:44:22]

lgtm

https://chromiumcodereview.appspot.com/13866034/diff/21001/ppapi/proxy/ppapi_...
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/13866034/diff/21001/ppapi/proxy/ppapi_...
ppapi/proxy/ppapi_messages.h:299: IPC_STRUCT_TRAITS_MEMBER(switch_names)
I like that you're having a whitelist for the switches. Do you want to point out
to the code in nacl_process_host.cc where the whitelist is implemented in a
comment to make reviewing easier?

[teravest, 2013-04-19 14:53:27]

On Thu, Apr 18, 2013 at 4:44 PM,  <jln@chromium.org> wrote:
> lgtm
>
>
>
https://chromiumcodereview.appspot.com/13866034/diff/21001/ppapi/proxy/ppapi_...
> File ppapi/proxy/ppapi_messages.h (right):
>
>
https://chromiumcodereview.appspot.com/13866034/diff/21001/ppapi/proxy/ppapi_...
> ppapi/proxy/ppapi_messages.h:299: IPC_STRUCT_TRAITS_MEMBER(switch_names)
> I like that you're having a whitelist for the switches. Do you want to
> point out to the code in nacl_process_host.cc where the whitelist is
> implemented in a comment to make reviewing easier?

Good idea. Done.

>
> https://chromiumcodereview.appspot.com/13866034/

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[teravest, 2013-04-19 14:53:31]

On Thu, Apr 18, 2013 at 4:44 PM,  <jln@chromium.org> wrote:
> lgtm
>
>
>
https://chromiumcodereview.appspot.com/13866034/diff/21001/ppapi/proxy/ppapi_...
> File ppapi/proxy/ppapi_messages.h (right):
>
>
https://chromiumcodereview.appspot.com/13866034/diff/21001/ppapi/proxy/ppapi_...
> ppapi/proxy/ppapi_messages.h:299: IPC_STRUCT_TRAITS_MEMBER(switch_names)
> I like that you're having a whitelist for the switches. Do you want to
> point out to the code in nacl_process_host.cc where the whitelist is
> implemented in a comment to make reviewing easier?

Good idea. Done.

>
> https://chromiumcodereview.appspot.com/13866034/

[commit-bot: I haz the power, 2013-04-19 19:43:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/13866034/30001

[teravest, 2013-04-22 17:36:19]

Committed patchset #7 manually as r195541 (presubmit successful).