Linux: inform the Zygote when it's waiting on a dead process

If the browser calls ProcessDied() and asks the Zygote to wait (without blocking)
on a dead process, the kernel might not be done destroying it and the Zygote may
mistakenly claim that the process is alive.

We now inform the Zygote over the IPC that the process is already dead so
that it can wait synchroneously.

BUG=157458
NOTRY=true


Review URL: https://chromiumcodereview.appspot.com/11316261

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

10 files changed, 100 insertions, 51 deletions

diff --git a/base/process_util.h b/base/process_util.h
index 2805e42..f2d374c 100644
--- a/base/process_util.h
+++ b/base/process_util.h
@@ -512,6 +512,15 @@ BASE_EXPORT bool KillProcessById(ProcessId process_id, int exit_code,
 BASE_EXPORT TerminationStatus GetTerminationStatus(ProcessHandle handle,
                                                    int* exit_code);
 
+#if defined(OS_POSIX)
+// Wait for the process to exit and get the termination status. See
+// GetTerminationStatus for more information. On POSIX systems, we can't call
+// WaitForExitCode and then GetTerminationStatus as the child will be reaped
+// when WaitForExitCode return and this information will be lost.
+BASE_EXPORT TerminationStatus WaitForTerminationStatus(ProcessHandle handle,
+                                                       int* exit_code);
+#endif  // defined(OS_POSIX)
+
 // Waits for process to exit. On POSIX systems, if the process hasn't been
 // signaled then puts the exit code in |exit_code|; otherwise it's considered
 // a failure. On Windows |exit_code| is always filled. Returns true on success,
diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc
index a19cc2a..284d00f 100644
--- a/base/process_util_posix.cc
+++ b/base/process_util_posix.cc
@@ -152,6 +152,50 @@ void ResetChildSignalHandlersToDefaults() {
   signal(SIGTERM, SIG_DFL);
 }
 
+TerminationStatus GetTerminationStatusImpl(ProcessHandle handle,
+                                           bool can_block,
+                                           int* exit_code) {
+  int status = 0;
+  const pid_t result = HANDLE_EINTR(waitpid(handle, &status,
+                                            can_block ? 0 : WNOHANG));
+  if (result == -1) {
+    DPLOG(ERROR) << "waitpid(" << handle << ")";
+    if (exit_code)
+      *exit_code = 0;
+    return TERMINATION_STATUS_NORMAL_TERMINATION;
+  } else if (result == 0) {
+    // the child hasn't exited yet.
+    if (exit_code)
+      *exit_code = 0;
+    return TERMINATION_STATUS_STILL_RUNNING;
+  }
+
+  if (exit_code)
+    *exit_code = status;
+
+  if (WIFSIGNALED(status)) {
+    switch (WTERMSIG(status)) {
+      case SIGABRT:
+      case SIGBUS:
+      case SIGFPE:
+      case SIGILL:
+      case SIGSEGV:
+        return TERMINATION_STATUS_PROCESS_CRASHED;
+      case SIGINT:
+      case SIGKILL:
+      case SIGTERM:
+        return TERMINATION_STATUS_PROCESS_WAS_KILLED;
+      default:
+        break;
+    }
+  }
+
+  if (WIFEXITED(status) && WEXITSTATUS(status) != 0)
+    return TERMINATION_STATUS_ABNORMAL_TERMINATION;
+
+  return TERMINATION_STATUS_NORMAL_TERMINATION;
+}
+
 }  // anonymous namespace
 
 ProcessId GetCurrentProcId() {
@@ -754,44 +798,12 @@ void RaiseProcessToHighPriority() {
 }
 
 TerminationStatus GetTerminationStatus(ProcessHandle handle, int* exit_code) {
-  int status = 0;
-  const pid_t result = HANDLE_EINTR(waitpid(handle, &status, WNOHANG));
-  if (result == -1) {
-    DPLOG(ERROR) << "waitpid(" << handle << ")";
-    if (exit_code)
-      *exit_code = 0;
-    return TERMINATION_STATUS_NORMAL_TERMINATION;
-  } else if (result == 0) {
-    // the child hasn't exited yet.
-    if (exit_code)
-      *exit_code = 0;
-    return TERMINATION_STATUS_STILL_RUNNING;
-  }
-
-  if (exit_code)
-    *exit_code = status;
-
-  if (WIFSIGNALED(status)) {
-    switch (WTERMSIG(status)) {
-      case SIGABRT:
-      case SIGBUS:
-      case SIGFPE:
-      case SIGILL:
-      case SIGSEGV:
-        return TERMINATION_STATUS_PROCESS_CRASHED;
-      case SIGINT:
-      case SIGKILL:
-      case SIGTERM:
-        return TERMINATION_STATUS_PROCESS_WAS_KILLED;
-      default:
-        break;
-    }
-  }
-
-  if (WIFEXITED(status) && WEXITSTATUS(status) != 0)
-    return TERMINATION_STATUS_ABNORMAL_TERMINATION;
+  return GetTerminationStatusImpl(handle, false /* can_block */, exit_code);
+}
 
-  return TERMINATION_STATUS_NORMAL_TERMINATION;
+TerminationStatus WaitForTerminationStatus(ProcessHandle handle,
+                                           int* exit_code) {
+  return GetTerminationStatusImpl(handle, true /* can_block */, exit_code);
 }
 
 bool WaitForExitCode(ProcessHandle handle, int* exit_code) {
diff --git a/content/browser/browser_child_process_host_impl.cc b/content/browser/browser_child_process_host_impl.cc
index f095e98..ffb51c0 100644
--- a/content/browser/browser_child_process_host_impl.cc
+++ b/content/browser/browser_child_process_host_impl.cc
@@ -194,7 +194,8 @@ base::TerminationStatus BrowserChildProcessHostImpl::GetTerminationStatus(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (!child_process_.get())  // If the delegate doesn't use Launch() helper.
     return base::GetTerminationStatus(data_.handle, exit_code);
-  return child_process_->GetChildTerminationStatus(exit_code);
+  return child_process_->GetChildTerminationStatus(false /* known_dead */,
+                                                   exit_code);
 }
 
 bool BrowserChildProcessHostImpl::OnMessageReceived(
diff --git a/content/browser/child_process_launcher.cc b/content/browser/child_process_launcher.cc
index bee658d..a418942 100644
--- a/content/browser/child_process_launcher.cc
+++ b/content/browser/child_process_launcher.cc
@@ -415,6 +415,7 @@ base::ProcessHandle ChildProcessLauncher::GetHandle() {
 }
 
 base::TerminationStatus ChildProcessLauncher::GetChildTerminationStatus(
+    bool known_dead,
     int* exit_code) {
   base::ProcessHandle handle = context_->process_.handle();
   if (handle == base::kNullProcessHandle) {
@@ -426,7 +427,7 @@ base::TerminationStatus ChildProcessLauncher::GetChildTerminationStatus(
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
   if (context_->zygote_) {
     context_->termination_status_ = ZygoteHostImpl::GetInstance()->
-        GetTerminationStatus(handle, &context_->exit_code_);
+        GetTerminationStatus(handle, known_dead, &context_->exit_code_);
   } else
 #endif
   {
diff --git a/content/browser/child_process_launcher.h b/content/browser/child_process_launcher.h
index e28419c..9ee3d41 100644
--- a/content/browser/child_process_launcher.h
+++ b/content/browser/child_process_launcher.h
@@ -53,11 +53,14 @@ class CONTENT_EXPORT ChildProcessLauncher {
   // Getter for the process handle.  Only call after the process has started.
   base::ProcessHandle GetHandle();
 
-  // Call this when the child process exits to know what happened to
-  // it.  |exit_code| is the exit code of the process if it exited
-  // (e.g. status from waitpid if on posix, from GetExitCodeProcess on
-  // Windows). |exit_code| may be NULL.
-  base::TerminationStatus GetChildTerminationStatus(int* exit_code);
+  // Call this when the child process exits to know what happened to it.
+  // |known_dead| can be true if we already know the process is dead as it can
+  // help the implemention figure the proper TerminationStatus.
+  // |exit_code| is the exit code of the process if it exited (e.g. status from
+  // waitpid if on posix, from GetExitCodeProcess on Windows). |exit_code| may
+  // be NULL.
+  base::TerminationStatus GetChildTerminationStatus(bool known_dead,
+                                                    int* exit_code);
 
   // Changes whether the process runs in the background or not.  Only call
   // this after the process has started.
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index f5e455f..3af3179 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -884,7 +884,7 @@ bool RenderProcessHostImpl::FastShutdownIfPossible() {
   if (!SuddenTerminationAllowed())
     return false;
 
-  ProcessDied();
+  ProcessDied(false /* already_dead */);
   fast_shutdown_started_ = true;
   return true;
 }
@@ -1065,7 +1065,7 @@ void RenderProcessHostImpl::OnChannelConnected(int32 peer_pid) {
 }
 
 void RenderProcessHostImpl::OnChannelError() {
-  ProcessDied();
+  ProcessDied(true /* already_dead */);
 }
 
 BrowserContext* RenderProcessHostImpl::GetBrowserContext() const {
@@ -1420,7 +1420,7 @@ void RenderProcessHostImpl::RegisterProcessHostForSite(
   map->RegisterProcess(site, process);
 }
 
-void RenderProcessHostImpl::ProcessDied() {
+void RenderProcessHostImpl::ProcessDied(bool already_dead) {
   // 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
@@ -1432,7 +1432,8 @@ void RenderProcessHostImpl::ProcessDied() {
   int exit_code = 0;
   base::TerminationStatus status =
       child_process_launcher_.get() ?
-      child_process_launcher_->GetChildTerminationStatus(&exit_code) :
+      child_process_launcher_->GetChildTerminationStatus(already_dead,
+                                                         &exit_code) :
       base::TERMINATION_STATUS_NORMAL_TERMINATION;
 
   RendererClosedDetails details(GetHandle(), status, exit_code);
diff --git a/content/browser/renderer_host/render_process_host_impl.h b/content/browser/renderer_host/render_process_host_impl.h
index ecf69c1c..e93525a 100644
--- a/content/browser/renderer_host/render_process_host_impl.h
+++ b/content/browser/renderer_host/render_process_host_impl.h
@@ -219,7 +219,7 @@ class CONTENT_EXPORT RenderProcessHostImpl
   void SetBackgrounded(bool backgrounded);
 
   // Handle termination of our process.
-  void ProcessDied();
+  void ProcessDied(bool already_dead);
 
   // 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
diff --git a/content/browser/zygote_host/zygote_host_impl_linux.cc b/content/browser/zygote_host/zygote_host_impl_linux.cc
index bbaa32a..308ad5d 100644
--- a/content/browser/zygote_host/zygote_host_impl_linux.cc
+++ b/content/browser/zygote_host/zygote_host_impl_linux.cc
@@ -451,10 +451,12 @@ void ZygoteHostImpl::EnsureProcessTerminated(pid_t process) {
 
 base::TerminationStatus ZygoteHostImpl::GetTerminationStatus(
     base::ProcessHandle handle,
+    bool known_dead,
     int* exit_code) {
   DCHECK(init_);
   Pickle pickle;
   pickle.WriteInt(kZygoteCommandGetTerminationStatus);
+  pickle.WriteBool(known_dead);
   pickle.WriteInt(handle);
 
   // Set this now to handle the early termination cases.
diff --git a/content/browser/zygote_host/zygote_host_impl_linux.h b/content/browser/zygote_host/zygote_host_impl_linux.h
index 8a6c778..1872840 100644
--- a/content/browser/zygote_host/zygote_host_impl_linux.h
+++ b/content/browser/zygote_host/zygote_host_impl_linux.h
@@ -37,7 +37,12 @@ class CONTENT_EXPORT ZygoteHostImpl : public ZygoteHost {
   // Get the termination status (and, optionally, the exit code) of
   // the process. |exit_code| is set to the exit code of the child
   // process. (|exit_code| may be NULL.)
+  // Unfortunately the Zygote can not accurately figure out if a process
+  // is already dead without waiting synchronously for it.
+  // |known_dead| should be set to true when we already know that the process
+  // is dead.
   base::TerminationStatus GetTerminationStatus(base::ProcessHandle handle,
+                                               bool known_dead,
                                                int* exit_code);
 
   // ZygoteHost implementation:
diff --git a/content/zygote/zygote_linux.cc b/content/zygote/zygote_linux.cc
index a5fb203..2ae6784 100644
--- a/content/zygote/zygote_linux.cc
+++ b/content/zygote/zygote_linux.cc
@@ -204,9 +204,11 @@ void Zygote::HandleReapRequest(int fd,
 void Zygote::HandleGetTerminationStatus(int fd,
                                         const Pickle& pickle,
                                         PickleIterator iter) {
+  bool known_dead;
   base::ProcessHandle child;
 
-  if (!pickle.ReadInt(&iter, &child)) {
+  if (!pickle.ReadBool(&iter, &known_dead) ||
+      !pickle.ReadInt(&iter, &child)) {
     LOG(WARNING) << "Error parsing GetTerminationStatus request "
                  << "from browser";
     return;
@@ -217,7 +219,20 @@ void Zygote::HandleGetTerminationStatus(int fd,
   if (UsingSUIDSandbox())
     child = real_pids_to_sandbox_pids[child];
   if (child) {
-    status = base::GetTerminationStatus(child, &exit_code);
+    if (known_dead) {
+      // If we know that the process is already dead and the kernel is cleaning
+      // it up, we do want to wait until it becomes a zombie and not risk
+      // returning eroneously that it is still running. However, we do not
+      // want to risk a bug where we're told a process is dead when it's not.
+      // By sending SIGKILL, we make sure that WaitForTerminationStatus will
+      // return quickly even in this case.
+      if (kill(child, SIGKILL)) {
+        PLOG(ERROR) << "kill (" << child << ")";
+      }
+      status = base::WaitForTerminationStatus(child, &exit_code);
+    } else {
+      status = base::GetTerminationStatus(child, &exit_code);
+    }
   } else {
     // Assume that if we can't find the child in the sandbox, then
     // it terminated normally.