Remove LoadModule SRPC for non-SFI mode.

Remove LoadModule SRPC for non-SFI mode.

This CL removes LoadModule SRPC for non-SFI mode. Instead, non-SFI mode
starts to pass nexe file to LaunchSelLdr, which is eventually passed to
NaClListener::OnStart().

TEST=Ran browser_tests --gtest_filter=*NonSfi*:*NonSFI* locally and trybots
BUG=333950
CQ_EXTRA_TRYBOTS=tryserver.chromium:linux_rel_precise32

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

[hidehiko, 2014-06-13 04:56:50]

PTAL.

Thank you for review in advance,
- hidehiko

https://codereview.chromium.org/337463002/diff/40001/ppapi/nacl_irt/manifest_...
File ppapi/nacl_irt/manifest_service.cc (right):

https://codereview.chromium.org/337463002/diff/40001/ppapi/nacl_irt/manifest_...
ppapi/nacl_irt/manifest_service.cc:20: // IPC channel is asynchronously set up.
So, the plugin may be try to send
FYI: this is a part of crrev.com/334593004. Rebasing will remove this diff.

[teravest, 2014-06-13 20:19:08]

lgtm

Nice! I think this looks much better.

[Mark Seaborn, 2014-06-17 00:50:31]

https://codereview.chromium.org/337463002/diff/40001/components/nacl/browser/...
File components/nacl/browser/nacl_host_message_filter.cc (right):

https://codereview.chromium.org/337463002/diff/40001/components/nacl/browser/...
components/nacl/browser/nacl_host_message_filter.cc:78:
launch_params.nexe_token_lo,
I'm not sure you should be plumbing the file token values through here (yet),
for a number of reasons:

 * They are only meaningful when using validation caching, and this change only
affects Non-SFI mode.

 * I'm not sure if validation caching is hooked up yet for loading the initial
