mac/linux: rework plugin channel file descriptor creation

mac/linux: rework plugin channel file descriptor creation

This CL fixes a bug where the same renderer could open several channels to the same plugin process, which end up having the same name. This CL makes it that there is only one channel for each (plugin, renderer) pair, just like on Windows.
The socketpair is created in the plugin process (which can ensure that only one channel per renderer gets created), and sends the renderer side through the browser process.

Note: this should essentially be a noop on Windows.

[Evan Martin, 2009-06-26 19:18:31]

http://codereview.chromium.org/149062/diff/1/2
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/149062/diff/1/2#newcode549
Line 549: const IPC::ChannelHandle &channel_handle) {
& on left

http://codereview.chromium.org/149062/diff/1/2#newcode552
Line 552: #ifdef OS_POSIX
if defined(...)

http://codereview.chromium.org/149062/diff/1/2#newcode555
Line 555: // dup()'ed FD gets closed.
I don't quite understand this.  ipc_message_utils says fds coming in from IPC
have auto_close=true.

http://codereview.chromium.org/149062/diff/1/2#newcode560
Line 560: channel_handle,
Should this be using reply_handle?

http://codereview.chromium.org/149062/diff/1/3
File chrome/browser/plugin_process_host.h (right):

http://codereview.chromium.org/149062/diff/1/3#newcode100
Line 100: void OnChannelCreated(const IPC::ChannelHandle &channel_handle);
& on left

http://codereview.chromium.org/149062/diff/1/5
File chrome/plugin/plugin_channel.cc (right):

http://codereview.chromium.org/149062/diff/1/5#newcode36
Line 36: #ifdef OS_POSIX
if defined

http://codereview.chromium.org/149062/diff/1/5#newcode50
Line 50: #ifdef OS_POSIX
if defined

http://codereview.chromium.org/149062/diff/1/5#newcode52
Line 52: if (renderer_fd_) {
You use -1 for "unset", so I think you should test for that here.  (0 is also a
valid fd...)

http://codereview.chromium.org/149062/diff/1/5#newcode153
Line 153: #ifdef OS_POSIX
if defined

http://codereview.chromium.org/149062/diff/1/5#newcode156
Line 156: // name. Keep the renderer side FD in the channel to be able to
transmit it
"in the channel" is a bit confusing, you mean as a member variable, yeah?

http://codereview.chromium.org/149062/diff/1/6
File chrome/plugin/plugin_channel.h (right):

http://codereview.chromium.org/149062/diff/1/6#newcode32
Line 32: #ifdef OS_POSIX
if defined

http://codereview.chromium.org/149062/diff/1/6#newcode76
Line 76: #ifdef OS_POSIX
if defined

http://codereview.chromium.org/149062/diff/1/6#newcode77
Line 77: // FD for the renderer side.
Could you elaborate on this one?  That is the local end of the pipe, or the fd
that will be passed over IPC to the other end?  Is it only for before the IPC
goes through, then cleared?  etc.

[jam, 2009-06-26 19:38:40]

lgtm.  yeah it's very important that there's only one channel between a plugin
and renderer, there are assumptions about that and if this breaks, deadlocks can
occur in some plugins (i.e. see
http://code.google.com/p/chromium/issues/detail?id=12624).  It would be great if
there could be a regression test for this, but we don't have hooks into this
information at the moment.

[Antoine Labour, 2009-06-26 20:23:46]

John: today, if multiple channels are created for the same renderer, the plugin
process will assert on mac and linux (in the code that maps channel names to
FDs).
Unit tests would be better indeed. I'll see what I can do there, but I'm not
sure that channel name -> FD map works in the single process case, and even the
channel name is generated using process IDs.
Is there an easy setup to do unit tests with multiple processes ?

http://codereview.chromium.org/149062/diff/1/2
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/149062/diff/1/2#newcode549
Line 549: const IPC::ChannelHandle &channel_handle) {
On 2009/06/26 19:18:31, Evan Martin wrote:
> & on left

Done.

http://codereview.chromium.org/149062/diff/1/2#newcode552
Line 552: #ifdef OS_POSIX
On 2009/06/26 19:18:31, Evan Martin wrote:
> if defined(...)

Done.

http://codereview.chromium.org/149062/diff/1/2#newcode555
Line 555: // dup()'ed FD gets closed.
On 2009/06/26 19:18:31, Evan Martin wrote:
> I don't quite understand this.  ipc_message_utils says fds coming in from IPC
> have auto_close=true.

Oh. I don't read docs, only code :) Message::ReadFileDescriptor implemented
otherwise. I fixed it to match the docs, I like it better that way.

http://codereview.chromium.org/149062/diff/1/2#newcode560
Line 560: channel_handle,
On 2009/06/26 19:18:31, Evan Martin wrote:
> Should this be using reply_handle?

Yes it should have. But with the modifications, it's channel_handle again.

http://codereview.chromium.org/149062/diff/1/3
File chrome/browser/plugin_process_host.h (right):

http://codereview.chromium.org/149062/diff/1/3#newcode100
Line 100: void OnChannelCreated(const IPC::ChannelHandle &channel_handle);
On 2009/06/26 19:18:31, Evan Martin wrote:
> & on left

Done.

http://codereview.chromium.org/149062/diff/1/5
File chrome/plugin/plugin_channel.cc (right):

http://codereview.chromium.org/149062/diff/1/5#newcode36
Line 36: #ifdef OS_POSIX
On 2009/06/26 19:18:31, Evan Martin wrote:
> if defined

Done.

http://codereview.chromium.org/149062/diff/1/5#newcode50
Line 50: #ifdef OS_POSIX
On 2009/06/26 19:18:31, Evan Martin wrote:
> if defined

Done.

http://codereview.chromium.org/149062/diff/1/5#newcode52
Line 52: if (renderer_fd_) {
On 2009/06/26 19:18:31, Evan Martin wrote:
> You use -1 for "unset", so I think you should test for that here.  (0 is also
a
> valid fd...)

Doh! done.

http://codereview.chromium.org/149062/diff/1/5#newcode153
Line 153: #ifdef OS_POSIX
On 2009/06/26 19:18:31, Evan Martin wrote:
> if defined

Done.

http://codereview.chromium.org/149062/diff/1/5#newcode156
Line 156: // name. Keep the renderer side FD in the channel to be able to
transmit it
On 2009/06/26 19:18:31, Evan Martin wrote:
> "in the channel" is a bit confusing, you mean as a member variable, yeah?

Yeah. Fixed the docs.

http://codereview.chromium.org/149062/diff/1/6
File chrome/plugin/plugin_channel.h (right):

http://codereview.chromium.org/149062/diff/1/6#newcode32
Line 32: #ifdef OS_POSIX
On 2009/06/26 19:18:31, Evan Martin wrote:
> if defined

Done.

http://codereview.chromium.org/149062/diff/1/6#newcode76
Line 76: #ifdef OS_POSIX
On 2009/06/26 19:18:31, Evan Martin wrote:
> if defined

Done.

http://codereview.chromium.org/149062/diff/1/6#newcode77
Line 77: // FD for the renderer side.
On 2009/06/26 19:18:31, Evan Martin wrote:
> Could you elaborate on this one?  That is the local end of the pipe, or the fd
> that will be passed over IPC to the other end?  Is it only for before the IPC
> goes through, then cleared?  etc.

Added some extra docs.

[Evan Martin, 2009-06-26 20:33:48]

On 2009/06/26 20:23:46, Antoine Labour wrote:
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > I don't quite understand this.  ipc_message_utils says fds coming in from
IPC
> > have auto_close=true.
> 
> Oh. I don't read docs, only code :) Message::ReadFileDescriptor implemented
> otherwise. I fixed it to match the docs, I like it better that way.

What about the other uses of FileDescriptor?  (It's used in IPC.)

I asked agl about this and he says the code was probably wrong.  Since the
behavior now relative the the behavior before is that we might close sockets
that others were depending on, he thinks it's ok if no tests fail.

[Evan Martin, 2009-06-26 20:36:12]

LGTM if you've checked that we can still create and close tabs

[jam, 2009-06-26 20:48:58]

On 2009/06/26 20:23:46, Antoine Labour wrote:
> John: today, if multiple channels are created for the same renderer, the
plugin
> process will assert on mac and linux (in the code that maps channel names to
> FDs).
> Unit tests would be better indeed. I'll see what I can do there, but I'm not
> sure that channel name -> FD map works in the single process case, and even
the
> channel name is generated using process IDs.

In Windows, single-process mode means everything runs in one process (renderers
& plugins).  I vaguely remember Evan saying this isn't possible because there
can only be one UI thread in a process, so you guys still have to run plugins in
a different process?  I'm not sure why it wouldn't work in that case, each
RenderProcessHost will have a fake renderer id so I assume there'll be a
different channel for it.

> Is there an easy setup to do unit tests with multiple processes ?

UI tests run chrome in multi-process mode, and there's also
in-browser-unit-tests, which are similar but more powerful since a binary is
loaded into the browser process so it has access to globals etc (ui tests just
control the browser using the automation API).

> 
> http://codereview.chromium.org/149062/diff/1/2
> File chrome/browser/plugin_process_host.cc (right):
> 
> http://codereview.chromium.org/149062/diff/1/2#newcode549
> Line 549: const IPC::ChannelHandle &channel_handle) {
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > & on left
> 
> Done.
> 
> http://codereview.chromium.org/149062/diff/1/2#newcode552
> Line 552: #ifdef OS_POSIX
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > if defined(...)
> 
> Done.
> 
> http://codereview.chromium.org/149062/diff/1/2#newcode555
> Line 555: // dup()'ed FD gets closed.
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > I don't quite understand this.  ipc_message_utils says fds coming in from
IPC
> > have auto_close=true.
> 
> Oh. I don't read docs, only code :) Message::ReadFileDescriptor implemented
> otherwise. I fixed it to match the docs, I like it better that way.
> 
> http://codereview.chromium.org/149062/diff/1/2#newcode560
> Line 560: channel_handle,
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > Should this be using reply_handle?
> 
> Yes it should have. But with the modifications, it's channel_handle again.
> 
> http://codereview.chromium.org/149062/diff/1/3
> File chrome/browser/plugin_process_host.h (right):
> 
> http://codereview.chromium.org/149062/diff/1/3#newcode100
> Line 100: void OnChannelCreated(const IPC::ChannelHandle &channel_handle);
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > & on left
> 
> Done.
> 
> http://codereview.chromium.org/149062/diff/1/5
> File chrome/plugin/plugin_channel.cc (right):
> 
> http://codereview.chromium.org/149062/diff/1/5#newcode36
> Line 36: #ifdef OS_POSIX
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > if defined
> 
> Done.
> 
> http://codereview.chromium.org/149062/diff/1/5#newcode50
> Line 50: #ifdef OS_POSIX
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > if defined
> 
> Done.
> 
> http://codereview.chromium.org/149062/diff/1/5#newcode52
> Line 52: if (renderer_fd_) {
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > You use -1 for "unset", so I think you should test for that here.  (0 is
also
> a
> > valid fd...)
> 
> Doh! done.
> 
> http://codereview.chromium.org/149062/diff/1/5#newcode153
> Line 153: #ifdef OS_POSIX
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > if defined
> 
> Done.
> 
> http://codereview.chromium.org/149062/diff/1/5#newcode156
> Line 156: // name. Keep the renderer side FD in the channel to be able to
> transmit it
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > "in the channel" is a bit confusing, you mean as a member variable, yeah?
> 
> Yeah. Fixed the docs.
> 
> http://codereview.chromium.org/149062/diff/1/6
> File chrome/plugin/plugin_channel.h (right):
> 
> http://codereview.chromium.org/149062/diff/1/6#newcode32
> Line 32: #ifdef OS_POSIX
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > if defined
> 
> Done.
> 
> http://codereview.chromium.org/149062/diff/1/6#newcode76
> Line 76: #ifdef OS_POSIX
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > if defined
> 
> Done.
> 
> http://codereview.chromium.org/149062/diff/1/6#newcode77
> Line 77: // FD for the renderer side.
> On 2009/06/26 19:18:31, Evan Martin wrote:
> > Could you elaborate on this one?  That is the local end of the pipe, or the
fd
> > that will be passed over IPC to the other end?  Is it only for before the
IPC
> > goes through, then cleared?  etc.
> 
> Added some extra docs.

[Antoine Labour, 2009-06-26 21:55:55]

On 2009/06/26 20:48:58, John Abd-El-Malek wrote:
> On 2009/06/26 20:23:46, Antoine Labour wrote:
> > John: today, if multiple channels are created for the same renderer, the
> plugin
> > process will assert on mac and linux (in the code that maps channel names to
> > FDs).
> > Unit tests would be better indeed. I'll see what I can do there, but I'm not
> > sure that channel name -> FD map works in the single process case, and even
> the
> > channel name is generated using process IDs.
> 
> In Windows, single-process mode means everything runs in one process
(renderers
> & plugins).  I vaguely remember Evan saying this isn't possible because there
> can only be one UI thread in a process, so you guys still have to run plugins
in
> a different process?  I'm not sure why it wouldn't work in that case, each
> RenderProcessHost will have a fake renderer id so I assume there'll be a
> different channel for it.
> 
> > Is there an easy setup to do unit tests with multiple processes ?
> 
> UI tests run chrome in multi-process mode, and there's also
> in-browser-unit-tests, which are similar but more powerful since a binary is
> loaded into the browser process so it has access to globals etc (ui tests just
> control the browser using the automation API).
> 
> > 
> > http://codereview.chromium.org/149062/diff/1/2
> > File chrome/browser/plugin_process_host.cc (right):
> > 
> > http://codereview.chromium.org/149062/diff/1/2#newcode549
> > Line 549: const IPC::ChannelHandle &channel_handle) {
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > & on left
> > 
> > Done.
> > 
> > http://codereview.chromium.org/149062/diff/1/2#newcode552
> > Line 552: #ifdef OS_POSIX
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > if defined(...)
> > 
> > Done.
> > 
> > http://codereview.chromium.org/149062/diff/1/2#newcode555
> > Line 555: // dup()'ed FD gets closed.
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > I don't quite understand this.  ipc_message_utils says fds coming in from
> IPC
> > > have auto_close=true.
> > 
> > Oh. I don't read docs, only code :) Message::ReadFileDescriptor implemented
> > otherwise. I fixed it to match the docs, I like it better that way.
> > 
> > http://codereview.chromium.org/149062/diff/1/2#newcode560
> > Line 560: channel_handle,
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > Should this be using reply_handle?
> > 
> > Yes it should have. But with the modifications, it's channel_handle again.
> > 
> > http://codereview.chromium.org/149062/diff/1/3
> > File chrome/browser/plugin_process_host.h (right):
> > 
> > http://codereview.chromium.org/149062/diff/1/3#newcode100
> > Line 100: void OnChannelCreated(const IPC::ChannelHandle &channel_handle);
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > & on left
> > 
> > Done.
> > 
> > http://codereview.chromium.org/149062/diff/1/5
> > File chrome/plugin/plugin_channel.cc (right):
> > 
> > http://codereview.chromium.org/149062/diff/1/5#newcode36
> > Line 36: #ifdef OS_POSIX
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > if defined
> > 
> > Done.
> > 
> > http://codereview.chromium.org/149062/diff/1/5#newcode50
> > Line 50: #ifdef OS_POSIX
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > if defined
> > 
> > Done.
> > 
> > http://codereview.chromium.org/149062/diff/1/5#newcode52
> > Line 52: if (renderer_fd_) {
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > You use -1 for "unset", so I think you should test for that here.  (0 is
> also
> > a
> > > valid fd...)
> > 
> > Doh! done.
> > 
> > http://codereview.chromium.org/149062/diff/1/5#newcode153
> > Line 153: #ifdef OS_POSIX
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > if defined
> > 
> > Done.
> > 
> > http://codereview.chromium.org/149062/diff/1/5#newcode156
> > Line 156: // name. Keep the renderer side FD in the channel to be able to
> > transmit it
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > "in the channel" is a bit confusing, you mean as a member variable, yeah?
> > 
> > Yeah. Fixed the docs.
> > 
> > http://codereview.chromium.org/149062/diff/1/6
> > File chrome/plugin/plugin_channel.h (right):
> > 
> > http://codereview.chromium.org/149062/diff/1/6#newcode32
> > Line 32: #ifdef OS_POSIX
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > if defined
> > 
> > Done.
> > 
> > http://codereview.chromium.org/149062/diff/1/6#newcode76
> > Line 76: #ifdef OS_POSIX
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > if defined
> > 
> > Done.
> > 
> > http://codereview.chromium.org/149062/diff/1/6#newcode77
> > Line 77: // FD for the renderer side.
> > On 2009/06/26 19:18:31, Evan Martin wrote:
> > > Could you elaborate on this one?  That is the local end of the pipe, or
the
> fd
> > > that will be passed over IPC to the other end?  Is it only for before the
> IPC
> > > goes through, then cleared?  etc.
> > 
> > Added some extra docs.

BTW the try servers came back green in all platforms. For some reason, they
never come up on the CL when I use git :/