Fix CRITICAL_SECTION test

I thought I had confirmed that this still allocated and ignored the flag
on older OSs, but I must have not had the PLOG active yet? I'm not sure
what I did. (I might try to blame VMware as it has an annoying habit of
caching old binaries when you use it's "Shared Folders" feature to point
at the dev machine's build dir.)

I confirmed that it does work on Win8 and Win10 but doesn't on Win XP
and Win 7.

R=mark@chromium.org
BUG=crashpad:52

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

client/crashpad_client_win.cc

@@ -23,6 +23,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "util/file/file_io.h"
+#include "util/win/critical_section_with_debug_info.h"
 #include "util/win/registration_protocol_win.h"
 #include "util/win/scoped_handle.h"
 
@@ -101,18 +102,6 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) {
   return EXCEPTION_CONTINUE_SEARCH;
 }
 
-BOOL CrashpadInitializeCriticalSectionEx(
-    CRITICAL_SECTION* critical_section,
-    DWORD spin_count,
-    DWORD flags) {
-  static decltype(InitializeCriticalSectionEx)* initialize_critical_section_ex =
-      reinterpret_cast<decltype(InitializeCriticalSectionEx)*>(GetProcAddress(
-          LoadLibrary(L"kernel32.dll"), "InitializeCriticalSectionEx"));
-  if (!initialize_critical_section_ex)
-    return false;
-  return initialize_critical_section_ex(critical_section, spin_count, flags);
-}
-
 }  // namespace
 
 namespace crashpad {
@@ -156,14 +145,10 @@ bool CrashpadClient::SetHandler(const std::string& ipc_port) {
   // debugger would instead inspect ntdll!RtlCriticalSectionList to get the head
   // of the list. But that is not an exported symbol, so on an arbitrary client
   // machine, we don't have a way of getting that pointer.
-  if (CrashpadInitializeCriticalSectionEx(
-          &g_critical_section_with_debug_info,
-          0,
-          RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO)) {
+  if (InitializeCriticalSectionWithDebugInfoIfPossible(
+          &g_critical_section_with_debug_info)) {
     message.registration.critical_section_address =
         reinterpret_cast<WinVMAddress>(&g_critical_section_with_debug_info);
-  } else {
-    PLOG(ERROR) << "InitializeCriticalSectionEx";
   }
 
   ServerToClientMessage response = {0};

handler/handler.gyp

@@ -82,6 +82,7 @@
             '../client/client.gyp:crashpad_client',
             '../third_party/mini_chromium/mini_chromium.gyp:base',
             '../tools/tools.gyp:crashpad_tool_support',
+            '../util/util.gyp:crashpad_util',
           ],
           'include_dirs': [
             '..',

handler/win/crashy_test_program.cc

@@ -24,6 +24,7 @@
 #include "base/logging.h"
 #include "client/crashpad_client.h"
 #include "tools/tool_support.h"
+#include "util/win/critical_section_with_debug_info.h"
 
 namespace crashpad {
 namespace {
@@ -77,18 +78,6 @@ void AllocateMemoryOfVariousProtections() {
   }
 }
 
-BOOL CrashpadInitializeCriticalSectionEx(
-    CRITICAL_SECTION* critical_section,
-    DWORD spin_count,
-    DWORD flags) {
-  static decltype(InitializeCriticalSectionEx)* initialize_critical_section_ex =
-      reinterpret_cast<decltype(InitializeCriticalSectionEx)*>(GetProcAddress(
-          LoadLibrary(L"kernel32.dll"), "InitializeCriticalSectionEx"));
-  if (!initialize_critical_section_ex)
-    return false;
-  return initialize_critical_section_ex(critical_section, spin_count, flags);
-}
-
 void SomeCrashyFunction() {
   // SetLastError and NTSTATUS so that we have something to view in !gle in
   // windbg. RtlNtStatusToDosError() stores STATUS_NO_SUCH_FILE into the
@@ -117,9 +106,10 @@ int CrashyMain(int argc, char* argv[]) {
 
   AllocateMemoryOfVariousProtections();
 
-  CrashpadInitializeCriticalSectionEx(
-      &g_test_critical_section, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO);
-  EnterCriticalSection(&g_test_critical_section);
+  if (InitializeCriticalSectionWithDebugInfoIfPossible(
+          &g_test_critical_section)) {
+    EnterCriticalSection(&g_test_critical_section);
+  }
 
   SomeCrashyFunction();
 

snapshot/win/end_to_end_test.py

@@ -15,6 +15,7 @@
 # limitations under the License.
 
 import os
+import platform
 import random
 import re
 import subprocess
@@ -182,7 +183,9 @@ def RunTests(cdb_path, dump_path, pipe_name):
   out = CdbRun(cdb_path, dump_path, '!locks')
   out.Check(r'CritSec crashy_program!crashpad::`anonymous namespace\'::'
             r'g_test_critical_section', 'lock was captured')
-  out.Check(r'\*\*\* Locked', 'lock debug info was captured, and is locked')
+  if float(platform.win32_ver()[0]) != 7:
+    # We can't allocate CRITICAL_SECTIONs with .DebugInfo on Win 7.
+    out.Check(r'\*\*\* Locked', 'lock debug info was captured, and is locked')
 
 
 def main(args):

util/util.gyp

@@ -152,6 +152,8 @@
         'win/capture_context.asm',
         'win/capture_context.h',
         'win/checked_win_address_range.h',
+        'win/critical_section_with_debug_info.cc',
+        'win/critical_section_with_debug_info.h',
         'win/exception_handler_server.cc',
         'win/exception_handler_server.h',
         'win/module_version.cc',

util/util_test.gyp

@@ -80,6 +80,7 @@
         'thread/thread_log_messages_test.cc',
         'thread/thread_test.cc',
         'win/capture_context_test.cc',
+        'win/critical_section_with_debug_info_test.cc',
         'win/exception_handler_server_test.cc',
         'win/process_info_test.cc',
         'win/scoped_process_suspend_test.cc',

util/win/critical_section_with_debug_info.cc

@@ -0,0 +1,76 @@
+// 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/critical_section_with_debug_info.h"
+
+#include "base/logging.h"
+
+namespace crashpad {
+
+namespace {
+
+BOOL CrashpadInitializeCriticalSectionEx(
+    CRITICAL_SECTION* critical_section,
+    DWORD spin_count,
+    DWORD flags) {
+  static decltype(InitializeCriticalSectionEx)* initialize_critical_section_ex =
+      reinterpret_cast<decltype(InitializeCriticalSectionEx)*>(GetProcAddress(
+          LoadLibrary(L"kernel32.dll"), "InitializeCriticalSectionEx"));
+  if (!initialize_critical_section_ex) {
+    PLOG(ERROR) << "GetProcAddress";
+    return false;
+  }
+  bool ret =
+      initialize_critical_section_ex(critical_section, spin_count, flags);
+  if (!ret) {
+    PLOG(ERROR) << "InitializeCriticalSectionEx";
+    return false;
+  }
+  return true;
+}
+
+}  // namespace
+
+bool InitializeCriticalSectionWithDebugInfoIfPossible(
+    CRITICAL_SECTION* critical_section) {
+  // On XP and Vista, a plain initialization causes the CRITICAL_SECTION to be
+  // allocated with .DebugInfo. On 8 and above, we can pass an additional flag
+  // to InitializeCriticalSectionEx() to force the .DebugInfo on. Before Win 8,
+  // that flag causes InitializeCriticalSectionEx() to fail. So, for XP, Vista,
+  // and 7 we use InitializeCriticalSection(), and for 8 and above,
+  // InitializeCriticalSectionEx() with the additional flag.
+  //
+  // TODO(scottmg): Try to find a solution for Win 7. It's unclear how to force
+  // it on for Win 7, however the Loader Lock does have .DebugInfo so there may
+  // be a way to do it. The comments in winnt.h imply that perhaps it's passed
+  // to InitializeCriticalSectionAndSpinCount() as the top bits of the spin
+  // count, but that doesn't appear to work. For now, we initialize a valid
+  // CRITICAL_SECTION, but without .DebugInfo.
+
+  const DWORD version = GetVersion();
+  const DWORD major_version = LOBYTE(LOWORD(version));
+  const DWORD minor_version = HIBYTE(LOWORD(version));
+  const bool win7_or_lower =
+      major_version < 6 || (major_version == 6 && minor_version <= 1);
+
+  if (win7_or_lower) {
+    InitializeCriticalSection(critical_section);
+    return true;
+  }
+
+  return CrashpadInitializeCriticalSectionEx(
+      critical_section, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO);
+}
+
+}  // namespace crashpad

util/win/critical_section_with_debug_info.h

@@ -0,0 +1,34 @@
+// 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.
+
+#ifndef CRASHPAD_UTIL_WIN_CRITICAL_SECTION_WITH_DEBUG_INFO_H_
+#define CRASHPAD_UTIL_WIN_CRITICAL_SECTION_WITH_DEBUG_INFO_H_
+
+#include <windows.h>
+
+namespace crashpad {
+
+//! \brief Equivalent to `InitializeCritialSection()`, but attempts to allocate
+//!     with a valid `.DebugInfo` field on versions of Windows where it's
+//!     possible to do so.
+//!
+//! \return `true` on success, or `false` on failure with a message logged.
+//!     Success means that the critical section was successfully initialized,
+//!     but it does not necessarily have a valid `.DebugInfo` field.
+bool InitializeCriticalSectionWithDebugInfoIfPossible(
+    CRITICAL_SECTION* critical_section);
+
+}  // namespace crashpad
+
+#endif  // CRASHPAD_UTIL_WIN_CRITICAL_SECTION_WITH_DEBUG_INFO_H_

util/win/critical_section_with_debug_info_test.cc

@@ -0,0 +1,34 @@
+// 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/critical_section_with_debug_info.h"
+
+#include "gtest/gtest.h"
+
+namespace crashpad {
+namespace test {
+namespace {
+
+TEST(CriticalSectionWithDebugInfo, CriticalSectionWithDebugInfo) {
+  CRITICAL_SECTION critical_section;
+  ASSERT_TRUE(
+      InitializeCriticalSectionWithDebugInfoIfPossible(&critical_section));
+  EnterCriticalSection(&critical_section);
+  LeaveCriticalSection(&critical_section);
+  DeleteCriticalSection(&critical_section);
+}
+
+}  // namespace
+}  // namespace test
+}  // namespace crashpad

util/win/registration_protocol_win.h

@@ -61,10 +61,10 @@ struct RegistrationRequest {
 
   //! \brief The address, in the client process's address space, of a
   //!     `CRITICAL_SECTION` allocated with a valid .DebugInfo field. This can
-  //!     be accomplished by using the
-  //!     `RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO` flag to
-  //!     `InitializeCriticalSectionEx()`. This value can be `0`, however then
-  //!     limited lock data will be available in minidumps.
+  //!     be accomplished by using
+  //!     InitializeCriticalSectionWithDebugInfoIfPossible() or equivalent. This
+  //!     value can be `0`, however then limited lock data will be available in
+  //!     minidumps.
   WinVMAddress critical_section_address;
 };