Enable HandlerVerifier on dcheck builds only

The HandleVerifier is currently enabled on Canary and Dev channel.
It is currently not running for a chrome developers local build.

This CL enables the HandleVerifier for builds with DCHECK_ON. It will
run on Canary builds for a reduced set of the population under the
albatross build instead of on all Canary/Dev. This helps isolate the
performance impact to builds with that intent in my mind and avoids
polluting performance diagnosis tools.

The developers can reproduce issues locally. A build with
dcheck_always_on and supporting the IAT/EAT patching will run the
HandleVerifier.

R=wfh@chromium.org
Bug: 1104342, 1103801

Change-Id: Ie565dcc99c8c2faa27f61bcc4533bb48494c0492
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2294562Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795650}

base/win/scoped_handle.h

@@ -10,9 +10,11 @@
 #include "base/base_export.h"
 #include "base/check_op.h"
 #include "base/compiler_specific.h"
+#include "base/dcheck_is_on.h"
 #include "base/gtest_prod_util.h"
 #include "base/location.h"
 #include "base/macros.h"
+#include "build/build_config.h"
 
 // TODO(rvargas): remove this with the rest of the verifier.
 #if defined(COMPILER_MSVC)
@@ -166,7 +168,15 @@ class BASE_EXPORT VerifierTraits {
   DISALLOW_IMPLICIT_CONSTRUCTORS(VerifierTraits);
 };
 
-using ScopedHandle = GenericScopedHandle<HandleTraits, VerifierTraits>;
+using UncheckedScopedHandle =
+    GenericScopedHandle<HandleTraits, DummyVerifierTraits>;
+using CheckedScopedHandle = GenericScopedHandle<HandleTraits, VerifierTraits>;
+
+#if DCHECK_IS_ON() && !defined(ARCH_CPU_64_BITS)
+using ScopedHandle = CheckedScopedHandle;
+#else
+using ScopedHandle = UncheckedScopedHandle;
+#endif
 
 // This function may be called by the embedder to disable the use of
 // VerifierTraits at runtime. It has no effect if DummyVerifierTraits is used

base/win/scoped_handle_test_dll.cc

@@ -32,7 +32,7 @@ DWORD __stdcall ThreadFunc(void* params) {
 
   ::SetEvent(thread_params->ready_event);
   ::WaitForSingleObject(thread_params->start_event, INFINITE);
-  ScopedHandle handle_holder(handle);
+  CheckedScopedHandle handle_holder(handle);
   return 0;
 }
 
@@ -88,7 +88,7 @@ bool InternalRunLocationTest() {
   HANDLE handle = ::CreateMutex(nullptr, false, nullptr);
   if (!handle)
     return false;
-  ScopedHandle handle_holder(handle);
+  CheckedScopedHandle handle_holder(handle);
 
   HMODULE verifier_module =
       base::win::internal::GetHandleVerifierModuleForTesting();

base/win/scoped_handle_unittest.cc

@@ -71,19 +71,19 @@ TEST(ScopedHandleTest, HandleVerifierDoubleTracking) {
   HANDLE handle = ::CreateMutex(nullptr, false, nullptr);
   ASSERT_NE(HANDLE(nullptr), handle);
 
-  base::win::ScopedHandle handle_holder(handle);
+  base::win::CheckedScopedHandle handle_holder(handle);
 
-  ASSERT_DEATH({ base::win::ScopedHandle handle_holder2(handle); }, "");
+  ASSERT_DEATH({ base::win::CheckedScopedHandle handle_holder2(handle); }, "");
 }
 
 TEST(ScopedHandleTest, HandleVerifierWrongOwner) {
   HANDLE handle = ::CreateMutex(nullptr, false, nullptr);
   ASSERT_NE(HANDLE(nullptr), handle);
 
-  base::win::ScopedHandle handle_holder(handle);
+  base::win::CheckedScopedHandle handle_holder(handle);
   ASSERT_DEATH(
       {
-        base::win::ScopedHandle handle_holder2;
+        base::win::CheckedScopedHandle handle_holder2;
         handle_holder2.handle_ = handle;
       },
       "");
@@ -97,7 +97,7 @@ TEST(ScopedHandleTest, HandleVerifierUntrackedHandle) {
 
   ASSERT_DEATH(
       {
-        base::win::ScopedHandle handle_holder;
+        base::win::CheckedScopedHandle handle_holder;
         handle_holder.handle_ = handle;
       },
       "");

chrome/app/chrome_main_delegate.cc

@@ -11,6 +11,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/cpu.h"
+#include "base/dcheck_is_on.h"
 #include "base/files/file_path.h"
 #include "base/i18n/rtl.h"
 #include "base/lazy_instance.h"
@@ -225,22 +226,6 @@ bool IsSandboxedProcess() {
   return is_sandboxed_process_func && is_sandboxed_process_func();
 }
 
-bool UseHooks() {
-#if defined(ARCH_CPU_64_BITS)
-  return false;
-#elif defined(NDEBUG)
-  version_info::Channel channel = chrome::GetChannel();
-  if (channel == version_info::Channel::CANARY ||
-      channel == version_info::Channel::DEV) {
-    return true;
-  }
-
-  return false;
-#else  // NDEBUG
-  return true;
-#endif
-}
-
 void SetUpExtendedCrashReporting(bool is_browser_process) {
   browser_watcher::ExtendedCrashReporting* extended_crash_reporting =
       browser_watcher::ExtendedCrashReporting::SetUpIfEnabled(
@@ -719,13 +704,17 @@ bool ChromeMainDelegate::BasicStartupComplete(int* exit_code) {
     return true;
   }
 
-  if (UseHooks())
-    base::debug::InstallHandleHooks();
-  else
-    base::win::DisableHandleVerifier();
-
+// HandleVerifier detects and reports incorrect handle manipulations. It tracks
+// handle operations on builds that support DCHECK only.
+// TODO(crbug/1104358): Support 64-bit handle hooks.
+#if DCHECK_IS_ON() && !defined(ARCH_CPU_64_BITS)
+  base::debug::InstallHandleHooks();
+#else
+  base::win::DisableHandleVerifier();
 #endif
 
+#endif  // defined(OS_WIN)
+
   chrome::RegisterPathProvider();
 #if defined(OS_CHROMEOS)
   chromeos::RegisterPathProvider();