Fix race in zygote_host_linux where socket was being read from and written to on different threads.

Made ZygoteHost methods all run on PROCESS_LAUNCHER thread. PostTask in linux from UI thread to PROCESS_LAUNCHER thread for DidProcessCrash. Changed DidProcessCrash to answer via a callback, which occurs asynchronously on linux. Rework cases in nacl_host, browser_render_process_host, and child_process_host where this method was being called to fit the callback model.

BUG=31737
TEST=none

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

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

12 files changed, 165 insertions, 77 deletions

diff --git a/chrome/browser/child_process_host.cc b/chrome/browser/child_process_host.cc
index 0c87021..2a828e0 100644
--- a/chrome/browser/child_process_host.cc
+++ b/chrome/browser/child_process_host.cc
@@ -179,23 +179,36 @@ void ChildProcessHost::Notify(NotificationType type) {
       ChromeThread::UI, FROM_HERE, new ChildNotificationTask(type, this));
 }
 
-bool ChildProcessHost::DidChildCrash() {
-  return child_process_->DidProcessCrash();
+void ChildProcessHost::DetermineDidChildCrash() {
+  child_process_->DetermineDidProcessCrash();
 }
 
 void ChildProcessHost::OnChildDied() {
+  // Either of these paths will lead to deleting this object in
+  // OnDidProcessCrashDetermined, so they should be the last calls in this
+  // method.
+  if (handle() != base::kNullProcessHandle) {
+    // Determine whether the process crashed or not. This method will invoke
+    // OnDidProcessCrashDetermined with the result. This may occur
+    // asynchronously.
+    DetermineDidChildCrash();
+  } else {
+    // We do not have a process, so it couldn't have crashed.
+    OnDidProcessCrashDetermined(false);
+  }
+}
+
+void ChildProcessHost::OnDidProcessCrashDetermined(bool did_crash) {
+  if (did_crash) {
+    OnProcessCrashed();
+    // Report that this child process crashed.
+    Notify(NotificationType::CHILD_PROCESS_CRASHED);
+    UMA_HISTOGRAM_COUNTS("ChildProcess.Crashes", this->type());
+  }
   if (handle() != base::kNullProcessHandle) {
-    bool did_crash = DidChildCrash();
-    if (did_crash) {
-      OnProcessCrashed();
-      // Report that this child process crashed.
-      Notify(NotificationType::CHILD_PROCESS_CRASHED);
-      UMA_HISTOGRAM_COUNTS("ChildProcess.Crashes", this->type());
-    }
     // Notify in the main loop of the disconnection.
     Notify(NotificationType::CHILD_PROCESS_HOST_DISCONNECTED);
   }
-
   delete this;
 }
 
@@ -278,6 +291,11 @@ void ChildProcessHost::ListenerHook::OnProcessLaunched() {
   host_->OnProcessLaunched();
 }
 
+void ChildProcessHost::ListenerHook::OnDidProcessCrashDetermined(
+    bool did_crash) {
+  host_->OnDidProcessCrashDetermined(did_crash);
+}
+
 
 ChildProcessHost::Iterator::Iterator()
     : all_(true), type_(UNKNOWN_PROCESS) {
diff --git a/chrome/browser/child_process_host.h b/chrome/browser/child_process_host.h
index e735940..7f0509a 100644
--- a/chrome/browser/child_process_host.h
+++ b/chrome/browser/child_process_host.h
@@ -113,6 +113,7 @@ class ChildProcessHost : public ResourceDispatcherHost::Receiver,
 
   // ChildProcessLauncher::Client implementation.
   virtual void OnProcessLaunched() {}
+  virtual void OnDidProcessCrashDetermined(bool did_crash);
 
   // Derived classes can override this to know if the process crashed.
   virtual void OnProcessCrashed() {}
@@ -120,7 +121,10 @@ class ChildProcessHost : public ResourceDispatcherHost::Receiver,
   bool opening_channel() { return opening_channel_; }
   const std::string& channel_id() { return channel_id_; }
 
-  virtual bool DidChildCrash();
+  // Determines whether the exited process crashed or exited normally.
+  // OnDidProcessCrashDetermined method will be called with the answer,
+  // possibly asynchronously.
+  virtual void DetermineDidChildCrash();
 
   // Called when the child process goes away.
   virtual void OnChildDied();
@@ -140,6 +144,7 @@ class ChildProcessHost : public ResourceDispatcherHost::Receiver,
     virtual void OnChannelConnected(int32 peer_pid);
     virtual void OnChannelError();
     virtual void OnProcessLaunched();
+    virtual void OnDidProcessCrashDetermined(bool did_crash);
    private:
     ChildProcessHost* host_;
   };
diff --git a/chrome/browser/child_process_launcher.cc b/chrome/browser/child_process_launcher.cc
index 206d8a7..c60f766 100644
--- a/chrome/browser/child_process_launcher.cc
+++ b/chrome/browser/child_process_launcher.cc
@@ -182,14 +182,14 @@ class ChildProcessLauncher::Context
         client_thread_id_, FROM_HERE,
         NewRunnableMethod(
             this,
-            &ChildProcessLauncher::Context::Notify,
+            &ChildProcessLauncher::Context::NotifyProcessLaunched,
 #if defined(OS_LINUX)
             use_zygote,
 #endif
             handle));
   }
 
