Explicitly stop the ExitCodeWatcher if crashpad fails to initialize

Fix for potential deadlock between crashpad and ExitCodeWatcher during
launch - Bug:1057774. In the case where we fail to fully initialize
crashpad, we need to explicitly stop the ExitCodeWatcher.

Bug: 1057774
Change-Id: I31befef2bf25477d38d78c4d136a76d3aaa3be52
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2087900Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Mike Rorke <mrorke@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#747767}

base/process/process.h

@@ -131,6 +131,21 @@ class BASE_EXPORT Process {
   // NOTE: On POSIX |exit_code| is ignored.
   bool Terminate(int exit_code, bool wait) const;
 
+#if defined(OS_WIN)
+  enum class WaitExitStatus {
+    PROCESS_EXITED,
+    STOP_EVENT_SIGNALED,
+    FAILED,
+  };
+
+  // Waits for the process to exit, or the specified |stop_event_handle| to be
+  // set. Returns value indicating which event was set. The given |exit_code|
+  // will be the exit code of the process.
+  WaitExitStatus WaitForExitOrEvent(
+      const base::win::ScopedHandle& stop_event_handle,
+      int* exit_code) const;
+#endif  // OS_WIN
+
   // Waits for the process to exit. Returns true on success.
   // On POSIX, if the process has been signaled then |exit_code| is set to -1.
   // On Linux this must be a child process, however on Mac and Windows it can be

base/process/process_unittest.cc

@@ -267,6 +267,38 @@ TEST_F(ProcessTest, WaitForExitWithTimeout) {
   process.Terminate(kDummyExitCode, false);
 }
 
+#if defined(OS_WIN)
+TEST_F(ProcessTest, WaitForExitOrEventWithProcessExit) {
+  Process process(SpawnChild("FastSleepyChildProcess"));
+  ASSERT_TRUE(process.IsValid());
+
+  base::win::ScopedHandle stop_watching_handle(
+      CreateEvent(nullptr, TRUE, FALSE, nullptr));
+
+  const int kDummyExitCode = 42;
+  int exit_code = kDummyExitCode;
+  EXPECT_EQ(process.WaitForExitOrEvent(stop_watching_handle, &exit_code),
+            base::Process::WaitExitStatus::PROCESS_EXITED);
+  EXPECT_EQ(0, exit_code);
+}
+
+TEST_F(ProcessTest, WaitForExitOrEventWithEventSet) {
+  Process process(SpawnChild("SleepyChildProcess"));
+  ASSERT_TRUE(process.IsValid());
+
+  base::win::ScopedHandle stop_watching_handle(
+      CreateEvent(nullptr, TRUE, TRUE, nullptr));
+
+  const int kDummyExitCode = 42;
+  int exit_code = kDummyExitCode;
+  EXPECT_EQ(process.WaitForExitOrEvent(stop_watching_handle, &exit_code),
+            base::Process::WaitExitStatus::STOP_EVENT_SIGNALED);
+  EXPECT_EQ(kDummyExitCode, exit_code);
+
+  process.Terminate(kDummyExitCode, false);
+}
+#endif  // OS_WIN
+
 // 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

base/process/process_win.cc

@@ -183,6 +183,35 @@ bool Process::Terminate(int exit_code, bool wait) const {
   return result;
 }
 
+Process::WaitExitStatus Process::WaitForExitOrEvent(
+    const base::win::ScopedHandle& stop_event_handle,
+    int* exit_code) const {
+  // Record the event that this thread is blocking upon (for hang diagnosis).
+  base::debug::ScopedProcessWaitActivity process_activity(this);
+
+  HANDLE events[] = {Handle(), stop_event_handle.Get()};
+  DWORD wait_result =
+      ::WaitForMultipleObjects(base::size(events), events, FALSE, INFINITE);
+
+  if (wait_result == WAIT_OBJECT_0) {
+    DWORD temp_code;  // Don't clobber out-parameters in case of failure.
+    if (!::GetExitCodeProcess(Handle(), &temp_code))
+      return Process::WaitExitStatus::FAILED;
+
+    if (exit_code)
+      *exit_code = temp_code;
+
+    Exited(temp_code);
+    return Process::WaitExitStatus::PROCESS_EXITED;
+  }
+
+  if (wait_result == WAIT_OBJECT_0 + 1) {
+    return Process::WaitExitStatus::STOP_EVENT_SIGNALED;
+  }
+
+  return Process::WaitExitStatus::FAILED;
+}
+
 bool Process::WaitForExit(int* exit_code) const {
   return WaitForExitWithTimeout(TimeDelta::FromMilliseconds(INFINITE),
                                 exit_code);

chrome/app/chrome_exe_main_win.cc

@@ -235,9 +235,15 @@ int main() {
 
     base::FilePath user_data_dir =
         command_line->GetSwitchValuePath(switches::kUserDataDir);
-    return crash_reporter::RunAsCrashpadHandler(
+    int crashpad_status = crash_reporter::RunAsCrashpadHandler(
         *base::CommandLine::ForCurrentProcess(), user_data_dir,
         switches::kProcessType, switches::kUserDataDir);
+    if (crashpad_status != 0 && exit_code_watcher) {
+      // Crashpad failed to initialize, explicitly stop the exit code watcher
+      // so the crashpad-handler process can exit with an error
+      exit_code_watcher->StopWatching();
+    }
+    return crashpad_status;
   } else if (process_type == crash_reporter::switches::kFallbackCrashHandler) {
     return RunFallbackCrashHandler(*command_line);
   }

components/browser_watcher/exit_code_watcher_win.cc

@@ -4,6 +4,8 @@
 
 #include "components/browser_watcher/exit_code_watcher_win.h"
 
+#include <windows.h>
+
 #include <utility>
 
 #include "base/logging.h"
@@ -15,18 +17,19 @@
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 
-#include <windows.h>
-
 namespace browser_watcher {
 
 const char kBrowserExitCodeHistogramName[] = "Stability.BrowserExitCodes";
 
 ExitCodeWatcher::ExitCodeWatcher()
-    : background_thread_("ExitCodeWatcherThread"), exit_code_(STILL_ACTIVE) {}
-
-ExitCodeWatcher::~ExitCodeWatcher() {
+    : background_thread_("ExitCodeWatcherThread"),
+      exit_code_(STILL_ACTIVE),
+      stop_watching_handle_(CreateEvent(nullptr, TRUE, FALSE, nullptr)) {
+  DCHECK(stop_watching_handle_.IsValid());
 }
 
+ExitCodeWatcher::~ExitCodeWatcher() {}
+
 bool ExitCodeWatcher::Initialize(base::Process process) {
   if (!process.IsValid()) {
     LOG(ERROR) << "Invalid parent handle, can't get parent process ID.";
@@ -69,13 +72,20 @@ bool ExitCodeWatcher::StartWatching() {
   return true;
 }
 
-void ExitCodeWatcher::WaitForExit() {
-  if (!process_.WaitForExit(&exit_code_)) {
-    LOG(ERROR) << "Failed to wait for process.";
-    return;
+void ExitCodeWatcher::StopWatching() {
+  if (stop_watching_handle_.IsValid()) {
+    SetEvent(stop_watching_handle_.Get());
   }
+}
 
-  WriteProcessExitCode(exit_code_);
+void ExitCodeWatcher::WaitForExit() {
+  base::Process::WaitExitStatus wait_result =
+      process_.WaitForExitOrEvent(stop_watching_handle_, &exit_code_);
+  if (wait_result == base::Process::WaitExitStatus::PROCESS_EXITED) {
+    WriteProcessExitCode(exit_code_);
+  } else if (wait_result == base::Process::WaitExitStatus::FAILED) {
+    LOG(ERROR) << "Failed to wait for process exit or stop event";
+  }
 }
 
 bool ExitCodeWatcher::WriteProcessExitCode(int exit_code) {

components/browser_watcher/exit_code_watcher_win.h

@@ -7,6 +7,7 @@
 #include "base/macros.h"
 #include "base/process/process.h"
 #include "base/threading/thread.h"
+#include "base/win/scoped_handle.h"
 
 namespace browser_watcher {
 
@@ -23,6 +24,8 @@ class ExitCodeWatcher {
 
   bool StartWatching();
 
+  void StopWatching();
+
   const base::Process& process() const { return process_; }
   int exit_code() const { return exit_code_; }
 
@@ -43,6 +46,9 @@ class ExitCodeWatcher {
   // The exit code of the watched process. Valid after WaitForExit.
   int exit_code_;
 
+  // Event handle to use to stop exit watcher thread
+  base::win::ScopedHandle stop_watching_handle_;
+
   DISALLOW_COPY_AND_ASSIGN(ExitCodeWatcher);
 };
 

components/browser_watcher/exit_code_watcher_win_unittest.cc

@@ -134,4 +134,25 @@ TEST_F(ExitCodeWatcherTest, ExitCodeWatcherOnExitedProcess) {
   EXPECT_TRUE(watcher.exit_code() == kExitCode);
 }
 
+TEST_F(ExitCodeWatcherTest, ExitCodeWatcherStopWatching) {
+  ScopedSleeperProcess sleeper;
+  ASSERT_NO_FATAL_FAILURE(sleeper.Launch());
+
+  ExitCodeWatcher watcher;
+
+  EXPECT_TRUE(watcher.Initialize(sleeper.process().Duplicate()));
+
+  EXPECT_TRUE(watcher.StartWatching());
+
+  base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());
+  watcher.StopWatching();
+
+  // Verify we got the expected exit code
+  EXPECT_TRUE(watcher.exit_code() == STILL_ACTIVE);
+
+  // Cleanup the sleeper, and make sure it's exited before we continue.
+  ASSERT_NO_FATAL_FAILURE(sleeper.Kill(kExitCode, true));
+  base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());
+}
+
 }  // namespace browser_watcher