Avoid execution of stack checks when HandleVerifier is not enabled

This CL is removing the stack checks executed in
  HandleVerifier::StartTracking(...)
  HandleVerifier::StopTracking(...)
These stack checks were executed even hen the HandleVerifier
is not enabled.

There are three major changes:

1) Lift out the body of the tracking functions to a separate
   function.

   void StartTracking(...) {
     if (enable_)
       StartTrackingImpl(...);  // StartTrackingImpl -> no-inline.
   }

   The intend of this change is to ensure no large local variable
   (e.g. StackFrame) is using space on the stack. When enable_ is
   false, no stack checks are executed.


2) Lift the error reporting to a no-inline function
     ReportErrorOnScopedHandleOperation(...)
   These error reporting function make a local copy of the creation
   stackframe. Even if the error reporting code was not executed, the
   required local variables were accounted into the total stack size
   used by the tracking functions.

3) Avoid copy of large objects to local variables

   Some tracking functions were making a local copy of the handle
   information.
      ScopedHandleVerifierInfo other = i->second;
   This class includes an instance of StackFrame (which is large).
   If possible, it's better to avoid the copy and the local variable
   needed on the stack.

Bug: 1103763
Change-Id: I01644028b85e4ee856b1a6c4fd4e107eb82dea07
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2291650
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788615}

base/win/scoped_handle_verifier.cc

@@ -10,6 +10,7 @@
 
 #include <unordered_map>
 
+#include "base/compiler_specific.h"
 #include "base/debug/alias.h"
 #include "base/debug/stack_trace.h"
 #include "base/synchronization/lock_impl.h"
@@ -25,17 +26,19 @@ void* GetHandleVerifier() {
 }
 }  // extern C
 
