These caused failures on debug win7 bots that need further investigation.

Reverting all the CLs, taking a step back, try again next year.

This reverts commit 7969a476e9.
This reverts commit fdd35a81b4.

BUG=571304,570912,481095

TBR=cpu@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#366445}

base/base.gypi

@@ -147,8 +147,6 @@
           'debug/alias.h',
           'debug/asan_invalid_access.cc',
           'debug/asan_invalid_access.h',
-          'debug/close_handle_hook_win.cc',
-          'debug/close_handle_hook_win.h',
           'debug/crash_logging.cc',
           'debug/crash_logging.h',
           'debug/debugger.cc',

base/debug/BUILD.gn

@@ -11,8 +11,6 @@ source_set("debug") {
     "alias.h",
     "asan_invalid_access.cc",
     "asan_invalid_access.h",
-    "close_handle_hook_win.cc",
-    "close_handle_hook_win.h",
     "crash_logging.cc",
     "crash_logging.h",
     "debugger.cc",

base/debug/close_handle_hook_win.h

@@ -1,23 +0,0 @@
-// Copyright 2015 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.
-
-#ifndef BASE_DEBUG_CLOSE_HANDLE_HOOK_WIN_H_
-#define BASE_DEBUG_CLOSE_HANDLE_HOOK_WIN_H_
-
-#include "base/base_export.h"
-
-namespace base {
-namespace debug {
-
-// Installs the hooks required to debug use of improper handles.
-// Returns true if the hooks were successfully installed.
-BASE_EXPORT bool InstallHandleHooks();
-
-// Removes the hooks installed by InstallHandleHooks().
-BASE_EXPORT void RemoveHandleHooks();
-
-}  // namespace debug
-}  // namespace base
-
-#endif  // BASE_DEBUG_CLOSE_HANDLE_HOOK_WIN_H_

base/test/run_all_unittests.cc

@@ -11,24 +11,13 @@
 #include "base/test/test_file_util.h"
 #endif
 
-#if defined(OS_WIN)
-#include "base/debug/close_handle_hook_win.h"
-#endif
-
 int main(int argc, char** argv) {
 #if defined(OS_ANDROID)
   JNIEnv* env = base::android::AttachCurrentThread();
   base::RegisterContentUriTestUtils(env);
 #endif
   base::TestSuite test_suite(argc, argv);
-#if defined(OS_WIN)
-  base::debug::InstallHandleHooks();
-#endif
-  int ret = base::LaunchUnitTests(
+  return base::LaunchUnitTests(
       argc, argv,
       base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite)));
-#if defined(OS_WIN)
-  base::debug::RemoveHandleHooks();
-#endif
-  return ret;
 }

base/win/scoped_handle.cc

@@ -41,7 +41,7 @@ base::LazyInstance<NativeLock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
 
 bool CloseHandleWrapper(HANDLE handle) {
   if (!::CloseHandle(handle))
-    LOG(FATAL) << "CloseHandle failed.";
+    CHECK(false);
   return true;
 }
 
@@ -163,7 +163,7 @@ void ActiveVerifier::StartTracking(HANDLE handle, const void* owner,
   if (!result.second) {
     Info other = result.first->second;
     base::debug::Alias(&other);
-    LOG(FATAL) << "Attempt to start tracking already tracked handle.";
+    CHECK(false);
   }
 }
 
@@ -175,12 +175,12 @@ void ActiveVerifier::StopTracking(HANDLE handle, const void* owner,
   AutoNativeLock lock(*lock_);
   HandleMap::iterator i = map_.find(handle);
   if (i == map_.end())
-    LOG(FATAL) << "Attempting to close an untracked handle.";
+    CHECK(false);
 
   Info other = i->second;
   if (other.owner != owner) {
     base::debug::Alias(&other);
-    LOG(FATAL) << "Attempting to close a handle not owned by opener.";
+    CHECK(false);
   }
 
   map_.erase(i);
@@ -201,7 +201,7 @@ void ActiveVerifier::OnHandleBeingClosed(HANDLE handle) {
 
   Info other = i->second;
   base::debug::Alias(&other);
-  LOG(FATAL) << "CloseHandle called on tracked handle.";
+  CHECK(false);
 }
 
 }  // namespace

base/win/scoped_handle_unittest.cc

@@ -2,9 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <windows.h>
-#include <winternl.h>
-
 #include "base/win/scoped_handle.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
