Use ChannelMojo for plugin-renderer channels.

Use ChannelMojo for plugin-renderer channels.

BUG=604282

[Anand Mistry (off Chromium), 2016-06-24 04:02:46]

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

[Anand Mistry (off Chromium), 2016-06-24 04:02:46]

bbudge@chromium.org: Please review changes in pepper
yzshen@chromium.org: Please review changes in ppapi_thread.cc
dcheng: For security in case there are any concerns.

[yzshen1, 2016-06-24 16:12:03]

On 2016/06/24 04:02:46, Anand Mistry wrote:
> mailto:bbudge@chromium.org: Please review changes in pepper
> mailto:yzshen@chromium.org: Please review changes in ppapi_thread.cc
> dcheng: For security in case there are any concerns.

 ppapi_thread.cc change LGTM

[bbudge, 2016-06-24 17:41:56]

pepper lgtm

[dcheng, 2016-06-25 00:06:38]

I don't think I have any particular insight. Looks pretty straightforward to me?
I'm assuming this is the standard way of setting up the mojo ipc channel.

[Anand Mistry (off Chromium), 2016-06-25 00:51:10]

On 2016/06/25 00:06:38, dcheng wrote:
> I don't think I have any particular insight. Looks pretty straightforward to
me?
> I'm assuming this is the standard way of setting up the mojo ipc channel.

Yup, pretty much. Create a message pipe pair, use one side locally and the pass
the other. The real magic is in https://codereview.chromium.org/2047233002/
where IPC::ChannelHandle can contain a mojo message pipe handle.

[dcheng, 2016-06-27 02:12:48]

https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (left):

https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:530: // connection is not established.
How come these checks aren't needed in the Mojo version? What makes it
impossible?

[Anand Mistry (off Chromium), 2016-06-27 02:25:59]

https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (left):

https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:530: // connection is not established.
On 2016/06/27 02:12:48, dcheng wrote:
> How come these checks aren't needed in the Mojo version? What makes it
> impossible?

fds are not used when using ChannelMojo (and the returned fd will be -1 and
always fail this check). One day when ChannelMojo rules the world,
TakeRendererFD() and ChannelHandle::socket will both be gone.

[dcheng, 2016-06-27 03:35:03]

https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (left):

https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:530: // connection is not established.
On 2016/06/27 02:25:59, Anand Mistry wrote:
> On 2016/06/27 02:12:48, dcheng wrote:
> > How come these checks aren't needed in the Mojo version? What makes it
> > impossible?
> 
> fds are not used when using ChannelMojo (and the returned fd will be -1 and
> always fail this check). One day when ChannelMojo rules the world,
> TakeRendererFD() and ChannelHandle::socket will both be gone.

OK, so the Mojo handles here are always valid?

[Anand Mistry (off Chromium), 2016-06-27 04:52:07]

https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (left):

https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:530: // connection is not established.
On 2016/06/27 03:35:02, dcheng wrote:
> On 2016/06/27 02:25:59, Anand Mistry wrote:
> > On 2016/06/27 02:12:48, dcheng wrote:
> > > How come these checks aren't needed in the Mojo version? What makes it
> > > impossible?
> > 
> > fds are not used when using ChannelMojo (and the returned fd will be -1 and
> > always fail this check). One day when ChannelMojo rules the world,
> > TakeRendererFD() and ChannelHandle::socket will both be gone.
> 
> OK, so the Mojo handles here are always valid?

Here, yes. But not necessarily in the receive side, so I've added a check to
those.

[Anand Mistry (off Chromium), 2016-06-27 06:49:37]

On 2016/06/27 04:52:07, Anand Mistry wrote:
>
https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
> File content/ppapi_plugin/ppapi_thread.cc (left):
> 
>
https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
> content/ppapi_plugin/ppapi_thread.cc:530: // connection is not established.
> On 2016/06/27 03:35:02, dcheng wrote:
> > On 2016/06/27 02:25:59, Anand Mistry wrote:
> > > On 2016/06/27 02:12:48, dcheng wrote:
> > > > How come these checks aren't needed in the Mojo version? What makes it
> > > > impossible?
> > > 
> > > fds are not used when using ChannelMojo (and the returned fd will be -1
and
> > > always fail this check). One day when ChannelMojo rules the world,
> > > TakeRendererFD() and ChannelHandle::socket will both be gone.
> > 
> > OK, so the Mojo handles here are always valid?
> 
> Here, yes. But not necessarily in the receive side, so I've added a check to
> those.

Um.... oh crap!

[Anand Mistry (off Chromium), 2016-06-27 07:08:25]

