Two HandleVerifier thread safety fixes.

base/win/scoped_handle.cc

Index: base/win/scoped_handle.cc
diff --git a/base/win/scoped_handle.cc b/base/win/scoped_handle.cc
index c429375b9cbb4a9a373508eb574b5e68627cc605..e046e0f0a52ecdb458ae802a4ad6e2fba1d1ece1 100644
--- a/base/win/scoped_handle.cc
+++ b/base/win/scoped_handle.cc
@@ -41,16 +41,11 @@ struct Info {
 };
 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 {
@@ -113,11 +108,28 @@ ActiveVerifier* ActiveVerifier::Get() {
   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
@@ -128,17 +140,27 @@ void ActiveVerifier::InstallVerifier() {
       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
 }
 
@@ -221,7 +243,7 @@ void ActiveVerifier::OnHandleBeingClosed(HANDLE handle) {
 }  // namespace
 
 void* GetHandleVerifier() {
-  return g_active_verifier;
+  return ActiveVerifier::Get();
 }
 
 namespace base {