nexe.  (I'd have to ask jvoung about that.)

 * It would be suboptimal for the *browser* to pass the token values to the NaCl
process.  If the NaCl process uses validation caching, it would just turn around
and send the token values back to the browser -- see
NaClProcessMsg_ResolveFileToken -- in order to get a guaranteed-safe FD back. 
It would be more efficient for the browser to send a guaranteed-safe FD in the
first place!

If you are going to plumb them through, it seems like it would be better to use
"struct NaClFileToken" rather than passing two int values around.

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

https://codereview.chromium.org/337463002/diff/40001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.h:191: base::PlatformFile nexe_file_;
Should this use base::File instead of PlatformFile?

There have been some recent changes to do that transition:
https://src.chromium.org/viewvc/chrome?view=rev&revision=274783

base::File is auto-destroyed, I think.

https://codereview.chromium.org/337463002/diff/40001/components/nacl/common/n...
File components/nacl/common/nacl_types.cc (right):

https://codereview.chromium.org/337463002/diff/40001/components/nacl/common/n...
components/nacl/common/nacl_types.cc:28: enable_crash_throttling(false) {
This should initialise nexe_token_lo/hi too (but see other comment).

https://codereview.chromium.org/337463002/diff/40001/components/nacl/loader/n...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/337463002/diff/40001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:460:
nacl::nonsfi::MainStart(NaClDescIoFromFileInfo(nexe_file_info, O_RDONLY));
Why move this to occur earlier?  It might reduce the chances of odd startup race
conditions if we keep this at the end of the function.

https://codereview.chromium.org/337463002/diff/40001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:483: // Get rid of this.
Nit: This comment should have a "TODO" in front of it.

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

https://codereview.chromium.org/337463002/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.h:48: bool
uses_nonsfi_mode,
Could you possibly do this removal in a separate change?

[hidehiko, 2014-06-18 08:43:32]

Thank you for review. PTAL.

+cc: ncbray@, as his CL crrev.com/338353008 is conflicting with this.

https://codereview.chromium.org/337463002/diff/40001/components/nacl/browser/...
File components/nacl/browser/nacl_host_message_filter.cc (right):

https://codereview.chromium.org/337463002/diff/40001/components/nacl/browser/...
components/nacl/browser/nacl_host_message_filter.cc:78:
launch_params.nexe_token_lo,
On 2014/06/17 00:50:30, Mark Seaborn wrote:
> I'm not sure you should be plumbing the file token values through here (yet),
> for a number of reasons:
> 
>  * They are only meaningful when using validation caching, and this change
only
> affects Non-SFI mode.
> 
>  * I'm not sure if validation caching is hooked up yet for loading the initial
> nexe.  (I'd have to ask jvoung about that.)
> 
>  * It would be suboptimal for the *browser* to pass the token values to the
NaCl
> process.  If the NaCl process uses validation caching, it would just turn
around
> and send the token values back to the browser -- see
> NaClProcessMsg_ResolveFileToken -- in order to get a guaranteed-safe FD back. 
> It would be more efficient for the browser to send a guaranteed-safe FD in the
> first place!
> 
> If you are going to plumb them through, it seems like it would be better to
use
> "struct NaClFileToken" rather than passing two int values around.

Indeed. I just followed the Justin's CL. Admittedly, we don't need them for
non-SFI. Removed.

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

https://codereview.chromium.org/337463002/diff/40001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.h:191: base::PlatformFile nexe_file_;
On 2014/06/17 00:50:30, Mark Seaborn wrote:
> Should this use base::File instead of PlatformFile?
> 
> There have been some recent changes to do that transition:
> https://src.chromium.org/viewvc/chrome?view=rev&revision=274783
> 
> base::File is auto-destroyed, I think.

Done.

https://codereview.chromium.org/337463002/diff/40001/components/nacl/common/n...
File components/nacl/common/nacl_types.cc (right):

https://codereview.chromium.org/337463002/diff/40001/components/nacl/common/n...
components/nacl/common/nacl_types.cc:28: enable_crash_throttling(false) {
On 2014/06/17 00:50:31, Mark Seaborn wrote:
> This should initialise nexe_token_lo/hi too (but see other comment).

Ack. Removed token_lo,hi.

https://codereview.chromium.org/337463002/diff/40001/components/nacl/loader/n...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/337463002/diff/40001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:460:
nacl::nonsfi::MainStart(NaClDescIoFromFileInfo(nexe_file_info, O_RDONLY));
On 2014/06/17 00:50:31, Mark Seaborn wrote:
> Why move this to occur earlier?  It might reduce the chances of odd startup
race
> conditions if we keep this at the end of the function.

Moved below.

https://codereview.chromium.org/337463002/diff/40001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:483: // Get rid of this.
On 2014/06/17 00:50:31, Mark Seaborn wrote:
> Nit: This comment should have a "TODO" in front of it.

Done.

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

https://codereview.chromium.org/337463002/diff/40001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.h:48: bool
uses_nonsfi_mode,
On 2014/06/17 00:50:31, Mark Seaborn wrote:
> Could you possibly do this removal in a separate change?

Done.

[Mark Seaborn, 2014-06-18 19:53:13]

LGTM

The FD ownership logic in plugin.cc might warrant some fixes before or after
this change, or additional comments.

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

https://codereview.chromium.org/337463002/diff/70001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:461: // Do not create them.
Nit: 'them' -> 'it', since you're referring to one thing?

https://codereview.chromium.org/337463002/diff/70001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:833: // Note: nexe_file_ still
keeps the ownership at this moment, because
Is this because a base::File can't be used as a parameter in IPC messages, and
params.nexe_file is a non-auto-freed type?  It might be worth commenting that
here.

https://codereview.chromium.org/337463002/diff/70001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:834: // this params may just be
destructed before sending IPC is properly
Nit: 'destroyed' rather than 'destructed'?  Also "this params" -> "params"?

https://codereview.chromium.org/337463002/diff/70001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:904: nexe_file_.TakePlatformFile();
Wouldn't it be cleaner to do this immediately before Send()?

After Send(), this will be referring to an FD that has already been close()'d
(since you pass auto_close=True to FileDescriptor above), which is slightly
dodgy.

https://codereview.chromium.org/337463002/diff/70001/components/nacl/common/n...
File components/nacl/common/nacl_types.cc (right):

https://codereview.chromium.org/337463002/diff/70001/components/nacl/common/n...
components/nacl/common/nacl_types.cc:10: NaClStartParams::NaClStartParams()
Init nexe_file here, since it's POD type.

https://codereview.chromium.org/337463002/diff/70001/components/nacl/common/n...
components/nacl/common/nacl_types.cc:22: NaClLaunchParams::NaClLaunchParams()
Ditto: Init nexe_file here, since it's POD type.

https://codereview.chromium.org/337463002/diff/70001/components/nacl/common/n...
File components/nacl/common/nacl_types.h (right):

https://codereview.chromium.org/337463002/diff/70001/components/nacl/common/n...
components/nacl/common/nacl_types.h:63: // NOTE: Any new fields added here must
also be added to the IPC
You missed this. :-)  See other comment...

https://codereview.chromium.org/337463002/diff/70001/components/nacl/loader/n...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/337463002/diff/70001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:478: NaClFileInfo nexe_file_info = {};
Since we don't need validation caching here, you shouldn't need to use
NaClFileInfo here.

So can you use the NaClDescIoDescFromDescAllocCtor() function rather than
NaClDescIoFromFileInfo(), please?  That would be cleaner than creating a dummy
NaClFileInfo.

It's OK to change include_rules to allow using src/trusted/desc/nacl_desc_io.h
for this.  (Finishing
https://code.google.com/p/nativeclient/issues/detail?id=2832 is still a work in
progress!)

https://codereview.chromium.org/337463002/diff/70001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:481:
nacl::nonsfi::MainStart(NaClDescIoFromFileInfo(nexe_file_info, O_RDONLY));
Pedantically, this should be NACL_ABI_O_RDONLY rather than O_RDONLY.

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

https://codereview.chromium.org/337463002/diff/70001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:177: PP_NaClFileInfo
launch_sel_ldr_file_info;
The names "launch_sel_ldr" and "load_module" are a bit generic-sounding.  Could
we perhaps name them something like:

PP_NaClFileInfo file_info_for_srpc; // Old code path using LoadModule SRPC call.
PP_NaClFileInfo file_info_for_ipc; // New code path using Chrome IPC.

Also, could initialise these both with kInvalidNaClFileInfo outside the
conditionals, instead of inside.

https://codereview.chromium.org/337463002/diff/70001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:181: // to the plugin.
"to the NaCl loader process", really.  "Plugin" is very ambiguous (here, the
sender is the trusted plugin).

https://codereview.chromium.org/337463002/diff/70001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:191: launch_sel_ldr_file_info,
This will take ownership of the FD when a message is sent via Chrome IPC...

Does LoadModule() get passed an owning or borrowed reference to the FD?  I think
it's an owning reference, but I think that means nexe_file_info_ will contain a
dangling reference to the FD, which is a bit dodgy.

https://codereview.chromium.org/337463002/diff/70001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:208: return;
...but would the FD get closed if we return early here?

https://codereview.chromium.org/337463002/diff/70001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:215: service_runtime,
load_module_file_info, pp::CompletionCallback());
...and does this existing SRPC code path take ownership of the FD?  (I am not
sure whether SRPC method calls take ownership of FD arguments -- I suspect they
don't, in which case is this a bug in the existing code?)

[hidehiko, 2014-06-19 07:02:00]

Thank you for your review.

jln@, could you take a look at _messages.h files as an OWNER, too?

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

https://codereview.chromium.org/337463002/diff/70001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:461: // Do not create them.
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> Nit: 'them' -> 'it', since you're referring to one thing?

Done.

https://codereview.chromium.org/337463002/diff/70001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:833: // Note: nexe_file_ still
keeps the ownership at this moment, because
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> Is this because a base::File can't be used as a parameter in IPC messages, and
> params.nexe_file is a non-auto-freed type?  It might be worth commenting that
> here.

Done.

https://codereview.chromium.org/337463002/diff/70001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:834: // this params may just be
destructed before sending IPC is properly
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> Nit: 'destroyed' rather than 'destructed'?  Also "this params" -> "params"?

Done.

https://codereview.chromium.org/337463002/diff/70001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:904: nexe_file_.TakePlatformFile();
On 2014/06/18 19:53:11, Mark Seaborn wrote:
> Wouldn't it be cleaner to do this immediately before Send()?
> 
> After Send(), this will be referring to an FD that has already been close()'d
> (since you pass auto_close=True to FileDescriptor above), which is slightly
> dodgy.

Done.

https://codereview.chromium.org/337463002/diff/70001/components/nacl/common/n...
File components/nacl/common/nacl_types.cc (right):

https://codereview.chromium.org/337463002/diff/70001/components/nacl/common/n...
components/nacl/common/nacl_types.cc:10: NaClStartParams::NaClStartParams()
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> Init nexe_file here, since it's POD type.

Done.

https://codereview.chromium.org/337463002/diff/70001/components/nacl/common/n...
components/nacl/common/nacl_types.cc:22: NaClLaunchParams::NaClLaunchParams()
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> Ditto: Init nexe_file here, since it's POD type.

Done.

https://codereview.chromium.org/337463002/diff/70001/components/nacl/common/n...
File components/nacl/common/nacl_types.h (right):

https://codereview.chromium.org/337463002/diff/70001/components/nacl/common/n...
components/nacl/common/nacl_types.h:63: // NOTE: Any new fields added here must
also be added to the IPC
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> You missed this. :-)  See other comment...

Good catch. Fixed.

https://codereview.chromium.org/337463002/diff/70001/components/nacl/loader/n...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/337463002/diff/70001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:478: NaClFileInfo nexe_file_info = {};
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> Since we don't need validation caching here, you shouldn't need to use
> NaClFileInfo here.
> 
> So can you use the NaClDescIoDescFromDescAllocCtor() function rather than
> NaClDescIoFromFileInfo(), please?  That would be cleaner than creating a dummy
> NaClFileInfo.
> 
> It's OK to change include_rules to allow using src/trusted/desc/nacl_desc_io.h
> for this.  (Finishing
> https://code.google.com/p/nativeclient/issues/detail?id=2832 is still a work
in
> progress!)

Done. No need to change DEPS, as it already has src/trusted/desc.

https://codereview.chromium.org/337463002/diff/70001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:481:
nacl::nonsfi::MainStart(NaClDescIoFromFileInfo(nexe_file_info, O_RDONLY));
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> Pedantically, this should be NACL_ABI_O_RDONLY rather than O_RDONLY.

Done.

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

https://codereview.chromium.org/337463002/diff/70001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:177: PP_NaClFileInfo
launch_sel_ldr_file_info;
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> The names "launch_sel_ldr" and "load_module" are a bit generic-sounding. 
Could
> we perhaps name them something like:
> 
> PP_NaClFileInfo file_info_for_srpc; // Old code path using LoadModule SRPC
call.
> PP_NaClFileInfo file_info_for_ipc; // New code path using Chrome IPC.
> 
> Also, could initialise these both with kInvalidNaClFileInfo outside the
> conditionals, instead of inside.

Done.

https://codereview.chromium.org/337463002/diff/70001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:181: // to the plugin.
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> "to the NaCl loader process", really.  "Plugin" is very ambiguous (here, the
> sender is the trusted plugin).

Done.

https://codereview.chromium.org/337463002/diff/70001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:191: launch_sel_ldr_file_info,
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> This will take ownership of the FD when a message is sent via Chrome IPC...
> 
> Does LoadModule() get passed an owning or borrowed reference to the FD?  I
think
> it's an owning reference, but I think that means nexe_file_info_ will contain
a
> dangling reference to the FD, which is a bit dodgy.

Good point. Can I work on it in a separate CL, to focus on SRPC replacement in
this CL? Could you see my comment below, too?

https://codereview.chromium.org/337463002/diff/70001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:208: return;
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> ...but would the FD get closed if we return early here?

Admittedly...

https://codereview.chromium.org/337463002/diff/70001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:215: service_runtime,
load_module_file_info, pp::CompletionCallback());
On 2014/06/18 19:53:12, Mark Seaborn wrote:
> ...and does this existing SRPC code path take ownership of the FD?  (I am not
> sure whether SRPC method calls take ownership of FD arguments -- I suspect
they
> don't, in which case is this a bug in the existing code?)

It takes.
So, here we have three problems.
1) If creation of the ServiceRuntime fails, the fd is leaked.
2) If module loading is successfully done (regardless of SRPC or Chrome IPC),
nexe_file_info_ would keep the dangling file descriptor, as SRPC/IPC takes the
ownership.
3) If Plugin instance is destroyed during the initialization process, the fd is
leaked, too. This is because CompletionCallback generated by CallbackFactory
does not have enough feature for RAII on cancellation.

