Use ChannelMojo for NaCl PPAPI channels.

Use ChannelMojo for NaCl PPAPI channels.

This switches *all* IPC channels used by NaCl (both for SFI and Non-SFI
NaCl) to using Mojo channels instead of Chrome IPC channels.  Note that
for SFI NaCl, the IPC still goes through NaClIPCAdaptor (whose use of
threads could potentially be simplified in a later change now that the
underlying transport is Mojo IPC).  Also, messages are still marshalled
using Chrome IPC encoding rather than Mojo encoding.

Since NaClIPCAdapter now uses ChannelMojo, it needs to pass a
SingleThreadTaskRunner for the thread where Channel will live. Thus,
this CL changes the task runner type to a SingleThreadTaskRunner (which
is the type that was already being passed in).

patch from issue 2151153002 at patchset 40001 (http://crrev.com/2151153002#ps40001)

BUG=604282
COLLABORATOR=amistry@chromium.org

Committed: https://crrev.com/e51768ce8e2092b649426e20503ffc12e725ae6f
Cr-Commit-Position: refs/heads/master@{#425888}

[Sam McNally, 2016-09-02 00:26:56]

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:27:19]

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:55:51]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_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 02:49:08]

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

[commit-bot: I haz the power, 2016-09-02 02:49:25]

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 04:11:43]

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

[commit-bot: I haz the power, 2016-09-02 04:11:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_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 06:04:07]

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

[commit-bot: I haz the power, 2016-09-02 06:04:29]

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 07:10:50]

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

[commit-bot: I haz the power, 2016-09-02 07:10:51]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-09-05 07:16: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 07:16:23]

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 08:31:47]

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

[commit-bot: I haz the power, 2016-09-05 08:31:48]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-09-21 07:08:43]

Description was changed from

==========
Use ChannelMojo for NaCl IRT channels in non-sfi mode.

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

to

==========
Use ChannelMojo for NaCl IRT channels in non-sfi mode.

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

[Sam McNally, 2016-09-21 07:11:42]

Patchset #2 (id:20001) has been deleted

[Sam McNally, 2016-09-21 07:14:23]

Patchset #4 (id:80001) has been deleted

[Sam McNally, 2016-09-21 07:14:24]

Patchset #3 (id:60001) has been deleted

[Sam McNally, 2016-09-21 07:14:26]

Patchset #2 (id:40001) has been deleted

[Sam McNally, 2016-09-21 07:14:28]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-09-21 07:15:09]

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:15:38]

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:30:22]

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:30:46]

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:33:31]

Patchset #3 (id:140001) has been deleted

[Sam McNally, 2016-09-21 07:33:37]

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

[Sam McNally, 2016-09-21 07:33:42]

Patchset #2 (id:120001) has been deleted

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

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-21 09:31:27]

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

[commit-bot: I haz the power, 2016-09-21 09:31:28]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-09-21 09:42:39]

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

[Sam McNally, 2016-09-26 00:35:17]

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

[commit-bot: I haz the power, 2016-09-26 00:35:30]

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-26 01:26:27]

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sam McNally, 2016-09-26 01:49:31]

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

[commit-bot: I haz the power, 2016-09-26 01:49:46]

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-26 02:26:57]

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sam McNally, 2016-09-26 02:27:59]

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

[commit-bot: I haz the power, 2016-09-26 02:28:14]

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-26 03:41:31]

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

[commit-bot: I haz the power, 2016-09-26 03:41:32]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-10-04 03:40:51]

ping

[Sam McNally, 2016-10-11 01:55:53]

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

[Sam McNally, 2016-10-11 01:55:54]

+bradnelson

[Mark Seaborn, 2016-10-12 00:38:01]

Sorry for being slow to look at this.  I've got out of the habit of doing NaCl
code reviews.

Since you're changing nacl_listener.cc, not just nonsfi_listener.cc, I think the
commit message isn't accurate:  It looks like you're switching these channels to
use Mojo in all cases, not just the Non-SFI case.  Did I understand the change
correctly?

