Use ChannelMojo from the browser to NaCl loader process.

Use ChannelMojo from the browser to NaCl loader process.

BUG=604282

[Anand Mistry (off Chromium), 2016-06-24 03:26:54]

amistry@chromium.org changed reviewers:
+ bbudge@chromium.org, dcheng@chromium.org, mseaborn@chromium.org,
sievers@chromium.org

[Anand Mistry (off Chromium), 2016-06-24 03:26:56]

sievers@chromium.org: Please review changes in content
bbudge@chromium.org: Please review changes in nacl_irt
mseaborn@chromium.org: Please review changes in nacl
dcheng: For security

[dcheng, 2016-06-24 23:19:27]

dcheng@chromium.org changed reviewers:
+ jln@chromium.org

[dcheng, 2016-06-24 23:19:27]

I don't think I'm comfortable commenting from a security perspective. Overall, I
think knowledge of this area is getting pretty thing on the security team. +jln,
do you mind taking a quick look? We should try to figure out how to do some
knowledge transfer here too

[Anand Mistry (off Chromium), 2016-06-30 01:40:21]

Ping! The windows issue is fixed, so this now passes all tests (the chromeos
test failure appears to be a flake).

[Anand Mistry (off Chromium), 2016-07-07 01:06:24]

Ping again!

[Mark Seaborn, 2016-07-08 18:22:25]

https://codereview.chromium.org/2081183005/diff/240001/content/zygote/zygote_...
File content/zygote/zygote_linux.cc (right):

https://codereview.chromium.org/2081183005/diff/240001/content/zygote/zygote_...
content/zygote/zygote_linux.cc:440: DCHECK_EQ(-1, ipc_channel_fd);
Is there a reason for changing this from a return to a DCHECK?

https://codereview.chromium.org/2081183005/diff/240001/ppapi/nacl_irt/irt_sta...
File ppapi/nacl_irt/irt_start.cc (right):

https://codereview.chromium.org/2081183005/diff/240001/ppapi/nacl_irt/irt_sta...
ppapi/nacl_irt/irt_start.cc:31: ppapi::StartUpPlugin(true);
Instead of parameterising whether ppapi::StartUpPlugin() calls
mojo::edk::Init(), how about just calling mojo::edk::Init() here?  That would be
clearer.

