Support non-SFI mode in NaCl manifest file.

Support non-SFI mode in NaCl manifest file.

Currently, the flag to enable non-SFI mode is checked just before launching the plugin in the browser process. However, even if the flag is set, we may need to run the plugin in SFI mode.
For example, trivially, if the plugin provides only SFI-mode binary.
To handle such a case, this CL adds entries to NaCl manifest file so that a plugin developer can declare if their plugin provides SFI-mode/non-SFI mode binaries.
In summary, NaCl works in non-SFI mode if;
1) --enable-nacl-nonsfi-mode is set to true, and
2) the plugin provides the binary for non-SFI mode.
So, some checks are moved from the browser to the renderer.

We need similar, but slightly different, a flag for non-SFI mode. Here is the naming rule:
1) enable_nonsfi_mode -> If non-SFI mode is enabled on the browser.
  (In more precise, if it is running on supported platform, and --enable-nacl-nonsfi-mode is set)
2) uses_nonsfi_mode -> If the specified plugin should run on non-SFI mode.
  This happens when the plugin provides the binary for non-SFI mode, and --enable-nacl-nonsfi-mode
  is set.

Note that, in later CLs, we split non-SFI mode nacl_helper from the current nacl_helper with linking it to the newlib.
Then, the uses_nonsfi_mode will become a flag to decide which helper the host should talk to.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3734
TEST=Ran trybot.

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

[hidehiko, 2014-02-27 07:14:48]

Dave, could you review ppapi/... files?
Julien, could you review *_messages.h files?
Mark, could you review all over this CL?

Thank you for your help in advance!
- hidehiko

https://codereview.chromium.org/177113009/diff/20001/ppapi/thunk/ppb_display_...
File ppapi/thunk/ppb_display_color_profile_private_thunk.cc (right):

https://codereview.chromium.org/177113009/diff/20001/ppapi/thunk/ppb_display_...
ppapi/thunk/ppb_display_color_profile_private_thunk.cc:52: 4) <<
"PPB_DisplayColorProfile_Private::RegisterColorProfileChangeCallback()";
This is generated by ppapi/generators/generator.py, but is this necessary to be
included in this CL? Or should I manually revert this file?

[Mark Seaborn, 2014-02-27 17:10:21]

https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:243: bool enable_nonsfi,
Can you rename this to "use_nonsfi_mode", so that:

* "enable_nonsfi_mode" always means "--enable-non-sfi-mode" was passed to the
browser.
* "use_nonsfi_mode" means the renderer or NMF file has requested using non-SFI
mode for this particular process.

https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:760: params.enable_nonsfi_mode =
enable_nonsfi_;
I think this means the renderer (if compromised) can enable non-SFI mode even if
"--enable-non-sfi-mode" wasn't passed to the browser.  That's undesirable for
security.  Can you make sure the browser doesn't let the renderer enable this if
the option wasn't passed?

https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.h (right):

https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.h:210: bool enable_nonsfi_;
Nit: can we call this "enable_nonsfi_mode_"?

https://codereview.chromium.org/177113009/diff/20001/components/nacl/common/n...
File components/nacl/common/nacl_host_messages.h (right):

https://codereview.chromium.org/177113009/diff/20001/components/nacl/common/n...
components/nacl/common/nacl_host_messages.h:30:
IPC_STRUCT_TRAITS_MEMBER(enable_nonsfi)
Can we call this "enable_nonsfi_mode", to match the existing option and
variable?  Same for the various function arguments.

https://codereview.chromium.org/177113009/diff/20001/ppapi/api/private/ppb_na...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/177113009/diff/20001/ppapi/api/private/ppb_na...
ppapi/api/private/ppb_nacl_private.idl:200: /* Return the true if the non-SFI
mode is enabled. */
"Return whether..."

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (right):

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:496: bool*
nonsfi_enabled,
This is very close to "nonsfi_enabled_", so it would be easy to mix the two up. 
How about renaming this to "use_nonsfi_mode", to distinguish between the mode
being enabled in the browser and also being used in the NMF file?  (See other
comment.)

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:538: if (nonsfi_enabled)
It might be a little simpler to require the caller to pass a valid pointer than
have a NULL check.

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:498: // In non-SFI
mode, open_resource() has not yet been supported, so we
"is not yet supported"

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:501: // Note that
there is on going work to replace SRPC by Chrome IPC (not only
You might link to the issue here...

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:503: // Chrome IPC for
open_resource() after the refacrogin is done.
"refactoring"

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/service_runtime.h (left):

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.h:213: bool
should_report_uma,
Renaming this should be commented in the commit message (or fixed separately)...

https://codereview.chromium.org/177113009/diff/20001/ppapi/thunk/ppb_display_...
File ppapi/thunk/ppb_display_color_profile_private_thunk.cc (right):

https://codereview.chromium.org/177113009/diff/20001/ppapi/thunk/ppb_display_...
ppapi/thunk/ppb_display_color_profile_private_thunk.cc:52: 4) <<
"PPB_DisplayColorProfile_Private::RegisterColorProfileChangeCallback()";
On 2014/02/27 07:14:49, hidehiko wrote:
> This is generated by ppapi/generators/generator.py, but is this necessary
> to be included in this CL? Or should I manually revert this file?