-  void Notify(
+  void NotifyProcessLaunched(
 #if defined(OS_LINUX)
       bool zygote,
 #endif
@@ -206,7 +206,61 @@ class ChildProcessLauncher::Context
     }
   }
 
+  void DetermineDidProcessCrash() {
+    DCHECK(ChromeThread::CurrentlyOn(client_thread_id_));
+    base::ProcessHandle handle = process_.handle();
+#if defined(OS_LINUX)
+    if (zygote_) {
+      ChromeThread::PostTask(
+          ChromeThread::PROCESS_LAUNCHER, FROM_HERE,
+          NewRunnableMethod(this,
+                            &Context::DetermineDidProcessCrashInternal,
+                            handle));
+    } else
+#endif
+    {
+      DetermineDidProcessCrashInternal(handle);
+    }
+  }
+
+  void DetermineDidProcessCrashInternal(base::ProcessHandle handle) {
+    bool did_crash, child_exited;
+#if defined(OS_LINUX)
+    if (zygote_) {
+      did_crash = Singleton<ZygoteHost>()->DidProcessCrash(handle,
+                                                           &child_exited);
+      ChromeThread::PostTask(
+          client_thread_id_, FROM_HERE,
+          NewRunnableMethod(this,
+                            &Context::OnDidProcessCrashDetermined,
+                            child_exited, did_crash));
+    } else
+#endif
+    {
+      did_crash = base::DidProcessCrash(&child_exited, handle);
+      OnDidProcessCrashDetermined(child_exited, did_crash);
+    }
+  }
+
+  void OnDidProcessCrashDetermined(bool child_exited, bool did_crash) {
+    DCHECK(ChromeThread::CurrentlyOn(client_thread_id_));
+
+    // POSIX: If the process crashed, then the kernel closed the socket for it
+    // and so the child has already died by the time we get here. Since
+    // DidProcessCrash called waitpid with WNOHANG, it'll reap the process.
+    // However, if DidProcessCrash didn't reap the child, we'll need to in
+    // Terminate via ProcessWatcher. So we can't close the handle here.
+    if (child_exited)
+      process_.Close();
+
+    if (client_)
+      client_->OnDidProcessCrashDetermined(did_crash);
+    // The client may have deleted us in the callback. Do not add anything
+    // after this point.
+  }
+
   void Terminate() {
+    DCHECK(ChromeThread::CurrentlyOn(client_thread_id_));
     if (!process_.handle())
       return;
 
@@ -295,27 +349,8 @@ base::ProcessHandle ChildProcessLauncher::GetHandle() {
   return context_->process_.handle();
 }
 
-bool ChildProcessLauncher::DidProcessCrash() {
-  bool did_crash, child_exited;
-  base::ProcessHandle handle = context_->process_.handle();
-#if defined(OS_LINUX)
-  if (context_->zygote_) {
-    did_crash = Singleton<ZygoteHost>()->DidProcessCrash(handle, &child_exited);
-  } else
-#endif
-  {
-    did_crash = base::DidProcessCrash(&child_exited, handle);
-  }
-
-  // POSIX: If the process crashed, then the kernel closed the socket for it
-  // and so the child has already died by the time we get here. Since
-  // DidProcessCrash called waitpid with WNOHANG, it'll reap the process.
-  // However, if DidProcessCrash didn't reap the child, we'll need to in
-  // Terminate via ProcessWatcher. So we can't close the handle here.
-  if (child_exited)
-    context_->process_.Close();
-
-  return did_crash;
+void ChildProcessLauncher::DetermineDidProcessCrash() {
+  context_->DetermineDidProcessCrash();
 }
 
 void ChildProcessLauncher::SetProcessBackgrounded(bool background) {
diff --git a/chrome/browser/child_process_launcher.h b/chrome/browser/child_process_launcher.h
index ce1c04d..01c5d16 100644
--- a/chrome/browser/child_process_launcher.h
+++ b/chrome/browser/child_process_launcher.h
@@ -21,6 +21,11 @@ class ChildProcessLauncher {
     // Will be called on the thread that the ChildProcessLauncher was
     // constructed on.
     virtual void OnProcessLaunched() = 0;
+
+    // Called as a response to DetermineDidProcessCrash on the thread that
+    // the ChildProcessLauncher was constructed on. This may be called
+    // during DetermineDidProcessCrash or asynchronously.
+    virtual void OnDidProcessCrashDetermined(bool did_crash) = 0;
   };
 
   // Launches the process asynchronously, calling the client when the result is
@@ -46,8 +51,10 @@ class ChildProcessLauncher {
   // Getter for the process handle.  Only call after the process has started.
   base::ProcessHandle GetHandle();
 
-  // Call this when the process exits to know if a process crashed or not.
-  bool DidProcessCrash();
+  // Determines whether the exited process crashed or exited normally.
+  // The Client's OnDidProcessCrashDetermined method will be called with the
+  // answer.
+  void DetermineDidProcessCrash();
 
   // Changes whether the process runs in the background or not.  Only call
   // this after the process has started.
diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc
index ad24a11..38833a0 100644
--- a/chrome/browser/metrics/metrics_service.cc
+++ b/chrome/browser/metrics/metrics_service.cc
@@ -481,7 +481,7 @@ void MetricsService::SetRecording(bool enabled) {
                    NotificationService::AllSources());
     registrar_.Add(this, NotificationType::LOAD_STOP,
                    NotificationService::AllSources());
-    registrar_.Add(this, NotificationType::RENDERER_PROCESS_CLOSED,
+    registrar_.Add(this, NotificationType::RENDERER_PROCESS_CRASHED,
                    NotificationService::AllSources());
     registrar_.Add(this, NotificationType::RENDERER_PROCESS_HANG,
                    NotificationService::AllSources());
@@ -557,17 +557,13 @@ void MetricsService::Observe(NotificationType type,
       LogLoadStarted();
       break;
 
-    case NotificationType::RENDERER_PROCESS_CLOSED:
+    case NotificationType::RENDERER_PROCESS_CRASHED:
       {
-        RenderProcessHost::RendererClosedDetails* process_details =
-            Details<RenderProcessHost::RendererClosedDetails>(details).ptr();
-        if (process_details->did_crash) {
-          if (process_details->was_extension_renderer) {
-            LogExtensionRendererCrash();
-          } else {
-            LogRendererCrash();
-          }
-        }
+        // The details are whether it is an extension process.
+        if (*Details<bool>(details).ptr())
+          LogExtensionRendererCrash();
+        else
+          LogRendererCrash();
       }
       break;
 
diff --git a/chrome/browser/nacl_host/nacl_process_host.cc b/chrome/browser/nacl_host/nacl_process_host.cc
index 2648b3e..0310e39 100644
--- a/chrome/browser/nacl_host/nacl_process_host.cc
+++ b/chrome/browser/nacl_host/nacl_process_host.cc
@@ -117,10 +117,19 @@ void NaClProcessHost::OnProcessLaunchedByBroker(base::ProcessHandle handle) {
   OnProcessLaunched();
 }
 
-bool NaClProcessHost::DidChildCrash() {
-  if (running_on_wow64_)
-    return base::DidProcessCrash(NULL, handle());
-  return ChildProcessHost::DidChildCrash();
+void NaClProcessHost::DetermineDidChildCrash() {
+  // Either of these paths will lead to deleting this object in
+  // OnDidProcessCrashDetermined in ChildProcessHost, so they should be the
+  // last calls in this method.
+  if (running_on_wow64_) {
+    bool did_crash = base::DidProcessCrash(NULL, handle());
+    OnDidProcessCrashDetermined(did_crash);
+  } else {
+    // Determine whether the process crashed or not. This method will invoke
+    // OnDidProcessCrashDetermined with the result. This may occur
+    // asynchronously.
+    ChildProcessHost::DetermineDidChildCrash();
+  }
 }
 
 void NaClProcessHost::OnChildDied() {
diff --git a/chrome/browser/nacl_host/nacl_process_host.h b/chrome/browser/nacl_host/nacl_process_host.h
index 28893fc..b6c3313 100644
--- a/chrome/browser/nacl_host/nacl_process_host.h
+++ b/chrome/browser/nacl_host/nacl_process_host.h
@@ -36,7 +36,8 @@ class NaClProcessHost : public ChildProcessHost {
   void OnProcessLaunchedByBroker(base::ProcessHandle handle);
 
  protected:
-  virtual bool DidChildCrash();
+  // Override ChildProcessHost methods.
+  virtual void DetermineDidChildCrash();
   virtual void OnChildDied();
 
  private:
diff --git a/chrome/browser/renderer_host/browser_render_process_host.cc b/chrome/browser/renderer_host/browser_render_process_host.cc
index fff6622..f7fb092 100644
--- a/chrome/browser/renderer_host/browser_render_process_host.cc
+++ b/chrome/browser/renderer_host/browser_render_process_host.cc
@@ -860,23 +860,12 @@ void BrowserRenderProcessHost::OnChannelError() {
   if (!channel_.get())
     return;
 
-  // NULL in single process mode or if fast termination happened.
-  bool did_crash =
-      child_process_.get() ? child_process_->DidProcessCrash() : false;
-
-  if (did_crash) {
-    UMA_HISTOGRAM_PERCENTAGE("BrowserRenderProcessHost.ChildCrashes",
-                             extension_process_ ? 2 : 1);
-  }
-
-  RendererClosedDetails details(did_crash, extension_process_);
   NotificationService::current()->Notify(
       NotificationType::RENDERER_PROCESS_CLOSED,
       Source<RenderProcessHost>(this),
-      Details<RendererClosedDetails>(&details));
+      NotificationService::NoDetails());
 
   WebCacheManager::GetInstance()->Remove(id());
-  child_process_.reset();
   channel_.reset();
 
   IDMap<IPC::Channel::Listener>::iterator iter(&listeners_);
@@ -888,6 +877,32 @@ void BrowserRenderProcessHost::OnChannelError() {
 
   ClearTransportDIBCache();
 
+  if (child_process_.get()) {
+    // Determine whether the process crashed or not. This method will invoke
+    // OnDidProcessCrashDetermined with the result. This may occur
+    // asynchronously.
+    child_process_->DetermineDidProcessCrash();
+  } else {
+    // This occurs in single process mode or if fast termination happened.
+    // Manually trigger OnDidProcessCrashDetermined, because we know this is
+    // not a crash.
+    OnDidProcessCrashDetermined(false);
+  }
+}
+
+void BrowserRenderProcessHost::OnDidProcessCrashDetermined(bool did_crash) {
+  child_process_.reset();
+
+  if (did_crash) {
+    UMA_HISTOGRAM_PERCENTAGE("BrowserRenderProcessHost.ChildCrashes",
+                             extension_process_ ? 2 : 1);
+
+    NotificationService::current()->Notify(
+        NotificationType::RENDERER_PROCESS_CRASHED,
+        Source<RenderProcessHost>(this),
+        Details<bool>(&extension_process_));
+  }
+
   // this object is not deleted at this point and may be reused later.
   // TODO(darin): clean this up
 }
diff --git a/chrome/browser/renderer_host/browser_render_process_host.h b/chrome/browser/renderer_host/browser_render_process_host.h
index 2a7f8b6..00c2a02 100644
--- a/chrome/browser/renderer_host/browser_render_process_host.h
+++ b/chrome/browser/renderer_host/browser_render_process_host.h
@@ -100,6 +100,7 @@ class BrowserRenderProcessHost : public RenderProcessHost,
 
   // ChildProcessLauncher::Client implementation.
   virtual void OnProcessLaunched();
+  virtual void OnDidProcessCrashDetermined(bool did_crash);
 
  private:
   friend class VisitRelayingRenderProcessHost;
diff --git a/chrome/browser/renderer_host/render_process_host.h b/chrome/browser/renderer_host/render_process_host.h
index 1ceeb42..1eb9804 100644
--- a/chrome/browser/renderer_host/render_process_host.h
+++ b/chrome/browser/renderer_host/render_process_host.h
@@ -46,16 +46,6 @@ class RenderProcessHost : public IPC::Channel::Sender,
     TYPE_EXTENSION,  // Renderer with extension privileges.
   };
 
-  // Details for RENDERER_PROCESS_CLOSED notifications.
-  struct RendererClosedDetails {
-    RendererClosedDetails(bool did_crash, bool was_extension_renderer) {
-      this->did_crash = did_crash;
-      this->was_extension_renderer = was_extension_renderer;
-    }
-    bool did_crash;
-    bool was_extension_renderer;
-  };
-
   explicit RenderProcessHost(Profile* profile);
   virtual ~RenderProcessHost();
 
diff --git a/chrome/browser/zygote_host_linux.cc b/chrome/browser/zygote_host_linux.cc
index 9885234..f28e2ad 100644
--- a/chrome/browser/zygote_host_linux.cc
+++ b/chrome/browser/zygote_host_linux.cc
@@ -19,6 +19,7 @@
 #include "base/string_util.h"
 #include "base/unix_domain_socket_posix.h"
 
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/renderer_host/render_sandbox_host_linux.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
@@ -193,6 +194,7 @@ void ZygoteHost::Init(const std::string& sandbox_cmd) {
 pid_t ZygoteHost::ForkRenderer(
     const std::vector<std::string>& argv,
     const base::GlobalDescriptors::Mapping& mapping) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::PROCESS_LAUNCHER));
   DCHECK(init_);
   Pickle pickle;
 
@@ -241,6 +243,7 @@ pid_t ZygoteHost::ForkRenderer(
 }
 
 void ZygoteHost::EnsureProcessTerminated(pid_t process) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::PROCESS_LAUNCHER));
   DCHECK(init_);
   Pickle pickle;
 
@@ -253,6 +256,7 @@ void ZygoteHost::EnsureProcessTerminated(pid_t process) {
 
 bool ZygoteHost::DidProcessCrash(base::ProcessHandle handle,
                                  bool* child_exited) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::PROCESS_LAUNCHER));
   DCHECK(init_);
   Pickle pickle;
   pickle.WriteInt(kCmdDidProcessCrash);
diff --git a/chrome/browser/zygote_host_linux.h b/chrome/browser/zygote_host_linux.h
index 53485e0..21118d5 100644
--- a/chrome/browser/zygote_host_linux.h
+++ b/chrome/browser/zygote_host_linux.h
@@ -26,15 +26,19 @@ class ZygoteHost {
  public:
   void Init(const std::string& sandbox_cmd);
 
-  // Tries to start a renderer process.  Returns its pid on success, otherwise
-  // base::kNullProcessHandle;
+  // Tries to start a renderer process. Returns its pid on success, otherwise
+  // base::kNullProcessHandle. Must be called on the PROCESS_LAUNCHER
+  // thread.
   pid_t ForkRenderer(const std::vector<std::string>& command_line,
                      const base::GlobalDescriptors::Mapping& mapping);
+
+  // Must be called on the PROCESS_LAUNCHER thread.
   void EnsureProcessTerminated(pid_t process);
 
   // Get the termination status (exit code) of the process and return true if
   // the status indicates the process crashed. |child_exited| is set to true
   // iff the child process has terminated. (|child_exited| may be NULL.)
+  // Must be called on the PROCESS_LAUNCHER thread.
   bool DidProcessCrash(base::ProcessHandle handle, bool* child_exited);
 
   // These are the command codes used on the wire between the browser and the
@@ -52,7 +56,10 @@ class ZygoteHost {
   ZygoteHost();
   ~ZygoteHost();
 
-  int control_fd_;  // the socket to the zygote
+  // The socket to the zygote, which should only be accessed on the
+  // PROCESS_LAUNCHER thread.
+  int control_fd_;
+
   pid_t pid_;
   bool init_;
   bool using_suid_sandbox_;