Make DidProcessCrash a bit more solid and accurate on Windows

I hit the NOTREACHED() inside it while debugging an unrelated problem.

Also, now there is one less special case in this function's contract.

TEST=none
BUG=38048

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

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

5 files changed, 38 insertions, 14 deletions

diff --git a/base/process_util.h b/base/process_util.h
index 705b90b..20f16a7 100644
--- a/base/process_util.h
+++ b/base/process_util.h
@@ -264,10 +264,6 @@ bool KillProcessById(ProcessId process_id, int exit_code, bool wait);
 // 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.)
-//
-// On Windows, it is an error to call this if the process hasn't terminated
-// yet. On POSIX, |child_exited| is set correctly since we detect terminate in
-// a different manner on POSIX.
 bool DidProcessCrash(bool* child_exited, ProcessHandle handle);
 
 // Waits for process to exit. In POSIX systems, if the process hasn't been
diff --git a/base/process_util_unittest.cc b/base/process_util_unittest.cc
index 296aaa9..b4d9b9f 100644
--- a/base/process_util_unittest.cc
+++ b/base/process_util_unittest.cc
@@ -78,6 +78,24 @@ TEST_F(ProcessUtilTest, KillSlowChild) {
   base::CloseProcessHandle(handle);
 }
 
+TEST_F(ProcessUtilTest, DidProcessCrash) {
+  remove("SlowChildProcess.die");
+  ProcessHandle handle = this->SpawnChild(L"SlowChildProcess");
+  ASSERT_NE(base::kNullProcessHandle, handle);
+
+  bool child_exited = true;
+  EXPECT_FALSE(base::DidProcessCrash(&child_exited, handle));
+  EXPECT_FALSE(child_exited);
+
+  FILE *fp = fopen("SlowChildProcess.die", "w");
+  fclose(fp);
+  EXPECT_TRUE(base::WaitForSingleProcess(handle, 5000));
+
+  EXPECT_FALSE(base::DidProcessCrash(&child_exited, handle));
+
+  base::CloseProcessHandle(handle);
+}
+
 // Ensure that the priority of a process is restored correctly after
 // backgrounding and restoring.
 // Note: a platform may not be willing or able to lower the priority of
diff --git a/base/process_util_win.cc b/base/process_util_win.cc
index 05d3d60..a845ac7 100644
--- a/base/process_util_win.cc
+++ b/base/process_util_win.cc
@@ -318,19 +318,33 @@ bool KillProcess(ProcessHandle process, int exit_code, bool wait) {
 bool DidProcessCrash(bool* child_exited, ProcessHandle handle) {
   DWORD exitcode = 0;
 
-  if (child_exited)
-    *child_exited = true;  // On Windows it an error to call this function if
-                           // the child hasn't already exited.
   if (!::GetExitCodeProcess(handle, &exitcode)) {
     NOTREACHED();
+    // Assume the child has exited.
+    if (child_exited)
+      *child_exited = true;
     return false;
   }
   if (exitcode == STILL_ACTIVE) {
-    // The process is likely not dead or it used 0x103 as exit code.
+    DWORD wait_result = WaitForSingleObject(handle, 0);
+    if (wait_result == WAIT_TIMEOUT) {
+      if (child_exited)
+        *child_exited = false;
+      return false;
+    }
+
+    DCHECK_EQ(WAIT_OBJECT_0, wait_result);
+
+    // Strange, the process used 0x103 (STILL_ACTIVE) as exit code.
     NOTREACHED();
+
     return false;
   }
 
+  // We're sure the child has exited.
+  if (child_exited)
+    *child_exited = true;
+
   // Warning, this is not generic code; it heavily depends on the way
   // the rest of the code kills a process.
 
diff --git a/chrome/browser/child_process_launcher.cc b/chrome/browser/child_process_launcher.cc
index ff555fe..206d8a7 100644
--- a/chrome/browser/child_process_launcher.cc
+++ b/chrome/browser/child_process_launcher.cc
@@ -312,8 +312,6 @@ bool ChildProcessLauncher::DidProcessCrash() {
   // 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.
-  //
-  // This is moot on Windows where |child_exited| will always be true.
   if (child_exited)
     context_->process_.Close();
 
diff --git a/chrome/browser/nacl_host/nacl_process_host.cc b/chrome/browser/nacl_host/nacl_process_host.cc
index 1e3bcff..2648b3e 100644
--- a/chrome/browser/nacl_host/nacl_process_host.cc
+++ b/chrome/browser/nacl_host/nacl_process_host.cc
@@ -118,10 +118,8 @@ void NaClProcessHost::OnProcessLaunchedByBroker(base::ProcessHandle handle) {
 }
 
 bool NaClProcessHost::DidChildCrash() {
-  if (running_on_wow64_) {
-    bool child_exited;
-    return base::DidProcessCrash(&child_exited, handle());
-  }
+  if (running_on_wow64_)
+    return base::DidProcessCrash(NULL, handle());
   return ChildProcessHost::DidChildCrash();
 }