Questions:
- does 1) happen actually? Is "new" operator set to return NULL on failure?
- For 3), is there any standard way to handle these case in NaCl? I guess this
is a typical but corner case. An idea is getting rid of CallbackFactory, instead
to handle cancellation by WeakRef etc. manually. If we can use base::Callback,
it can be solved more easily, but pp::CompletionCallback does not have enough
power. Another idea is extend pp::CallbackFactory/CompletionCallback to support
rvalue concept.

Anyway, as this looks not a regression, I'd like to separate it from this CL.
WDYT?

[jln (very slow on Chromium), 2014-06-19 18:06:32]

On 2014/06/19 07:02:00, hidehiko wrote:
> Thank you for your review.
> 
> jln@, could you take a look at _messages.h files as an OWNER, too?

*_messages.h lgtm given the rest of the review.

[Mark Seaborn, 2014-06-19 22:36:48]

LGTM

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

https://codereview.chromium.org/337463002/diff/70001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:191: launch_sel_ldr_file_info,
On 2014/06/19 07:01:59, hidehiko wrote:
> On 2014/06/18 19:53:12, Mark Seaborn wrote:
> > This will take ownership of the FD when a message is sent via Chrome IPC...
> > 
> > Does LoadModule() get passed an owning or borrowed reference to the FD?  I
> think
> > it's an owning reference, but I think that means nexe_file_info_ will
contain
> a
> > dangling reference to the FD, which is a bit dodgy.
> 
> Good point. Can I work on it in a separate CL, to focus on SRPC replacement in
> this CL? Could you see my comment below, too?