[Mark Seaborn, 2016-10-12 00:57:24]

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

https://codereview.chromium.org/2301103003/diff/200001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:259: // Only ChannelMojo is
supported.
It looks like this comment should apply to Windows too.

https://codereview.chromium.org/2301103003/diff/200001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:275: #if defined(OS_POSIX)
This block shouldn't be necessary if "Only ChannelMojo is supported" as noted
above.

https://codereview.chromium.org/2301103003/diff/200001/components/nacl/loader...
File components/nacl/loader/nacl_ipc_adapter.h (right):

https://codereview.chromium.org/2301103003/diff/200001/components/nacl/loader...
components/nacl/loader/nacl_ipc_adapter.h:113: const
scoped_refptr<base::SingleThreadTaskRunner>& runner,
Can you comment on the reason for this in the commit message, please?  Does Mojo
require a different type of TaskRunner, or is this just a cleanup?

https://codereview.chromium.org/2301103003/diff/200001/content/renderer/peppe...
File content/renderer/pepper/host_dispatcher_wrapper.cc (right):

https://codereview.chromium.org/2301103003/diff/200001/content/renderer/peppe...
content/renderer/pepper/host_dispatcher_wrapper.cc:45:
channel_handle.mojo_handle.is_valid());
Presumably this shouldn't be conditional on OS_POSIX any more?

Maybe IPC::ChannelHandle could provide a method for sanity-checking that
mojo_handle is filled out and the other fields aren't, since this check is
cropping up in multiple places?

[Sam McNally, 2016-10-12 01:18:25]

Description was changed from

==========
Use ChannelMojo for NaCl IRT channels in non-sfi mode.

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

to

==========
Use ChannelMojo for NaCl PPAPI channels.

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

[Sam McNally, 2016-10-12 01:48:41]

Description was changed from

==========
Use ChannelMojo for NaCl PPAPI channels.

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

to

==========
Use ChannelMojo for NaCl PPAPI channels.

Since NaClIPCAdapter now uses ChannelMojo, it needs to pass a
SingleThreadTaskRunner for the thread where Channel will live. Thus,
this CL changes the task runner type to a SingleThreadTaskRunner (which
is the type that was already being passed in).

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

[Sam McNally, 2016-10-12 01:52:28]

Description was changed from

==========
Use ChannelMojo for NaCl PPAPI channels.

Since NaClIPCAdapter now uses ChannelMojo, it needs to pass a
SingleThreadTaskRunner for the thread where Channel will live. Thus,
this CL changes the task runner type to a SingleThreadTaskRunner (which
is the type that was already being passed in).

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

to

==========
Use ChannelMojo for NaCl PPAPI channels.

Note that this only affects the channels used directly from the IRT for
non-SFI NaCl. For SFI NaCl, the IRT still communicates via an adapter;
what is changed in that case is the adapter uses ChannelMojo to
communicate with other processes.

Since NaClIPCAdapter now uses ChannelMojo, it needs to pass a
SingleThreadTaskRunner for the thread where Channel will live. Thus,
this CL changes the task runner type to a SingleThreadTaskRunner (which
is the type that was already being passed in).

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

[Sam McNally, 2016-10-12 02:32:57]

Patchset #6 (id:240001) has been deleted

[Sam McNally, 2016-10-12 02:36:00]

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

[commit-bot: I haz the power, 2016-10-12 02:36:12]

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-10-12 02:36:16]

Description was changed from

==========
Use ChannelMojo for NaCl PPAPI channels.

Note that this only affects the channels used directly from the IRT for
non-SFI NaCl. For SFI NaCl, the IRT still communicates via an adapter;
what is changed in that case is the adapter uses ChannelMojo to
communicate with other processes.

