Fix getting exit status for browser child process.

This makes BrowserChildProcessHost::GetTerminationStatus() similar to
RenderProcessHostImpl::GetChildTerminationStatus().

Linux requires waitpid() to block to get an exit status reliably. Otherwise
the kernel is at liberty to not reap the process, even if it's already currently
exiting or dead.

If we know that a process is dead, we should make use of the known_dead
argument to ChildProcessLauncher::GetChildTerminationStatus(), as does
RenderProcessHostImpl::GetChildTerminationStatus().

A discussion of this tricky issue is available in crbug.com/157458 and the
related CL.

This will, among other things, fix NaCl's crash throttling security measure.

BUG=274827,nativeclient:359
R=mseaborn@chromium.org, piman@chromium.org

Review URL: https://codereview.chromium.org/23019012

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

7 files changed, 26 insertions, 10 deletions

diff --git a/chrome/browser/nacl_host/nacl_process_host.cc b/chrome/browser/nacl_host/nacl_process_host.cc
index 492e06f..ce6aae7 100644
--- a/chrome/browser/nacl_host/nacl_process_host.cc
+++ b/chrome/browser/nacl_host/nacl_process_host.cc
@@ -266,10 +266,10 @@ NaClProcessHost::~NaClProcessHost() {
   // Report exit status only if the process was successfully started.
   if (process_->GetData().handle != base::kNullProcessHandle) {
     int exit_code = 0;
-    process_->GetTerminationStatus(&exit_code);
+    process_->GetTerminationStatus(false /* known_dead */, &exit_code);
     std::string message =
         base::StringPrintf("NaCl process exited with status %i (0x%x)",
-                         exit_code, exit_code);
+                           exit_code, exit_code);
     if (exit_code == 0) {
       LOG(INFO) << message;
     } else {
diff --git a/content/browser/browser_child_process_host_impl.cc b/content/browser/browser_child_process_host_impl.cc
index 780dc6a..172674e 100644
--- a/content/browser/browser_child_process_host_impl.cc
+++ b/content/browser/browser_child_process_host_impl.cc
@@ -223,11 +223,11 @@ void BrowserChildProcessHostImpl::NotifyProcessInstanceCreated(
 }
 
 base::TerminationStatus BrowserChildProcessHostImpl::GetTerminationStatus(
-    int* exit_code) {
+    bool known_dead, int* exit_code) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (!child_process_)  // If the delegate doesn't use Launch() helper.
     return base::GetTerminationStatus(data_.handle, exit_code);
-  return child_process_->GetChildTerminationStatus(false /* known_dead */,
+  return child_process_->GetChildTerminationStatus(known_dead,
                                                    exit_code);
 }
 
@@ -264,7 +264,8 @@ void BrowserChildProcessHostImpl::OnChildDisconnected() {
   if (child_process_.get() || data_.handle) {
     DCHECK(data_.handle != base::kNullProcessHandle);
     int exit_code;
-    base::TerminationStatus status = GetTerminationStatus(&exit_code);
+    base::TerminationStatus status = GetTerminationStatus(
+        true /* known_dead */, &exit_code);
     switch (status) {
       case base::TERMINATION_STATUS_PROCESS_CRASHED:
       case base::TERMINATION_STATUS_ABNORMAL_TERMINATION: {
diff --git a/content/browser/browser_child_process_host_impl.h b/content/browser/browser_child_process_host_impl.h
index 61971d6..866808f 100644
--- a/content/browser/browser_child_process_host_impl.h
+++ b/content/browser/browser_child_process_host_impl.h
@@ -51,7 +51,8 @@ class CONTENT_EXPORT BrowserChildProcessHostImpl
       CommandLine* cmd_line) OVERRIDE;
   virtual const ChildProcessData& GetData() const OVERRIDE;
   virtual ChildProcessHost* GetHost() const OVERRIDE;
-  virtual base::TerminationStatus GetTerminationStatus(int* exit_code) OVERRIDE;
+  virtual base::TerminationStatus GetTerminationStatus(
+      bool known_dead, int* exit_code) OVERRIDE;
   virtual void SetName(const string16& name) OVERRIDE;
   virtual void SetHandle(base::ProcessHandle handle) OVERRIDE;
 
diff --git a/content/browser/child_process_launcher.h b/content/browser/child_process_launcher.h
index 5a6e1f9..fdbb58c 100644
--- a/content/browser/child_process_launcher.h
+++ b/content/browser/child_process_launcher.h
@@ -58,6 +58,11 @@ class CONTENT_EXPORT ChildProcessLauncher {
   // 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.
+  // On Linux, the use of |known_dead| is subtle and can be crucial if an
+  // accurate status is important. With |known_dead| set to false, a dead
+  // process could be seen as running. With |known_dead| set to true, the
+  // process will be killed if it was still running. See ZygoteHostImpl for
+  // more discussion of Linux implementation details.
   // |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.
diff --git a/content/browser/gpu/gpu_process_host.cc b/content/browser/gpu/gpu_process_host.cc
index 7839ea7..29d914e 100644
--- a/content/browser/gpu/gpu_process_host.cc
+++ b/content/browser/gpu/gpu_process_host.cc
@@ -520,7 +520,8 @@ GpuProcessHost::~GpuProcessHost() {
   std::string message;
   if (!in_process_) {
     int exit_code;
-    base::TerminationStatus status = process_->GetTerminationStatus(&exit_code);
+    base::TerminationStatus status = process_->GetTerminationStatus(
+        false /* known_dead */, &exit_code);
     UMA_HISTOGRAM_ENUMERATION("GPU.GPUProcessTerminationStatus",
                               status,
                               base::TERMINATION_STATUS_MAX_ENUM);
@@ -1045,7 +1046,7 @@ void GpuProcessHost::OnProcessLaunched() {
 void GpuProcessHost::OnProcessCrashed(int exit_code) {
   SendOutstandingReplies();
   GpuDataManagerImpl::GetInstance()->ProcessCrashed(
-      process_->GetTerminationStatus(NULL));
+      process_->GetTerminationStatus(true /* known_dead */, NULL));
 }
 
 GpuProcessHost::GpuProcessKind GpuProcessHost::kind() {
diff --git a/content/browser/zygote_host/zygote_host_impl_linux.h b/content/browser/zygote_host/zygote_host_impl_linux.h
index 9027fe7..6a1aca5 100644
--- a/content/browser/zygote_host/zygote_host_impl_linux.h
+++ b/content/browser/zygote_host/zygote_host_impl_linux.h
@@ -40,7 +40,11 @@ class CONTENT_EXPORT ZygoteHostImpl : public ZygoteHost {
   // 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.
+  // is dead. When |known_dead| is false, processes could be seen as
+  // still running, even when they're not. When |known_dead| is true, the
+  // process will be SIGKILL-ed first (which should have no effect if it was
+  // really dead). This is to prevent a waiting waitpid() from blocking in
+  // a single-threaded Zygote. See crbug.com/157458.
   base::TerminationStatus GetTerminationStatus(base::ProcessHandle handle,
                                                bool known_dead,
                                                int* exit_code);
diff --git a/content/public/browser/browser_child_process_host.h b/content/public/browser/browser_child_process_host.h
index 7553478..977b1b4 100644
--- a/content/public/browser/browser_child_process_host.h
+++ b/content/public/browser/browser_child_process_host.h
@@ -61,7 +61,11 @@ class CONTENT_EXPORT BrowserChildProcessHost : public IPC::Sender {
   // status returned when the process exited (for posix, as returned
   // from waitpid(), for Windows, as returned from
   // GetExitCodeProcess()).  |exit_code| may be NULL.
-  virtual base::TerminationStatus GetTerminationStatus(int* exit_code) = 0;
+  // |known_dead| indicates that the child is already dead. On Linux, this
+  // information is necessary to retrieve accurate information. See
+  // ChildProcessLauncher::GetChildTerminationStatus() for more details.
+  virtual base::TerminationStatus GetTerminationStatus(
+      bool known_dead, int* exit_code) = 0;
 
   // Sets the user-visible name of the process.
   virtual void SetName(const string16& name) = 0;