Yes, feel free to leave that to a separate change.  I'm not expecting you to fix
existing bugs in this changeset, just expecting no regressions to be added. :-)

https://codereview.chromium.org/337463002/diff/70001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin.cc:215: service_runtime,
load_module_file_info, pp::CompletionCallback());
On 2014/06/19 07:02:00, hidehiko wrote:
> On 2014/06/18 19:53:12, Mark Seaborn wrote:
> > ...and does this existing SRPC code path take ownership of the FD?  (I am
not
> > sure whether SRPC method calls take ownership of FD arguments -- I suspect
> they
> > don't, in which case is this a bug in the existing code?)
> 
> It takes.
> So, here we have three problems.
> 1) If creation of the ServiceRuntime fails, the fd is leaked.
> 2) If module loading is successfully done (regardless of SRPC or Chrome IPC),
> nexe_file_info_ would keep the dangling file descriptor, as SRPC/IPC takes the
> ownership.
> 3) If Plugin instance is destroyed during the initialization process, the fd
is
> leaked, too. This is because CompletionCallback generated by CallbackFactory
> does not have enough feature for RAII on cancellation.
> 
> Questions:
> - does 1) happen actually? Is "new" operator set to return NULL on failure?
> - For 3), is there any standard way to handle these case in NaCl? I guess this
> is a typical but corner case. An idea is getting rid of CallbackFactory,
instead
> to handle cancellation by WeakRef etc. manually. If we can use base::Callback,
> it can be solved more easily, but pp::CompletionCallback does not have enough
> power. Another idea is extend pp::CallbackFactory/CompletionCallback to
support
> rvalue concept.

