Use ChannelMojo from the browser to NaCl loader process.

Use ChannelMojo from the browser to NaCl loader process.

patch from issue 2081183005 at patchset 340001 (http://crrev.com/2081183005#ps340001)

BUG=604282
COLLABORATOR=amistry@chromium.org

Committed: https://crrev.com/9e8780772cbf802e57c366eb8113d38af95ffd89
Cr-Commit-Position: refs/heads/master@{#420245}

[Sam McNally, 2016-09-02 00:02:50]

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

[commit-bot: I haz the power, 2016-09-02 00:03:32]

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-09-02 01:09:30]

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

[commit-bot: I haz the power, 2016-09-02 01:09:31]

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

[Sam McNally, 2016-09-02 01:26:30]

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

[commit-bot: I haz the power, 2016-09-02 01:26:56]

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-09-02 02:38:16]

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

[commit-bot: I haz the power, 2016-09-02 02:38:17]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-09-02 03:06:40]

Patchset #2 (id:20001) has been deleted

[Sam McNally, 2016-09-02 04:33:04]

Description was changed from

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

patch from issue 2081183005 at patchset 340001
(http://crrev.com/2081183005#ps340001)

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

to

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

patch from issue 2081183005 at patchset 340001
(http://crrev.com/2081183005#ps340001)

BUG=604282
COLLABORATOR=amistry@chromium.org
==========

[Sam McNally, 2016-09-02 04:33:04]

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

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

Anand is no longer working on chrome so I'm picking up this CL from
http://crrev.com/2081183005.

[Mark Seaborn, 2016-09-02 19:16:27]

LGTM

https://codereview.chromium.org/2305913002/diff/40001/components/nacl/loader/...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/2305913002/diff/40001/components/nacl/loader/...
components/nacl/loader/nacl_helper_linux.cc:124: mojo::edk::Init();
Looking at this again, I'm rather unclear on why the calls to mojo::edk::Init()
changed in this CL, since it looks like it was getting called twice for Non-SFI
NaCl before this CL.  This call here would be the first call.

If the CL works without changing the mojo::edk::Init() calls, maybe you could
leave that part out and change the mojo::edk::Init() calls in separate CL for
clarity, as a cleanup?

Or otherwise, can you put a note in the commit message about why this aspect
needs to be changed?

[Sam McNally, 2016-09-05 00:03:43]

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

[commit-bot: I haz the power, 2016-09-05 00:03:54]

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-09-05 01:03:46]

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

[commit-bot: I haz the power, 2016-09-05 01:03:47]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-09-05 05:02:16]

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

[commit-bot: I haz the power, 2016-09-05 05:02:24]

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-09-05 06:07:39]

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

[commit-bot: I haz the power, 2016-09-05 06:07:40]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-09-05 07:12:25]

https://codereview.chromium.org/2305913002/diff/40001/components/nacl/loader/...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/2305913002/diff/40001/components/nacl/loader/...
components/nacl/loader/nacl_helper_linux.cc:124: mojo::edk::Init();
On 2016/09/02 19:16:27, Mark Seaborn wrote:
> Looking at this again, I'm rather unclear on why the calls to
mojo::edk::Init()
> changed in this CL, since it looks like it was getting called twice for
Non-SFI
> NaCl before this CL.  This call here would be the first call.
> 
> If the CL works without changing the mojo::edk::Init() calls, maybe you could
> leave that part out and change the mojo::edk::Init() calls in separate CL for
> clarity, as a cleanup?
> 
> Or otherwise, can you put a note in the commit message about why this aspect
> needs to be changed?

As far as I can tell, it was incorrect before, but this CL without the fix
applied makes NaClBrowserTestNonSfiMode.Irt flaky. I've split it out in
https://codereview.chromium.org/2305323002.

[Sam McNally, 2016-09-12 07:21:49]

jln: ping

[Sam McNally, 2016-09-19 03:14:44]

On 2016/09/12 07:21:49, Sam McNally wrote:
> jln: ping

Is there someone else who could review this?

[jln (very slow on Chromium), 2016-09-19 22:43:01]

On 2016/09/19 03:14:44, Sam McNally wrote:
> On 2016/09/12 07:21:49, Sam McNally wrote:
> > jln: ping
> 
> Is there someone else who could review this?

What do you need reviewed?

[Sam McNally, 2016-09-19 23:30:47]

On 2016/09/19 22:43:01, jln (very slow on Chromium) wrote:
> On 2016/09/19 03:14:44, Sam McNally wrote:
> > On 2016/09/12 07:21:49, Sam McNally wrote:
> > > jln: ping
> > 
> > Is there someone else who could review this?
> 
> What do you need reviewed?

The security impact of this CL. This is a continuation of
https://codereview.chromium.org/2081183005 where dcheng added you as a reviewer
in https://codereview.chromium.org/2081183005#msg4.

[jln (very slow on Chromium), 2016-09-20 00:45:44]

