win: Fix incorrect thread suspend count due to ScopedProcessSuspend

After https://codereview.chromium.org/1303173011/, the thread suspend
count would be one too large because the count is adjusted when the
process is suspended. Counteract this by passing in whether the
process is suspended or not so that the thread's suspension count
can be adjusted.

Add a test to sanity-check thread suspend count.

R=mark@chromium.org

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

handler/win/crash_report_exception_handler.cc

@@ -48,7 +48,8 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException(
   ScopedProcessSuspend suspend(process);
 
   ProcessSnapshotWin process_snapshot;
-  if (!process_snapshot.Initialize(process)) {
+  if (!process_snapshot.Initialize(process,
+                                   ProcessSuspensionState::kSuspended)) {
     LOG(WARNING) << "ProcessSnapshotWin::Initialize failed";
     return kFailedTerminationCode;
   }

snapshot/crashpad_info_client_options_test.cc

@@ -75,7 +75,8 @@ TEST(CrashpadInfoClientOptions, OneModule) {
   ASSERT_TRUE(process_snapshot.Initialize(mach_task_self()));
 #elif defined(OS_WIN)
   ProcessSnapshotWin process_snapshot;
-  ASSERT_TRUE(process_snapshot.Initialize(GetCurrentProcess()));
+  ASSERT_TRUE(process_snapshot.Initialize(GetCurrentProcess(),
+                                          ProcessSuspensionState::kRunning));
 #else
 #error Port.
 #endif  // OS_MACOSX
@@ -188,7 +189,8 @@ TEST(CrashpadInfoClientOptions, TwoModules) {
   ASSERT_TRUE(process_snapshot.Initialize(mach_task_self()));
 #elif defined(OS_WIN)
   ProcessSnapshotWin process_snapshot;
-  ASSERT_TRUE(process_snapshot.Initialize(GetCurrentProcess()));
+  ASSERT_TRUE(process_snapshot.Initialize(GetCurrentProcess(),
+                                          ProcessSuspensionState::kRunning));
 #else
 #error Port.
 #endif  // OS_MACOSX

snapshot/win/exception_snapshot_win_test.cc

@@ -26,6 +26,7 @@
 #include "util/win/exception_handler_server.h"
 #include "util/win/registration_protocol_win.h"
 #include "util/win/scoped_handle.h"
+#include "util/win/scoped_process_suspend.h"
 
 namespace crashpad {
 namespace test {
@@ -62,8 +63,9 @@ class ExceptionSnapshotWinTest : public testing::Test {
     unsigned int ExceptionHandlerServerException(
         HANDLE process,
         WinVMAddress exception_information_address) override {
+      ScopedProcessSuspend suspend(process);
       ProcessSnapshotWin snapshot;
-      snapshot.Initialize(process);
+      snapshot.Initialize(process, ProcessSuspensionState::kSuspended);
       snapshot.InitializeException(exception_information_address);
 
       // Confirm the exception record was read correctly.

snapshot/win/pe_image_annotations_reader_test.cc

@@ -55,7 +55,8 @@ class TestPEImageAnnotationsReader final : public WinMultiprocess {
 
   void WinMultiprocessParent() override {
     ProcessReaderWin process_reader;
-    ASSERT_TRUE(process_reader.Initialize(ChildProcess()));
+    ASSERT_TRUE(process_reader.Initialize(ChildProcess(),
+                                          ProcessSuspensionState::kRunning));
 
     // Wait for the child process to indicate that it's done setting up its
     // annotations via the CrashpadInfo interface.

snapshot/win/pe_image_reader_test.cc

@@ -28,7 +28,8 @@ namespace {
 TEST(PEImageReader, DebugDirectory) {
   PEImageReader pe_image_reader;
   ProcessReaderWin process_reader;
-  ASSERT_TRUE(process_reader.Initialize(GetCurrentProcess()));
+  ASSERT_TRUE(process_reader.Initialize(GetCurrentProcess(),
+                                        ProcessSuspensionState::kRunning));
   HMODULE self = reinterpret_cast<HMODULE>(&__ImageBase);
   MODULEINFO module_info;
   ASSERT_TRUE(GetModuleInformation(

snapshot/win/process_reader_win.cc

@@ -117,8 +117,8 @@ template <class Traits>
 void FillThreadContextAndSuspendCount(
     const process_types::SYSTEM_EXTENDED_THREAD_INFORMATION<Traits>&
         thread_info,
-    ProcessReaderWin::Thread* thread) {
+    ProcessReaderWin::Thread* thread,
-
+    ProcessSuspensionState suspension_state) {
   // Don't suspend the thread if it's this thread. This is really only for test
   // binaries, as we won't be walking ourselves, in general.
   bool is_current_thread = thread_info.ClientId.UniqueThread ==
@@ -130,6 +130,7 @@ void FillThreadContextAndSuspendCount(
   // TODO(scottmg): Handle cross-bitness in this function.
 
   if (is_current_thread) {
+    DCHECK(suspension_state == ProcessSuspensionState::kRunning);
     thread->suspend_count = 0;
     RtlCaptureContext(&thread->context);
   } else {
@@ -138,7 +139,11 @@ void FillThreadContextAndSuspendCount(
       PLOG(ERROR) << "SuspendThread failed";
       return;
     }
-    thread->suspend_count = previous_suspend_count;
+    DCHECK(previous_suspend_count > 0 ||
+           suspension_state == ProcessSuspensionState::kRunning);
+    thread->suspend_count =
+        previous_suspend_count -
+        (suspension_state == ProcessSuspensionState::kSuspended ? 1 : 0);
 
     memset(&thread->context, 0, sizeof(thread->context));
     thread->context.ContextFlags = CONTEXT_ALL;
@@ -171,6 +176,7 @@ ProcessReaderWin::ProcessReaderWin()
       process_info_(),
       threads_(),
       modules_(),
+      suspension_state_(),
       initialized_threads_(false),
       initialized_() {
 }
@@ -178,10 +184,12 @@ ProcessReaderWin::ProcessReaderWin()
 ProcessReaderWin::~ProcessReaderWin() {
 }
 
-bool ProcessReaderWin::Initialize(HANDLE process) {
+bool ProcessReaderWin::Initialize(HANDLE process,
+                                  ProcessSuspensionState suspension_state) {
   INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
 
   process_ = process;
+  suspension_state_ = suspension_state;
   process_info_.Initialize(process);
 
   INITIALIZATION_STATE_SET_VALID(initialized_);
@@ -254,7 +262,7 @@ const std::vector<ProcessReaderWin::Thread>& ProcessReaderWin::Threads() {
     Thread thread;
     thread.id = thread_info.ClientId.UniqueThread;
 
-    FillThreadContextAndSuspendCount(thread_info, &thread);
+    FillThreadContextAndSuspendCount(thread_info, &thread, suspension_state_);
 
     // TODO(scottmg): I believe we could reverse engineer the PriorityClass from
     // the Priority, BasePriority, and

snapshot/win/process_reader_win.h

@@ -26,6 +26,15 @@
 
 namespace crashpad {
 
+//! \brief State of process being read by ProcessReaderWin.
+enum class ProcessSuspensionState : bool {
+  //! \brief The process has not been suspended.
+  kRunning,
+
+  //! \brief The process is suspended.
+  kSuspended,
+};
+
 //! \brief Accesses information about another process, identified by a `HANDLE`.
 class ProcessReaderWin {
  public:
@@ -52,11 +61,17 @@ class ProcessReaderWin {
   //!
   //! \param[in] process Process handle, must have `PROCESS_QUERY_INFORMATION`,
   //!     `PROCESS_VM_READ`, and `PROCESS_DUP_HANDLE` access.
+  //! \param[in] suspension_state Whether \a process has already been suspended
+  //!     by the caller. Typically, this will be
+  //!     ProcessSuspensionState::kSuspended, except for testing uses and where
+  //!     the reader is reading itself.
   //!
   //! \return `true` on success, indicating that this object will respond
   //!     validly to further method calls. `false` on failure. On failure, no
   //!     further method calls should be made.
-  bool Initialize(HANDLE process);
+  //!
+  //! \sa ScopedProcessSuspend
+  bool Initialize(HANDLE process, ProcessSuspensionState suspension_state);
 
   //! \return `true` if the target task is a 64-bit process.
   bool Is64Bit() const { return process_info_.Is64Bit(); }
@@ -96,6 +111,7 @@ class ProcessReaderWin {
   ProcessInfo process_info_;
   std::vector<Thread> threads_;
   std::vector<ProcessInfo::Module> modules_;
+  ProcessSuspensionState suspension_state_;
   bool initialized_threads_;
   InitializationStateDcheck initialized_;
 

snapshot/win/process_reader_win_test.cc

@@ -19,6 +19,7 @@
 
 #include "gtest/gtest.h"
 #include "test/win/win_multiprocess.h"
+#include "util/win/scoped_process_suspend.h"
 
 namespace crashpad {
 namespace test {
@@ -26,7 +27,8 @@ namespace {
 
 TEST(ProcessReaderWin, SelfBasic) {
   ProcessReaderWin process_reader;
-  ASSERT_TRUE(process_reader.Initialize(GetCurrentProcess()));
+  ASSERT_TRUE(process_reader.Initialize(GetCurrentProcess(),
+                                        ProcessSuspensionState::kRunning));
 
 #if !defined(ARCH_CPU_64_BITS)
   EXPECT_FALSE(process_reader.Is64Bit());
@@ -55,7 +57,8 @@ class ProcessReaderChild final : public WinMultiprocess {
  private:
   void WinMultiprocessParent() override {
     ProcessReaderWin process_reader;
-    ASSERT_TRUE(process_reader.Initialize(ChildProcess()));
+    ASSERT_TRUE(process_reader.Initialize(ChildProcess(),
+                                          ProcessSuspensionState::kRunning));
 
 #if !defined(ARCH_CPU_64_BITS)
     EXPECT_FALSE(process_reader.Is64Bit());
@@ -90,7 +93,8 @@ TEST(ProcessReaderWin, ChildBasic) {
 
 TEST(ProcessReaderWin, SelfOneThread) {
   ProcessReaderWin process_reader;
-  ASSERT_TRUE(process_reader.Initialize(GetCurrentProcess()));
+  ASSERT_TRUE(process_reader.Initialize(GetCurrentProcess(),
+                                        ProcessSuspensionState::kRunning));
 
   const std::vector<ProcessReaderWin::Thread>& threads =
       process_reader.Threads();
@@ -110,6 +114,56 @@ TEST(ProcessReaderWin, SelfOneThread) {
   EXPECT_EQ(0, threads[0].suspend_count);
 }
 
+class ProcessReaderChildThreadSuspendCount final : public WinMultiprocess {
+ public:
+  ProcessReaderChildThreadSuspendCount() : WinMultiprocess() {}
+  ~ProcessReaderChildThreadSuspendCount() {}
+
+ private:
+  void WinMultiprocessParent() override {
+    {
+      ProcessReaderWin process_reader;
+      ASSERT_TRUE(process_reader.Initialize(ChildProcess(),
+                                            ProcessSuspensionState::kRunning));
+
+      const auto& threads = process_reader.Threads();
+      ASSERT_FALSE(threads.empty());
+      for (const auto& thread : threads)
+        EXPECT_EQ(0u, thread.suspend_count);
+    }
+
+    {
+      ScopedProcessSuspend suspend(ChildProcess());
+
+      ProcessReaderWin process_reader;
+      ASSERT_TRUE(process_reader.Initialize(
+          ChildProcess(), ProcessSuspensionState::kSuspended));
+
+      // Confirm that thread counts are adjusted correctly for the process being
+      // suspended.
+      const auto& threads = process_reader.Threads();
+      ASSERT_FALSE(threads.empty());
+      for (const auto& thread : threads)
+        EXPECT_EQ(0u, thread.suspend_count);
+    }
+  }
+
+  void WinMultiprocessChild() override {
+    WinVMAddress address = reinterpret_cast<WinVMAddress>(kTestMemory);
+    CheckedWriteFile(WritePipeHandle(), &address, sizeof(address));
+
+    // Wait for the parent to signal that it's OK to exit by closing its end of
+    // the pipe.
+    CheckedReadFileAtEOF(ReadPipeHandle());
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(ProcessReaderChildThreadSuspendCount);
+};
+
+TEST(ProcessReaderWin, ChildThreadSuspendCounts) {
+  WinMultiprocess::Run<ProcessReaderChildThreadSuspendCount>();
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace crashpad

snapshot/win/process_snapshot_win.cc

@@ -38,12 +38,13 @@ ProcessSnapshotWin::ProcessSnapshotWin()
 ProcessSnapshotWin::~ProcessSnapshotWin() {
 }
 
-bool ProcessSnapshotWin::Initialize(HANDLE process) {
+bool ProcessSnapshotWin::Initialize(HANDLE process,
+                                    ProcessSuspensionState suspension_state) {
   INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
 
   GetTimeOfDay(&snapshot_time_);
 
-  if (!process_reader_.Initialize(process))
+  if (!process_reader_.Initialize(process, suspension_state))
     return false;
 
   system_.Initialize(&process_reader_);

snapshot/win/process_snapshot_win.h

@@ -52,10 +52,14 @@ class ProcessSnapshotWin final : public ProcessSnapshot {
   //! \brief Initializes the object.
   //!
   //! \param[in] process The handle to create a snapshot from.
+  //! \param[in] suspension_state Whether \a process has been suspended by the
+  //!     caller.
   //!
   //! \return `true` if the snapshot could be created, `false` otherwise with
   //!     an appropriate message logged.
-  bool Initialize(HANDLE process);
+  //!
+  //! \sa ScopedProcessSuspend
+  bool Initialize(HANDLE process, ProcessSuspensionState suspension_state);
 
   //! \brief Initializes the object's exception.
   //!

snapshot/win/system_snapshot_win_test.cc

@@ -41,7 +41,8 @@ class SystemSnapshotWinTest : public testing::Test {
 
   // testing::Test:
   void SetUp() override {
-    ASSERT_TRUE(process_reader_.Initialize(GetCurrentProcess()));
+    ASSERT_TRUE(process_reader_.Initialize(GetCurrentProcess(),
+                                           ProcessSuspensionState::kRunning));
     system_snapshot_.Initialize(&process_reader_);
   }
 

tools/generate_dump.cc

@@ -182,7 +182,10 @@ int GenerateDumpMain(int argc, char* argv[]) {
     }
 #elif defined(OS_WIN)
     ProcessSnapshotWin process_snapshot;
-    if (!process_snapshot.Initialize(process.get())) {
+    if (!process_snapshot.Initialize(process.get(),
+                                     options.suspend
+                                         ? ProcessSuspensionState::kSuspended
+                                         : ProcessSuspensionState::kRunning)) {
       return EXIT_FAILURE;
     }
 #endif  // OS_MACOSX