Refactor plugin process code which checks with the browser process before shutdown, to avoid races in which the browser process thinks the process is fine to use while it's shutting down. I also removed PluginProcess/WorkerProcess since they didn't have any code in them now.

I removed the plugin process code which waits 10 seconds before shutting itself down.  That was a premature optimization, since testing with/without this didn't show any difference (see http://www/~jabdelmalek/chrome/test/plugins/processes.html).  In both cases, the plugin on a page would get recreated in less than 100ms, even with reusing or starting a plugin process from scratch.  We already spawn new renderer processes on back and forth if it's a different origin, and the plugin will be in the cache anyways.


Review URL: http://codereview.chromium.org/53091

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

7 files changed, 90 insertions, 48 deletions

diff --git a/chrome/common/child_process.cc b/chrome/common/child_process.cc
index 27b55b7..55f5bb0 100644
--- a/chrome/common/child_process.cc
+++ b/chrome/common/child_process.cc
@@ -35,32 +35,22 @@ ChildProcess::~ChildProcess() {
   child_process_ = NULL;
 }
 
-// Called on any thread
 void ChildProcess::AddRefProcess() {
-  base::AtomicRefCountInc(&ref_count_);
+  DCHECK(MessageLoop::current() == child_thread_->message_loop());
+  ref_count_++;
 }
 
-// Called on any thread
 void ChildProcess::ReleaseProcess() {
-  DCHECK(!base::AtomicRefCountIsZero(&ref_count_));
+  DCHECK(MessageLoop::current() == child_thread_->message_loop());
+  DCHECK(ref_count_);
   DCHECK(child_process_);
-  if (!base::AtomicRefCountDec(&ref_count_))
-    child_process_->OnFinalRelease();
+  if (--ref_count_)
+    return;
+
+  child_thread_->OnProcessFinalRelease();
 }
 
 base::WaitableEvent* ChildProcess::GetShutDownEvent() {
   DCHECK(child_process_);
   return &child_process_->shutdown_event_;
 }
-
-// Called on any thread
-bool ChildProcess::ProcessRefCountIsZero() {
-  return base::AtomicRefCountIsZero(&ref_count_);
-}
-
-void ChildProcess::OnFinalRelease() {
-  if (child_thread_.get()) {
-    child_thread_->owner_loop()->PostTask(
-        FROM_HERE, new MessageLoop::QuitTask());
-  }
-}
diff --git a/chrome/common/child_process.h b/chrome/common/child_process.h
index 5844b5b..23bfad8 100644
--- a/chrome/common/child_process.h
+++ b/chrome/common/child_process.h
@@ -7,7 +7,6 @@
 
 #include <string>
 #include <vector>
-#include "base/atomic_ref_count.h"
 #include "base/basictypes.h"
 #include "base/message_loop.h"
 #include "base/scoped_ptr.h"