On 2016/09/19 23:30:47, Sam McNally wrote:
> On 2016/09/19 22:43:01, jln (very slow on Chromium) wrote:
> > On 2016/09/19 03:14:44, Sam McNally wrote:
> > > On 2016/09/12 07:21:49, Sam McNally wrote:
> > > > jln: ping
> > > 
> > > Is there someone else who could review this?
> > 
> > What do you need reviewed?
> 
> The security impact of this CL. This is a continuation of
> https://codereview.chromium.org/2081183005 where dcheng added you as a
reviewer
> in https://codereview.chromium.org/2081183005#msg4.

I'm not working on Chrome and while I'm still helping out with some reviews in
areas I own, this is far away from it.

My best advice would be to ask dcheng or email the security team to see who
could help you out. But be more precise about what you need, explain what the CL
is doing and why and what worries you.

[dcheng, 2016-09-21 05:18:33]

The code change seems fine. My questions mostly revolve around comments (some
are questions about things not in this CL; it's fine to address those
separately)

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

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/broker/...
components/nacl/broker/nacl_broker_listener.cc:107: // Mojo channel setup is a
little bit convoluted, so let me explain. The
Nit: It'd be nice to avoid pronouns in this comment phrasing if possible
(https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M)

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/broker/...
components/nacl/broker/nacl_broker_listener.cc:108: // normal way an initial
Mojo message pipe is setup is that a token (string)
token => secret string token, I believe the token is supposed to be unique and
unguessable, to make it hard for another process to hijack the ipc connection?

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/broker/...
components/nacl/broker/nacl_broker_listener.cc:149: mojo_child_token);
I'm not entirely sure what the difference between token and child_token are here
(and the docs don't really clarify, so maybe we could improve those in a
followup CL)

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/broker/...
components/nacl/broker/nacl_broker_listener.cc:153:
mojo::FuseMessagePipes(std::move(loader_message_pipe),
#include <utility> (here and elsewhere)

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/broker/...
components/nacl/broker/nacl_broker_listener.cc:161:
mojo::edk::ChildProcessLaunched(loader_process.Handle(),
I guess this is to update state in the parent on successful launch? The comment
style in
https://cs.chromium.org/chromium/src/mojo/edk/embedder/embedder.h?rcl=0&l=44
almost makes it sound like it's actually a callback that gets invoked by the
mojo system (which I found a bit confusing)

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

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:589:
mojo_child_token_.shrink_to_fit();
Is the intention to reclaim memory? What about using STLClearObject from
base/stl_util.h here?

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/loader/...
File components/nacl/loader/nacl_helper_win_64.cc (right):

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/loader/...
components/nacl/loader/nacl_helper_win_64.cc:48:
mojo::edk::SetParentPipeHandle(std::move(platform_channel));
I'm a bit confused about this one as well. The documentation says:
  // Should be called as early as possible in the child process with the handle
  // that the parent received from ChildProcessLaunched.

But ChildProcessLaunched() doesn't appear to return anything or have any out
values.

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:184: io_thread_.task_runner().get()));
No .get() to avoid a scoped_refptr -> raw ptr -> scoped_refptr conversion

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_listener.cc:46:
io_thread_.task_runner().get()));
Ditto: omit .get()

[Sam McNally, 2016-09-21 06:53:35]

sammc@chromium.org changed reviewers:
- bbudge@chromium.org, jln@chromium.org, sievers@chromium.org

[Sam McNally, 2016-09-21 07:05:51]

Patchset #6 (id:120001) has been deleted

[Sam McNally, 2016-09-21 07:06:56]

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

[commit-bot: I haz the power, 2016-09-21 07:07:37]

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

[Sam McNally, 2016-09-21 07:49:45]

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

[Sam McNally, 2016-09-21 07:49:47]

+jam for //content and new DEPS on //mojo.

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

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/broker/...
components/nacl/broker/nacl_broker_listener.cc:107: // Mojo channel setup is a
little bit convoluted, so let me explain. The
On 2016/09/21 05:18:33, dcheng wrote:
> Nit: It'd be nice to avoid pronouns in this comment phrasing if possible
>
(https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M)

Done.

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/broker/...
components/nacl/broker/nacl_broker_listener.cc:108: // normal way an initial
Mojo message pipe is setup is that a token (string)
On 2016/09/21 05:18:33, dcheng wrote:
> token => secret string token, I believe the token is supposed to be unique and
> unguessable, to make it hard for another process to hijack the ipc connection?

Done.

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/broker/...
components/nacl/broker/nacl_broker_listener.cc:149: mojo_child_token);
On 2016/09/21 05:18:33, dcheng wrote:
> I'm not entirely sure what the difference between token and child_token are
here
> (and the docs don't really clarify, so maybe we could improve those in a
> followup CL)