I think it would be cleaner to revert this file, but I'll leave it up to you.

[hidehiko, 2014-02-28 06:41:54]

Thank you for your review. PTAL?

https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:243: bool enable_nonsfi,
On 2014/02/27 17:10:22, Mark Seaborn wrote:
> Can you rename this to "use_nonsfi_mode", so that:
> 
> * "enable_nonsfi_mode" always means "--enable-non-sfi-mode" was passed to the
> browser.
> * "use_nonsfi_mode" means the renderer or NMF file has requested using non-SFI
> mode for this particular process.

Done.

https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:760: params.enable_nonsfi_mode =
enable_nonsfi_;
On 2014/02/27 17:10:22, Mark Seaborn wrote:
> I think this means the renderer (if compromised) can enable non-SFI mode even
if
> "--enable-non-sfi-mode" wasn't passed to the browser.  That's undesirable for
> security.  Can you make sure the browser doesn't let the renderer enable this
if
> the option wasn't passed?

As we chatted, the binary won't be compatible, so we cannot run under SFI mode.
Instead, I added a sanity-check if the non-sfi mode is enabled actually or not.

https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.h (right):

https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.h:210: bool enable_nonsfi_;
On 2014/02/27 17:10:22, Mark Seaborn wrote:
> Nit: can we call this "enable_nonsfi_mode_"?

Changed to uses_nonsfi_mode_.

https://codereview.chromium.org/177113009/diff/20001/components/nacl/common/n...
File components/nacl/common/nacl_host_messages.h (right):

https://codereview.chromium.org/177113009/diff/20001/components/nacl/common/n...
components/nacl/common/nacl_host_messages.h:30:
IPC_STRUCT_TRAITS_MEMBER(enable_nonsfi)
On 2014/02/27 17:10:22, Mark Seaborn wrote:
> Can we call this "enable_nonsfi_mode", to match the existing option and
> variable?  Same for the various function arguments.

Renamed to uses_nonsfi_mode

https://codereview.chromium.org/177113009/diff/20001/ppapi/api/private/ppb_na...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/177113009/diff/20001/ppapi/api/private/ppb_na...
ppapi/api/private/ppb_nacl_private.idl:200: /* Return the true if the non-SFI
mode is enabled. */
On 2014/02/27 17:10:22, Mark Seaborn wrote:
> "Return whether..."

Done.

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (right):

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:496: bool*
nonsfi_enabled,
On 2014/02/27 17:10:22, Mark Seaborn wrote:
> This is very close to "nonsfi_enabled_", so it would be easy to mix the two
up. 
> How about renaming this to "use_nonsfi_mode", to distinguish between the mode
> being enabled in the browser and also being used in the NMF file?  (See other
> comment.)

Done.

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:538: if (nonsfi_enabled)
On 2014/02/27 17:10:22, Mark Seaborn wrote:
> It might be a little simpler to require the caller to pass a valid pointer
than
> have a NULL check.

Done.

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:498: // In non-SFI
mode, open_resource() has not yet been supported, so we
On 2014/02/27 17:10:22, Mark Seaborn wrote:
> "is not yet supported"

