Don't auto-close the renderer-side plugin channel file descriptor in the

renderer process.  Wait until the renderer has proved that it has access to
its end of the socket before closing it manually.

BUG=38459
TEST=Test case from  bug 26754  comment 9 (affected Macs only):
      1. Have lots of bookmarks (import Safari defaults)
      2. Right-click on bookmark bar, and choose "Open All Bookmarks"
     Expect: no crash, no sad tabs, none of the following messages logged:
      a. LOG(ERROR) << "Refusing use of missing IPC channel " ...
      b. LOG(FATAL) << "Denying attempt to reuse initial IPC channel for " ...
     This test should be repeated many times.
Review URL: http://codereview.chromium.org/1066001

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

3 files changed, 46 insertions, 28 deletions

diff --git a/chrome/plugin/plugin_channel.cc b/chrome/plugin/plugin_channel.cc
index 44779a8..bf14db2 100644
--- a/chrome/plugin/plugin_channel.cc
+++ b/chrome/plugin/plugin_channel.cc
@@ -18,6 +18,7 @@
 #include "chrome/plugin/webplugin_proxy.h"
 
 #if defined(OS_POSIX)
+#include "base/eintr_wrapper.h"
 #include "ipc/ipc_channel_posix.h"
 #endif
 
@@ -168,14 +169,14 @@ PluginChannel::PluginChannel()
 }
 
 PluginChannel::~PluginChannel() {
-  if (renderer_handle_)
-    base::CloseProcessHandle(renderer_handle_);
 #if defined(OS_POSIX)
-  // If we still have the renderer FD, close it.
-  if (renderer_fd_ != -1) {
-    close(renderer_fd_);
-  }
+  // Won't be needing this any more.
+  CloseRendererFD();
 #endif
+
+  if (renderer_handle_)
+    base::CloseProcessHandle(renderer_handle_);
+
   MessageLoop::current()->PostDelayedTask(FROM_HERE, new PluginReleaseTask(),
                                           kPluginReleaseTimeMS);
 }