On 2016/06/27 06:49:37, Anand Mistry wrote:
> On 2016/06/27 04:52:07, Anand Mistry wrote:
> >
>
https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
> > File content/ppapi_plugin/ppapi_thread.cc (left):
> > 
> >
>
https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
> > content/ppapi_plugin/ppapi_thread.cc:530: // connection is not established.
> > On 2016/06/27 03:35:02, dcheng wrote:
> > > On 2016/06/27 02:25:59, Anand Mistry wrote:
> > > > On 2016/06/27 02:12:48, dcheng wrote:
> > > > > How come these checks aren't needed in the Mojo version? What makes it
> > > > > impossible?
> > > > 
> > > > fds are not used when using ChannelMojo (and the returned fd will be -1
> and
> > > > always fail this check). One day when ChannelMojo rules the world,
> > > > TakeRendererFD() and ChannelHandle::socket will both be gone.
> > > 
> > > OK, so the Mojo handles here are always valid?
> > 
> > Here, yes. But not necessarily in the receive side, so I've added a check to
> > those.
> 
> Um.... oh crap!

Ok. So the channel still uses fds if it comes from a NaCl process. So this
change only handles one case which is a non-nacl pepper plugin. Do you think
it's worth holding back on this CL until NaCl is switched to ChannelMojo?

[dcheng, 2016-06-27 07:52:39]

On 2016/06/27 07:08:25, Anand Mistry wrote:
> On 2016/06/27 06:49:37, Anand Mistry wrote:
> > On 2016/06/27 04:52:07, Anand Mistry wrote:
> > >
> >
>
https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
> > > File content/ppapi_plugin/ppapi_thread.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
> > > content/ppapi_plugin/ppapi_thread.cc:530: // connection is not
established.
> > > On 2016/06/27 03:35:02, dcheng wrote:
> > > > On 2016/06/27 02:25:59, Anand Mistry wrote:
> > > > > On 2016/06/27 02:12:48, dcheng wrote:
> > > > > > How come these checks aren't needed in the Mojo version? What makes
it
> > > > > > impossible?
> > > > > 
> > > > > fds are not used when using ChannelMojo (and the returned fd will be
-1
> > and
> > > > > always fail this check). One day when ChannelMojo rules the world,
> > > > > TakeRendererFD() and ChannelHandle::socket will both be gone.
> > > > 
> > > > OK, so the Mojo handles here are always valid?
> > > 
> > > Here, yes. But not necessarily in the receive side, so I've added a check
to
> > > those.
> > 
> > Um.... oh crap!
> 
> Ok. So the channel still uses fds if it comes from a NaCl process. So this
> change only handles one case which is a non-nacl pepper plugin. Do you think
> it's worth holding back on this CL until NaCl is switched to ChannelMojo?

It's up to you, but if this isn't on the critical blocking path for anything, it
seems like it might be cleaner. Honestly, you probably know these bits better
than me though, so I'll defer to your judgement.

[Anand Mistry (off Chromium), 2016-06-28 09:45:24]

On 2016/06/27 07:52:39, dcheng wrote:
> On 2016/06/27 07:08:25, Anand Mistry wrote:
> > On 2016/06/27 06:49:37, Anand Mistry wrote:
> > > On 2016/06/27 04:52:07, Anand Mistry wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
> > > > File content/ppapi_plugin/ppapi_thread.cc (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2094623002/diff/60001/content/ppapi_plugin/pp...
> > > > content/ppapi_plugin/ppapi_thread.cc:530: // connection is not
> established.
> > > > On 2016/06/27 03:35:02, dcheng wrote:
> > > > > On 2016/06/27 02:25:59, Anand Mistry wrote:
> > > > > > On 2016/06/27 02:12:48, dcheng wrote:
> > > > > > > How come these checks aren't needed in the Mojo version? What
makes
> it
> > > > > > > impossible?
> > > > > > 
> > > > > > fds are not used when using ChannelMojo (and the returned fd will be
> -1
> > > and
> > > > > > always fail this check). One day when ChannelMojo rules the world,
> > > > > > TakeRendererFD() and ChannelHandle::socket will both be gone.
> > > > > 
> > > > > OK, so the Mojo handles here are always valid?
> > > > 
> > > > Here, yes. But not necessarily in the receive side, so I've added a
check
> to
> > > > those.
> > > 
> > > Um.... oh crap!
> > 
> > Ok. So the channel still uses fds if it comes from a NaCl process. So this
> > change only handles one case which is a non-nacl pepper plugin. Do you think
> > it's worth holding back on this CL until NaCl is switched to ChannelMojo?
> 
> It's up to you, but if this isn't on the critical blocking path for anything,
it
> seems like it might be cleaner. Honestly, you probably know these bits better
> than me though, so I'll defer to your judgement.

Thanks. I think I'll drop this CL for now.