Done.

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:501: // Note that
there is on going work to replace SRPC by Chrome IPC (not only
On 2014/02/27 17:10:22, Mark Seaborn wrote:
> You might link to the issue here...

Done.

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:503: // Chrome IPC for
open_resource() after the refacrogin is done.
On 2014/02/27 17:10:22, Mark Seaborn wrote:
> "refactoring"

Oops. Done.

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/service_runtime.h (left):

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.h:213: bool
should_report_uma,
On 2014/02/27 17:10:22, Mark Seaborn wrote:
> Renaming this should be commented in the commit message (or fixed
separately)...

Ok, reverted.

https://codereview.chromium.org/177113009/diff/20001/ppapi/thunk/ppb_display_...
File ppapi/thunk/ppb_display_color_profile_private_thunk.cc (right):

https://codereview.chromium.org/177113009/diff/20001/ppapi/thunk/ppb_display_...
ppapi/thunk/ppb_display_color_profile_private_thunk.cc:52: 4) <<
"PPB_DisplayColorProfile_Private::RegisterColorProfileChangeCallback()";
On 2014/02/27 17:10:22, Mark Seaborn wrote:
> On 2014/02/27 07:14:49, hidehiko wrote:
> > This is generated by ppapi/generators/generator.py, but is this necessary
> > to be included in this CL? Or should I manually revert this file?
> 
> I think it would be cleaner to revert this file, but I'll leave it up to you.

I see. Just reverted.

[hidehiko, 2014-03-04 01:15:10]

Hi, friendly ping?



On 2014/02/28 06:41:54, hidehiko wrote:
> Thank you for your review. PTAL?
> 
>
https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
> File components/nacl/browser/nacl_process_host.cc (right):
> 
>
https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
> components/nacl/browser/nacl_process_host.cc:243: bool enable_nonsfi,
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > Can you rename this to "use_nonsfi_mode", so that:
> > 
> > * "enable_nonsfi_mode" always means "--enable-non-sfi-mode" was passed to
the
> > browser.
> > * "use_nonsfi_mode" means the renderer or NMF file has requested using
non-SFI
> > mode for this particular process.
> 
> Done.
> 
>
https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
> components/nacl/browser/nacl_process_host.cc:760: params.enable_nonsfi_mode =
> enable_nonsfi_;
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > I think this means the renderer (if compromised) can enable non-SFI mode
even
> if
> > "--enable-non-sfi-mode" wasn't passed to the browser.  That's undesirable
for
> > security.  Can you make sure the browser doesn't let the renderer enable
this
> if
> > the option wasn't passed?
> 
> As we chatted, the binary won't be compatible, so we cannot run under SFI
mode.
> Instead, I added a sanity-check if the non-sfi mode is enabled actually or
not.
> 
>
https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
> File components/nacl/browser/nacl_process_host.h (right):
> 
>
https://codereview.chromium.org/177113009/diff/20001/components/nacl/browser/...
> components/nacl/browser/nacl_process_host.h:210: bool enable_nonsfi_;
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > Nit: can we call this "enable_nonsfi_mode_"?
> 
> Changed to uses_nonsfi_mode_.
> 
>
https://codereview.chromium.org/177113009/diff/20001/components/nacl/common/n...
> File components/nacl/common/nacl_host_messages.h (right):
> 
>
https://codereview.chromium.org/177113009/diff/20001/components/nacl/common/n...
> components/nacl/common/nacl_host_messages.h:30:
> IPC_STRUCT_TRAITS_MEMBER(enable_nonsfi)
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > Can we call this "enable_nonsfi_mode", to match the existing option and
> > variable?  Same for the various function arguments.
> 
> Renamed to uses_nonsfi_mode
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/api/private/ppb_na...
> File ppapi/api/private/ppb_nacl_private.idl (right):
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/api/private/ppb_na...
> ppapi/api/private/ppb_nacl_private.idl:200: /* Return the true if the non-SFI
> mode is enabled. */
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > "Return whether..."
> 
> Done.
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
> File ppapi/native_client/src/trusted/plugin/json_manifest.cc (right):
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/json_manifest.cc:496: bool*
> nonsfi_enabled,
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > This is very close to "nonsfi_enabled_", so it would be easy to mix the two
> up. 
> > How about renaming this to "use_nonsfi_mode", to distinguish between the
mode
> > being enabled in the browser and also being used in the NMF file?  (See
other
> > comment.)
> 
> Done.
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/json_manifest.cc:538: if
(nonsfi_enabled)
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > It might be a little simpler to require the caller to pass a valid pointer
> than
> > have a NULL check.
> 
> Done.
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
> File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/service_runtime.cc:498: // In non-SFI
> mode, open_resource() has not yet been supported, so we
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > "is not yet supported"
> 
> Done.
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/service_runtime.cc:501: // Note that
> there is on going work to replace SRPC by Chrome IPC (not only
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > You might link to the issue here...
> 
> Done.
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/service_runtime.cc:503: // Chrome IPC
for
> open_resource() after the refacrogin is done.
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > "refactoring"
> 
> Oops. Done.
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
> File ppapi/native_client/src/trusted/plugin/service_runtime.h (left):
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/service_runtime.h:213: bool
> should_report_uma,
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > Renaming this should be commented in the commit message (or fixed
> separately)...
> 
> Ok, reverted.
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/thunk/ppb_display_...
> File ppapi/thunk/ppb_display_color_profile_private_thunk.cc (right):
> 
>
https://codereview.chromium.org/177113009/diff/20001/ppapi/thunk/ppb_display_...
> ppapi/thunk/ppb_display_color_profile_private_thunk.cc:52: 4) <<
> "PPB_DisplayColorProfile_Private::RegisterColorProfileChangeCallback()";
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > On 2014/02/27 07:14:49, hidehiko wrote:
> > > This is generated by ppapi/generators/generator.py, but is this necessary
> > > to be included in this CL? Or should I manually revert this file?
> > 
> > I think it would be cleaner to revert this file, but I'll leave it up to
you.
> 
> I see. Just reverted.