@@ -256,6 +257,12 @@ base::WaitableEvent* PluginChannel::GetModalDialogEvent(
 }
 
 void PluginChannel::OnChannelConnected(int32 peer_pid) {
+#if defined(OS_POSIX)
+  // By this point, the renderer must have its own copy of the plugin channel
+  // FD.
+  CloseRendererFD();
+#endif
+
   base::ProcessHandle handle;
   if (!base::OpenProcessHandle(peer_pid, &handle)) {
     NOTREACHED();
@@ -265,6 +272,11 @@ void PluginChannel::OnChannelConnected(int32 peer_pid) {
 }
 
 void PluginChannel::OnChannelError() {
+#if defined(OS_POSIX)
+  // Won't be needing this any more.
+  CloseRendererFD();
+#endif
+
   base::CloseProcessHandle(renderer_handle_);
   renderer_handle_ = 0;
   PluginChannelBase::OnChannelError();
@@ -294,12 +306,23 @@ bool PluginChannel::Init(MessageLoop* ipc_message_loop, bool create_pipe_now) {
   // name. Keep the renderer side FD as a member variable in the PluginChannel
   // to be able to transmit it through IPC.
   int plugin_fd;
-  IPC::SocketPair(&plugin_fd, &renderer_fd_);
+  if (!IPC::SocketPair(&plugin_fd, &renderer_fd_))
+    return false;
   IPC::AddChannelSocket(channel_name(), plugin_fd);
 #endif
+
   if (!PluginChannelBase::Init(ipc_message_loop, create_pipe_now))
     return false;
 
   channel_->AddFilter(filter_.get());
   return true;
 }
+
+#if defined(OS_POSIX)
+void PluginChannel::CloseRendererFD() {
+  if (renderer_fd_ != -1) {
+    HANDLE_EINTR(close(renderer_fd_));
+    renderer_fd_ = -1;
+  }
+}
+#endif
diff --git a/chrome/plugin/plugin_channel.h b/chrome/plugin/plugin_channel.h
index 35c0799..22bcd99 100644
--- a/chrome/plugin/plugin_channel.h
+++ b/chrome/plugin/plugin_channel.h
@@ -23,9 +23,6 @@ class PluginChannel : public PluginChannelBase {
   // Get a new PluginChannel object for the current process to talk to the
   // given renderer process. The renderer ID is an opaque unique ID generated
   // by the browser.
-  //
-  // POSIX only: If |channel_fd| > 0, use that file descriptor for the
-  // channel socket.
   static PluginChannel* GetPluginChannel(int renderer_id,
                                          MessageLoop* ipc_message_loop);
 
@@ -43,23 +40,15 @@ class PluginChannel : public PluginChannelBase {
   // dialog to come up.
   base::WaitableEvent* GetModalDialogEvent(gfx::NativeViewId containing_window);
 
-#if defined(OS_POSIX)
-  // When first created, the PluginChannel gets assigned the file descriptor
-  // for the renderer.
-  // After the first time we pass it through the IPC, we don't need it anymore,
-  // and we close it. At that time, we reset renderer_fd_ to -1.
-  int DisownRendererFd() {
-    int value = renderer_fd_;
-    renderer_fd_ = -1;
-    return value;
-  }
-#endif
-
   bool in_send() { return in_send_ != 0; }
 
   bool off_the_record() { return off_the_record_; }
   void set_off_the_record(bool value) { off_the_record_ = value; }
 
+#if defined(OS_POSIX)
+  int renderer_fd() const { return renderer_fd_; }
+#endif
+
  protected:
   // IPC::Channel::Listener implementation:
   virtual void OnChannelConnected(int32 peer_pid);
@@ -84,6 +73,13 @@ class PluginChannel : public PluginChannelBase {
   void OnDestroyInstance(int instance_id, IPC::Message* reply_msg);
   void OnGenerateRouteID(int* route_id);
 
+#if defined(OS_POSIX)
+  // Close the plugin process' copy of the renderer's side of the plugin
+  // channel.  This can be called after the renderer is known to have its own
+  // copy of renderer_fd_.
+  void CloseRendererFD();
+#endif
+
   std::vector<scoped_refptr<WebPluginDelegateStub> > plugin_stubs_;
 
   // Handle to the renderer process who is on the other side of the channel.
@@ -93,8 +89,9 @@ class PluginChannel : public PluginChannelBase {
   int renderer_id_;
 
 #if defined(OS_POSIX)
-  // FD for the renderer end of the pipe. It is stored until we send it over
-  // IPC after which it is cleared. It will be closed by the IPC mechanism.
+  // FD for the renderer end of the socket. It is closed when the IPC layer
+  // indicates that the channel is connected, proving that the renderer has
+  // access to its side of the socket.
   int renderer_fd_;
 #endif
 
diff --git a/chrome/plugin/plugin_thread.cc b/chrome/plugin/plugin_thread.cc
index 59d243d..bd06bbd 100644
--- a/chrome/plugin/plugin_thread.cc
+++ b/chrome/plugin/plugin_thread.cc
@@ -142,10 +142,8 @@ void PluginThread::OnCreateChannel(int renderer_id,
   if (channel.get()) {
     channel_handle.name = channel->channel_name();
 #if defined(OS_POSIX)
-    // On POSIX, pass the renderer-side FD. Also mark it as auto-close so that
-    // it gets closed after it has been sent.
-    int renderer_fd = channel->DisownRendererFd();
-    channel_handle.socket = base::FileDescriptor(renderer_fd, true);
+    // On POSIX, pass the renderer-side FD.
+    channel_handle.socket = base::FileDescriptor(channel->renderer_fd(), false);
 #endif
     channel->set_off_the_record(off_the_record);
   }