+namespace base {
+namespace win {
+namespace internal {
+
 namespace {
 
-base::win::internal::ScopedHandleVerifier* g_active_verifier = nullptr;
+ScopedHandleVerifier* g_active_verifier = nullptr;
 using GetHandleVerifierFn = void* (*)();
 using HandleMap =
-    std::unordered_map<HANDLE,
-                       base::win::internal::ScopedHandleVerifierInfo,
-                       base::win::internal::HandleHash>;
+    std::unordered_map<HANDLE, ScopedHandleVerifierInfo, HandleHash>;
 using NativeLock = base::internal::LockImpl;
 
-NativeLock* GetLock() {
+NativeLock* GetScopedHandleVerifierLock() {
   static auto* native_lock = new NativeLock();
   return native_lock;
 }
@@ -53,11 +56,26 @@ class AutoNativeLock {
   DISALLOW_COPY_AND_ASSIGN(AutoNativeLock);
 };
 
-}  // namespace
+NOINLINE void ReportErrorOnScopedHandleOperation(
+    const base::debug::StackTrace& creation_stack) {
+  auto creation_stack_copy = creation_stack;
+  base::debug::Alias(&creation_stack_copy);
+  CHECK(false);
+  __builtin_unreachable();
+}
 
-namespace base {
-namespace win {
-namespace internal {
+NOINLINE void ReportErrorOnScopedHandleOperation(
+    const base::debug::StackTrace& creation_stack,
+    const ScopedHandleVerifierInfo& other) {
+  auto other_copy = other;
+  base::debug::Alias(&other_copy);
+  auto creation_stack_copy = creation_stack;
+  base::debug::Alias(&creation_stack_copy);
+  CHECK(false);
+  __builtin_unreachable();
+}
+
+}  // namespace
 
 ScopedHandleVerifier::ScopedHandleVerifier(bool enabled)
     : enabled_(enabled), lock_(GetLock()) {}
@@ -76,12 +94,12 @@ bool CloseHandleWrapper(HANDLE handle) {
   return true;
 }
 
-// Assigns the g_active_verifier global within the GetLock() lock.
+// Assigns the g_active_verifier global within the ScopedHandleVerifier lock.
 // If |existing_verifier| is non-null then |enabled| is ignored.
 void ThreadSafeAssignOrCreateScopedHandleVerifier(
     ScopedHandleVerifier* existing_verifier,
     bool enabled) {
-  AutoNativeLock lock(*GetLock());
+  AutoNativeLock lock(*GetScopedHandleVerifierLock());
   // Another thread in this module might be trying to assign the global
   // verifier, so check that within the lock here.
   if (g_active_verifier)
@@ -142,90 +160,92 @@ bool ScopedHandleVerifier::CloseHandle(HANDLE handle) {
 
 // static
 NativeLock* ScopedHandleVerifier::GetLock() {
-  return ::GetLock();
+  return GetScopedHandleVerifierLock();
 }
 
 void ScopedHandleVerifier::StartTracking(HANDLE handle,
                                          const void* owner,
                                          const void* pc1,
                                          const void* pc2) {
-  if (!enabled_)
-    return;
-  TRACE_EVENT0("base", "ScopedHandleVerifier::StartTracking");
+  if (enabled_)
+    StartTrackingImpl(handle, owner, pc1, pc2);
+}
+
+void ScopedHandleVerifier::StopTracking(HANDLE handle,
+                                        const void* owner,
+                                        const void* pc1,
+                                        const void* pc2) {
+  if (enabled_)
+    StopTrackingImpl(handle, owner, pc1, pc2);
+}
+
+void ScopedHandleVerifier::Disable() {
+  enabled_ = false;
+}
+
+void ScopedHandleVerifier::OnHandleBeingClosed(HANDLE handle) {
+  if (enabled_)
+    OnHandleBeingClosedImpl(handle);
+}
+
+HMODULE ScopedHandleVerifier::GetModule() const {
+  return CURRENT_MODULE();
+}
+
+NOINLINE void ScopedHandleVerifier::StartTrackingImpl(HANDLE handle,
+                                                      const void* owner,
+                                                      const void* pc1,
+                                                      const void* pc2) {
+  TRACE_EVENT0("base", "ScopedHandleVerifier::StartTrackingImpl");
 
   // Grab the thread id before the lock.
   DWORD thread_id = GetCurrentThreadId();
 
   // Grab the thread stacktrace before the lock.
-  ScopedHandleVerifierInfo handle_info = {owner, pc1, pc2,
-                                          base::debug::StackTrace(), thread_id};
-  std::pair<HANDLE, ScopedHandleVerifierInfo> item(handle, handle_info);
+  auto item = std::make_pair(
+      handle, ScopedHandleVerifierInfo{owner, pc1, pc2,
+                                       base::debug::StackTrace(), thread_id});
 
   AutoNativeLock lock(*lock_);
   std::pair<HandleMap::iterator, bool> result = map_.insert(item);
   if (!result.second) {
-    ScopedHandleVerifierInfo other = result.first->second;
-    base::debug::Alias(&other);
-    auto creation_stack = creation_stack_;
-    base::debug::Alias(&creation_stack);
-    CHECK(false);  // Attempt to start tracking already tracked handle.
+    // Attempt to start tracking already tracked handle.
+    ReportErrorOnScopedHandleOperation(creation_stack_, result.first->second);
   }
 }
 
-void ScopedHandleVerifier::StopTracking(HANDLE handle,
-                                        const void* owner,
-                                        const void* pc1,
-                                        const void* pc2) {
-  if (!enabled_)
-    return;
-  TRACE_EVENT0("base", "ScopedHandleVerifier::StopTracking");
+NOINLINE void ScopedHandleVerifier::StopTrackingImpl(HANDLE handle,
+                                                     const void* owner,
+                                                     const void* pc1,
+                                                     const void* pc2) {
+  TRACE_EVENT0("base", "ScopedHandleVerifier::StopTrackingImpl");
 
   AutoNativeLock lock(*lock_);
   HandleMap::iterator i = map_.find(handle);
   if (i == map_.end()) {
-    auto creation_stack = creation_stack_;
-    base::debug::Alias(&creation_stack);
-    CHECK(false);  // Attempting to close an untracked handle.
+    // Attempting to close an untracked handle.
+    ReportErrorOnScopedHandleOperation(creation_stack_);
   }
 
-  ScopedHandleVerifierInfo other = i->second;
-  if (other.owner != owner) {
-    base::debug::Alias(&other);
-    auto creation_stack = creation_stack_;
-    base::debug::Alias(&creation_stack);
-    CHECK(false);  // Attempting to close a handle not owned by opener.
+  if (i->second.owner != owner) {
+    // Attempting to close a handle not owned by opener.
+    ReportErrorOnScopedHandleOperation(creation_stack_, i->second);
   }
 
   map_.erase(i);
 }
 
-void ScopedHandleVerifier::Disable() {
-  enabled_ = false;
-}
-
-void ScopedHandleVerifier::OnHandleBeingClosed(HANDLE handle) {
-  if (!enabled_)
-    return;
-
+NOINLINE void ScopedHandleVerifier::OnHandleBeingClosedImpl(HANDLE handle) {
+  TRACE_EVENT0("base", "ScopedHandleVerifier::OnHandleBeingClosedImpl");
   if (closing_.Get())
     return;
 
-  TRACE_EVENT0("base", "ScopedHandleVerifier::OnHandleBeingClosed");
-
   AutoNativeLock lock(*lock_);
   HandleMap::iterator i = map_.find(handle);
-  if (i == map_.end())
-    return;
-
-  ScopedHandleVerifierInfo other = i->second;
-  base::debug::Alias(&other);
-  auto creation_stack = creation_stack_;
-  base::debug::Alias(&creation_stack);
-  CHECK(false);  // CloseHandle called on tracked handle.
-}
-
-HMODULE ScopedHandleVerifier::GetModule() const {
-  return CURRENT_MODULE();
+  if (i != map_.end()) {
+    // CloseHandle called on tracked handle.
+    ReportErrorOnScopedHandleOperation(creation_stack_, i->second);
+  }
 }
 
 HMODULE GetHandleVerifierModuleForTesting() {

base/win/scoped_handle_verifier.h

@@ -66,6 +66,12 @@ class [[clang::lto_visibility_public]] ScopedHandleVerifier {
  private:
   ~ScopedHandleVerifier();  // Not implemented.
 
+  void StartTrackingImpl(HANDLE handle, const void* owner, const void* pc1,
+                         const void* pc2);
+  void StopTrackingImpl(HANDLE handle, const void* owner, const void* pc1,
+                        const void* pc2);
+  void OnHandleBeingClosedImpl(HANDLE handle);
+
   static base::internal::LockImpl* GetLock();
   static void InstallVerifier();