Two HandleVerifier thread safety fixes.

base/win/scoped_handle.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/win/scoped_handle.h"
 
 #include <stddef.h>
 
 #include <unordered_map>
 
@@ skipping 23 matching lines @@
 
 struct Info {
   const void* owner;
   const void* pc1;
   const void* pc2;
   base::debug::StackTrace stack;
   DWORD thread_id;
 };
 typedef std::unordered_map<HANDLE, Info, HandleHash> HandleMap;
 
-// g_lock protects the handle map and setting g_active_verifier.
+// g_lock protects the handle map and setting g_active_verifier within this
+// module.
 typedef base::internal::LockImpl NativeLock;
 base::LazyInstance<NativeLock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
 
-bool CloseHandleWrapper(HANDLE handle) {
-  if (!::CloseHandle(handle))
-    CHECK(false);  // CloseHandle failed.
-  return true;
-}
-
 // Simple automatic locking using a native critical section so it supports
 // recursive locking.
 class AutoNativeLock {
  public:
   explicit AutoNativeLock(NativeLock& lock) : lock_(lock) {
     lock_.Lock();
   }
 
   ~AutoNativeLock() {
     lock_.Unlock();
@@ skipping 42 matching lines @@
 ActiveVerifier* g_active_verifier = NULL;
 
 // static
 ActiveVerifier* ActiveVerifier::Get() {
   if (!g_active_verifier)
     ActiveVerifier::InstallVerifier();
 
   return g_active_verifier;
 }
 
+bool CloseHandleWrapper(HANDLE handle) {
+  if (!::CloseHandle(handle))
+    CHECK(false);  // CloseHandle failed.
+  return true;
+}
+
+void ThreadSafeAssignOrCreateActiveVerifier(ActiveVerifier* existing_verifier,
+                                            bool enabled = true) {

[scottmg, 2016/03/10 22:11:22]

Is this allowed now? I think just passing true in the one call that doesn't pass
true/false is better.

[Will Harris, 2016/03/10 22:14:23]

it's not disallowed, and I see other examples in base/ so I went with it.

[scottmg, 2016/03/10 22:17:16]

OK, I still don't think there's any utility when 3/4 calls pass an explicit
value.

[Will Harris, 2016/03/10 22:18:57]

it tickled my OCD passing an ignored value for the case on line 163, but sure I
can change it back.

+  AutoNativeLock lock(g_lock.Get());
+  // Another thread in this module might be trying to assign the global
+  // verifier, so check that within the lock here.
+  if (g_active_verifier)
+    return;
+  g_active_verifier =
+      existing_verifier ? existing_verifier : new ActiveVerifier(enabled);
+}
+
 // static
 void ActiveVerifier::InstallVerifier() {
 #if defined(COMPONENT_BUILD)
-  AutoNativeLock lock(g_lock.Get());
-  g_active_verifier = new ActiveVerifier(true);
+  // Component build has one Active Verifier per module.
+  ThreadSafeAssignOrCreateActiveVerifier(nullptr, true);
 #else
   // If you are reading this, wondering why your process seems deadlocked, take
   // a look at your DllMain code and remove things that should not be done
   // there, like doing whatever gave you that nice windows handle you are trying
   // to store in a ScopedHandle.
   HMODULE main_module = ::GetModuleHandle(NULL);
   GetHandleVerifierFn get_handle_verifier =
       reinterpret_cast<GetHandleVerifierFn>(::GetProcAddress(
           main_module, "GetHandleVerifier"));
 
+  // This should only happen if running in a DLL is linked with base but the
+  // hosting EXE is not. In this case, create an ActiveVerifier for the current
+  // module but leave it disabled.
   if (!get_handle_verifier) {
-    g_active_verifier = new ActiveVerifier(false);
+    ThreadSafeAssignOrCreateActiveVerifier(nullptr, false);
     return;
   }
 
-  ActiveVerifier* verifier =
+  // Check if in the main module.
+  if (get_handle_verifier == GetHandleVerifier) {
+    ThreadSafeAssignOrCreateActiveVerifier(nullptr, true);
+    return;
+  }
+
+  ActiveVerifier* main_module_verifier =
       reinterpret_cast<ActiveVerifier*>(get_handle_verifier());
 
-  // This lock only protects against races in this module, which is fine.
-  AutoNativeLock lock(g_lock.Get());
-  g_active_verifier = verifier ? verifier : new ActiveVerifier(true);
+  // Main module should always on-demand create a verifier.
+  DCHECK(main_module_verifier);
+
+  ThreadSafeAssignOrCreateActiveVerifier(main_module_verifier);
 #endif
 }
 
 bool ActiveVerifier::CloseHandle(HANDLE handle) {
   if (!enabled_)
     return CloseHandleWrapper(handle);
 
   closing_.Set(true);
   CloseHandleWrapper(handle);
   closing_.Set(false);
@@ skipping 62 matching lines @@
 
   Info other = i->second;
   base::debug::Alias(&other);
   base::debug::Alias(&creation_stack_);
   CHECK(false);  // CloseHandle called on tracked handle.
 }
 
 }  // namespace
 
 void* GetHandleVerifier() {
-  return g_active_verifier;
+  return ActiveVerifier::Get();
 }
 
 namespace base {
 namespace win {
 
 // Static.
 bool HandleTraits::CloseHandle(HANDLE handle) {
   return ActiveVerifier::Get()->CloseHandle(handle);
 }
 
@@ skipping 12 matching lines @@
 void DisableHandleVerifier() {
   return ActiveVerifier::Get()->Disable();
 }
 
 void OnHandleBeingClosed(HANDLE handle) {
   return ActiveVerifier::Get()->OnHandleBeingClosed(handle);
 }
 
 }  // namespace win
 }  // namespace base