Since NaClIPCAdapter now uses ChannelMojo, it needs to pass a
SingleThreadTaskRunner for the thread where Channel will live. Thus,
this CL changes the task runner type to a SingleThreadTaskRunner (which
is the type that was already being passed in).

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

to

==========
Use ChannelMojo for NaCl PPAPI channels.

Note that this only affects the channels used directly from the IRT for
non-SFI NaCl. For SFI NaCl, the IRT still communicates via an adapter;
what is changed in that case is the adapter uses ChannelMojo to
communicate with other processes.

Since NaClIPCAdapter now uses ChannelMojo, it needs to pass a
SingleThreadTaskRunner for the thread where Channel will live. Thus,
this CL changes the task runner type to a SingleThreadTaskRunner (which
is the type that was already being passed in).

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

[Sam McNally, 2016-10-12 03:15:32]

On 2016/10/12 00:38:01, Mark Seaborn wrote:
> Sorry for being slow to look at this.  I've got out of the habit of doing NaCl
> code reviews.
> 
> Since you're changing nacl_listener.cc, not just nonsfi_listener.cc, I think
the
> commit message isn't accurate:  It looks like you're switching these channels
to
> use Mojo in all cases, not just the Non-SFI case.  Did I understand the change
> correctly?

Yes. Updated the description.

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

https://codereview.chromium.org/2301103003/diff/200001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:259: // Only ChannelMojo is
supported.
On 2016/10/12 00:57:24, Mark Seaborn wrote:
> It looks like this comment should apply to Windows too.

Done.

https://codereview.chromium.org/2301103003/diff/200001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:275: #if defined(OS_POSIX)
On 2016/10/12 00:57:24, Mark Seaborn wrote:
> This block shouldn't be necessary if "Only ChannelMojo is supported" as noted
> above.

Done.

https://codereview.chromium.org/2301103003/diff/200001/components/nacl/loader...
File components/nacl/loader/nacl_ipc_adapter.h (right):

https://codereview.chromium.org/2301103003/diff/200001/components/nacl/loader...
components/nacl/loader/nacl_ipc_adapter.h:113: const
scoped_refptr<base::SingleThreadTaskRunner>& runner,
On 2016/10/12 00:57:24, Mark Seaborn wrote:
> Can you comment on the reason for this in the commit message, please?  Does
Mojo
> require a different type of TaskRunner, or is this just a cleanup?

ChannelMojo expects to receive the task runner that it's going to used on,
rather than using the thread where Connect() is called (like the other Channel
implementations). The default is to use the current thread, which was incorrect
here, but ignored for other channel types. The task runner parameter for
creating the channel is a SingleThreadTaskRunner, which is the type that's
passed to this constructor anyway.

Changing to a scoped_refptr is just cleanup.

https://codereview.chromium.org/2301103003/diff/200001/content/renderer/peppe...
File content/renderer/pepper/host_dispatcher_wrapper.cc (right):

https://codereview.chromium.org/2301103003/diff/200001/content/renderer/peppe...
content/renderer/pepper/host_dispatcher_wrapper.cc:45:
channel_handle.mojo_handle.is_valid());
On 2016/10/12 00:57:24, Mark Seaborn wrote:
> Presumably this shouldn't be conditional on OS_POSIX any more?
> 
> Maybe IPC::ChannelHandle could provide a method for sanity-checking that
> mojo_handle is filled out and the other fields aren't, since this check is
> cropping up in multiple places?

Allowing non-ChannelMojo handles is still necessary for non-NaCl plugins.
https://codereview.chromium.org/2302053004/ will change that and simplify this
check.

[commit-bot: I haz the power, 2016-10-12 03:28:53]

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

[commit-bot: I haz the power, 2016-10-12 03:28:54]

Dry run: This issue passed the CQ dry run.

[Mark Seaborn, 2016-10-13 20:59:36]

On 11 October 2016 at 20:15, <sammc@chromium.org> wrote:

> On 2016/10/12 00:38:01, Mark Seaborn wrote:
> > Sorry for being slow to look at this. I've got out of the habit of doing
> NaCl
> > code reviews.
> >
> > Since you're changing nacl_listener.cc, not just nonsfi_listener.cc, I
> think the
> > commit message isn't accurate: It looks like you're switching these
> channels to
> > use Mojo in all cases, not just the Non-SFI case. Did I understand the
> change
> > correctly?
>
> Yes. Updated the description.
>

The description now says:  "Note that this only affects the channels used
directly from the IRT for non-SFI NaCl. For SFI NaCl, the IRT still
communicates via an adapter; what is changed in that case is the adapter
uses ChannelMojo to communicate with other processes."

That first sentence is misleading, because you're still saying that this
only affects Non-SFI NaCl, which isn't the case.

How about saying something like this:  "This switches *all* IPC channels
used by NaCl (both for SFI and Non-SFI NaCl) to using Mojo channels instead
of Chrome IPC channels.  Note that for SFI NaCl, the IPC still goes through
NaClIPCAdaptor (whose use of threads could potentially be simplified in a
later change now that the underlying transport is Mojo IPC).  Also,
messages are still marshalled using Chrome IPC encoding rather than Mojo
encoding."

Is that correct, or are there uses of Chrome IPC channels that are not
switched over by this change?  Did I use the right terminology regarding
types of channel?

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sam McNally, 2016-10-13 21:16:43]

Description was changed from

==========
Use ChannelMojo for NaCl PPAPI channels.

Note that this only affects the channels used directly from the IRT for
non-SFI NaCl. For SFI NaCl, the IRT still communicates via an adapter;
what is changed in that case is the adapter uses ChannelMojo to
communicate with other processes.

Since NaClIPCAdapter now uses ChannelMojo, it needs to pass a
SingleThreadTaskRunner for the thread where Channel will live. Thus,
this CL changes the task runner type to a SingleThreadTaskRunner (which
is the type that was already being passed in).

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

to

==========
Use ChannelMojo for NaCl PPAPI channels.

This switches *all* IPC channels used by NaCl (both for SFI and Non-SFI
NaCl) to using Mojo channels instead of Chrome IPC channels.  Note that
for SFI NaCl, the IPC still goes through NaClIPCAdaptor (whose use of
threads could potentially be simplified in a later change now that the
underlying transport is Mojo IPC).  Also, messages are still marshalled
using Chrome IPC encoding rather than Mojo encoding.

Since NaClIPCAdapter now uses ChannelMojo, it needs to pass a
SingleThreadTaskRunner for the thread where Channel will live. Thus,
this CL changes the task runner type to a SingleThreadTaskRunner (which
is the type that was already being passed in).

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

[Sam McNally, 2016-10-13 21:23:01]

On 2016/10/13 20:59:36, Mark Seaborn wrote:
> On 11 October 2016 at 20:15, <mailto:sammc@chromium.org> wrote:
> 
> > On 2016/10/12 00:38:01, Mark Seaborn wrote:
> > > Sorry for being slow to look at this. I've got out of the habit of doing
> > NaCl
> > > code reviews.
> > >
> > > Since you're changing nacl_listener.cc, not just nonsfi_listener.cc, I
> > think the
> > > commit message isn't accurate: It looks like you're switching these
> > channels to
> > > use Mojo in all cases, not just the Non-SFI case. Did I understand the
> > change
> > > correctly?
> >
> > Yes. Updated the description.
> >
> 
> The description now says:  "Note that this only affects the channels used
> directly from the IRT for non-SFI NaCl. For SFI NaCl, the IRT still
> communicates via an adapter; what is changed in that case is the adapter
> uses ChannelMojo to communicate with other processes."
> 
> That first sentence is misleading, because you're still saying that this
> only affects Non-SFI NaCl, which isn't the case.
> 
> How about saying something like this:  "This switches *all* IPC channels
> used by NaCl (both for SFI and Non-SFI NaCl) to using Mojo channels instead
> of Chrome IPC channels.  Note that for SFI NaCl, the IPC still goes through
> NaClIPCAdaptor (whose use of threads could potentially be simplified in a
> later change now that the underlying transport is Mojo IPC).  Also,
> messages are still marshalled using Chrome IPC encoding rather than Mojo
> encoding."
> 
> Is that correct, or are there uses of Chrome IPC channels that are not
> switched over by this change?  Did I use the right terminology regarding
> types of channel?