I don't know the answer.  Justin probably knows this code better than me.

> Anyway, as this looks not a regression, I'd like to separate it from this CL.
> WDYT?

That's OK with me.

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

https://codereview.chromium.org/337463002/diff/110001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:901: // Here we are about sending
the IPC, so release file descriptors to delegate
Nit: "about to send the IPC"

[hidehiko, 2014-06-20 04:02:50]

Thank you for review! Sending to CQ.

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

https://codereview.chromium.org/337463002/diff/110001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:901: // Here we are about sending
the IPC, so release file descriptors to delegate
On 2014/06/19 22:36:48, Mark Seaborn wrote:
> Nit: "about to send the IPC"

Done.

[hidehiko, 2014-06-20 04:02:55]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-06-20 04:03:47]

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

[hidehiko, 2014-06-20 05:18:59]

The CQ bit was unchecked by hidehiko@chromium.org

[hidehiko, 2014-06-20 09:09:07]

On 2014/06/20 05:18:59, hidehiko wrote:
> The CQ bit was unchecked by mailto:hidehiko@chromium.org

Waited for r278645. Rebased. Again, sending to CQ.

[hidehiko, 2014-06-20 09:09:13]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-06-20 09:12:36]

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

[commit-bot: I haz the power, 2014-06-20 15:15:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-20 15:44:03]

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

