Use an explicit PID for duplicating Pepper handles rather than the Channel's.

When the browser process launches the plugin, it explicitly tells each side the PID of the other side, and we now use this PID for sharing handles. Previously we'd use the PID from the IPC channel.

Using the PID from the IPC channel creates a race condition because the PID isn't set until the "hello" message from the opposite side is processed, which isn't guaranteed at any particular time.

BUG=168222

Review URL: https://codereview.chromium.org/11722017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@175190 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 49 insertions, 17 deletions

diff --git a/ppapi/proxy/broker_dispatcher.cc b/ppapi/proxy/broker_dispatcher.cc
index 97fca5c..7187852 100644
--- a/ppapi/proxy/broker_dispatcher.cc
+++ b/ppapi/proxy/broker_dispatcher.cc
@@ -21,9 +21,11 @@ BrokerDispatcher::~BrokerDispatcher() {
 
 bool BrokerDispatcher::InitBrokerWithChannel(
     ProxyChannel::Delegate* delegate,
+    base::ProcessId peer_pid,
     const IPC::ChannelHandle& channel_handle,
     bool is_client) {
-  return ProxyChannel::InitWithChannel(delegate, channel_handle, is_client);
+  return ProxyChannel::InitWithChannel(delegate, peer_pid, channel_handle,
+                                       is_client);
 }
 
 bool BrokerDispatcher::OnMessageReceived(const IPC::Message& msg) {
diff --git a/ppapi/proxy/broker_dispatcher.h b/ppapi/proxy/broker_dispatcher.h
index de83b2b1..2da4a47 100644
--- a/ppapi/proxy/broker_dispatcher.h
+++ b/ppapi/proxy/broker_dispatcher.h
@@ -20,6 +20,7 @@ class PPAPI_PROXY_EXPORT BrokerDispatcher : public ProxyChannel {
   // The delegate pointer must outlive this class, ownership is not
   // transferred.
   virtual bool InitBrokerWithChannel(ProxyChannel::Delegate* delegate,
+                                     base::ProcessId peer_pid,
                                      const IPC::ChannelHandle& channel_handle,
                                      bool is_client);
 
diff --git a/ppapi/proxy/host_dispatcher.cc b/ppapi/proxy/host_dispatcher.cc
index 7124c17..cea7ab3 100644
--- a/ppapi/proxy/host_dispatcher.cc
+++ b/ppapi/proxy/host_dispatcher.cc
@@ -87,10 +87,12 @@ HostDispatcher::~HostDispatcher() {
 
 bool HostDispatcher::InitHostWithChannel(
     Delegate* delegate,
+    base::ProcessId peer_pid,
     const IPC::ChannelHandle& channel_handle,
     bool is_client,
     const ppapi::Preferences& preferences) {
-  if (!Dispatcher::InitWithChannel(delegate, channel_handle, is_client))
+  if (!Dispatcher::InitWithChannel(delegate, peer_pid, channel_handle,
+                                   is_client))
     return false;
   AddIOThreadMessageFilter(sync_status_.get());
 
diff --git a/ppapi/proxy/host_dispatcher.h b/ppapi/proxy/host_dispatcher.h
index 98a7ad5..1b0c9a7 100644
--- a/ppapi/proxy/host_dispatcher.h
+++ b/ppapi/proxy/host_dispatcher.h
@@ -58,6 +58,7 @@ class PPAPI_PROXY_EXPORT HostDispatcher : public Dispatcher {
   // The delegate pointer must outlive this class, ownership is not
   // transferred.
   virtual bool InitHostWithChannel(Delegate* delegate,
+                                   base::ProcessId peer_pid,
                                    const IPC::ChannelHandle& channel_handle,
                                    bool is_client,
                                    const Preferences& preferences);
diff --git a/ppapi/proxy/plugin_dispatcher.cc b/ppapi/proxy/plugin_dispatcher.cc
index 1b9c80a..812b2e9 100644
--- a/ppapi/proxy/plugin_dispatcher.cc
+++ b/ppapi/proxy/plugin_dispatcher.cc
@@ -155,9 +155,11 @@ const void* PluginDispatcher::GetPluginInterface(
 
 bool PluginDispatcher::InitPluginWithChannel(
     PluginDelegate* delegate,
+    base::ProcessId peer_pid,
     const IPC::ChannelHandle& channel_handle,
     bool is_client) {
-  if (!Dispatcher::InitWithChannel(delegate, channel_handle, is_client))
+  if (!Dispatcher::InitWithChannel(delegate, peer_pid, channel_handle,
+                                   is_client))
     return false;
   plugin_delegate_ = delegate;
   plugin_dispatcher_id_ = plugin_delegate_->Register(this);
diff --git a/ppapi/proxy/plugin_dispatcher.h b/ppapi/proxy/plugin_dispatcher.h
index 2fb0395..1c213b6 100644
--- a/ppapi/proxy/plugin_dispatcher.h
+++ b/ppapi/proxy/plugin_dispatcher.h
@@ -126,6 +126,7 @@ class PPAPI_PROXY_EXPORT PluginDispatcher
   // The delegate pointer must outlive this class, ownership is not
   // transferred.
   bool InitPluginWithChannel(PluginDelegate* delegate,
+                             base::ProcessId peer_pid,
                              const IPC::ChannelHandle& channel_handle,
                              bool is_client);
 
diff --git a/ppapi/proxy/plugin_main_nacl.cc b/ppapi/proxy/plugin_main_nacl.cc
index 04d9be3..8733b9c 100644
--- a/ppapi/proxy/plugin_main_nacl.cc
+++ b/ppapi/proxy/plugin_main_nacl.cc
@@ -66,7 +66,7 @@ class PpapiDispatcher : public ProxyChannel,
   virtual base::WaitableEvent* GetShutdownEvent() OVERRIDE;
   virtual IPC::PlatformFileForTransit ShareHandleWithRemote(
       base::PlatformFile handle,
-      const IPC::SyncChannel& channel,
+      base::ProcessId peer_pid,
       bool should_close_source) OVERRIDE;
   virtual std::set<PP_Instance>* GetGloballySeenInstanceIDSet() OVERRIDE;
   virtual uint32 Register(PluginDispatcher* plugin_dispatcher) OVERRIDE;
@@ -104,7 +104,10 @@ PpapiDispatcher::PpapiDispatcher(scoped_refptr<base::MessageLoopProxy> io_loop)
       shutdown_event_(true, false) {
   IPC::ChannelHandle channel_handle(
       "NaCl IPC", base::FileDescriptor(NACL_IPC_FD, false));
-  InitWithChannel(this, channel_handle, false);  // Channel is server.
+  // We don't have/need a PID since handle sharing happens outside of the
+  // NaCl sandbox.
+  InitWithChannel(this, base::kNullProcessId, channel_handle,
+                  false);  // Channel is server.
   channel()->AddFilter(
       new components::ChildTraceMessageFilter(message_loop_));
 }
@@ -119,7 +122,7 @@ base::WaitableEvent* PpapiDispatcher::GetShutdownEvent() {
 
 IPC::PlatformFileForTransit PpapiDispatcher::ShareHandleWithRemote(
     base::PlatformFile handle,
-    const IPC::SyncChannel& channel,
+    base::ProcessId peer_pid,
     bool should_close_source) {
   return IPC::InvalidPlatformFileForTransit();
 }
@@ -190,7 +193,8 @@ void PpapiDispatcher::OnMsgCreateNaClChannel(
       new PluginDispatcher(::PPP_GetInterface, permissions, incognito);
   // The channel handle's true name is not revealed here.
   IPC::ChannelHandle channel_handle("nacl", handle.descriptor());
-  if (!dispatcher->InitPluginWithChannel(this, channel_handle, false)) {
+  if (!dispatcher->InitPluginWithChannel(this, base::kNullProcessId,
+                                         channel_handle, false)) {
     delete dispatcher;
     return;
   }
diff --git a/ppapi/proxy/ppapi_messages.h b/ppapi/proxy/ppapi_messages.h
index 71501e3..e29aa5b 100644
--- a/ppapi/proxy/ppapi_messages.h
+++ b/ppapi/proxy/ppapi_messages.h
@@ -299,8 +299,9 @@ IPC_MESSAGE_CONTROL2(PpapiMsg_LoadPlugin,
 
 // Creates a channel to talk to a renderer. The plugin will respond with
 // PpapiHostMsg_ChannelCreated.
-IPC_MESSAGE_CONTROL2(PpapiMsg_CreateChannel,
-                     int /* renderer_id */,
+IPC_MESSAGE_CONTROL3(PpapiMsg_CreateChannel,
+                     base::ProcessId /* renderer_pid */,
+                     int /* renderer_child_id */,
                      bool /* incognito */)
 
 // Creates a channel to talk to a renderer. This message is only used by the
diff --git a/ppapi/proxy/ppapi_proxy_test.cc b/ppapi/proxy/ppapi_proxy_test.cc
index 5f38908..1af1dfc 100644
--- a/ppapi/proxy/ppapi_proxy_test.cc
+++ b/ppapi/proxy/ppapi_proxy_test.cc
@@ -202,6 +202,7 @@ void PluginProxyTestHarness::SetUpHarnessWithChannel(
       PpapiPermissions(),
       false));
   plugin_dispatcher_->InitPluginWithChannel(&plugin_delegate_mock_,
+                                            base::kNullProcessId,
                                             channel_handle,
                                             is_client);
   plugin_delegate_mock_.set_browser_sender(plugin_dispatcher_.get());
@@ -230,7 +231,7 @@ PluginProxyTestHarness::PluginDelegateMock::GetShutdownEvent() {
 IPC::PlatformFileForTransit
 PluginProxyTestHarness::PluginDelegateMock::ShareHandleWithRemote(
     base::PlatformFile handle,
-    const IPC::SyncChannel& /* channel */,
+    base::ProcessId /* remote_pid */,
     bool should_close_source) {
   return IPC::GetFileHandleForProcess(handle,
                                       base::Process::Current().handle(),
@@ -338,7 +339,8 @@ void HostProxyTestHarness::SetUpHarnessWithChannel(
       status_receiver_.release(),
       PpapiPermissions::AllPermissions()));
   ppapi::Preferences preferences;
-  host_dispatcher_->InitHostWithChannel(&delegate_mock_, channel_handle,
+  host_dispatcher_->InitHostWithChannel(&delegate_mock_,
+                                        base::kNullProcessId, channel_handle,
                                         is_client, preferences);
   HostDispatcher::SetForInstance(pp_instance(), host_dispatcher_.get());
 }
@@ -361,7 +363,7 @@ base::WaitableEvent* HostProxyTestHarness::DelegateMock::GetShutdownEvent() {
 IPC::PlatformFileForTransit
 HostProxyTestHarness::DelegateMock::ShareHandleWithRemote(
     base::PlatformFile handle,
-    const IPC::SyncChannel& /* channel */,
+    base::ProcessId /* remote_pid */,
     bool should_close_source) {
   return IPC::GetFileHandleForProcess(handle,
                                       base::Process::Current().handle(),
diff --git a/ppapi/proxy/ppapi_proxy_test.h b/ppapi/proxy/ppapi_proxy_test.h
index a72368e..e8c15c1 100644
--- a/ppapi/proxy/ppapi_proxy_test.h
+++ b/ppapi/proxy/ppapi_proxy_test.h
@@ -120,7 +120,7 @@ class PluginProxyTestHarness : public ProxyTestHarnessBase {
     virtual base::WaitableEvent* GetShutdownEvent() OVERRIDE;
     virtual IPC::PlatformFileForTransit ShareHandleWithRemote(
         base::PlatformFile handle,
-        const IPC::SyncChannel& /* channel */,
+        base::ProcessId remote_pid,
         bool should_close_source) OVERRIDE;
 
     // PluginDispatcher::PluginDelegate implementation.
@@ -202,7 +202,7 @@ class HostProxyTestHarness : public ProxyTestHarnessBase {
     virtual base::WaitableEvent* GetShutdownEvent();
     virtual IPC::PlatformFileForTransit ShareHandleWithRemote(
         base::PlatformFile handle,
-        const IPC::SyncChannel& /* channel */,
+        base::ProcessId remote_pid,
         bool should_close_source) OVERRIDE;
 
    private:
diff --git a/ppapi/proxy/proxy_channel.cc b/ppapi/proxy/proxy_channel.cc
index d1c47a5..81eb9a4 100644
--- a/ppapi/proxy/proxy_channel.cc
+++ b/ppapi/proxy/proxy_channel.cc
@@ -4,6 +4,8 @@
 
 #include "ppapi/proxy/proxy_channel.h"
 
+#include "base/logging.h"
+#include "base/process_util.h"
 #include "ipc/ipc_platform_file.h"
 #include "ipc/ipc_test_sink.h"
 
@@ -16,6 +18,7 @@ namespace proxy {
 
 ProxyChannel::ProxyChannel()
     : delegate_(NULL),
+      peer_pid_(base::kNullProcessId),
       test_sink_(NULL) {
 }
 
@@ -24,9 +27,11 @@ ProxyChannel::~ProxyChannel() {
 }
 
 bool ProxyChannel::InitWithChannel(Delegate* delegate,
+                                   base::ProcessId peer_pid,
                                    const IPC::ChannelHandle& channel_handle,
                                    bool is_client) {
   delegate_ = delegate;
+  peer_pid_ = peer_pid;
   IPC::Channel::Mode mode = is_client ? IPC::Channel::MODE_CLIENT
                                       : IPC::Channel::MODE_SERVER;
   channel_.reset(new IPC::SyncChannel(channel_handle, mode, this,
@@ -38,6 +43,9 @@ bool ProxyChannel::InitWithChannel(Delegate* delegate,
 void ProxyChannel::InitWithTestSink(IPC::TestSink* test_sink) {
   DCHECK(!test_sink_);
   test_sink_ = test_sink;
+#if !defined(OS_NACL)
+  peer_pid_ = base::GetCurrentProcId();
+#endif
 }
 
 void ProxyChannel::OnChannelError() {
@@ -65,7 +73,8 @@ IPC::PlatformFileForTransit ProxyChannel::ShareHandleWithRemote(
     }
     return IPC::InvalidPlatformFileForTransit();
   }
-  return delegate_->ShareHandleWithRemote(handle, *channel_,
+  DCHECK(peer_pid_ != base::kNullProcessId);
+  return delegate_->ShareHandleWithRemote(handle, peer_pid_,
                                           should_close_source);
 }
 
diff --git a/ppapi/proxy/proxy_channel.h b/ppapi/proxy/proxy_channel.h
index bb08f79..d35f98a 100644
--- a/ppapi/proxy/proxy_channel.h
+++ b/ppapi/proxy/proxy_channel.h
@@ -47,7 +47,7 @@ class PPAPI_PROXY_EXPORT ProxyChannel
     // guarantees as ProxyChannel::ShareHandleWithRemote below.
     virtual IPC::PlatformFileForTransit ShareHandleWithRemote(
         base::PlatformFile handle,
-        const IPC::SyncChannel& channel,
+        base::ProcessId remote_pid,
         bool should_close_source) = 0;
   };
 
@@ -55,7 +55,8 @@ class PPAPI_PROXY_EXPORT ProxyChannel
 
   // Alternative to InitWithChannel() for unit tests that want to send all
   // messages sent via this channel to the given test sink. The test sink
-  // must outlive this class.
+  // must outlive this class. In this case, the peer PID will be the current
+  // process ID.
   void InitWithTestSink(IPC::TestSink* test_sink);
 
   // Shares a file handle (HANDLE / file descriptor) with the remote side. It
@@ -90,6 +91,7 @@ class PPAPI_PROXY_EXPORT ProxyChannel
   // The delegate pointer must outlive this class, ownership is not
   // transferred.
   virtual bool InitWithChannel(Delegate* delegate,
+                               base::ProcessId peer_pid,
                                const IPC::ChannelHandle& channel_handle,
                                bool is_client);
 
@@ -101,6 +103,11 @@ class PPAPI_PROXY_EXPORT ProxyChannel
   // Non-owning pointer. Guaranteed non-NULL after init is called.
   ProxyChannel::Delegate* delegate_;
 
+  // PID of the remote process. Use this instead of the Channel::peer_pid since
+  // this is set synchronously on construction rather than waiting on the
+  // "hello" message from the peer (which introduces a race condition).
+  base::ProcessId peer_pid_;
+
   // When we're unit testing, this will indicate the sink for the messages to
   // be deposited so they can be inspected by the test. When non-NULL, this
   // indicates that the channel should not be used.