@@ -40,34 +39,21 @@ class ChildProcess {
   base::WaitableEvent* GetShutDownEvent();
 
   // These are used for ref-counting the child process.  The process shuts
-  // itself down when the ref count reaches 0.  These functions may be called
-  // on any thread.
+  // itself down when the ref count reaches 0.
   // For example, in the renderer process, generally each tab managed by this
   // process will hold a reference to the process, and release when closed.
   void AddRefProcess();
   void ReleaseProcess();
 
- protected:
-  friend class ChildThread;
-
   // Getter for the one ChildProcess object for this process.
   static ChildProcess* current() { return child_process_; }
 
- protected:
-  bool ProcessRefCountIsZero();
-
-  // Derived classes can override this to alter the behavior when the ref count
-  // reaches 0. The default implementation calls Quit on the main message loop
-  // which causes the process to shutdown. Note, this can be called on any
-  // thread. (See ReleaseProcess)
-  virtual void OnFinalRelease();
-
  private:
   // NOTE: make sure that child_thread_ is listed before shutdown_event_, since
   // it depends on it (indirectly through IPC::SyncChannel).
   scoped_ptr<ChildThread> child_thread_;
 
-  base::AtomicRefCount ref_count_;
+  int ref_count_;
 
   // An event that will be signalled when we shutdown.
   base::WaitableEvent shutdown_event_;
diff --git a/chrome/common/child_process_host.cc b/chrome/common/child_process_host.cc
index 4269678..6618554 100644
--- a/chrome/common/child_process_host.cc
+++ b/chrome/common/child_process_host.cc
@@ -17,7 +17,12 @@
 #include "chrome/common/process_watcher.h"
 #include "chrome/common/result_codes.h"
 
-typedef std::list<ChildProcessInfo*> ChildProcessList;
+#if defined(OS_WIN)
+#include "chrome/common/plugin_messages.h"
+#endif
+
+namespace {
+typedef std::list<ChildProcessHost*> ChildProcessList;
 
 // The NotificationTask is used to notify about plugin process connection/
 // disconnection. It is needed because the notifications in the
@@ -39,6 +44,9 @@ class ChildNotificationTask : public Task {
   ChildProcessInfo info_;
 };
 
+}  // namespace
+
+
 
 ChildProcessHost::ChildProcessHost(
     ProcessType type, ResourceDispatcherHost* resource_dispatcher_host)
@@ -143,10 +151,22 @@ void ChildProcessHost::ListenerHook::OnMessageReceived(
   bool msg_is_ok = true;
   bool handled = host_->resource_dispatcher_host_->OnMessageReceived(
       msg, host_, &msg_is_ok);
-  if (!handled)
-    host_->OnMessageReceived(msg);
 
-  if (!msg_is_ok) 
+  if (!handled) {
+#if defined(OS_WIN)
+    if (msg.type() == PluginProcessHostMsg_ShutdownRequest::ID) {
+      // Must remove the process from the list now, in case it gets used for a
+      // new instance before our watcher tells us that the process terminated.
+      Singleton<ChildProcessList>::get()->remove(host_);
+      if (host_->CanShutdown())
+        host_->Send(new PluginProcessMsg_Shutdown());
+#endif
+    } else {
+      host_->OnMessageReceived(msg);
+    }
+  }
+
+  if (!msg_is_ok)
     base::KillProcess(host_->handle(), ResultCodes::KILLED_BAD_MESSAGE, false);
 
 #ifdef IPC_MESSAGE_LOG_ENABLED
@@ -158,6 +178,7 @@ void ChildProcessHost::ListenerHook::OnMessageReceived(
 void ChildProcessHost::ListenerHook::OnChannelConnected(int32 peer_pid) {
   host_->opening_channel_ = false;
   host_->OnChannelConnected(peer_pid);
+  host_->Send(new PluginProcessMsg_AskBeforeShutdown());
 
   // Notify in the main loop of the connection.
   host_->Notify(NotificationType::CHILD_PROCESS_HOST_CONNECTED);
@@ -186,7 +207,7 @@ ChildProcessHost::Iterator::Iterator(ProcessType type)
     ++(*this);
 }
 
-ChildProcessInfo* ChildProcessHost::Iterator::operator++() {
+ChildProcessHost* ChildProcessHost::Iterator::operator++() {
   do {
     ++iterator_;
     if (Done())
diff --git a/chrome/common/child_process_host.h b/chrome/common/child_process_host.h
index cef940c..d4d2dd9 100644
--- a/chrome/common/child_process_host.h
+++ b/chrome/common/child_process_host.h
@@ -38,21 +38,24 @@ class ChildProcessHost : public ResourceDispatcherHost::Receiver,
    public:
     Iterator();
     Iterator(ProcessType type);
-    ChildProcessInfo* operator->() { return *iterator_; }
-    ChildProcessInfo* operator*() { return *iterator_; }
-    ChildProcessInfo* operator++();
+    ChildProcessHost* operator->() { return *iterator_; }
+    ChildProcessHost* operator*() { return *iterator_; }
+    ChildProcessHost* operator++();
     bool Done();
 
    private:
     bool all_;
     ProcessType type_;
-    std::list<ChildProcessInfo*>::iterator iterator_;
+    std::list<ChildProcessHost*>::iterator iterator_;
   };
 
  protected:
   ChildProcessHost(ProcessType type,
                    ResourceDispatcherHost* resource_dispatcher_host);
 
+  // Derived classes return true if it's ok to shut down the child process.
+  virtual bool CanShutdown() = 0;
+
   // Creates the IPC channel.  Returns true iff it succeeded.
   bool CreateChannel();
 
diff --git a/chrome/common/child_thread.cc b/chrome/common/child_thread.cc
index 237e53c..22c82c0 100644
--- a/chrome/common/child_thread.cc
+++ b/chrome/common/child_thread.cc
@@ -11,13 +11,18 @@
 #include "chrome/common/ipc_logging.h"
 #include "webkit/glue/webkit_glue.h"
 
+#if defined(OS_WIN)
+#include "chrome/common/plugin_messages.h"
+#endif
+
 // V8 needs a 1MB stack size.
 const size_t ChildThread::kV8StackSize = 1024 * 1024;
 
 ChildThread::ChildThread(Thread::Options options)
     : Thread("Chrome_ChildThread"),
       owner_loop_(MessageLoop::current()),
-      options_(options) {
+      options_(options),
+      check_with_browser_before_shutdown_(false) {
   DCHECK(owner_loop_);
   channel_name_ = CommandLine::ForCurrentProcess()->GetSwitchValue(
       switches::kProcessChannelID);
@@ -66,6 +71,18 @@ void ChildThread::OnMessageReceived(const IPC::Message& msg) {
   if (resource_dispatcher_->OnMessageReceived(msg))
     return;
 
+#if defined(OS_WIN)
+  if (msg.type() == PluginProcessMsg_AskBeforeShutdown::ID) {
+    check_with_browser_before_shutdown_ = true;
+    return;
+  }
+
+  if (msg.type() == PluginProcessMsg_Shutdown::ID) {
+    owner_loop_->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+    return;
+  }
+#endif
+
   if (msg.routing_id() == MSG_ROUTING_CONTROL) {
     OnControlMessageReceived(msg);
   } else {
@@ -97,3 +114,16 @@ void ChildThread::CleanUp() {
   channel_.reset();
   resource_dispatcher_.reset();
 }
+
+void ChildThread::OnProcessFinalRelease() {
+  if (!check_with_browser_before_shutdown_) {
+    owner_loop_->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+    return;
+  }
+
+  // The child process shutdown sequence is a request response based mechanism,
+  // where we send out an initial feeler request to the child process host
+  // instance in the browser to verify if it's ok to shutdown the child process.
+  // The browser then sends back a response if it's ok to shutdown.
+  Send(new PluginProcessHostMsg_ShutdownRequest);
+}
diff --git a/chrome/common/child_thread.h b/chrome/common/child_thread.h
index c499cb5..66fcc09 100644
--- a/chrome/common/child_thread.h
+++ b/chrome/common/child_thread.h
@@ -41,6 +41,9 @@ class ChildThread : public IPC::Channel::Listener,
   // Overrides the channel name.  Used for --single-process mode.
   void SetChannelName(const std::wstring& name) { channel_name_ = name; }
 
+  // Called when the process refcount is 0.
+  void OnProcessFinalRelease();
+
  protected:
   // The required stack size if V8 runs on a thread.
   static const size_t kV8StackSize;
@@ -77,6 +80,11 @@ class ChildThread : public IPC::Channel::Listener,
   // NOTE: this object lives on the owner thread.
   scoped_ptr<ResourceDispatcher> resource_dispatcher_;
 
+  // If true, checks with the browser process before shutdown.  This avoids race
+  // conditions if the process refcount is 0 but there's an IPC message inflight
+  // that would addref it.
+  bool check_with_browser_before_shutdown_;
+
   DISALLOW_EVIL_CONSTRUCTORS(ChildThread);
 };
 
diff --git a/chrome/common/plugin_messages_internal.h b/chrome/common/plugin_messages_internal.h
index b9c7a58..61c53f0 100644
--- a/chrome/common/plugin_messages_internal.h
+++ b/chrome/common/plugin_messages_internal.h
@@ -16,15 +16,19 @@ IPC_BEGIN_MESSAGES(PluginProcess)
   IPC_MESSAGE_CONTROL1(PluginProcessMsg_CreateChannel,
                        bool /* off_the_record */)
 
-  IPC_MESSAGE_CONTROL1(PluginProcessMsg_ShutdownResponse,
-                       bool /* ok to shutdown */)
-
   // Allows a chrome plugin loaded in the browser process to send arbitrary
   // data to an instance of the same plugin loaded in a plugin process.
   IPC_MESSAGE_CONTROL1(PluginProcessMsg_PluginMessage,
                        std::vector<uint8> /* opaque data */)
 
-  IPC_MESSAGE_CONTROL0(PluginProcessMsg_BrowserShutdown)
+  // The following messages are used by all child processes, even though they
+  // are listed under PluginProcess.  It seems overkill to define ChildProcess.
+  // Tells the child process it should stop.
+  IPC_MESSAGE_CONTROL0(PluginProcessMsg_AskBeforeShutdown)
+
+  // Sent in response to PluginProcessHostMsg_ShutdownRequest to tell the child
+  // process that it's safe to shutdown.
+  IPC_MESSAGE_CONTROL0(PluginProcessMsg_Shutdown)
 
 IPC_END_MESSAGES(PluginProcess)