Trying to reland r101435 : Better account crashes for the BrowserChildProcessHost

original code review:
http://codereview.chromium.org/7888070

In this change we remove renderer_host piece, which I landed already and fixes the
problem which was a missing break (line 170, see patch set 2).

TBR=jam
BUG=96059
TEST=see bug
Review URL: http://codereview.chromium.org/7919016

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

5 files changed, 96 insertions, 33 deletions

diff --git a/content/browser/browser_child_process_host.cc b/content/browser/browser_child_process_host.cc
index 40b6598..184ac82 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,71 @@ 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);
+      return;
+#else
+      break;
+#endif
+    }
+
+    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/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);