[Mark Seaborn, 2014-03-04 04:03:32]

+jvoung for the manifest reading part.

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/service_runtime.h (left):

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.h:213: bool
should_report_uma,
On 2014/02/28 06:41:54, hidehiko wrote:
> On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > Renaming this should be commented in the commit message (or fixed
> separately)...
> 
> Ok, reverted.

Would you mind preparing a separate change to fix this?  It's really helpful to
fix mistakes like this when you find them, but generally best to do it in a
separate change for clarity.

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (left):

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:499: if ((sandbox_isa_
== kPortableKey) || (has_portable && !has_isa)) {
The existing logic seems convoluted, so I wonder if this can be simplified?  Why
doesn't this do:

if (dictionary.isMember(sandbox_isa_)) {
  chosen_isa = sandbox_isa_; // may be kPortableKey
} else if (dictionary.isMember(kPortableKey)) {
  chosen_isa = kPortableKey;
}

Then the logical extension for non-SFI mode is:

bool nonsfi_isa = GetNonSFIKey(sandbox_isa_);
if (sandbox_isa_ != kPortableKey &&
    nonsfi_enabled &&
    dictionary.isMember(nonsfi_isa)) {
  chosen_isa = nonsfi_isa;
} else if (dictionary.isMember(sandbox_isa_)) {
  chosen_isa = sandbox_isa_;  // May be kPortableKey
} else if (dictionary.isMember(kPortableKey)) {
  chosen_isa = kPortableKey;
}

This question is directed at Jan, who I'd like to have sign off on the
src/trusted/plugin/ change.

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (right):

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:516: // True iff the
non-SFI mode is enabled, and the nmf as the -nonsfi entry.
"as" -> "has".  But this comment echoes the code, so you could drop it.

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:518: nonsfi_enabled_ &&
dictionary.isMember(GetNonSFIKey(sandbox_isa_));
If the manifest has "portable-nonsfi", would this return true?

[hidehiko, 2014-03-04 15:06:33]

Thank you for your review, Mark.
Jan, Julian, Dave, could you kindly take a look, too?

Thanks,
- hidehiko

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/service_runtime.h (left):

https://codereview.chromium.org/177113009/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.h:213: bool
should_report_uma,
Sure, done. crrev.com/177353007.

On 2014/03/04 04:03:32, Mark Seaborn wrote:
> On 2014/02/28 06:41:54, hidehiko wrote:
> > On 2014/02/27 17:10:22, Mark Seaborn wrote:
> > > Renaming this should be commented in the commit message (or fixed
> > separately)...
> > 
> > Ok, reverted.
> 
> Would you mind preparing a separate change to fix this?  It's really helpful
to
> fix mistakes like this when you find them, but generally best to do it in a
> separate change for clarity.

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (left):

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:499: if ((sandbox_isa_
== kPortableKey) || (has_portable && !has_isa)) {
Thank you for comment.
Jan, WDYT?

I'll update the code after Jan's reply.

On 2014/03/04 04:03:32, Mark Seaborn wrote:
> The existing logic seems convoluted, so I wonder if this can be simplified? 
Why
> doesn't this do:
> 
> if (dictionary.isMember(sandbox_isa_)) {
>   chosen_isa = sandbox_isa_; // may be kPortableKey
> } else if (dictionary.isMember(kPortableKey)) {
>   chosen_isa = kPortableKey;
> }
> 
> Then the logical extension for non-SFI mode is:
> 
> bool nonsfi_isa = GetNonSFIKey(sandbox_isa_);
> if (sandbox_isa_ != kPortableKey &&
>     nonsfi_enabled &&
>     dictionary.isMember(nonsfi_isa)) {
>   chosen_isa = nonsfi_isa;
> } else if (dictionary.isMember(sandbox_isa_)) {
>   chosen_isa = sandbox_isa_;  // May be kPortableKey
> } else if (dictionary.isMember(kPortableKey)) {
>   chosen_isa = kPortableKey;
> }
> 
> This question is directed at Jan, who I'd like to have sign off on the
> src/trusted/plugin/ change.

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (right):

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:516: // True iff the
non-SFI mode is enabled, and the nmf as the -nonsfi entry.
On 2014/03/04 04:03:32, Mark Seaborn wrote:
> "as" -> "has".  But this comment echoes the code, so you could drop it.

Acknowledged. The code will gone.

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:518: nonsfi_enabled_ &&
dictionary.isMember(GetNonSFIKey(sandbox_isa_));
On 2014/03/04 04:03:32, Mark Seaborn wrote:
> If the manifest has "portable-nonsfi", would this return true?

Acknowledged. The code will gone.

[jvoung (off chromium), 2014-03-04 16:24:17]

https://codereview.chromium.org/177113009/diff/60001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/177113009/diff/60001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:311: if (!sender->Send(new
NaClHostMsg_NaClIsNonSFIEnabled(&enabled))) {
Btw, is this because the flag is not passed from the browser -> renderer? Could
you just have it be passed along from browser -> renderer?

AppendExtraCommandLineSwitches in 
chrome/browser/chrome_content_browser_client.cc

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (left):

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:499: if ((sandbox_isa_
== kPortableKey) || (has_portable && !has_isa)) {
On 2014/03/04 04:03:32, Mark Seaborn wrote:
> The existing logic seems convoluted, so I wonder if this can be simplified? 
Why
> doesn't this do:
> 
> if (dictionary.isMember(sandbox_isa_)) {
>   chosen_isa = sandbox_isa_; // may be kPortableKey
> } else if (dictionary.isMember(kPortableKey)) {
>   chosen_isa = kPortableKey;
> }
> 
> Then the logical extension for non-SFI mode is:
> 
> bool nonsfi_isa = GetNonSFIKey(sandbox_isa_);
> if (sandbox_isa_ != kPortableKey &&
>     nonsfi_enabled &&
>     dictionary.isMember(nonsfi_isa)) {
>   chosen_isa = nonsfi_isa;
> } else if (dictionary.isMember(sandbox_isa_)) {
>   chosen_isa = sandbox_isa_;  // May be kPortableKey
> } else if (dictionary.isMember(kPortableKey)) {
>   chosen_isa = kPortableKey;
> }
> 
> This question is directed at Jan, who I'd like to have sign off on the
> src/trusted/plugin/ change.

That looks okay. The main constraints are:

* PNaCl uses portable, and never uses anything else.
* NaCl will first try the sandbox isa, but falls back to portable if available.

I think the code was written to follow the structure of IsValidISADictionary(),
where it checks if sandbox_isa_ == kPortableKey first, and then branches...
except that it then deviated from the structure and folded in the case where
sandbox_isa_  != kPortableKey w/ "has_portable && !has_isa"

Anyway, Mark's suggestion is okay. Or if you want to make it more clear that
PNaCl can never use anything other than kPortableKey, you could do something
like:


if (sandbox_isa_ == kPortableKey) {
  chosen_isa = kPortableKey;
} else {
  if (nonsfi_enabled && dictionary.isMember(nonsfi_isa)) {
    chosen_isa = nonsfi_isa;
  } else if (dictionary.isMember(sandbox_isa_)) {
    chosen_isa = sandbox_isa_;
  } else if (dictionary.isMember(kPortableKey)) {
    chosen_isa = kPortableKey;
  } else {
    NOTREACHED();
  }
}

[Mark Seaborn, 2014-03-04 16:33:59]

https://codereview.chromium.org/177113009/diff/60001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/177113009/diff/60001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:311: if (!sender->Send(new
NaClHostMsg_NaClIsNonSFIEnabled(&enabled))) {
On 2014/03/04 16:24:18, jvoung (cr) wrote:
> Btw, is this because the flag is not passed from the browser -> renderer?
Could
> you just have it be passed along from browser -> renderer?
> 
> AppendExtraCommandLineSwitches in 
> chrome/browser/chrome_content_browser_client.cc

Command line arguments don't work well for the NaCl trusted plugin because it's
in a DLL.  It can't/shouldn't use base/ for parsing command line arguments.

[jvoung (off chromium), 2014-03-04 16:39:42]

https://codereview.chromium.org/177113009/diff/60001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/177113009/diff/60001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:311: if (!sender->Send(new
NaClHostMsg_NaClIsNonSFIEnabled(&enabled))) {
On 2014/03/04 16:34:00, Mark Seaborn wrote:
> On 2014/03/04 16:24:18, jvoung (cr) wrote:
> > Btw, is this because the flag is not passed from the browser -> renderer?
> Could
> > you just have it be passed along from browser -> renderer?
> > 
> > AppendExtraCommandLineSwitches in 
> > chrome/browser/chrome_content_browser_client.cc
> 
> Command line arguments don't work well for the NaCl trusted plugin because
it's
> in a DLL.  It can't/shouldn't use base/ for parsing command line arguments.

I meant that this code could then use base/ for parsing command line arguments,
like Are3DInterfacesDisabled(), instead of sending a message (not the code in
src/trusted/plugin).

[Mark Seaborn, 2014-03-04 17:29:06]

https://codereview.chromium.org/177113009/diff/60001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/177113009/diff/60001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:311: if (!sender->Send(new
NaClHostMsg_NaClIsNonSFIEnabled(&enabled))) {
On 2014/03/04 16:39:42, jvoung (cr) wrote:
> On 2014/03/04 16:34:00, Mark Seaborn wrote:
> > On 2014/03/04 16:24:18, jvoung (cr) wrote:
> > > Btw, is this because the flag is not passed from the browser -> renderer?
> > Could
> > > you just have it be passed along from browser -> renderer?
> > > 
> > > AppendExtraCommandLineSwitches in 
> > > chrome/browser/chrome_content_browser_client.cc
> > 
> > Command line arguments don't work well for the NaCl trusted plugin because
> it's
> > in a DLL.  It can't/shouldn't use base/ for parsing command line arguments.
> 
> I meant that this code could then use base/ for parsing command line
arguments,
> like Are3DInterfacesDisabled(), instead of sending a message (not the code in
> src/trusted/plugin).

Oh right, of course. :-)  Yes, I agree it would make more sense to inspect a
command line argument here, and not have the NaClHostMsg_NaClIsNonSFIEnabled
IPC.

