Simplify EnsureProcessTerminated() implementations.

EnsureProcessTerminated() is used by a parent process to ensure that a
child process it expects should exit "soon" actually does so. The child
process is asynchronously monitored, and forcibly terminated if it
fails to exit in a timely manner. Under POSIX platforms the child
process Id is also reaped with waitpid(), to release its exited
"zombie" process structure.

We had ended up with separate implementations for each platform, each
with different properties.

This CL makes the following changes:
- Reimplements POSIX EnsureProcessTerminated using base::Process.
- Provides a common implementation for platforms on which there is no
  need to waitpid() to cleanup zombie processes.

The Mac specialization will be replaced with an asynchronous
implementation in a subsequent patch.

Bug: 806451, 750756
Change-Id: If251dc13e7ad0a0cffb4f1921897a89305d6cb21
Reviewed-on: https://chromium-review.googlesource.com/920799
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547508}

base/process/kill.cc

@@ -4,7 +4,10 @@
 
 #include "base/process/kill.h"
 
+#include "base/bind.h"
 #include "base/process/process_iterator.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/time/time.h"
 
 namespace base {
 
@@ -28,4 +31,31 @@ bool KillProcesses(const FilePath::StringType& executable_name,
   return result;
 }
 
+#if defined(OS_WIN) || defined(OS_FUCHSIA)
+// Common implementation for platforms under which |process| is a handle to
+// the process, rather than an identifier that must be "reaped".
+void EnsureProcessTerminated(Process process) {
+  DCHECK(!process.is_current());
+
+  if (process.WaitForExitWithTimeout(TimeDelta(), nullptr))
+    return;
+
+  PostDelayedTaskWithTraits(
+      FROM_HERE,
+      {TaskPriority::BACKGROUND, TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+      BindOnce(
+          [](Process process) {
+            if (process.WaitForExitWithTimeout(TimeDelta(), nullptr))
+              return;
+#if defined(OS_WIN)
+            process.Terminate(win::kProcessKilledExitCode, false);
+#else
+            process.Terminate(-1, false);
+#endif
+          },
+          std::move(process)),
+      TimeDelta::FromSeconds(2));
+}
+#endif  // defined(OS_WIN) || defined(OS_FUCHSIA)
+
 }  // namespace base

base/process/kill.h

@@ -110,8 +110,22 @@ BASE_EXPORT TerminationStatus GetTerminationStatus(ProcessHandle handle,
 //
 BASE_EXPORT TerminationStatus GetKnownDeadTerminationStatus(
     ProcessHandle handle, int* exit_code);
+
+#if defined(OS_LINUX)
+// Spawns a thread to wait asynchronously for the child |process| to exit
+// and then reaps it.
+BASE_EXPORT void EnsureProcessGetsReaped(Process process);
+#endif  // defined(OS_LINUX)
 #endif  // defined(OS_POSIX) && !defined(OS_FUCHSIA)
 
+// Registers |process| to be asynchronously monitored for termination, forcibly
+// terminated if necessary, and reaped on exit. The caller should have signalled
+// |process| to exit before calling this API. The API will allow a couple of
+// seconds grace period before forcibly terminating |process|.
+// TODO(https://crbug.com/806451): The Mac implementation currently blocks the
+// calling thread for up to two seconds.
+BASE_EXPORT void EnsureProcessTerminated(Process process);
+
 // These are only sparingly used, and not needed on Fuchsia. They could be
 // implemented if necessary.
 #if !defined(OS_FUCHSIA)
@@ -136,28 +150,6 @@ BASE_EXPORT bool CleanupProcesses(const FilePath::StringType& executable_name,
                                   const ProcessFilter* filter);
 #endif  // !defined(OS_FUCHSIA)
 
-// This method ensures that the specified process eventually terminates, and
-// then it closes the given process handle.
-//
-// It assumes that the process has already been signalled to exit, and it
-// begins by waiting a small amount of time for it to exit.  If the process
-// does not appear to have exited, then this function starts to become
-// aggressive about ensuring that the process terminates.
-//
-// On Linux this method does not block the calling thread.
-// On OS X and Fuchsia, this method may block for up to 2 seconds.
-//
-// NOTE: The process must have been opened with the PROCESS_TERMINATE and
-// SYNCHRONIZE permissions.
-//
-BASE_EXPORT void EnsureProcessTerminated(Process process);
-
-#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_FUCHSIA)
-// The nicer version of EnsureProcessTerminated() that is patient and will
-// wait for |pid| to finish and then reap it.
-BASE_EXPORT void EnsureProcessGetsReaped(ProcessId pid);
-#endif
-
 }  // namespace base
 
 #endif  // BASE_PROCESS_KILL_H_

base/process/kill_fuchsia.cc

@@ -50,20 +50,4 @@ TerminationStatus GetTerminationStatus(ProcessHandle handle, int* exit_code) {
              : TERMINATION_STATUS_ABNORMAL_TERMINATION;
 }
 
-void EnsureProcessTerminated(Process process) {
-  DCHECK(!process.is_current());
-
-  // Wait for up to two seconds for the process to terminate, and then kill it
-  // forcefully if it hasn't already exited.
-  zx_signals_t signals;
-  if (zx_object_wait_one(process.Handle(), ZX_TASK_TERMINATED,
-                         zx_deadline_after(ZX_SEC(2)), &signals) == ZX_OK) {
-    DCHECK(signals & ZX_TASK_TERMINATED);
-    // If already signaled, then the process is terminated.
-    return;
-  }
-
-  process.Terminate(/*exit_code=*/1, /*wait=*/true);
-}
-
 }  // namespace base