@@ -33,38 +30,3 @@ TEST(ScopedHandleTest, ScopedHandle) {
   handle_holder = handle_source.Pass();
   EXPECT_EQ(magic_error, ::GetLastError());
 }
-
-TEST(ScopedHandleTest, ActiveVerifierCloseTracked) {
-  HANDLE handle = ::CreateMutex(nullptr, FALSE, nullptr);
-  ASSERT_NE(HANDLE(NULL), handle);
-  ASSERT_DEATH({
-    base::win::ScopedHandle handle_holder(handle);
-    // Calling CloseHandle on a tracked handle should crash.
-    ::CloseHandle(handle);
-  }, "CloseHandle called on tracked handle.");
-}
-
-TEST(ScopedHandleTest, ActiveVerifierTrackedHasBeenClosed) {
-  HANDLE handle = ::CreateMutex(nullptr, FALSE, nullptr);
-  ASSERT_NE(HANDLE(NULL), handle);
-  typedef NTSTATUS(WINAPI * NtCloseFunc)(HANDLE);
-  NtCloseFunc ntclose = reinterpret_cast<NtCloseFunc>(
-      GetProcAddress(GetModuleHandle(L"ntdll.dll"), "NtClose"));
-  ASSERT_NE(nullptr, ntclose);
-
-  ASSERT_DEATH({
-    base::win::ScopedHandle handle_holder(handle);
-    ntclose(handle);
-    // Destructing a ScopedHandle with an illegally closed handle should fail.
-  }, "CloseHandle failed.");
-}
-
-TEST(ScopedHandleTest, ActiveVerifierDoubleTracking) {
-  HANDLE handle = ::CreateMutex(nullptr, FALSE, nullptr);
-  ASSERT_NE(HANDLE(NULL), handle);
-
-  ASSERT_DEATH({
-    base::win::ScopedHandle handle_holder(handle);
-    base::win::ScopedHandle handle_holder2(handle);
-  }, "Attempt to start tracking already tracked handle.");
-}

chrome/BUILD.gn

@@ -294,6 +294,8 @@ if (is_mac || is_win) {
       "app/chrome_main_delegate.h",
       "app/chrome_main_mac.h",
       "app/chrome_main_mac.mm",
+      "app/close_handle_hook_win.cc",
+      "app/close_handle_hook_win.h",
       "app/delay_load_hook_win.cc",
       "app/delay_load_hook_win.h",
     ]
@@ -393,6 +395,8 @@ if (is_mac || is_win) {
         "app/chrome_main.cc",
         "app/chrome_main_delegate.cc",
         "app/chrome_main_delegate.h",
+        "app/close_handle_hook_win.cc",
+        "app/close_handle_hook_win.h",
       ]
 
       configs += [ "//build/config/compiler:wexit_time_destructors" ]

chrome/app/BUILD.gn