[hidehiko, 2014-03-05 08:15:32]

Thank you for review. PTAL?

https://codereview.chromium.org/177113009/diff/60001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/177113009/diff/60001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:311: if (!sender->Send(new
NaClHostMsg_NaClIsNonSFIEnabled(&enabled))) {
On 2014/03/04 17:29:07, Mark Seaborn wrote:
> On 2014/03/04 16:39:42, jvoung (cr) wrote:
> > On 2014/03/04 16:34:00, Mark Seaborn wrote:
> > > On 2014/03/04 16:24:18, jvoung (cr) wrote:
> > > > Btw, is this because the flag is not passed from the browser ->
renderer?
> > > Could
> > > > you just have it be passed along from browser -> renderer?
> > > > 
> > > > AppendExtraCommandLineSwitches in 
> > > > chrome/browser/chrome_content_browser_client.cc
> > > 
> > > Command line arguments don't work well for the NaCl trusted plugin because
> > it's
> > > in a DLL.  It can't/shouldn't use base/ for parsing command line
arguments.
> > 
> > I meant that this code could then use base/ for parsing command line
> arguments,
> > like Are3DInterfacesDisabled(), instead of sending a message (not the code
in
> > src/trusted/plugin).
> 
> Oh right, of course. :-)  Yes, I agree it would make more sense to inspect a
> command line argument here, and not have the NaClHostMsg_NaClIsNonSFIEnabled
> IPC.