[Mark Seaborn, 2016-07-08 20:26:19]

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
File components/nacl/broker/nacl_broker_listener.cc (right):

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:44: mojo::edk::Init();
How about putting this global-init stuff in NaClBrokerMain(), in
components/nacl/loader/nacl_helper_win_64.cc, rather than in this constructor?

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:45: mojo_ipc_support_.reset(new
mojo::edk::ScopedIPCSupport(
mojo_ipc_support_ is never read, so I don't think it needs to be a member, does
it?

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:114: const std::string&
loader_channel_id) {
Should this be called "token" rather than "id" now?

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:117:
mojo::ScopedMessagePipeHandle loader_handle(
Nit: With this addition, there's "loader_handle_in_browser" (a process handle)
and "loader_handle" (a pipe handle).  That naming is a bit confusing.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:132: // Mojo IPC setup.
This is a largish chunk of plumbing for creating a process.  How and where does
this plumbing happen for normal Chrome child processes (such as renderers)?  Is
it spread across a larger area of code?

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:150: CHECK_EQ(MOJO_RESULT_OK,
mojo::FuseMessagePipes(std::move(loader_handle),
Does this cause messages to be forwarded by the NaCl-broker process between the
browser and NaCl-loader processes, or does Mojo implement "path shortening" so
that the messages go directly between the browser and NaCl-loader (as I believe
they currently do)?

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:178: channel_->Send(new
NaClProcessMsg_LoaderLaunched(loader_channel_id,
Is loader_channel_id "consumed" by the call to
mojo::edk::CreateChildMessagePipe() above?

I notice that it still being passed along here.  I think it is being used
secondarily as an ID for associating this reply with the request.  That might be
worth a comment here to avoid potential confusion.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/browse...
File components/nacl/browser/nacl_broker_host_win.cc (right):

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/browse...
components/nacl/browser/nacl_broker_host_win.cc:47:
process_.reset(content::BrowserChildProcessHost::Create(
It looks like this Create() call could be moved into Init().  Then you don't
need to make mojo_child_token_ a field of NaClBrokerHost.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/browse...
components/nacl/browser/nacl_broker_host_win.cc:93: bool
NaClBrokerHost::LaunchLoader(const std::string& loader_channel_id) {
"token" rather than "id" now?

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/browse...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:192: base::ScopedFD TakeIpcFd()
override { return base::ScopedFD(-1); }
Nit: I think you can use base::ScopedFD() without an argument here.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:576:
process_->GetHost()->CreateChannelMojo(mojo_child_token_);
Since mojo_child_token_ isn't used after this, you could reset it to save a
little memory.

[Anand Mistry (off Chromium), 2016-07-12 07:26:23]

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
File components/nacl/broker/nacl_broker_listener.cc (right):

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:44: mojo::edk::Init();
On 2016/07/08 20:26:19, Mark Seaborn wrote:
> How about putting this global-init stuff in NaClBrokerMain(), in
> components/nacl/loader/nacl_helper_win_64.cc, rather than in this constructor?

Done.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:45: mojo_ipc_support_.reset(new
mojo::edk::ScopedIPCSupport(
On 2016/07/08 20:26:19, Mark Seaborn wrote:
> mojo_ipc_support_ is never read, so I don't think it needs to be a member,
does
> it?

It needs to exist for the life of this object. But moving it out with the rest
of the init code mean it no longer needs to be here.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:114: const std::string&
loader_channel_id) {
On 2016/07/08 20:26:19, Mark Seaborn wrote:
> Should this be called "token" rather than "id" now?

Done.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:117:
mojo::ScopedMessagePipeHandle loader_handle(
On 2016/07/08 20:26:19, Mark Seaborn wrote:
> Nit: With this addition, there's "loader_handle_in_browser" (a process handle)
> and "loader_handle" (a pipe handle).  That naming is a bit confusing.

s/handle/message_pipe

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:132: // Mojo IPC setup.
On 2016/07/08 20:26:19, Mark Seaborn wrote:
> This is a largish chunk of plumbing for creating a process.  How and where
does
> this plumbing happen for normal Chrome child processes (such as renderers)? 
Is
> it spread across a larger area of code?

Most Chrome code uses content::ChildProcessLauncher to create child processes.
That class does the browser-side setup. On the child side, ChildThreadImpl is a
common base class, so it has the child setup code.

There are a small number of special cases which for whatever reason don't use
ChildProcessLauncher/ChildThreadImpl. In those, this dance is pretty much
repeated.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:150: CHECK_EQ(MOJO_RESULT_OK,
mojo::FuseMessagePipes(std::move(loader_handle),
On 2016/07/08 20:26:19, Mark Seaborn wrote:
> Does this cause messages to be forwarded by the NaCl-broker process between
the
> browser and NaCl-loader processes, or does Mojo implement "path shortening" so
> that the messages go directly between the browser and NaCl-loader (as I
believe
> they currently do)?

Yes, Mojo does "path shortening". It may take a few round-trips and so the first
few messages might go via the broker, but Mojo does actively try to create
direct connections between the two endpoints.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/broker...
components/nacl/broker/nacl_broker_listener.cc:178: channel_->Send(new
NaClProcessMsg_LoaderLaunched(loader_channel_id,
On 2016/07/08 20:26:19, Mark Seaborn wrote:
> Is loader_channel_id "consumed" by the call to
> mojo::edk::CreateChildMessagePipe() above?

Yes. Just like before.

> I notice that it still being passed along here.  I think it is being used
> secondarily as an ID for associating this reply with the request.  That might
be
> worth a comment here to avoid potential confusion.

Added comment.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/browse...
File components/nacl/browser/nacl_broker_host_win.cc (right):

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/browse...
components/nacl/browser/nacl_broker_host_win.cc:47:
process_.reset(content::BrowserChildProcessHost::Create(
On 2016/07/08 20:26:19, Mark Seaborn wrote:
> It looks like this Create() call could be moved into Init().  Then you don't
> need to make mojo_child_token_ a field of NaClBrokerHost.

Done.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/browse...
components/nacl/browser/nacl_broker_host_win.cc:93: bool
NaClBrokerHost::LaunchLoader(const std::string& loader_channel_id) {
On 2016/07/08 20:26:19, Mark Seaborn wrote:
> "token" rather than "id" now?

Done.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/browse...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:192: base::ScopedFD TakeIpcFd()
override { return base::ScopedFD(-1); }
On 2016/07/08 20:26:19, Mark Seaborn wrote:
> Nit: I think you can use base::ScopedFD() without an argument here.

Although it's correct here, I've undone this change because it would be better
for me to eliminate SandboxedProcessLauncherDelegate::TakeIpcFD and
ChildProcessHost::TakeClientFileDescriptor everywhere at the same time instead
of in a piecemeal manner.

https://codereview.chromium.org/2081183005/diff/160001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:576:
process_->GetHost()->CreateChannelMojo(mojo_child_token_);
On 2016/07/08 20:26:19, Mark Seaborn wrote:
> Since mojo_child_token_ isn't used after this, you could reset it to save a
> little memory.

Done.

https://codereview.chromium.org/2081183005/diff/240001/content/zygote/zygote_...
File content/zygote/zygote_linux.cc (right):

https://codereview.chromium.org/2081183005/diff/240001/content/zygote/zygote_...
content/zygote/zygote_linux.cc:440: DCHECK_EQ(-1, ipc_channel_fd);
On 2016/07/08 18:22:25, Mark Seaborn wrote:
> Is there a reason for changing this from a return to a DCHECK?

Because the expected situation is different. Before, it could be -1 if say the
socketpair() failed. Now, we expect it to _always_ be -1, unless there's a
programming error.

https://codereview.chromium.org/2081183005/diff/240001/ppapi/nacl_irt/irt_sta...
File ppapi/nacl_irt/irt_start.cc (right):

https://codereview.chromium.org/2081183005/diff/240001/ppapi/nacl_irt/irt_sta...
ppapi/nacl_irt/irt_start.cc:31: ppapi::StartUpPlugin(true);
On 2016/07/08 18:22:25, Mark Seaborn wrote:
> Instead of parameterising whether ppapi::StartUpPlugin() calls
> mojo::edk::Init(), how about just calling mojo::edk::Init() here?  That would
be
> clearer.

It's not in this CL because it's not necessary yet, but there's more to this
than just calling mojo::edk::Init(). We also need to initialize Mojo's internal
task runner using the IO thread created by StartUpPlugin(). But that's for a
follow-up.

[Anand Mistry (off Chromium), 2016-07-20 04:58:49]

Ping!

[Anand Mistry (off Chromium), 2016-07-20 05:23:33]

The CQ bit was checked by amistry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 05:23:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 06:31:45]

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

[commit-bot: I haz the power, 2016-07-20 06:31:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Anand Mistry (off Chromium), 2016-07-21 01:22:35]

rebase

[Sam McNally, 2016-09-01 08:33:05]

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

[Sam McNally, 2016-09-01 08:33:06]

ping?

[Mark Seaborn, 2016-09-01 18:43:31]

LGTM.  Can you just address my nit about StartUpPlugin(), please?

https://codereview.chromium.org/2081183005/diff/240001/content/zygote/zygote_...
File content/zygote/zygote_linux.cc (right):

https://codereview.chromium.org/2081183005/diff/240001/content/zygote/zygote_...
content/zygote/zygote_linux.cc:440: DCHECK_EQ(-1, ipc_channel_fd);
On 2016/07/12 07:26:23, Anand Mistry (off Chromium) wrote:
> On 2016/07/08 18:22:25, Mark Seaborn wrote:
> > Is there a reason for changing this from a return to a DCHECK?
> 
> Because the expected situation is different. Before, it could be -1 if say the
> socketpair() failed. Now, we expect it to _always_ be -1, unless there's a
> programming error.

Ah yes, sorry, I missed that.

https://codereview.chromium.org/2081183005/diff/240001/ppapi/nacl_irt/irt_sta...
File ppapi/nacl_irt/irt_start.cc (right):

https://codereview.chromium.org/2081183005/diff/240001/ppapi/nacl_irt/irt_sta...
ppapi/nacl_irt/irt_start.cc:31: ppapi::StartUpPlugin(true);
On 2016/07/12 07:26:23, Anand Mistry (off Chromium) wrote:
> On 2016/07/08 18:22:25, Mark Seaborn wrote:
> > Instead of parameterising whether ppapi::StartUpPlugin() calls
> > mojo::edk::Init(), how about just calling mojo::edk::Init() here?  That
would
> be
> > clearer.
> 
> It's not in this CL because it's not necessary yet, but there's more to this
> than just calling mojo::edk::Init(). We also need to initialize Mojo's
internal
> task runner using the IO thread created by StartUpPlugin(). But that's for a
> follow-up.

I see.  I'd still prefer if you didn't parameterise this function in this CL. 
Would you mind keeping the code simpler/clearer for now, and leave the extra
complexity for later changes when it becomes necessary?

[Sam McNally, 2016-09-01 23:34:03]

Description was changed from

==========
Use ChannelMojo from the browser to NaCl loader process.

BUG=604282
==========

to

==========
Use ChannelMojo from the browser to NaCl loader process.

BUG=604282
==========

[Sam McNally, 2016-09-01 23:34:03]

sammc@chromium.org changed reviewers:
- sammc@chromium.org

[Sam McNally, 2016-09-02 04:42:10]

https://codereview.chromium.org/2081183005/diff/240001/ppapi/nacl_irt/irt_sta...
File ppapi/nacl_irt/irt_start.cc (right):

https://codereview.chromium.org/2081183005/diff/240001/ppapi/nacl_irt/irt_sta...
ppapi/nacl_irt/irt_start.cc:31: ppapi::StartUpPlugin(true);
On 2016/09/01 18:43:31, Mark Seaborn wrote:
> On 2016/07/12 07:26:23, Anand Mistry (off Chromium) wrote:
> > On 2016/07/08 18:22:25, Mark Seaborn wrote:
> > > Instead of parameterising whether ppapi::StartUpPlugin() calls
> > > mojo::edk::Init(), how about just calling mojo::edk::Init() here?  That
> would
> > be
> > > clearer.
> > 
> > It's not in this CL because it's not necessary yet, but there's more to this
> > than just calling mojo::edk::Init(). We also need to initialize Mojo's
> internal
> > task runner using the IO thread created by StartUpPlugin(). But that's for a
> > follow-up.
> 
> I see.  I'd still prefer if you didn't parameterise this function in this CL. 
> Would you mind keeping the code simpler/clearer for now, and leave the extra
> complexity for later changes when it becomes necessary?

Done in https://codereview.chromium.org/2305913002/.