win: Be more careful about child process exit codes

Checking child process’ exit codes would have helped catch bug
crashpad:160 sooner. Instead, we had a flaky hang that was difficult to
reproduce locally.

Bug: crashpad:160
Test: crashpad_snapshot_test ExceptionSnapshotWinTest.ChildCrash*:ProcessSnapshotTest.CrashpadInfoChild*:SimulateCrash.ChildDumpWithoutCrashing*, crashpad_util_test ProcessInfo.OtherProcess
Change-Id: I73bd2be1437d05f0501a146dcb9efbe3b8e0f8b7
Reviewed-on: https://chromium-review.googlesource.com/459039
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>

snapshot/win/crashpad_snapshot_test_image_reader.cc

@@ -19,6 +19,8 @@
 #include "util/file/file_io.h"
 #include "util/win/scoped_handle.h"
 
+namespace {
+
 DWORD WINAPI LotsOfReferencesThreadProc(void* param) {
   LONG* count = reinterpret_cast<LONG*>(param);
 
@@ -33,10 +35,13 @@ DWORD WINAPI LotsOfReferencesThreadProc(void* param) {
   return 0;
 }
 
+}  // namespace
+
 int wmain(int argc, wchar_t* argv[]) {
   CHECK_EQ(argc, 2);
 
   crashpad::ScopedKernelHANDLE done(CreateEvent(nullptr, true, false, argv[1]));
+  PCHECK(done.is_valid()) << "CreateEvent";
 
   PCHECK(LoadLibrary(L"crashpad_snapshot_test_image_reader_module.dll"))
       << "LoadLibrary";
@@ -80,8 +85,8 @@ int wmain(int argc, wchar_t* argv[]) {
   crashpad::CheckedWriteFile(out, &c, sizeof(c));
 
   // Parent process says we can exit.
-  CHECK_EQ(WAIT_OBJECT_0, WaitForSingleObject(done.get(), INFINITE));
+  PCHECK(WaitForSingleObject(done.get(), INFINITE) == WAIT_OBJECT_0)
+      << "WaitForSingleObject";
 
   return 0;
 }
-

snapshot/win/exception_snapshot_win_test.cc

@@ -22,6 +22,7 @@
 #include "client/crashpad_client.h"
 #include "gtest/gtest.h"
 #include "snapshot/win/process_snapshot_win.h"
+#include "test/errors.h"
 #include "test/paths.h"
 #include "test/win/child_launcher.h"
 #include "util/file/file_io.h"
@@ -122,7 +123,9 @@ class CrashingDelegate : public ExceptionHandlerServer::Delegate {
 void TestCrashingChild(const base::string16& directory_modification) {
   // Set up the registration server on a background thread.
   ScopedKernelHANDLE server_ready(CreateEvent(nullptr, false, false, nullptr));
+  ASSERT_TRUE(server_ready.is_valid()) << ErrorMessage("CreateEvent");
   ScopedKernelHANDLE completed(CreateEvent(nullptr, false, false, nullptr));
+  ASSERT_TRUE(completed.is_valid()) << ErrorMessage("CreateEvent");
   CrashingDelegate delegate(server_ready.get(), completed.get());
 
   ExceptionHandlerServer exception_handler_server(true);
@@ -133,7 +136,8 @@ void TestCrashingChild(const base::string16& directory_modification) {
   ScopedStopServerAndJoinThread scoped_stop_server_and_join_thread(
       &exception_handler_server, &server_thread);
 
-  WaitForSingleObject(server_ready.get(), INFINITE);
+  EXPECT_EQ(WAIT_OBJECT_0, WaitForSingleObject(server_ready.get(), INFINITE))
+      << ErrorMessage("WaitForSingleObject");
 
   // Spawn a child process, passing it the pipe name to connect to.
   base::FilePath test_executable = Paths::Executable();
@@ -154,7 +158,10 @@ void TestCrashingChild(const base::string16& directory_modification) {
   delegate.set_break_near(break_near_address);
 
   // Wait for the child to crash and the exception information to be validated.
-  WaitForSingleObject(completed.get(), INFINITE);
+  EXPECT_EQ(WAIT_OBJECT_0, WaitForSingleObject(completed.get(), INFINITE))
+      << ErrorMessage("WaitForSingleObject");
+
+  EXPECT_EQ(EXCEPTION_BREAKPOINT, child.WaitForExit());
 }
 
 TEST(ExceptionSnapshotWinTest, ChildCrash) {
@@ -224,7 +231,9 @@ void TestDumpWithoutCrashingChild(
     const base::string16& directory_modification) {
   // Set up the registration server on a background thread.
   ScopedKernelHANDLE server_ready(CreateEvent(nullptr, false, false, nullptr));
+  ASSERT_TRUE(server_ready.is_valid()) << ErrorMessage("CreateEvent");
   ScopedKernelHANDLE completed(CreateEvent(nullptr, false, false, nullptr));
+  ASSERT_TRUE(completed.is_valid()) << ErrorMessage("CreateEvent");
   SimulateDelegate delegate(server_ready.get(), completed.get());
 
   ExceptionHandlerServer exception_handler_server(true);
@@ -235,7 +244,8 @@ void TestDumpWithoutCrashingChild(
   ScopedStopServerAndJoinThread scoped_stop_server_and_join_thread(
       &exception_handler_server, &server_thread);
 
-  WaitForSingleObject(server_ready.get(), INFINITE);
+  EXPECT_EQ(WAIT_OBJECT_0, WaitForSingleObject(server_ready.get(), INFINITE))
+      << ErrorMessage("WaitForSingleObject");
 
   // Spawn a child process, passing it the pipe name to connect to.
   base::FilePath test_executable = Paths::Executable();
@@ -256,7 +266,10 @@ void TestDumpWithoutCrashingChild(
   delegate.set_dump_near(dump_near_address);
 
   // Wait for the child to crash and the exception information to be validated.
-  WaitForSingleObject(completed.get(), INFINITE);
+  EXPECT_EQ(WAIT_OBJECT_0, WaitForSingleObject(completed.get(), INFINITE))
+      << ErrorMessage("WaitForSingleObject");
+
+  EXPECT_EQ(0, child.WaitForExit());
 }
 
 TEST(SimulateCrash, ChildDumpWithoutCrashing) {

snapshot/win/process_snapshot_win_test.cc

@@ -20,6 +20,7 @@
 #include "snapshot/win/module_snapshot_win.h"
 #include "snapshot/win/pe_image_reader.h"
 #include "snapshot/win/process_reader_win.h"
+#include "test/errors.h"
 #include "test/paths.h"
 #include "test/win/child_launcher.h"
 #include "util/file/file_io.h"
@@ -35,7 +36,7 @@ void TestImageReaderChild(const base::string16& directory_modification) {
   done_uuid.InitializeWithNew();
   ScopedKernelHANDLE done(
       CreateEvent(nullptr, true, false, done_uuid.ToString16().c_str()));
-  ASSERT_TRUE(done.get());
+  ASSERT_TRUE(done.is_valid()) << ErrorMessage("CreateEvent");
 
   base::FilePath test_executable = Paths::Executable();
   std::wstring child_test_executable =
@@ -52,59 +53,63 @@ void TestImageReaderChild(const base::string16& directory_modification) {
       LoggingReadFileExactly(child.stdout_read_handle(), &c, sizeof(c)));
   ASSERT_EQ(' ', c);
 
-  ScopedProcessSuspend suspend(child.process_handle());
+  {
+    ScopedProcessSuspend suspend(child.process_handle());
 
-  ProcessSnapshotWin process_snapshot;
-  ASSERT_TRUE(process_snapshot.Initialize(
-      child.process_handle(), ProcessSuspensionState::kSuspended, 0, 0));
+    ProcessSnapshotWin process_snapshot;
+    ASSERT_TRUE(process_snapshot.Initialize(
+        child.process_handle(), ProcessSuspensionState::kSuspended, 0, 0));
 
-  ASSERT_GE(process_snapshot.Modules().size(), 2u);
+    ASSERT_GE(process_snapshot.Modules().size(), 2u);
 
-  UUID uuid;
-  DWORD age;
-  std::string pdbname;
-  const std::string suffix(".pdb");
+    UUID uuid;
+    DWORD age;
+    std::string pdbname;
+    const std::string suffix(".pdb");
 
-  // Check the main .exe to see that we can retrieve its sections.
-  auto module = reinterpret_cast<const internal::ModuleSnapshotWin*>(
-      process_snapshot.Modules()[0]);
-  ASSERT_TRUE(module->pe_image_reader().DebugDirectoryInformation(
-      &uuid, &age, &pdbname));
-  EXPECT_NE(std::string::npos,
-            pdbname.find("crashpad_snapshot_test_image_reader"));
-  EXPECT_EQ(
-      0,
-      pdbname.compare(pdbname.size() - suffix.size(), suffix.size(), suffix));
+    // Check the main .exe to see that we can retrieve its sections.
+    auto module = reinterpret_cast<const internal::ModuleSnapshotWin*>(
+        process_snapshot.Modules()[0]);
+    ASSERT_TRUE(module->pe_image_reader().DebugDirectoryInformation(
+        &uuid, &age, &pdbname));
+    EXPECT_NE(std::string::npos,
+              pdbname.find("crashpad_snapshot_test_image_reader"));
+    EXPECT_EQ(
+        0,
+        pdbname.compare(pdbname.size() - suffix.size(), suffix.size(), suffix));
 
-  // Check the dll it loads too.
-  module = reinterpret_cast<const internal::ModuleSnapshotWin*>(
-      process_snapshot.Modules().back());
-  ASSERT_TRUE(module->pe_image_reader().DebugDirectoryInformation(
-      &uuid, &age, &pdbname));
-  EXPECT_NE(std::string::npos,
-            pdbname.find("crashpad_snapshot_test_image_reader_module"));
-  EXPECT_EQ(
-      0,
-      pdbname.compare(pdbname.size() - suffix.size(), suffix.size(), suffix));
+    // Check the dll it loads too.
+    module = reinterpret_cast<const internal::ModuleSnapshotWin*>(
+        process_snapshot.Modules().back());
+    ASSERT_TRUE(module->pe_image_reader().DebugDirectoryInformation(
+        &uuid, &age, &pdbname));
+    EXPECT_NE(std::string::npos,
+              pdbname.find("crashpad_snapshot_test_image_reader_module"));
+    EXPECT_EQ(
+        0,
+        pdbname.compare(pdbname.size() - suffix.size(), suffix.size(), suffix));
 
-  // Sum the size of the extra memory in all the threads and confirm it's near
-  // the limit that the child process set in its CrashpadInfo.
-  EXPECT_GE(process_snapshot.Threads().size(), 100u);
+    // Sum the size of the extra memory in all the threads and confirm it's near
+    // the limit that the child process set in its CrashpadInfo.
+    EXPECT_GE(process_snapshot.Threads().size(), 100u);
 
-  size_t extra_memory_total = 0;
-  for (const auto* thread : process_snapshot.Threads()) {
-    for (const auto* extra_memory : thread->ExtraMemory()) {
-      extra_memory_total += extra_memory->Size();
+    size_t extra_memory_total = 0;
+    for (const auto* thread : process_snapshot.Threads()) {
+      for (const auto* extra_memory : thread->ExtraMemory()) {
+        extra_memory_total += extra_memory->Size();
+      }
     }
+
+    // Confirm that less than 1M of extra data was gathered. The cap is set to
+    // only 100K, but there are other "extra memory" regions that aren't
+    // included in the cap. (Completely uncapped it would be > 10M.)
+    EXPECT_LT(extra_memory_total, 1000000u);
   }
 
-  // We confirm we gathered less than 1M of extra data. The cap is set to only
-  // 100K, but there are other "extra memory" regions that aren't included in
-  // the cap. (Completely uncapped it would be > 10M.)
-  EXPECT_LT(extra_memory_total, 1000000u);
-
   // Tell the child it can terminate.
-  SetEvent(done.get());
+  EXPECT_TRUE(SetEvent(done.get())) << ErrorMessage("SetEvent");
+
+  EXPECT_EQ(0, child.WaitForExit());
 }
 
 TEST(ProcessSnapshotTest, CrashpadInfoChild) {

util/win/process_info_test.cc

@@ -137,7 +137,7 @@ void TestOtherProcess(const base::string16& directory_modification) {
 
   ScopedKernelHANDLE done(
       CreateEvent(nullptr, true, false, done_uuid.ToString16().c_str()));
-  ASSERT_TRUE(done.get());
+  ASSERT_TRUE(done.get()) << ErrorMessage("CreateEvent");
 
   base::FilePath test_executable = Paths::Executable();
 
@@ -162,7 +162,9 @@ void TestOtherProcess(const base::string16& directory_modification) {
   ASSERT_TRUE(process_info.Initialize(child.process_handle()));
 
   // Tell the test it's OK to shut down now that we've read our data.
-  EXPECT_TRUE(SetEvent(done.get()));
+  EXPECT_TRUE(SetEvent(done.get())) << ErrorMessage("SetEvent");
+
+  EXPECT_EQ(0, child.WaitForExit());
 
   std::vector<ProcessInfo::Module> modules;
   EXPECT_TRUE(process_info.Modules(&modules));