Great to know. Done.

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (left):

https://codereview.chromium.org/177113009/diff/60001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:499: if ((sandbox_isa_
== kPortableKey) || (has_portable && !has_isa)) {
Thank you for comment. Done in Jan's way.

On 2014/03/04 16:24:18, jvoung (cr) wrote:
> On 2014/03/04 04:03:32, Mark Seaborn wrote:
> > The existing logic seems convoluted, so I wonder if this can be simplified? 
> Why
> > doesn't this do:
> > 
> > if (dictionary.isMember(sandbox_isa_)) {
> >   chosen_isa = sandbox_isa_; // may be kPortableKey
> > } else if (dictionary.isMember(kPortableKey)) {
> >   chosen_isa = kPortableKey;
> > }
> > 
> > Then the logical extension for non-SFI mode is:
> > 
> > bool nonsfi_isa = GetNonSFIKey(sandbox_isa_);
> > if (sandbox_isa_ != kPortableKey &&
> >     nonsfi_enabled &&
> >     dictionary.isMember(nonsfi_isa)) {
> >   chosen_isa = nonsfi_isa;
> > } else if (dictionary.isMember(sandbox_isa_)) {
> >   chosen_isa = sandbox_isa_;  // May be kPortableKey
> > } else if (dictionary.isMember(kPortableKey)) {
> >   chosen_isa = kPortableKey;
> > }
> > 
> > This question is directed at Jan, who I'd like to have sign off on the
> > src/trusted/plugin/ change.
> 
> That looks okay. The main constraints are:
> 
> * PNaCl uses portable, and never uses anything else.
> * NaCl will first try the sandbox isa, but falls back to portable if
available.
> 
> I think the code was written to follow the structure of
IsValidISADictionary(),
> where it checks if sandbox_isa_ == kPortableKey first, and then branches...
> except that it then deviated from the structure and folded in the case where
> sandbox_isa_  != kPortableKey w/ "has_portable && !has_isa"
> 
> Anyway, Mark's suggestion is okay. Or if you want to make it more clear that
> PNaCl can never use anything other than kPortableKey, you could do something
> like:
> 
> 
> if (sandbox_isa_ == kPortableKey) {
>   chosen_isa = kPortableKey;
> } else {
>   if (nonsfi_enabled && dictionary.isMember(nonsfi_isa)) {
>     chosen_isa = nonsfi_isa;
>   } else if (dictionary.isMember(sandbox_isa_)) {
>     chosen_isa = sandbox_isa_;
>   } else if (dictionary.isMember(kPortableKey)) {
>     chosen_isa = kPortableKey;
>   } else {
>     NOTREACHED();
>   }
> }

[jvoung (off chromium), 2014-03-05 17:08:44]

json changes LG

[Mark Seaborn, 2014-03-05 17:44:31]

https://codereview.chromium.org/177113009/diff/90001/ppapi/api/private/ppb_na...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/177113009/diff/90001/ppapi/api/private/ppb_na...
ppapi/api/private/ppb_nacl_private.idl:200: PP_Bool IsNonSFIEnabled();
Could you name this "IsNonSFIModeEnabled" for consistency?

https://codereview.chromium.org/177113009/diff/90001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (left):

https://codereview.chromium.org/177113009/diff/90001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:494: // The call to
IsValidISADictionary() above guarantees that either
Please keep this comment in some form.  See comment below...

https://codereview.chromium.org/177113009/diff/90001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (right):

https://codereview.chromium.org/177113009/diff/90001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:524: DCHECK(false);  //
Should not reach here.
Can you add an explanation like: "because the earlier IsValidISADictionary()
call checked that the manifest covers the current architecture"

https://codereview.chromium.org/177113009/diff/90001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/177113009/diff/90001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:1058:
set_uses_nonsfi_mode(uses_nonsfi_mode);
Why not:
  uses_nonsfi_mode_ = uses_nonsfi_mode;
Looks like Plugin doesn't need a method for this?

[Mark Seaborn, 2014-03-05 17:45:05]

LGTM with the changes in my previous post.

[dmichael (off chromium), 2014-03-05 18:42:15]

ppapi lgtm

[hidehiko, 2014-03-06 05:31:11]

Thank you for review!
Julien, could you take a look at messages.h changes?

https://codereview.chromium.org/177113009/diff/90001/ppapi/api/private/ppb_na...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/177113009/diff/90001/ppapi/api/private/ppb_na...
ppapi/api/private/ppb_nacl_private.idl:200: PP_Bool IsNonSFIEnabled();
On 2014/03/05 17:44:32, Mark Seaborn wrote:
> Could you name this "IsNonSFIModeEnabled" for consistency?

Done.

https://codereview.chromium.org/177113009/diff/90001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (left):

https://codereview.chromium.org/177113009/diff/90001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:494: // The call to
IsValidISADictionary() above guarantees that either
On 2014/03/05 17:44:32, Mark Seaborn wrote:
> Please keep this comment in some form.  See comment below...

Done.

https://codereview.chromium.org/177113009/diff/90001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (right):

https://codereview.chromium.org/177113009/diff/90001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:524: DCHECK(false);  //
Should not reach here.
On 2014/03/05 17:44:32, Mark Seaborn wrote:
> Can you add an explanation like: "because the earlier IsValidISADictionary()
> call checked that the manifest covers the current architecture"

Done.

https://codereview.chromium.org/177113009/diff/90001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/177113009/diff/90001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:1058:
set_uses_nonsfi_mode(uses_nonsfi_mode);
On 2014/03/05 17:44:32, Mark Seaborn wrote:
> Why not:
>   uses_nonsfi_mode_ = uses_nonsfi_mode;
> Looks like Plugin doesn't need a method for this?

Done to remove getter/setter, instead, let's access the member directly.

[jln (very slow on Chromium), 2014-03-06 22:53:26]

*_messages.h lgtm rubberstamp, based on the size of change and Mark's review.

Please, let me know if I missed a non trivial change to *_messages.h

[hidehiko, 2014-03-07 01:53:33]

Thank you for review!
Sending to CQ...

On 2014/03/06 22:53:26, jln wrote:
> *_messages.h lgtm rubberstamp, based on the size of change and Mark's review.
> 
> Please, let me know if I missed a non trivial change to *_messages.h

FYI: what change I made in *_messages.h is just renaming the fields, and all the
changes are just trivial.

[hidehiko, 2014-03-07 01:53:41]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-03-07 01:58:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/177113009/110001

[commit-bot: I haz the power, 2014-03-07 01:58:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 01:58:18]

Failed to apply patch for
ppapi/native_client/src/trusted/plugin/service_runtime.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ppapi/native_client/src/trusted/plugin/service_runtime.h
  Hunk #4 FAILED at 214.
  1 out of 5 hunks FAILED -- saving rejects to file
ppapi/native_client/src/trusted/plugin/service_runtime.h.rej

Patch:       ppapi/native_client/src/trusted/plugin/service_runtime.h
Index: ppapi/native_client/src/trusted/plugin/service_runtime.h
diff --git a/ppapi/native_client/src/trusted/plugin/service_runtime.h
b/ppapi/native_client/src/trusted/plugin/service_runtime.h
index
3094cdb310b872be72753971a021f805c3c400c5..8962ada816ef54700302caa3d6d361a18858f65d
100644
--- a/ppapi/native_client/src/trusted/plugin/service_runtime.h
+++ b/ppapi/native_client/src/trusted/plugin/service_runtime.h
@@ -51,6 +51,7 @@ struct SelLdrStartParams {
   SelLdrStartParams(const nacl::string& url,
                     bool uses_irt,
                     bool uses_ppapi,
+                    bool uses_nonsfi_mode,
                     bool enable_dev_interfaces,
                     bool enable_dyncode_syscalls,
                     bool enable_exception_handling,
@@ -58,6 +59,7 @@ struct SelLdrStartParams {
       : url(url),
         uses_irt(uses_irt),
         uses_ppapi(uses_ppapi),
+        uses_nonsfi_mode(uses_nonsfi_mode),
         enable_dev_interfaces(enable_dev_interfaces),
         enable_dyncode_syscalls(enable_dyncode_syscalls),
         enable_exception_handling(enable_exception_handling),
@@ -66,6 +68,7 @@ struct SelLdrStartParams {
   nacl::string url;
   bool uses_irt;
   bool uses_ppapi;
+  bool uses_nonsfi_mode;
   bool enable_dev_interfaces;
   bool enable_dyncode_syscalls;
   bool enable_exception_handling;
@@ -211,6 +214,7 @@ class ServiceRuntime {
   ServiceRuntime(Plugin* plugin,
                  const Manifest* manifest,
                  bool should_report_uma,
+                 bool uses_nonsfi_mode,
                  pp::CompletionCallback init_done_cb,
                  pp::CompletionCallback crash_cb);
   // The destructor terminates the sel_ldr process.
@@ -266,6 +270,7 @@ class ServiceRuntime {
   NaClSrpcChannel command_channel_;
   Plugin* plugin_;
   bool main_service_runtime_;
+  bool uses_nonsfi_mode_;
   nacl::ReverseService* reverse_service_;
   nacl::scoped_ptr<nacl::SelLdrLauncherBase> subprocess_;

[hidehiko, 2014-03-07 05:07:03]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-03-07 05:08:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/177113009/130001

[hidehiko, 2014-03-07 05:54:43]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-03-07 05:57:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/177113009/130001

[commit-bot: I haz the power, 2014-03-07 06:26:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 06:26:42]

Try jobs failed on following builders: mac_chromium_rel

[hidehiko, 2014-03-07 06:27:44]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-03-07 06:30:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/177113009/130001

[commit-bot: I haz the power, 2014-03-07 15:44:49]

Change committed as 255623