Better account crashes for the BrowserChildProcessHost (and derived) for windowsThe issue here is that OnChildDied() can be called before the process is actually dead, in suchcases the GetChildTerminationStatus returns TERMINATION_STATUS_STILL_RUNNING, because we don'thave yet the actual return code which we ignore in BrowserChildProcessHost.Now we wait until we have the actual return code. This is going to increase the countfor the following histograms:ChildProcess.CrashesChildProcess.KillsAnd will also increase the counts of histograms (if any) on the derived classes:PluginProcessHostUtilityProcessHostGpuProcessHostPpapiPluginProcessHostNaClProcessHostWorkerProcessHostProfileImportProcessHost

and also for renderers:BrowserRenderProcessHostBUG=96059TEST=see bug
Review URL: http://codereview.chromium.org/7888070

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

7 files changed, 156 insertions, 49 deletions

diff --git a/content/browser/browser_child_process_host.cc b/content/browser/browser_child_process_host.cc
index 40b6598..aefd741 100644
--- a/content/browser/browser_child_process_host.cc
+++ b/content/browser/browser_child_process_host.cc
@@ -23,6 +23,10 @@
 #include "content/common/process_watcher.h"
 #include "content/common/result_codes.h"
 
+#if defined(OS_WIN)
+#include "base/synchronization/waitable_event.h"
+#endif  // OS_WIN
+
 namespace {
 
 typedef std::list<BrowserChildProcessHost*> ChildProcessList;
@@ -54,7 +58,8 @@ class ChildNotificationTask : public Task {
 BrowserChildProcessHost::BrowserChildProcessHost(
     ChildProcessInfo::ProcessType type)
     : ChildProcessInfo(type, -1),
-      ALLOW_THIS_IN_INITIALIZER_LIST(client_(this)) {
+      ALLOW_THIS_IN_INITIALIZER_LIST(client_(this)),
+      disconnect_was_alive_(false) {
   AddFilter(new TraceMessageFilter);
 
   g_child_process_list.Get().push_back(this);
@@ -123,37 +128,69 @@ base::TerminationStatus BrowserChildProcessHost::GetChildTerminationStatus(
   return child_process_->GetChildTerminationStatus(exit_code);
 }
 
-void BrowserChildProcessHost::OnChildDied() {
-  // This may be called by both the channel's OnChannelError handler
-  // as well as the process launcher's OnProcessLaunched handler.
-  if (handle() != base::kNullProcessHandle) {
-    int exit_code;
-    base::TerminationStatus status = GetChildTerminationStatus(&exit_code);
-    switch (status) {
-      case base::TERMINATION_STATUS_PROCESS_CRASHED:
-      case base::TERMINATION_STATUS_ABNORMAL_TERMINATION: {
-        OnProcessCrashed(exit_code);
-
-        // Report that this child process crashed.
-        Notify(content::NOTIFICATION_CHILD_PROCESS_CRASHED);
-        UMA_HISTOGRAM_COUNTS("ChildProcess.Crashes", this->type());
-        break;
+// The ChildProcessHost default implementation calls OnChildDied() always
+// but at this layer and below we need to have the final child process exit
+// code to properly bucket crashes vs kills. At least on Windows we can do
+// this if we wait until the process handle is signaled, however this means
+// that this function can be called twice: once from the actual channel error
+// and once from OnWaitableEventSignaled().
+void BrowserChildProcessHost::OnChildDisconnected() {
+  DCHECK(handle() != base::kNullProcessHandle);
+  int exit_code;
+  base::TerminationStatus status = GetChildTerminationStatus(&exit_code);
+  switch (status) {
+    case base::TERMINATION_STATUS_PROCESS_CRASHED:
+    case base::TERMINATION_STATUS_ABNORMAL_TERMINATION: {
+      OnProcessCrashed(exit_code);
+      // Report that this child process crashed.
+      Notify(content::NOTIFICATION_CHILD_PROCESS_CRASHED);
+      UMA_HISTOGRAM_COUNTS("ChildProcess.Crashes", this->type());
+      if (disconnect_was_alive_) {
+        UMA_HISTOGRAM_COUNTS("ChildProcess.CrashesWasAlive", this->type());
       }
-      case base::TERMINATION_STATUS_PROCESS_WAS_KILLED: {
-        OnProcessWasKilled(exit_code);
-
-        // Report that this child process was killed.
-        Notify(content::NOTIFICATION_CHILD_PROCESS_WAS_KILLED);
-        UMA_HISTOGRAM_COUNTS("ChildProcess.Kills", this->type());
-        break;
+      break;
+    }
+    case base::TERMINATION_STATUS_PROCESS_WAS_KILLED: {
+      OnProcessWasKilled(exit_code);
+      // Report that this child process was killed.
+      Notify(content::NOTIFICATION_CHILD_PROCESS_WAS_KILLED);
+      UMA_HISTOGRAM_COUNTS("ChildProcess.Kills", this->type());
+      if (disconnect_was_alive_) {
+        UMA_HISTOGRAM_COUNTS("ChildProcess.KillsWasAlive", this->type());
       }
-      default:
-        break;
+      break;
     }
-    // Notify in the main loop of the disconnection.
-    Notify(content::NOTIFICATION_CHILD_PROCESS_HOST_DISCONNECTED);
+    case base::TERMINATION_STATUS_STILL_RUNNING: {
+      // exit code not yet available.
+      disconnect_was_alive_ = true;
+#if defined(OS_WIN)
+      child_watcher_.StartWatching(new base::WaitableEvent(handle()), this);
+#endif
+      return;
+    }
+
+    default:
+      break;
   }
-  ChildProcessHost::OnChildDied();
+  // Notify in the main loop of the disconnection.
+  Notify(content::NOTIFICATION_CHILD_PROCESS_HOST_DISCONNECTED);
+  OnChildDied();
+}
+
+// The child process handle has been signaled so the exit code is finally
+// available. Unfortunately STILL_ACTIVE (0x103) is a valid exit code in
+// which case we should not call OnChildDisconnected() or else we will be
+// waiting forever.
+void BrowserChildProcessHost::OnWaitableEventSignaled(
+    base::WaitableEvent* waitable_event) {
+#if defined (OS_WIN)
+  unsigned long exit_code = 0;
+  GetExitCodeProcess(waitable_event->Release(), &exit_code);
+  delete waitable_event;
+  if (exit_code == STILL_ACTIVE)
+    OnChildDied();
+  BrowserChildProcessHost::OnChildDisconnected();
+#endif
 }
 
 void BrowserChildProcessHost::ShutdownStarted() {
diff --git a/content/browser/browser_child_process_host.h b/content/browser/browser_child_process_host.h
index bf5b881..499b632 100644
--- a/content/browser/browser_child_process_host.h
+++ b/content/browser/browser_child_process_host.h
@@ -8,11 +8,16 @@
 
 #include <list>
 
+#include "base/synchronization/waitable_event_watcher.h"
 #include "content/browser/child_process_launcher.h"
 #include "content/common/child_process_host.h"
 #include "content/common/child_process_info.h"
 #include "content/common/content_export.h"
 
+namespace base {
+class WaitableEvent;
+}
+
 // Plugins/workers and other child processes that live on the IO thread should
 // derive from this class.
 //
@@ -21,10 +26,14 @@
 class CONTENT_EXPORT BrowserChildProcessHost :
     public ChildProcessHost,
     public ChildProcessInfo,
-    public ChildProcessLauncher::Client {
+    public ChildProcessLauncher::Client,
+    public base::WaitableEventWatcher::Delegate {
  public:
   virtual ~BrowserChildProcessHost();
 
+  virtual void OnWaitableEventSignaled(
+      base::WaitableEvent* waitable_event) OVERRIDE;
+
   // Terminates all child processes and deletes each ChildProcessHost instance.
   static void TerminateAll();
 
@@ -87,7 +96,7 @@ class CONTENT_EXPORT BrowserChildProcessHost :
   virtual base::TerminationStatus GetChildTerminationStatus(int* exit_code);
 
   // Overrides from ChildProcessHost
-  virtual void OnChildDied();
+  virtual void OnChildDisconnected();
   virtual void ShutdownStarted();
   virtual void Notify(int type);
   // Extends the base class implementation and removes this host from
@@ -111,6 +120,10 @@ class CONTENT_EXPORT BrowserChildProcessHost :
   };
   ClientHook client_;
   scoped_ptr<ChildProcessLauncher> child_process_;
+#if defined(OS_WIN)
+  base::WaitableEventWatcher child_watcher_;
+#endif
+  bool disconnect_was_alive_;
 };
 
 #endif  // CONTENT_BROWSER_BROWSER_CHILD_PROCESS_HOST_H_
diff --git a/content/browser/gpu/gpu_process_host.cc b/content/browser/gpu/gpu_process_host.cc
index c2af5bd..85d6b5f 100644
--- a/content/browser/gpu/gpu_process_host.cc
+++ b/content/browser/gpu/gpu_process_host.cc
@@ -501,7 +501,7 @@ void GpuProcessHost::OnChildDied() {
                               content::RESULT_CODE_LAST_CODE);
   }
 
-  BrowserChildProcessHost::OnChildDied();
+  ChildProcessHost::OnChildDied();
 }
 
 void GpuProcessHost::OnProcessCrashed(int exit_code) {
diff --git a/content/browser/renderer_host/browser_render_process_host.cc b/content/browser/renderer_host/browser_render_process_host.cc
index 8578251..8c3034c 100644
--- a/content/browser/renderer_host/browser_render_process_host.cc
+++ b/content/browser/renderer_host/browser_render_process_host.cc
@@ -93,6 +93,7 @@
 
 #if defined(OS_WIN)
 #include <objbase.h>
+#include "base/synchronization/waitable_event.h"
 #include "content/common/section_util_win.h"
 #endif
 
@@ -650,7 +651,7 @@ bool BrowserRenderProcessHost::FastShutdownIfPossible() {
     return false;
 
   child_process_launcher_.reset();
-  ProcessDied();
+  ProcessDied(base::TERMINATION_STATUS_NORMAL_TERMINATION, 0, false);
   fast_shutdown_started_ = true;
   return true;
 }
@@ -801,15 +802,6 @@ void BrowserRenderProcessHost::OnChannelConnected(int32 peer_pid) {
 }
 
 void BrowserRenderProcessHost::OnChannelError() {
-  ProcessDied();
-}
-
-void BrowserRenderProcessHost::ProcessDied() {
-  // Our child process has died.  If we didn't expect it, it's a crash.
-  // In any case, we need to let everyone know it's gone.
-  // The OnChannelError notification can fire multiple times due to nested sync
-  // calls to a renderer. If we don't have a valid channel here it means we
-  // already handled the error.
   if (!channel_.get())
     return;
 
@@ -821,15 +813,54 @@ void BrowserRenderProcessHost::ProcessDied() {
       child_process_launcher_->GetChildTerminationStatus(&exit_code) :
       base::TERMINATION_STATUS_NORMAL_TERMINATION;
 
+#if defined(OS_WIN)
+  if (!run_renderer_in_process()) {
+    if (status == base::TERMINATION_STATUS_STILL_RUNNING) {
+      HANDLE process = child_process_launcher_->GetHandle();
+      child_process_watcher_.StartWatching(
+          new base::WaitableEvent(process), this);
+      return;
+    }
+  }
+#endif
+  ProcessDied(status, exit_code, false);
+}
+
+// Called when the renderer process handle has been signaled.
+void BrowserRenderProcessHost::OnWaitableEventSignaled(
+    base::WaitableEvent* waitable_event) {
+#if defined (OS_WIN)
+  DCHECK(child_process_launcher_.get());
+  int exit_code = 0;
+  base::TerminationStatus status =
+      child_process_launcher_->GetChildTerminationStatus(&exit_code);
+  ProcessDied(status, exit_code, true);
+#endif
+}
+
+void BrowserRenderProcessHost::ProcessDied(
+    base::TerminationStatus status, int exit_code, bool was_alive) {
+  // Our child process has died.  If we didn't expect it, it's a crash.
+  // In any case, we need to let everyone know it's gone.
+  // The OnChannelError notification can fire multiple times due to nested sync
+  // calls to a renderer. If we don't have a valid channel here it means we
+  // already handled the error.
+
   if (status == base::TERMINATION_STATUS_PROCESS_CRASHED ||
       status == base::TERMINATION_STATUS_ABNORMAL_TERMINATION) {
     UMA_HISTOGRAM_PERCENTAGE("BrowserRenderProcessHost.ChildCrashes",
                              is_extension_process_ ? 2 : 1);
-  }
-
-  if (status == base::TERMINATION_STATUS_PROCESS_WAS_KILLED) {
+    if (was_alive) {
+      UMA_HISTOGRAM_PERCENTAGE("BrowserRenderProcessHost.ChildCrashesWasAlive",
+                               is_extension_process_ ? 2 : 1);
+    }
+  } else if (status == base::TERMINATION_STATUS_PROCESS_WAS_KILLED) {
     UMA_HISTOGRAM_PERCENTAGE("BrowserRenderProcessHost.ChildKills",
                              is_extension_process_ ? 2 : 1);
+    if (was_alive) {
+      UMA_HISTOGRAM_PERCENTAGE("BrowserRenderProcessHost.ChildKillsWasAlive",
+                               is_extension_process_ ? 2 : 1);
+    }
   }
 
   RendererClosedDetails details(status, exit_code, is_extension_process_);
diff --git a/content/browser/renderer_host/browser_render_process_host.h b/content/browser/renderer_host/browser_render_process_host.h
index 8e95b02..d209b03 100644
--- a/content/browser/renderer_host/browser_render_process_host.h
+++ b/content/browser/renderer_host/browser_render_process_host.h
@@ -12,6 +12,7 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "base/process.h"
+#include "base/synchronization/waitable_event_watcher.h"
 #include "base/timer.h"
 #include "content/browser/child_process_launcher.h"
 #include "content/browser/renderer_host/render_process_host.h"
@@ -23,6 +24,7 @@ class RenderWidgetHelper;
 
 namespace base {
 class SharedMemory;
+class WaitableEvent;
 }
 
 // Implements a concrete RenderProcessHost for the browser process for talking
@@ -40,7 +42,8 @@ class SharedMemory;
 // are correlated with IDs. This way, the Views and the corresponding ViewHosts
 // communicate through the two process objects.
 class BrowserRenderProcessHost : public RenderProcessHost,
-                                 public ChildProcessLauncher::Client {
+                                 public ChildProcessLauncher::Client,
+                                 public base::WaitableEventWatcher::Delegate {
  public:
   explicit BrowserRenderProcessHost(content::BrowserContext* browser_context);
   virtual ~BrowserRenderProcessHost();
@@ -76,6 +79,10 @@ class BrowserRenderProcessHost : public RenderProcessHost,
   // ChildProcessLauncher::Client implementation.
   virtual void OnProcessLaunched();
 
+  // base::WaitableEventWatcher::Delegate implementation.
+  virtual void OnWaitableEventSignaled(
+      base::WaitableEvent* waitable_event) OVERRIDE;
+
  private:
   friend class VisitRelayingRenderProcessHost;
 
@@ -102,8 +109,11 @@ class BrowserRenderProcessHost : public RenderProcessHost,
   // Callers can reduce the RenderProcess' priority.
   void SetBackgrounded(bool backgrounded);
 
-  // Handle termination of our process.
-  void ProcessDied();
+  // Handle termination of our process. |was_alive| indicates that when we
+  // tried to retrieve the exit code the process had not finished yet.
+  void ProcessDied(base::TerminationStatus status,
+                   int exit_code,
+                   bool was_alive);
 
   // The count of currently visible widgets.  Since the host can be a container
   // for multiple widgets, it uses this count to determine when it should be
@@ -150,6 +160,11 @@ class BrowserRenderProcessHost : public RenderProcessHost,
   // because the queued messages may have dependencies on the init messages.
   std::queue<IPC::Message*> queued_messages_;
 
+#if defined(OS_WIN)
+  // Used to wait until the renderer dies to get an accurrate exit code.
+  base::WaitableEventWatcher child_process_watcher_;
+#endif
+
   DISALLOW_COPY_AND_ASSIGN(BrowserRenderProcessHost);
 };
 
diff --git a/content/common/child_process_host.cc b/content/common/child_process_host.cc
index bcaca6d..b8a3b95 100644
--- a/content/common/child_process_host.cc
+++ b/content/common/child_process_host.cc
@@ -225,6 +225,10 @@ void ChildProcessHost::OnChildDied() {
   delete this;
 }
 
+void ChildProcessHost::OnChildDisconnected() {
+  OnChildDied();
+}
+
 void ChildProcessHost::ShutdownStarted() {
 }
 
@@ -301,7 +305,7 @@ void ChildProcessHost::ListenerHook::OnChannelError() {
     host_->filters_[i]->OnChannelError();
 
   // This will delete host_, which will also destroy this!
-  host_->OnChildDied();
+  host_->OnChildDisconnected();
 }
 
 void ChildProcessHost::ForceShutdown() {
diff --git a/content/common/child_process_host.h b/content/common/child_process_host.h
index 5ffb8d9..5b965b9 100644
--- a/content/common/child_process_host.h
+++ b/content/common/child_process_host.h
@@ -121,10 +121,17 @@ class CONTENT_EXPORT ChildProcessHost : public IPC::Channel::Listener,
   const std::string& channel_id() { return channel_id_; }
   IPC::Channel* channel() { return channel_.get(); }
 
-  // Called when the child process goes away.
+  // Called when the child process goes away. See OnChildDisconnected().
   virtual void OnChildDied();
+
+  // Called when the child process unexpected closes the IPC channel. The
+  // default action is to call OnChildDied(). Subclasses might want to override
+  // this behavior.
+  virtual void OnChildDisconnected();
+
   // Notifies the derived class that we told the child process to kill itself.
   virtual void ShutdownStarted();
+
   // Subclasses can implement specific notification methods.
   virtual void Notify(int type);