Yes. Done.
> 
> Cheers,
> Mark
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2016-10-13 21:27:52]

Do you mind if I get https://codereview.chromium.org/1882753002/ landed first,
to make sure that the Non-SFI case doesn't get broken?

https://codereview.chromium.org/2301103003/diff/200001/components/nacl/loader...
File components/nacl/loader/nacl_ipc_adapter.h (right):

https://codereview.chromium.org/2301103003/diff/200001/components/nacl/loader...
components/nacl/loader/nacl_ipc_adapter.h:113: const
scoped_refptr<base::SingleThreadTaskRunner>& runner,
On 2016/10/12 03:15:32, Sam McNally wrote:
> On 2016/10/12 00:57:24, Mark Seaborn wrote:
> > Can you comment on the reason for this in the commit message, please?  Does
> Mojo
> > require a different type of TaskRunner, or is this just a cleanup?
> 
> ChannelMojo expects to receive the task runner that it's going to used on,
> rather than using the thread where Connect() is called (like the other Channel
> implementations).

Ah, that's a nice improvement.  I remember that Chrome IPC's reuse of the thread
that called Connect() has caused some big contortions in the code.

https://codereview.chromium.org/2301103003/diff/200001/content/renderer/peppe...
File content/renderer/pepper/host_dispatcher_wrapper.cc (right):

https://codereview.chromium.org/2301103003/diff/200001/content/renderer/peppe...
content/renderer/pepper/host_dispatcher_wrapper.cc:45:
channel_handle.mojo_handle.is_valid());
On 2016/10/12 03:15:32, Sam McNally wrote:
> On 2016/10/12 00:57:24, Mark Seaborn wrote:
> > Presumably this shouldn't be conditional on OS_POSIX any more?
> > 
> > Maybe IPC::ChannelHandle could provide a method for sanity-checking that
> > mojo_handle is filled out and the other fields aren't, since this check is
> > cropping up in multiple places?
> 
> Allowing non-ChannelMojo handles is still necessary for non-NaCl plugins.
> https://codereview.chromium.org/2302053004/ will change that and simplify this
> check.

OK.  My comment still stands for other places where the code is checking
channel_handle.mojo_handle.is_valid() -- it seems like it would be nice if those
could also check that the other fields are not filled out, given that
IPC::ChannelHandle has become fairly complex now.  What do you think?

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

