Fix the race causing flaky CrashpadClient tests.

Bug: crashpad:81
Change-Id: I3cb115440638df909d1c0cdfd01c824ac0d0b073
Reviewed-on: https://chromium-review.googlesource.com/458592
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>

client/client_test.gyp

@@ -22,6 +22,7 @@
       'type': 'executable',
       'dependencies': [
         'client.gyp:crashpad_client',
+        '../compat/compat.gyp:crashpad_compat',
         '../handler/handler.gyp:crashpad_handler',
         '../test/test.gyp:crashpad_gmock_main',
         '../test/test.gyp:crashpad_test',

client/crashpad_client_win_test.cc

@@ -14,25 +14,187 @@
 
 #include "client/crashpad_client.h"
 
+#include <tlhelp32.h>
+
+#include <vector>
+
 #include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/logging.h"
 #include "gtest/gtest.h"
+#include "test/errors.h"
 #include "test/paths.h"
 #include "test/scoped_temp_dir.h"
+#include "util/win/process_info.h"
 #include "test/win/win_multiprocess.h"
+#include "util/win/scoped_handle.h"
 #include "util/win/termination_codes.h"
 
 namespace crashpad {
 namespace test {
 namespace {
 
-void StartAndUseHandler() {
-  ScopedTempDir temp_dir;
+class ScopedEnvironmentVariable {
+ public:
+  explicit ScopedEnvironmentVariable(const wchar_t* name);
+  ~ScopedEnvironmentVariable();
+
+  std::wstring GetValue() const;
+
+  // Sets this environment variable to |new_value|. If |new_value| is nullptr
+  // this environment variable will be undefined.
+  void SetValue(const wchar_t* new_value) const;
+
+ private:
+  std::wstring GetValueImpl(bool* is_defined) const;
+
+  std::wstring original_value_;
+  const wchar_t* name_;
+  bool was_defined_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedEnvironmentVariable);
+};
+
+ScopedEnvironmentVariable::ScopedEnvironmentVariable(const wchar_t* name)
+    : name_(name) {
+  original_value_ = GetValueImpl(&was_defined_);
+}
+
+ScopedEnvironmentVariable::~ScopedEnvironmentVariable() {
+  if (was_defined_)
+    SetValue(original_value_.data());
+  else
+    SetValue(nullptr);
+}
+
+std::wstring ScopedEnvironmentVariable::GetValue() const {
+  bool dummy;
+  return GetValueImpl(&dummy);
+}
+
+std::wstring ScopedEnvironmentVariable::GetValueImpl(bool* is_defined) const {
+  // The length returned is inclusive of the terminating zero, except
+  // if the variable doesn't exist, in which case the return value is zero.
+  DWORD len = GetEnvironmentVariable(name_, nullptr, 0);
+  if (len == 0) {
+    *is_defined = false;
+    return L"";
+  }
+
+  *is_defined = true;
+
+  std::wstring ret;
+  ret.resize(len);
+  // The length returned on success is exclusive of the terminating zero.
+  len = GetEnvironmentVariable(name_, &ret[0], len);
+  ret.resize(len);
+
+  return ret;
+}
+
+void ScopedEnvironmentVariable::SetValue(const wchar_t* new_value) const {
+  SetEnvironmentVariable(name_, new_value);
+}
+
+// Returns the process IDs of all processes that have |parent_pid| as
+// parent process ID.
+std::vector<pid_t> GetPotentialChildProcessesOf(pid_t parent_pid) {
+  ScopedFileHANDLE snapshot(CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0));
+  if (!snapshot.is_valid()) {
+    ADD_FAILURE() << ErrorMessage("CreateToolhelp32Snapshot");
+    return std::vector<pid_t>();
+  }
+
+  PROCESSENTRY32 entry = {sizeof(entry)};
+  if (!Process32First(snapshot.get(), &entry)) {
+    ADD_FAILURE() << ErrorMessage("Process32First");
+    return std::vector<pid_t>();
+  }
+
+  std::vector<pid_t> child_pids;
+  do {
+    if (entry.th32ParentProcessID == parent_pid)
+      child_pids.push_back(entry.th32ProcessID);
+  } while (Process32Next(snapshot.get(), &entry));
+
+  return child_pids;
+}
+
+ULARGE_INTEGER GetProcessCreationTime(HANDLE process) {
+  ULARGE_INTEGER ret = {};
+  FILETIME creation_time;
+  FILETIME dummy;
+  if (GetProcessTimes(process, &creation_time, &dummy, &dummy, &dummy)) {
+    ret.LowPart = creation_time.dwLowDateTime;
+    ret.HighPart = creation_time.dwHighDateTime;
+  } else {
+    ADD_FAILURE() << ErrorMessage("GetProcessTimes");
+  }
+
+  return ret;
+}
+
+// Waits for the processes directly created by |parent| - and specifically not
+// their offspring. For this to work without race, |parent| has to be suspended
+// or have exited.
+void WaitForAllChildProcessesOf(HANDLE parent) {
+  pid_t parent_pid = GetProcessId(parent);
+  std::vector<pid_t> child_pids = GetPotentialChildProcessesOf(parent_pid);
+
+  ULARGE_INTEGER parent_creationtime = GetProcessCreationTime(parent);
+  for (pid_t child_pid : child_pids) {
+    // Try and open the process. This may fail for reasons such as:
+    // 1. The process isn't |parent|'s child process, but rather a
+    //    higher-privilege sub-process of an earlier process that had
+    //    |parent|'s PID.
+    // 2. The process no longer exists, e.g. it exited after enumeration.
+    ScopedKernelHANDLE child_process(
+        OpenProcess(PROCESS_VM_READ | PROCESS_QUERY_INFORMATION | SYNCHRONIZE,
+                    false,
+                    child_pid));
+    if (!child_process.is_valid())
+      continue;
+
+    // Check that the child now has the right parent PID, as its PID may have
+    // been reused after the enumeration above.
+    ProcessInfo child_info;
+    if (!child_info.Initialize(child_process.get())) {
+      // This can happen if child_process has exited after the handle is opened.
+      LOG(ERROR) << "ProcessInfo::Initialize, pid: " << child_pid;
+      continue;
+    }
+
+    if (parent_pid != child_info.ParentProcessID()) {
+      // The child's process ID was reused after enumeration.
+      continue;
+    }
+
+    // We successfully opened |child_process| and it has |parent|'s PID for
+    // parent process ID. However, this could still be a sub-process of another
+    // process that earlier had |parent|'s PID. To make sure, check that
+    // |child_process| was created after |parent_process|.
+    ULARGE_INTEGER process_creationtime =
+        GetProcessCreationTime(child_process.get());
+    if (process_creationtime.QuadPart < parent_creationtime.QuadPart)
+      continue;
+
+    DWORD err = WaitForSingleObject(child_process.get(), INFINITE);
+    if (err == WAIT_FAILED) {
+      ADD_FAILURE() << ErrorMessage("WaitForSingleObject");
+    } else if (err != WAIT_OBJECT_0) {
+      ADD_FAILURE() << "WaitForSingleObject returned " << err;
+    }
+  }
+}
+
+void StartAndUseHandler(const base::FilePath& temp_dir) {
   base::FilePath handler_path = Paths::Executable().DirName().Append(
       FILE_PATH_LITERAL("crashpad_handler.com"));
+
   CrashpadClient client;
   ASSERT_TRUE(client.StartHandler(handler_path,
-                                  temp_dir.path(),
+                                  temp_dir,
                                   base::FilePath(),
                                   "",
                                   std::map<std::string, std::string>(),
@@ -42,9 +204,35 @@ void StartAndUseHandler() {
   ASSERT_TRUE(client.WaitForHandlerStart(INFINITE));
 }
 
-class StartWithInvalidHandles final : public WinMultiprocess {
+// Name of the environment variable used to communicate the name of the
+// temp directory from parent to child process.
+constexpr wchar_t kTempDirEnvName[] = L"CRASHPAD_TEST_TEMP_DIR";
+
+class WinMultiprocessWithTempDir : public WinMultiprocess {
  public:
-  StartWithInvalidHandles() : WinMultiprocess() {}
+  WinMultiprocessWithTempDir()
+      : WinMultiprocess(), temp_dir_env_(kTempDirEnvName) {}
+
+  void WinMultiprocessParentBeforeChild() override {
+    temp_dir_ = base::WrapUnique(new ScopedTempDir);
+    temp_dir_env_.SetValue(temp_dir_->path().value().c_str());
+  }
+
+  void WinMultiprocessParentAfterChild(HANDLE child) override {
+    WaitForAllChildProcessesOf(child);
+    temp_dir_.reset();
+  }
+
+ protected:
+  std::unique_ptr<ScopedTempDir> temp_dir_;
+  ScopedEnvironmentVariable temp_dir_env_;
+
+  DISALLOW_COPY_AND_ASSIGN(WinMultiprocessWithTempDir);
+};
+
+class StartWithInvalidHandles final : public WinMultiprocessWithTempDir {
+ public:
+  StartWithInvalidHandles() : WinMultiprocessWithTempDir() {}
   ~StartWithInvalidHandles() {}
 
  private:
@@ -56,7 +244,7 @@ class StartWithInvalidHandles final : public WinMultiprocess {
     SetStdHandle(STD_OUTPUT_HANDLE, INVALID_HANDLE_VALUE);
     SetStdHandle(STD_ERROR_HANDLE, INVALID_HANDLE_VALUE);
 
-    StartAndUseHandler();
+    StartAndUseHandler(base::FilePath(temp_dir_env_.GetValue()));
 
     SetStdHandle(STD_OUTPUT_HANDLE, original_stdout);
     SetStdHandle(STD_ERROR_HANDLE, original_stderr);
@@ -67,9 +255,9 @@ TEST(CrashpadClient, StartWithInvalidHandles) {
   WinMultiprocess::Run<StartWithInvalidHandles>();
 }
 
-class StartWithSameStdoutStderr final : public WinMultiprocess {
+class StartWithSameStdoutStderr final : public WinMultiprocessWithTempDir {
  public:
-  StartWithSameStdoutStderr() : WinMultiprocess() {}
+  StartWithSameStdoutStderr() : WinMultiprocessWithTempDir() {}
   ~StartWithSameStdoutStderr() {}
 
  private:
@@ -80,7 +268,7 @@ class StartWithSameStdoutStderr final : public WinMultiprocess {
     HANDLE original_stderr = GetStdHandle(STD_ERROR_HANDLE);
     SetStdHandle(STD_OUTPUT_HANDLE, original_stderr);
 
-    StartAndUseHandler();
+    StartAndUseHandler(base::FilePath(temp_dir_env_.GetValue()));
 
     SetStdHandle(STD_OUTPUT_HANDLE, original_stdout);
   }

test/win/win_multiprocess.h

@@ -43,12 +43,16 @@ class WinMultiprocess {
     ASSERT_NO_FATAL_FAILURE(
         WinChildProcess::EntryPoint<ChildProcessHelper<T>>());
     // If WinChildProcess::EntryPoint returns, we are in the parent process.
+    T parent_process;
+    WinMultiprocess* parent_multiprocess = &parent_process;
+
+    parent_multiprocess->WinMultiprocessParentBeforeChild();
+
     std::unique_ptr<WinChildProcess::Handles> child_handles =
         WinChildProcess::Launch();
     ASSERT_TRUE(child_handles.get());
-    T parent_process;
     parent_process.child_handles_ = child_handles.get();
-    static_cast<WinMultiprocess*>(&parent_process)->WinMultiprocessParent();
+    parent_multiprocess->WinMultiprocessParent();
 
     // Close our side of the handles now that we're done. The child can
     // use this to know when it's safe to complete.
@@ -62,6 +66,9 @@ class WinMultiprocess {
     DWORD exit_code;
     ASSERT_TRUE(GetExitCodeProcess(child_handles->process.get(), &exit_code));
     ASSERT_EQ(parent_process.exit_code_, exit_code);
+
+    parent_multiprocess->WinMultiprocessParentAfterChild(
+        child_handles->process.get());
   }
 
  protected:
@@ -162,6 +169,20 @@ class WinMultiprocess {
   //! Subclasses must implement this method to define how the parent operates.
   virtual void WinMultiprocessParent() = 0;
 
+  //! \brief The optional routine run in parent before the child is spawned.
+  //!
+  //! Subclasses may implement this method to prepare the environment for
+  //! the child process.
+  virtual void WinMultiprocessParentBeforeChild() {}
+
+  //! \brief The optional routine run in parent after the child exits.
+  //!
+  //! Subclasses may implement this method to clean up the environment after
+  //! the child process has exited.
+  //!
+  //! \param[in] child A handle to the exited child process.
+  virtual void WinMultiprocessParentAfterChild(HANDLE child) {}
+
   //! \brief The subclass-provided child routine.
   //!
   //! Test failures should be reported via gtest: `EXPECT_*()`, `ASSERT_*()`,