win: Fix use of THREAD_ALL_ACCESS on XP

util/win/scoped_process_suspend_test.cc

 // Copyright 2015 The Crashpad Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
 //     http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
 #include "util/win/scoped_process_suspend.h"
 
 #include <tlhelp32.h>
 
 #include <algorithm>
 #include <vector>
 
 #include "gtest/gtest.h"
 #include "test/win/win_child_process.h"
+#include "util/win/xp_compat.h"
 
 namespace crashpad {
 namespace test {
 namespace {
 
 // There is no per-process suspend count on Windows, only a per-thread suspend
 // count. NtSuspendProcess just suspends all threads of a given process. So,
 // verify that all thread's suspend counts match the desired suspend count.
 bool SuspendCountMatches(HANDLE process, DWORD desired_suspend_count) {
   DWORD process_id = GetProcessId(process);
 
   ScopedKernelHANDLE snapshot(CreateToolhelp32Snapshot(TH32CS_SNAPTHREAD, 0));
   if (!snapshot.is_valid())
     return false;
 
   THREADENTRY32 te;
   te.dwSize = sizeof(te);
   if (!Thread32First(snapshot.get(), &te))
     return false;
   do {
     if (te.dwSize >= offsetof(THREADENTRY32, th32OwnerProcessID) +
                          sizeof(te.th32OwnerProcessID) &&
         te.th32OwnerProcessID == process_id) {
       ScopedKernelHANDLE thread(
-          OpenThread(THREAD_ALL_ACCESS, false, te.th32ThreadID));
+          OpenThread(kXPThreadAllAccess, false, te.th32ThreadID));
+      EXPECT_TRUE(thread.is_valid());

[Mark Mentovai, 2015/09/11 23:03:33]

ASSERT_TRUE()? Otherwise everything that follows is suspect too.

All of these things that are ASSERTing or EXPECTing on a win32 call can stream
ErrorMessage from test/errors.h:
  ASSERT_TRUE(thread.is_valid()) << ErrorMessage("OpenThread");

[scottmg, 2015/09/11 23:32:52]

Yeah, I just hate ASSERT because there's no "return false;" version. Am I using
it wrong?

Added ErrorMessage()s.

[Mark Mentovai, 2015/09/11 23:36:28]

scottmg wrote:
No, that’s right, forgot this was a bool-returning function.

       DWORD result = SuspendThread(thread.get());
       EXPECT_NE(result, static_cast<DWORD>(-1));
       if (result != static_cast<DWORD>(-1))
         ResumeThread(thread.get());

[Mark Mentovai, 2015/09/11 23:03:33]

Let’s harden this a bit more by EXPECT_TRUE()ing this guy’s result too. Maybe
the Thread32First() above, also.

[scottmg, 2015/09/11 23:32:52]

Done (_NE -1).
I think return false; is sufficient since it only makes sense to use this
function in an EXPECT_TRUE.

[Mark Mentovai, 2015/09/11 23:36:28]

scottmg wrote:
Yeah, but then from the caller’s perspective it looks like the suspend count was
wrong, when in reality we couldn’t even enumerate threads, and you’d waste
minutes debugging the wrong thing when trying to figure it out. For that reason,
it does make sense to log a special failure for Thread32First() and even
CreateToolhelp32Snapshot().

[scottmg, 2015/09/11 23:43:46]

Done.

       if (result != desired_suspend_count)
         return false;
     }
     te.dwSize = sizeof(te);
   } while (Thread32Next(snapshot.get(), &te));
 
   return true;
 }
 
 class ScopedProcessSuspendTest final : public WinChildProcess {
@@ skipping 34 matching lines @@
   EXPECT_TRUE(SuspendCountMatches(handles->process.get(), 0));
 
   // Tell the child it's OK to terminate.
   char c = ' ';
   EXPECT_TRUE(WriteFile(handles->write.get(), &c, sizeof(c)));
 }
 
 }  // namespace
 }  // namespace test
 }  // namespace crashpad