[commit-bot: I haz the power, 2014-06-20 15:44:04]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[hidehiko, 2014-06-20 16:33:39]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-06-20 16:36:04]

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

[commit-bot: I haz the power, 2014-06-21 01:15:45]

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

[commit-bot: I haz the power, 2014-06-21 01:15:46]

Failed to apply patch for ppapi/c/private/ppb_nacl_private.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ppapi/c/private/ppb_nacl_private.h
  Hunk #1 FAILED at 3.
  Hunk #2 succeeded at 230 (offset -7 lines).
  Hunk #3 succeeded at 249 (offset -7 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
ppapi/c/private/ppb_nacl_private.h.rej

Patch:       ppapi/c/private/ppb_nacl_private.h
Index: ppapi/c/private/ppb_nacl_private.h
diff --git a/ppapi/c/private/ppb_nacl_private.h
b/ppapi/c/private/ppb_nacl_private.h
index
9df8723a42027a7b4aede22329912666057f45bf..9e0107e1ff3a625d9c9cf804a2d100068edb4d52
100644
--- a/ppapi/c/private/ppb_nacl_private.h
+++ b/ppapi/c/private/ppb_nacl_private.h
@@ -3,7 +3,7 @@
  * found in the LICENSE file.
  */
 
-/* From private/ppb_nacl_private.idl modified Fri Jun 13 15:14:51 2014. */
+/* From private/ppb_nacl_private.idl modified Wed Jun 18 17:20:04 2014. */
 
 #ifndef PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
 #define PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