@@ -317,6 +317,8 @@ source_set("test_support") {
   sources = [
     "chrome_main_delegate.cc",
     "chrome_main_delegate.h",
+    "close_handle_hook_win.cc",
+    "close_handle_hook_win.h",
   ]
 
   deps = [

chrome/app/chrome_main_delegate.cc

@@ -55,7 +55,7 @@
 #include <atlbase.h>
 #include <malloc.h>
 #include <algorithm>
-#include "base/debug/close_handle_hook_win.h"
+#include "chrome/app/close_handle_hook_win.h"
 #include "chrome/common/child_process_logging.h"
 #include "chrome/common/v8_breakpad_support_win.h"
 #include "components/crash/content/app/crashpad.h"
@@ -499,25 +499,8 @@ bool ChromeMainDelegate::BasicStartupComplete(int* exit_code) {
     return true;
   }
 
-  // Handle verifier is enabled on 32-bit only, and only in Debug builds or
-  // Release builds running on Dev or Canary channels.
-  bool handle_verifier_enabled = false;
-#if !defined(ARCH_CPU_X86_64)
-#if defined(NDEBUG)
-  version_info::Channel channel = chrome::GetChannel();
-  if (channel == version_info::Channel::CANARY ||
-      channel == version_info::Channel::DEV) {
-    handle_verifier_enabled = true;
-  }
-#else
-  // Always enabled in Debug.
-  handle_verifier_enabled = true;
-#endif  // NDEBUG
-#endif  // ARCH_CPU_X86_64
-
-  if (!handle_verifier_enabled || !base::debug::InstallHandleHooks())
-    base::win::DisableHandleVerifier();
-#endif  // OS_WIN
+  InstallHandleHooks();
+#endif
 
   chrome::RegisterPathProvider();
 #if defined(OS_CHROMEOS)
@@ -906,7 +889,7 @@ void ChromeMainDelegate::ProcessExiting(const std::string& process_type) {
 #endif  // !defined(OS_ANDROID)
 
 #if defined(OS_WIN)
-  base::debug::RemoveHandleHooks();
+  RemoveHandleHooks();
 #endif
 }
 

chrome/app/close_handle_hook_win.cc

@@ -1,8 +1,8 @@
-// Copyright 2015 The Chromium Authors. All rights reserved.
+// Copyright 2014 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/debug/close_handle_hook_win.h"
+#include "chrome/app/close_handle_hook_win.h"
 
 #include <Windows.h>
 #include <psapi.h>
@@ -15,6 +15,8 @@
 #include "base/win/iat_patch_function.h"
 #include "base/win/pe_image.h"
 #include "base/win/scoped_handle.h"
+#include "chrome/common/channel_info.h"
+#include "components/version_info/version_info.h"
 
 namespace {
 
@@ -119,25 +121,25 @@ void AutoProtectMemory::RevertProtection() {
 }
 
 // Performs an EAT interception.
-bool EATPatch(HMODULE module, const char* function_name,
+void EATPatch(HMODULE module, const char* function_name,
               void* new_function, void** old_function) {
   if (!module)
-    return false;
+    return;
 
   base::win::PEImage pe(module);
   if (!pe.VerifyMagic())
-    return false;
+    return;
 
   DWORD* eat_entry = pe.GetExportEntry(function_name);
   if (!eat_entry)
-    return false;
+    return;
 
   if (!(*old_function))
     *old_function = pe.RVAToAddr(*eat_entry);
 
   AutoProtectMemory memory;
   if (!memory.ChangeProtection(eat_entry, sizeof(DWORD)))
-    return false;
+    return;
 
   // Perform the patch.
 #pragma warning(push)
@@ -146,7 +148,6 @@ bool EATPatch(HMODULE module, const char* function_name,
   *eat_entry = reinterpret_cast<DWORD>(new_function) -
                reinterpret_cast<DWORD>(module);
 #pragma warning(pop)
-  return true;
 }
 
 // Performs an IAT interception.
@@ -186,9 +187,9 @@ class HandleHooks {
   HandleHooks() {}
   ~HandleHooks() {}
 
-  bool AddIATPatch(HMODULE module);
-  bool AddEATPatch();
-  bool Unpatch();
+  void AddIATPatch(HMODULE module);
+  void AddEATPatch();
+  void Unpatch();
 
  private:
   std::vector<base::win::IATPatchFunction*> hooks_;
@@ -196,87 +197,89 @@ class HandleHooks {
 };
 base::LazyInstance<HandleHooks> g_hooks = LAZY_INSTANCE_INITIALIZER;
 
-bool HandleHooks::AddIATPatch(HMODULE module) {
+void HandleHooks::AddIATPatch(HMODULE module) {
   if (!module)
-    return false;
+    return;
 
   base::win::IATPatchFunction* patch = NULL;
   patch = IATPatch(module, "CloseHandle", &CloseHandleHook,
                    reinterpret_cast<void**>(&g_close_function));
   if (!patch)
-    return false;
+    return;
   hooks_.push_back(patch);
 
   patch = IATPatch(module, "DuplicateHandle", &DuplicateHandleHook,
                    reinterpret_cast<void**>(&g_duplicate_function));
   if (!patch)
-    return false;
+    return;
   hooks_.push_back(patch);
-  return true;
 }
 
-bool HandleHooks::AddEATPatch() {
-   // An attempt to restore the entry on the table at destruction is not safe.
+void HandleHooks::AddEATPatch() {
   // An attempt to restore the entry on the table at destruction is not safe.
-  return (EATPatch(GetModuleHandleA("kernel32.dll"), "CloseHandle",
-                   &CloseHandleHook,
-                   reinterpret_cast<void**>(&g_close_function)) &&
-          EATPatch(GetModuleHandleA("kernel32.dll"), "DuplicateHandle",
-                   &DuplicateHandleHook,
-                   reinterpret_cast<void**>(&g_duplicate_function)));
+  EATPatch(GetModuleHandleA("kernel32.dll"), "CloseHandle",
+           &CloseHandleHook, reinterpret_cast<void**>(&g_close_function));
+  EATPatch(GetModuleHandleA("kernel32.dll"), "DuplicateHandle",
+           &DuplicateHandleHook,
+           reinterpret_cast<void**>(&g_duplicate_function));
 }
 
-bool HandleHooks::Unpatch() {
-  DWORD err = NO_ERROR;
+void HandleHooks::Unpatch() {
   for (std::vector<base::win::IATPatchFunction*>::iterator it = hooks_.begin();
        it != hooks_.end(); ++it) {
-    err = (*it)->Unpatch();
-    if (err != NO_ERROR)
-      break;
+    (*it)->Unpatch();
     delete *it;
   }
-  return (err == NO_ERROR);
 }
 
-bool PatchLoadedModules(HandleHooks* hooks) {
+bool UseHooks() {
+#if defined(ARCH_CPU_X86_64)
+  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 PatchLoadedModules(HandleHooks* hooks) {
   const DWORD kSize = 256;
   DWORD returned;
   scoped_ptr<HMODULE[]> modules(new HMODULE[kSize]);
   if (!EnumProcessModules(GetCurrentProcess(), modules.get(),
                           kSize * sizeof(HMODULE), &returned)) {
-    return false;
+    return;
   }
   returned /= sizeof(HMODULE);
   returned = std::min(kSize, returned);
 
-  bool success = false;
-
   for (DWORD current = 0; current < returned; current++) {
-    success = hooks->AddIATPatch(modules[current]);
-    if (!success)
-      break;
+    hooks->AddIATPatch(modules[current]);
   }
-
-  return success;
 }
 
 }  // namespace
 
-namespace base {
-namespace debug {
+void InstallHandleHooks() {
+  if (UseHooks()) {
+    HandleHooks* hooks = g_hooks.Pointer();
 
-bool InstallHandleHooks() {
-  HandleHooks* hooks = g_hooks.Pointer();
-
-  // Performing EAT interception first is safer in the presence of other
-  // threads attempting to call CloseHandle.
-  return (hooks->AddEATPatch() && PatchLoadedModules(hooks));
+    // Performing EAT interception first is safer in the presence of other
+    // threads attempting to call CloseHandle.
+    hooks->AddEATPatch();
+    PatchLoadedModules(hooks);
+  } else {
+    base::win::DisableHandleVerifier();
+  }
 }
 
 void RemoveHandleHooks() {
-  // We are patching all loaded modules without forcing them to stay in memory,
+  // We are partching all loaded modules without forcing them to stay in memory,
   // removing patches is not safe.
 }
-
-}  // namespace debug
-}  // namespace base

chrome/app/close_handle_hook_win.h

@@ -0,0 +1,14 @@
+// Copyright 2014 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.
+
+#ifndef CHROME_APP_CLOSE_HANDLE_HOOK_WIN_H_
+#define CHROME_APP_CLOSE_HANDLE_HOOK_WIN_H_
+
+// Installs the hooks required to debug use of improper handles.
+void InstallHandleHooks();
+
+// Removes the hooks installed by InstallHandleHooks().
+void RemoveHandleHooks();
+
+#endif  // CHROME_APP_CLOSE_HANDLE_HOOK_WIN_H_

chrome/chrome_dll.gypi

@@ -116,6 +116,8 @@
             'app/chrome_main_delegate.h',
             'app/chrome_main_mac.h',
             'app/chrome_main_mac.mm',
+            'app/close_handle_hook_win.cc',
+            'app/close_handle_hook_win.h',
             'app/delay_load_hook_win.cc',
             'app/delay_load_hook_win.h',
           ],
@@ -362,6 +364,8 @@
             'app/chrome_main.cc',
             'app/chrome_main_delegate.cc',
             'app/chrome_main_delegate.h',
+            'app/close_handle_hook_win.cc',
+            'app/close_handle_hook_win.h',
           ],
           'conditions': [
             ['OS=="win" and win_use_allocator_shim==1', {

chrome/chrome_tests_unit.gypi

@@ -1725,6 +1725,8 @@
       'sources': [
         'app/chrome_main_delegate.cc',
         'app/chrome_main_delegate.h',
+        'app/close_handle_hook_win.cc',
+        'app/close_handle_hook_win.h',
         'browser/browsing_data/mock_browsing_data_appcache_helper.cc',
         'browser/browsing_data/mock_browsing_data_appcache_helper.h',
         'browser/browsing_data/mock_browsing_data_cache_storage_helper.cc',