mojo_channel_token is the secret token shared with the child process.
mojo_child_token is the |child_token| passed to ChildProcessLaunched() so that
child launch failures break the returned message pipe.

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/broker/...
components/nacl/broker/nacl_broker_listener.cc:153:
mojo::FuseMessagePipes(std::move(loader_message_pipe),
On 2016/09/21 05:18:33, dcheng wrote:
> #include <utility> (here and elsewhere)

Done.

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/broker/...
components/nacl/broker/nacl_broker_listener.cc:161:
mojo::edk::ChildProcessLaunched(loader_process.Handle(),
On 2016/09/21 05:18:32, dcheng wrote:
> I guess this is to update state in the parent on successful launch? The
comment
> style in
> https://cs.chromium.org/chromium/src/mojo/edk/embedder/embedder.h?rcl=0&l=44
> almost makes it sound like it's actually a callback that gets invoked by the
> mojo system (which I found a bit confusing)

Correct.

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

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:589:
mojo_child_token_.shrink_to_fit();
On 2016/09/21 05:18:33, dcheng wrote:
> Is the intention to reclaim memory? What about using STLClearObject from
> base/stl_util.h here?

Done.

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/loader/...
File components/nacl/loader/nacl_helper_win_64.cc (right):

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/loader/...
components/nacl/loader/nacl_helper_win_64.cc:48:
mojo::edk::SetParentPipeHandle(std::move(platform_channel));
On 2016/09/21 05:18:33, dcheng wrote:
> I'm a bit confused about this one as well. The documentation says:
>   // Should be called as early as possible in the child process with the
handle
>   // that the parent received from ChildProcessLaunched.
> 
> But ChildProcessLaunched() doesn't appear to return anything or have any out
> values.

Previously, there was an overload of ChildProcessLaunched() that returned a
ScopedPlatformHandle to pass to the child instead of taking the parent end. That
comment seems to have been neglected.

From the mojo system's perspective, when launching a new process the embedder
needs to create a pipe (usually in the parent process) and pass one end to
ChildProcessLaunched() in the parent process and the other to
SetParentPipeHandle() in the child process. mojo::edk::PlatformChannelPair
provides an implementation of the pipe and passing one end to the child.

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:184: io_thread_.task_runner().get()));
On 2016/09/21 05:18:33, dcheng wrote:
> No .get() to avoid a scoped_refptr -> raw ptr -> scoped_refptr conversion

Done.

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):

https://codereview.chromium.org/2305913002/diff/80001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_listener.cc:46:
io_thread_.task_runner().get()));
On 2016/09/21 05:18:33, dcheng wrote:
> Ditto: omit .get()

Done.

[commit-bot: I haz the power, 2016-09-21 08:16:21]

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

[commit-bot: I haz the power, 2016-09-21 08:16:22]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-21 20:32:33]

On 2016/09/21 07:49:47, Sam McNally wrote:
> +jam for //content and new DEPS on //mojo.

lgtm

[dcheng, 2016-09-22 01:55:45]

lgtm

[Sam McNally, 2016-09-22 02:50:06]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-09-22 02:50:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org
Link to the patchset: https://codereview.chromium.org/2305913002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-09-22 02:50:32]

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-09-22 02:56:43]

Description was changed from

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

patch from issue 2081183005 at patchset 340001
(http://crrev.com/2081183005#ps340001)

BUG=604282
COLLABORATOR=amistry@chromium.org
==========

to

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

patch from issue 2081183005 at patchset 340001
(http://crrev.com/2081183005#ps340001)

BUG=604282
COLLABORATOR=amistry@chromium.org
==========

[commit-bot: I haz the power, 2016-09-22 02:56:44]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-09-22 02:59:34]

Description was changed from

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

patch from issue 2081183005 at patchset 340001
(http://crrev.com/2081183005#ps340001)

BUG=604282
COLLABORATOR=amistry@chromium.org
==========

to

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

patch from issue 2081183005 at patchset 340001
(http://crrev.com/2081183005#ps340001)

BUG=604282
COLLABORATOR=amistry@chromium.org

Committed: https://crrev.com/9e8780772cbf802e57c366eb8113d38af95ffd89
Cr-Commit-Position: refs/heads/master@{#420245}
==========

[commit-bot: I haz the power, 2016-09-22 02:59:35]

Patchset 6 (id:??) landed as
https://crrev.com/9e8780772cbf802e57c366eb8113d38af95ffd89
Cr-Commit-Position: refs/heads/master@{#420245}

[Ken Rockot(use gerrit already), 2016-09-26 21:21:06]

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

[Ken Rockot(use gerrit already), 2016-09-26 21:21:08]

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

https://codereview.chromium.org/2305913002/diff/140001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:315:
process_.reset(content::BrowserChildProcessHost::Create(
I'm not sure why this CL works, but this is not correct use of
BrowserChildProcessHost. It doesn't take a child token, it only takes a service
name. It assumes responsibility for generating its own child token and uses
that.

I noticed this from seeing logged errors from ServiceManager complaining that
someone is trying to connect to an invalid service name, the name being a random
token string.