@@ -237,6 +237,9 @@ struct PPB_NaCl_Private_1_0 {
    * indicates that the nexe run by sel_ldr will use the PPAPI APIs.
    * This implies that LaunchSelLdr is run from the main thread.  If a nexe
    * does not need PPAPI, then it can run off the main thread.
+   * The |nexe_file_info| is currently used only in non-SFI mode. It is the
+   * file handle for the main nexe file, which should be initially loaded.
+   * LaunchSelLdr takes the ownership of the file handle.
    * The |uses_irt| flag indicates whether the IRT should be loaded in this
    * NaCl process.  This is true for ABI stable nexes.
    * The |uses_nonsfi_mode| flag indicates whether or not nonsfi-mode should
@@ -253,6 +256,7 @@ struct PPB_NaCl_Private_1_0 {
       PP_Instance instance,
       PP_Bool main_service_runtime,
       const char* alleged_url,
+      const struct PP_NaClFileInfo* nexe_file_info,
       PP_Bool uses_irt,
       PP_Bool uses_ppapi,
       PP_Bool uses_nonsfi_mode,

[hidehiko, 2014-06-23 03:42:34]

Oops... Conflicted while in the CQ.
Rebased again. Submitting...

On 2014/06/21 01:15:46, I haz the power (commit-bot) wrote:
> Failed to apply patch for ppapi/c/private/ppb_nacl_private.h:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file ppapi/c/private/ppb_nacl_private.h
>   Hunk #1 FAILED at 3.
>   Hunk #2 succeeded at 230 (offset -7 lines).
>   Hunk #3 succeeded at 249 (offset -7 lines).
>   1 out of 3 hunks FAILED -- saving rejects to file
> ppapi/c/private/ppb_nacl_private.h.rej
> 
> Patch:       ppapi/c/private/ppb_nacl_private.h
> Index: ppapi/c/private/ppb_nacl_private.h
> diff --git a/ppapi/c/private/ppb_nacl_private.h
> b/ppapi/c/private/ppb_nacl_private.h
> index
>
9df8723a42027a7b4aede22329912666057f45bf..9e0107e1ff3a625d9c9cf804a2d100068edb4d52
> 100644
> --- a/ppapi/c/private/ppb_nacl_private.h
> +++ b/ppapi/c/private/ppb_nacl_private.h
> @@ -3,7 +3,7 @@
>   * found in the LICENSE file.
>   */
>  
> -/* From private/ppb_nacl_private.idl modified Fri Jun 13 15:14:51 2014. */
> +/* From private/ppb_nacl_private.idl modified Wed Jun 18 17:20:04 2014. */
>  
>  #ifndef PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
>  #define PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
> @@ -237,6 +237,9 @@ struct PPB_NaCl_Private_1_0 {
>     * indicates that the nexe run by sel_ldr will use the PPAPI APIs.
>     * This implies that LaunchSelLdr is run from the main thread.  If a nexe
>     * does not need PPAPI, then it can run off the main thread.
> +   * The |nexe_file_info| is currently used only in non-SFI mode. It is the
> +   * file handle for the main nexe file, which should be initially loaded.
> +   * LaunchSelLdr takes the ownership of the file handle.
>     * The |uses_irt| flag indicates whether the IRT should be loaded in this
>     * NaCl process.  This is true for ABI stable nexes.
>     * The |uses_nonsfi_mode| flag indicates whether or not nonsfi-mode should
> @@ -253,6 +256,7 @@ struct PPB_NaCl_Private_1_0 {
>        PP_Instance instance,
>        PP_Bool main_service_runtime,
>        const char* alleged_url,
> +      const struct PP_NaClFileInfo* nexe_file_info,
>        PP_Bool uses_irt,
>        PP_Bool uses_ppapi,
>        PP_Bool uses_nonsfi_mode,

[hidehiko, 2014-06-23 03:42:39]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-06-23 03:43:25]

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

[commit-bot: I haz the power, 2014-06-23 07:22:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-23 09:44:12]

Change committed as 279069