https://codereview.chromium.org/2301103003/diff/260001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:219: class
NaClProcessHost::ScopedChannelHandle {
Presumably at some point this could be removed and we could just use a
ScopedMessagePipeHandle?  Not sure if that's worth a TODO to point that out.

https://codereview.chromium.org/2301103003/diff/260001/components/nacl/loader...
File components/nacl/loader/nacl_listener.cc (left):

https://codereview.chromium.org/2301103003/diff/260001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:369:
trusted_listener_->TakeClientChannelHandle(),
You should be able to remove TakeClientChannelHandle() from NaClTrustedListener.

https://codereview.chromium.org/2301103003/diff/260001/ppapi/nacl_irt/plugin_...
File ppapi/nacl_irt/plugin_startup.cc (right):

https://codereview.chromium.org/2301103003/diff/260001/ppapi/nacl_irt/plugin_...
ppapi/nacl_irt/plugin_startup.cc:30: return handle && (handle->socket.fd != -1
|| handle->mojo_handle.is_valid());
Shouldn't this only accept Mojo handles?

[Sam McNally, 2016-10-14 02:35:03]

On 2016/10/13 21:27:52, Mark Seaborn wrote:
> Do you mind if I get https://codereview.chromium.org/1882753002/ landed first,
> to make sure that the Non-SFI case doesn't get broken?
> 
Sure.

https://codereview.chromium.org/2301103003/diff/200001/content/renderer/peppe...
File content/renderer/pepper/host_dispatcher_wrapper.cc (right):

https://codereview.chromium.org/2301103003/diff/200001/content/renderer/peppe...
content/renderer/pepper/host_dispatcher_wrapper.cc:45:
channel_handle.mojo_handle.is_valid());
On 2016/10/13 21:27:51, Mark Seaborn wrote:
> On 2016/10/12 03:15:32, Sam McNally wrote:
> > On 2016/10/12 00:57:24, Mark Seaborn wrote:
> > > Presumably this shouldn't be conditional on OS_POSIX any more?
> > > 
> > > Maybe IPC::ChannelHandle could provide a method for sanity-checking that
> > > mojo_handle is filled out and the other fields aren't, since this check is
> > > cropping up in multiple places?
> > 
> > Allowing non-ChannelMojo handles is still necessary for non-NaCl plugins.
> > https://codereview.chromium.org/2302053004/ will change that and simplify
this
> > check.
> 
> OK.  My comment still stands for other places where the code is checking
> channel_handle.mojo_handle.is_valid() -- it seems like it would be nice if
those
> could also check that the other fields are not filled out, given that
> IPC::ChannelHandle has become fairly complex now.  What do you think?

Done. ChannelHandle shouldn't even allow this, but I don't think it's worth
changing at this point.

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

https://codereview.chromium.org/2301103003/diff/260001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:219: class
NaClProcessHost::ScopedChannelHandle {
On 2016/10/13 21:27:51, Mark Seaborn wrote:
> Presumably at some point this could be removed and we could just use a
> ScopedMessagePipeHandle?  Not sure if that's worth a TODO to point that out.

How about now? :)

https://codereview.chromium.org/2301103003/diff/260001/components/nacl/loader...
File components/nacl/loader/nacl_listener.cc (left):

https://codereview.chromium.org/2301103003/diff/260001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:369:
trusted_listener_->TakeClientChannelHandle(),
On 2016/10/13 21:27:51, Mark Seaborn wrote:
> You should be able to remove TakeClientChannelHandle() from
NaClTrustedListener.

Done.

https://codereview.chromium.org/2301103003/diff/260001/ppapi/nacl_irt/plugin_...
File ppapi/nacl_irt/plugin_startup.cc (right):

https://codereview.chromium.org/2301103003/diff/260001/ppapi/nacl_irt/plugin_...
ppapi/nacl_irt/plugin_startup.cc:30: return handle && (handle->socket.fd != -1
|| handle->mojo_handle.is_valid());
On 2016/10/13 21:27:51, Mark Seaborn wrote:
> Shouldn't this only accept Mojo handles?

It's used by both SFI and non-SFI. In SFI mode they still look like FDs. I've
added ifdefs to make it clearer.

[Sam McNally, 2016-10-17 04:56:45]

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

[commit-bot: I haz the power, 2016-10-17 04:56:53]

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-10-17 06:02:46]

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

[commit-bot: I haz the power, 2016-10-17 06:02:47]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sam McNally, 2016-10-17 07:13:49]

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

[commit-bot: I haz the power, 2016-10-17 07:14:05]

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-10-17 08:09:37]

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

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

Dry run: This issue passed the CQ dry run.

[Mark Seaborn, 2016-10-17 21:28:27]

LGTM, thanks

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

https://codereview.chromium.org/2301103003/diff/320001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:904: // Note: here, because some
FDs/handles for the NaCl loader process are
I think you can remove this comment, since you removed the need for that
"has_error" flag.  Apparently, in the new code path, creating handle pairs can't
fail.