base/process/kill_posix.cc

@@ -12,12 +12,11 @@
 
 #include "base/debug/activity_tracker.h"
 #include "base/files/file_util.h"
-#include "base/files/scoped_file.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/process/process_iterator.h"
-#include "base/synchronization/waitable_event.h"
+#include "base/task_scheduler/post_task.h"
 #include "base/threading/platform_thread.h"
 #include "build/build_config.h"
 
@@ -136,81 +135,50 @@ bool CleanupProcesses(const FilePath::StringType& executable_name,
 
 namespace {
 
-// A thread class which waits for the given child to exit and reaps it.
-// If the child doesn't exit within a couple of seconds, kill it.
 class BackgroundReaper : public PlatformThread::Delegate {
  public:
-  BackgroundReaper(pid_t child, unsigned timeout)
-      : child_(child),
-        timeout_(timeout) {
-  }
+  BackgroundReaper(base::Process child_process, const TimeDelta& wait_time)
+      : child_process_(std::move(child_process)), wait_time_(wait_time) {}
 
-  // Overridden from PlatformThread::Delegate:
   void ThreadMain() override {
-    WaitForChildToDie();
-    delete this;
-  }
-
-  void WaitForChildToDie() {
-    // Wait forever case.
-    if (timeout_ == 0) {
-      pid_t r = HANDLE_EINTR(waitpid(child_, nullptr, 0));
-      if (r != child_) {
-        DPLOG(ERROR) << "While waiting for " << child_
-                     << " to terminate, we got the following result: " << r;
-      }
-      return;
-    }
-
-    // There's no good way to wait for a specific child to exit in a timed
-    // fashion. (No kqueue on Linux), so we just loop and sleep.
-
-    // Wait for 2 * timeout_ 500 milliseconds intervals.
-    for (unsigned i = 0; i < 2 * timeout_; ++i) {
-      PlatformThread::Sleep(TimeDelta::FromMilliseconds(500));
-      if (Process(child_).WaitForExitWithTimeout(TimeDelta(), nullptr))
-        return;
-    }
-
-    if (kill(child_, SIGKILL) == 0) {
-      // SIGKILL is uncatchable. Since the signal was delivered, we can
-      // just wait for the process to die now in a blocking manner.
-      Process(child_).WaitForExit(nullptr);
-    } else {
-      DLOG(ERROR) << "While waiting for " << child_ << " to terminate we"
-                  << " failed to deliver a SIGKILL signal (" << errno << ").";
+    if (!wait_time_.is_zero()) {
+      child_process_.WaitForExitWithTimeout(wait_time_, nullptr);
+      kill(child_process_.Handle(), SIGKILL);
     }
+    child_process_.WaitForExit(nullptr);
+    delete this;
   }
 
  private:
-  const pid_t child_;
-  // Number of seconds to wait, if 0 then wait forever and do not attempt to
-  // kill |child_|.
-  const unsigned timeout_;
-
+  Process child_process_;
+  const TimeDelta wait_time_;
   DISALLOW_COPY_AND_ASSIGN(BackgroundReaper);
 };
 
 }  // namespace
 
 void EnsureProcessTerminated(Process process) {
-  // If the child is already dead, then there's nothing to do.
+  DCHECK(!process.is_current());
+
   if (process.WaitForExitWithTimeout(TimeDelta(), nullptr))
     return;
 
-  const unsigned timeout = 2;  // seconds
-  BackgroundReaper* reaper = new BackgroundReaper(process.Pid(), timeout);
-  PlatformThread::CreateNonJoinable(0, reaper);
+  PlatformThread::CreateNonJoinable(
+      0, new BackgroundReaper(std::move(process), TimeDelta::FromSeconds(2)));
 }
 
-void EnsureProcessGetsReaped(ProcessId pid) {
+#if defined(OS_LINUX)
+void EnsureProcessGetsReaped(Process process) {
+  DCHECK(!process.is_current());
+
   // If the child is already dead, then there's nothing to do.
-  if (Process(pid).WaitForExitWithTimeout(TimeDelta(), nullptr))
+  if (process.WaitForExitWithTimeout(TimeDelta(), nullptr))
     return;
 
-  BackgroundReaper* reaper = new BackgroundReaper(pid, 0);
-  PlatformThread::CreateNonJoinable(0, reaper);
+  PlatformThread::CreateNonJoinable(
+      0, new BackgroundReaper(std::move(process), TimeDelta()));
 }
+#endif  // defined(OS_LINUX)
 
 #endif  // !defined(OS_MACOSX)
 #endif  // !defined(OS_NACL_NONSFI)

base/process/kill_win.cc

@@ -4,40 +4,19 @@
 
 #include "base/process/kill.h"
 
+#include <algorithm>
+
 #include <windows.h>
 #include <io.h>
 #include <stdint.h>
 
-#include <algorithm>
-#include <utility>
-
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/debug/activity_tracker.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/process/memory.h"
 #include "base/process/process_iterator.h"
-#include "base/task_scheduler/post_task.h"
 
 namespace base {
 
-namespace {
-
-bool CheckForProcessExitAndReport(const Process& process) {
-  if (WaitForSingleObject(process.Handle(), 0) == WAIT_OBJECT_0) {
-    int exit_code;
-    TerminationStatus status =
-        GetTerminationStatus(process.Handle(), &exit_code);
-    DCHECK_NE(TERMINATION_STATUS_STILL_RUNNING, status);
-    process.Exited(exit_code);
-    return true;
-  }
-  return false;
-}
-
-}  // namespace
-
 TerminationStatus GetTerminationStatus(ProcessHandle handle, int* exit_code) {
   DCHECK(exit_code);
 
@@ -134,24 +113,4 @@ bool CleanupProcesses(const FilePath::StringType& executable_name,
   return false;
 }
 
-void EnsureProcessTerminated(Process process) {
-  DCHECK(!process.is_current());
-
-  // If already signaled, then we are done!
-  if (CheckForProcessExitAndReport(process))
-    return;
-
-  PostDelayedTaskWithTraits(
-      FROM_HERE,
-      {TaskPriority::BACKGROUND, TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
-      BindOnce(
-          [](Process process) {
-            if (CheckForProcessExitAndReport(process))
-              return;
-            process.Terminate(win::kProcessKilledExitCode, false);
-          },
-          std::move(process)),
-      TimeDelta::FromSeconds(2));
-}
-
 }  // namespace base

base/process/process_posix.cc

@@ -201,8 +201,11 @@ bool WaitForExitWithTimeoutImpl(base::ProcessHandle handle,
   }
 
   int status;
-  if (!WaitpidWithTimeout(handle, &status, timeout))
-    return exited;
+  if (!WaitpidWithTimeout(handle, &status, timeout)) {
+    // If multiple threads wait on the same |handle| then one wait will succeed
+    // and the other will fail with errno set to ECHILD.
+    return exited || (errno == ECHILD);
+  }
   if (WIFSIGNALED(status)) {
     if (exit_code)
       *exit_code = -1;

base/process/process_util_unittest.cc

@@ -30,6 +30,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/test/multiprocess_test.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread.h"
@@ -489,6 +490,47 @@ TEST_F(ProcessUtilTest, GetTerminationStatusSigTerm) {
 }
 #endif  // defined(OS_POSIX)
 
+TEST_F(ProcessUtilTest, EnsureTerminationUndying) {
+  test::ScopedTaskEnvironment task_environment;
+
+  Process child_process = SpawnChild("process_util_test_never_die");
+  ASSERT_TRUE(child_process.IsValid());
+
+  EnsureProcessTerminated(child_process.Duplicate());
+
+  // Allow a generous timeout, to cope with slow/loaded test bots.
+  EXPECT_TRUE(child_process.WaitForExitWithTimeout(
+      TestTimeouts::action_max_timeout(), nullptr));
+}
+
+MULTIPROCESS_TEST_MAIN(process_util_test_never_die) {
+  while (1) {
+    PlatformThread::Sleep(TimeDelta::FromSeconds(500));
+  }
+  return kSuccess;
+}
+
+TEST_F(ProcessUtilTest, EnsureTerminationGracefulExit) {
+  test::ScopedTaskEnvironment task_environment;
+
+  Process child_process = SpawnChild("process_util_test_die_immediately");
+  ASSERT_TRUE(child_process.IsValid());
+
+  // Wait for the child process to actually exit.
+  child_process.Duplicate().WaitForExitWithTimeout(
+      TestTimeouts::action_max_timeout(), nullptr);
+
+  EnsureProcessTerminated(child_process.Duplicate());
+
+  // Verify that the process is really, truly gone.
+  EXPECT_TRUE(child_process.WaitForExitWithTimeout(
+      TestTimeouts::action_max_timeout(), nullptr));
+}
+
+MULTIPROCESS_TEST_MAIN(process_util_test_die_immediately) {
+  return kSuccess;
+}
+
 #if defined(OS_WIN)
 // TODO(estade): if possible, port this test.
 TEST_F(ProcessUtilTest, GetAppOutput) {
@@ -1054,60 +1096,6 @@ TEST_F(ProcessUtilTest, GetParentProcessId) {
 }
 #endif  // !defined(OS_FUCHSIA)
 
-// TODO(port): port those unit tests.
-bool IsProcessDead(ProcessHandle child) {
-#if defined(OS_FUCHSIA)
-  // ProcessHandle is an zx_handle_t, not a pid on Fuchsia, so waitpid() doesn't
-  // make sense.
-  zx_signals_t signals;
-  // Timeout of 0 to check for termination, but non-blocking.
-  if (zx_object_wait_one(child, ZX_TASK_TERMINATED, 0, &signals) == ZX_OK) {
-    DCHECK(signals & ZX_TASK_TERMINATED);
-    return true;
-  }
-  return false;
-#else
-  // waitpid() will actually reap the process which is exactly NOT what we
-  // want to test for.  The good thing is that if it can't find the process
-  // we'll get a nice value for errno which we can test for.
-  const pid_t result = HANDLE_EINTR(waitpid(child, nullptr, WNOHANG));
-  return result == -1 && errno == ECHILD;
-#endif
-}
-
-TEST_F(ProcessUtilTest, DelayedTermination) {
-  Process child_process = SpawnChild("process_util_test_never_die");
-  ASSERT_TRUE(child_process.IsValid());
-  EnsureProcessTerminated(child_process.Duplicate());
-  int exit_code;
-  child_process.WaitForExitWithTimeout(TimeDelta::FromSeconds(5), &exit_code);
-
-  // Check that process was really killed.
-  EXPECT_TRUE(IsProcessDead(child_process.Handle()));
-}
-
-MULTIPROCESS_TEST_MAIN(process_util_test_never_die) {
-  while (1) {
-    sleep(500);
-  }
-  return kSuccess;
-}
-
-TEST_F(ProcessUtilTest, ImmediateTermination) {
-  Process child_process = SpawnChild("process_util_test_die_immediately");
-  ASSERT_TRUE(child_process.IsValid());
-  // Give it time to die.
-  sleep(2);
-  EnsureProcessTerminated(child_process.Duplicate());
-
-  // Check that process was really killed.
-  EXPECT_TRUE(IsProcessDead(child_process.Handle()));
-}
-
-MULTIPROCESS_TEST_MAIN(process_util_test_die_immediately) {
-  return kSuccess;
-}
-
 #if !defined(OS_ANDROID) && !defined(OS_FUCHSIA)
 class WriteToPipeDelegate : public LaunchOptions::PreExecDelegate {
  public:

chrome/browser/extensions/api/messaging/native_message_process_host.cc

@@ -66,10 +66,10 @@ NativeMessageProcessHost::~NativeMessageProcessHost() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (process_.IsValid()) {
-    // Kill the host process if necessary to make sure we don't leave zombies.
-    // On OSX and Fuchsia base::EnsureProcessTerminated() may block, so we have
-    // to post a task on the blocking pool.
-#if defined(OS_MACOSX) || defined(OS_FUCHSIA)
+// Kill the host process if necessary to make sure we don't leave zombies.
+// TODO(https://crbug.com/806451): On OSX EnsureProcessTerminated() may
+// block, so we have to post a task on the blocking pool.
+#if defined(OS_MACOSX)
     base::PostTaskWithTraits(
         FROM_HERE, {base::MayBlock(), base::TaskPriority::BACKGROUND},
         base::BindOnce(&base::EnsureProcessTerminated, Passed(&process_)));

chrome/browser/platform_util_linux.cc

@@ -54,7 +54,7 @@ void RunCommand(const std::string& command,
 
   base::Process process = base::LaunchProcess(argv, options);
   if (process.IsValid())
-    base::EnsureProcessGetsReaped(process.Pid());
+    base::EnsureProcessGetsReaped(std::move(process));
 }
 
 void XDGOpen(const base::FilePath& working_directory, const std::string& path) {

chrome/browser/printing/printer_manager_dialog_linux.cc

@@ -46,7 +46,7 @@ bool OpenPrinterConfigDialog(const char* const* command) {
   base::Process process = base::LaunchProcess(argv, base::LaunchOptions());
   if (!process.IsValid())
     return false;
-  base::EnsureProcessGetsReaped(process.Pid());
+  base::EnsureProcessGetsReaped(std::move(process));
   return true;
 }
 

chrome/browser/ui/webui/settings_utils_linux.cc

@@ -81,7 +81,7 @@ bool StartProxyConfigUtil(const char* const command[]) {
     LOG(ERROR) << "StartProxyConfigUtil failed to start " << command[0];
     return false;
   }
-  base::EnsureProcessGetsReaped(process.Pid());
+  base::EnsureProcessGetsReaped(std::move(process));
   return true;
 }
 

content/browser/child_process_launcher_helper_linux.cc

@@ -141,6 +141,7 @@ bool ChildProcessLauncherHelper::TerminateProcess(const base::Process& process,
 // static
 void ChildProcessLauncherHelper::ForceNormalProcessTerminationSync(
     ChildProcessLauncherHelper::Process process) {
+  DCHECK(CurrentlyOnProcessLauncherTaskRunner());
   process.process.Terminate(RESULT_CODE_NORMAL_EXIT, false);
   // On POSIX, we must additionally reap the child.
   if (process.zygote) {

content/browser/zygote_host/zygote_host_impl_linux.cc

@@ -211,7 +211,7 @@ pid_t ZygoteHostImpl::LaunchZygote(
 
     if (real_pid != pid) {
       // Reap the sandbox.
-      base::EnsureProcessGetsReaped(pid);
+      base::EnsureProcessGetsReaped(std::move(process));
     }
     pid = real_pid;
   }
@@ -290,7 +290,7 @@ void ZygoteHostImpl::AdjustRendererOOMScore(base::ProcessHandle pid,
   base::Process sandbox_helper_process =
       base::LaunchProcess(adj_oom_score_cmdline, options);
   if (sandbox_helper_process.IsValid())
-    base::EnsureProcessGetsReaped(sandbox_helper_process.Pid());
+    base::EnsureProcessGetsReaped(std::move(sandbox_helper_process));
 }
 #endif