https://codereview.chromium.org/2301103003/diff/320001/ipc/ipc_channel_handle.h
File ipc/ipc_channel_handle.h (right):

https://codereview.chromium.org/2301103003/diff/320001/ipc/ipc_channel_handle...
ipc/ipc_channel_handle.h:61: return mojo_handle.is_valid();
How about also: "&& name.empty()"

[Sam McNally, 2016-10-17 22:25:38]

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

[commit-bot: I haz the power, 2016-10-17 22:26:29]

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-10-17 22:49:59]

Patchset #10 (id:340001) has been deleted

[Sam McNally, 2016-10-17 22:50:18]

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

[commit-bot: I haz the power, 2016-10-17 22:50:53]

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-10-17 23:01:16]

sammc@chromium.org changed reviewers:
+ raymes@chromium.org, rockot@chromium.org
- bradnelson@chromium.org

[Sam McNally, 2016-10-17 23:01:17]

+raymes for content/renderer/pepper/host_dispatcher_wrapper.cc

+rockot for //ipc

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

https://codereview.chromium.org/2301103003/diff/320001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:904: // Note: here, because some
FDs/handles for the NaCl loader process are
On 2016/10/17 21:28:26, Mark Seaborn wrote:
> I think you can remove this comment, since you removed the need for that
> "has_error" flag.  Apparently, in the new code path, creating handle pairs
can't
> fail.

Done.

https://codereview.chromium.org/2301103003/diff/320001/ipc/ipc_channel_handle.h
File ipc/ipc_channel_handle.h (right):

https://codereview.chromium.org/2301103003/diff/320001/ipc/ipc_channel_handle...
ipc/ipc_channel_handle.h:61: return mojo_handle.is_valid();
On 2016/10/17 21:28:26, Mark Seaborn wrote:
> How about also: "&& name.empty()"

Done.

[raymes, 2016-10-17 23:07:23]

https://codereview.chromium.org/2301103003/diff/360001/content/renderer/peppe...
File content/renderer/pepper/host_dispatcher_wrapper.cc (left):

https://codereview.chromium.org/2301103003/diff/360001/content/renderer/peppe...
content/renderer/pepper/host_dispatcher_wrapper.cc:40: if
(channel_handle.name.empty())
if (channel_handle.name.empty() && !channel_handle.mojo_handle.is_valid()) ?

[Sam McNally, 2016-10-17 23:12:43]

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

[Sam McNally, 2016-10-17 23:12:52]

https://codereview.chromium.org/2301103003/diff/360001/content/renderer/peppe...
File content/renderer/pepper/host_dispatcher_wrapper.cc (left):

https://codereview.chromium.org/2301103003/diff/360001/content/renderer/peppe...
content/renderer/pepper/host_dispatcher_wrapper.cc:40: if
(channel_handle.name.empty())
On 2016/10/17 23:07:23, raymes wrote:
> if (channel_handle.name.empty() && !channel_handle.mojo_handle.is_valid()) ?

Done (using is_mojo_channel_handle() here and below).

[commit-bot: I haz the power, 2016-10-17 23:13:39]

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-10-17 23:18:09]

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

[commit-bot: I haz the power, 2016-10-17 23:19:06]

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-10-17 23:19:17]

Patchset #12 (id:400001) has been deleted

[Sam McNally, 2016-10-17 23:19:47]

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

[commit-bot: I haz the power, 2016-10-17 23:20:16]

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

[raymes, 2016-10-17 23:20:32]

lgtm

[commit-bot: I haz the power, 2016-10-18 00:26:10]

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

[commit-bot: I haz the power, 2016-10-18 00:26:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sam McNally, 2016-10-18 00:30:30]

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

[commit-bot: I haz the power, 2016-10-18 00:31:04]

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-10-18 02:10:48]

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

[commit-bot: I haz the power, 2016-10-18 02:10:50]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-10-18 04:48:14]

lgtm

[Sam McNally, 2016-10-18 04:51:09]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-10-18 04:51:10]

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/2301103003/#ps350015
(title: " ")

[commit-bot: I haz the power, 2016-10-18 04:51:31]

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-10-18 05:49:00]

Description was changed from

==========
Use ChannelMojo for NaCl PPAPI channels.

This switches *all* IPC channels used by NaCl (both for SFI and Non-SFI
NaCl) to using Mojo channels instead of Chrome IPC channels.  Note that
for SFI NaCl, the IPC still goes through NaClIPCAdaptor (whose use of
threads could potentially be simplified in a later change now that the
underlying transport is Mojo IPC).  Also, messages are still marshalled
using Chrome IPC encoding rather than Mojo encoding.

Since NaClIPCAdapter now uses ChannelMojo, it needs to pass a
SingleThreadTaskRunner for the thread where Channel will live. Thus,
this CL changes the task runner type to a SingleThreadTaskRunner (which
is the type that was already being passed in).

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

to

==========
Use ChannelMojo for NaCl PPAPI channels.

This switches *all* IPC channels used by NaCl (both for SFI and Non-SFI
NaCl) to using Mojo channels instead of Chrome IPC channels.  Note that
for SFI NaCl, the IPC still goes through NaClIPCAdaptor (whose use of
threads could potentially be simplified in a later change now that the
underlying transport is Mojo IPC).  Also, messages are still marshalled
using Chrome IPC encoding rather than Mojo encoding.

Since NaClIPCAdapter now uses ChannelMojo, it needs to pass a
SingleThreadTaskRunner for the thread where Channel will live. Thus,
this CL changes the task runner type to a SingleThreadTaskRunner (which
is the type that was already being passed in).

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

[commit-bot: I haz the power, 2016-10-18 05:49:02]

Committed patchset #12 (id:350015)

[commit-bot: I haz the power, 2016-10-18 05:50:31]

Description was changed from

==========
Use ChannelMojo for NaCl PPAPI channels.

This switches *all* IPC channels used by NaCl (both for SFI and Non-SFI
NaCl) to using Mojo channels instead of Chrome IPC channels.  Note that
for SFI NaCl, the IPC still goes through NaClIPCAdaptor (whose use of
threads could potentially be simplified in a later change now that the
underlying transport is Mojo IPC).  Also, messages are still marshalled
using Chrome IPC encoding rather than Mojo encoding.

Since NaClIPCAdapter now uses ChannelMojo, it needs to pass a
SingleThreadTaskRunner for the thread where Channel will live. Thus,
this CL changes the task runner type to a SingleThreadTaskRunner (which
is the type that was already being passed in).

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

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

to

==========
Use ChannelMojo for NaCl PPAPI channels.

This switches *all* IPC channels used by NaCl (both for SFI and Non-SFI
NaCl) to using Mojo channels instead of Chrome IPC channels.  Note that
for SFI NaCl, the IPC still goes through NaClIPCAdaptor (whose use of
threads could potentially be simplified in a later change now that the
underlying transport is Mojo IPC).  Also, messages are still marshalled
using Chrome IPC encoding rather than Mojo encoding.

Since NaClIPCAdapter now uses ChannelMojo, it needs to pass a
SingleThreadTaskRunner for the thread where Channel will live. Thus,
this CL changes the task runner type to a SingleThreadTaskRunner (which
is the type that was already being passed in).

patch from issue 2151153002 at patchset 40001
(http://crrev.com/2151153002#ps40001)

BUG=604282
COLLABORATOR=amistry@chromium.org

Committed: https://crrev.com/e51768ce8e2092b649426e20503ffc12e725ae6f
Cr-Commit-Position: refs/heads/master@{#425888}
==========

[commit-bot: I haz the power, 2016-10-18 05:50:33]

Patchset 12 (id:??) landed as
https://crrev.com/e51768ce8e2092b649426e20503ffc12e725ae6f
Cr-Commit-Position: refs/heads/master@{#425888}