Stack sampling profiler: fix leaf function unwinding and remove blacklisting

base/profiler/win32_stack_frame_unwinder.cc

Index: base/profiler/win32_stack_frame_unwinder.cc
diff --git a/base/profiler/win32_stack_frame_unwinder.cc b/base/profiler/win32_stack_frame_unwinder.cc
index 74075cbec5f0e0bfb41d692e621b5382b2b81659..0e7f5b252e294280ff77819f78760ce597b396af 100644
--- a/base/profiler/win32_stack_frame_unwinder.cc
+++ b/base/profiler/win32_stack_frame_unwinder.cc
@@ -7,10 +7,6 @@
 #include <windows.h>
 #include <utility>
 
-#include "base/containers/hash_tables.h"
-#include "base/memory/singleton.h"
-#include "base/stl_util.h"
-
 namespace base {
 
 // Win32UnwindFunctions -------------------------------------------------------
@@ -104,58 +100,6 @@ ScopedModuleHandle Win32UnwindFunctions::GetModuleForProgramCounter(
   return ScopedModuleHandle(module_handle);
 }
 
-// LeafUnwindBlacklist --------------------------------------------------------
-
-// Records modules that are known to have functions that violate the Microsoft
-// x64 calling convention and would be dangerous to manually unwind if
-// encountered as the last frame on the call stack. Functions like these have
-// been observed in injected third party modules that either do not provide
-// function unwind information, or do not provide the required function prologue
-// and epilogue. The former case was observed in several AV products and the
-// latter in a WndProc function associated with Actual Window
-// Manager/aimemb64.dll. See https://crbug.com/476422.
-class LeafUnwindBlacklist {
- public:
-  static LeafUnwindBlacklist* GetInstance();
-
-  // Returns true if |module| has been blacklisted.
-  bool IsBlacklisted(const void* module) const;
-
-  // Records |module| for blacklisting.
-  void BlacklistModule(const void* module);
-
- private:
-  friend struct DefaultSingletonTraits<LeafUnwindBlacklist>;
-
-  LeafUnwindBlacklist();
-  ~LeafUnwindBlacklist();
-
-  // The set of modules known to have functions that violate the Microsoft x64
-  // calling convention.
-  base::hash_set<const void*> blacklisted_modules_;
-
-  DISALLOW_COPY_AND_ASSIGN(LeafUnwindBlacklist);
-};
-
-// static
-LeafUnwindBlacklist* LeafUnwindBlacklist::GetInstance() {
-  // Leaky for performance reasons.
-  return Singleton<LeafUnwindBlacklist,
-                   LeakySingletonTraits<LeafUnwindBlacklist>>::get();
-}
-
-bool LeafUnwindBlacklist::IsBlacklisted(const void* module) const {
-  return ContainsKey(blacklisted_modules_, module);
-}
-
-void LeafUnwindBlacklist::BlacklistModule(const void* module) {
-  CHECK(module);
-  blacklisted_modules_.insert(module);
-}
-
-LeafUnwindBlacklist::LeafUnwindBlacklist() {}
-LeafUnwindBlacklist::~LeafUnwindBlacklist() {}
-
 }  // namespace
 
 // Win32StackFrameUnwinder ----------------------------------------------------
@@ -172,8 +116,6 @@ Win32StackFrameUnwinder::~Win32StackFrameUnwinder() {}
 bool Win32StackFrameUnwinder::TryUnwind(CONTEXT* context,
                                         ScopedModuleHandle* module) {
 #ifdef _WIN64
-  CHECK(!at_top_frame_ || unwind_info_present_for_all_frames_);
-
   ScopedModuleHandle frame_module =
       unwind_functions_->GetModuleForProgramCounter(context->Rip);
   if (!frame_module.IsValid()) {
@@ -203,74 +145,25 @@ bool Win32StackFrameUnwinder::TryUnwind(CONTEXT* context,
                                      context);
     at_top_frame_ = false;
   } else {
-    // RtlLookupFunctionEntry didn't find unwind information. This could mean
-    // the code at the instruction pointer is in:
-    //
-    // 1. a true leaf function (i.e. a function that neither calls a function,
-    //    nor allocates any stack space itself) in which case the return
-    //    address is at RSP, or
-    //
-    // 2. a function that doesn't adhere to the Microsoft x64 calling
-    //    convention, either by not providing the required unwind information,
-    //    or by not having the prologue or epilogue required for unwinding;
-    //    this case has been observed in crash data in injected third party
-    //    DLLs.
-    //
-    // In valid code, case 1 can only occur (by definition) as the last frame
-    // on the stack. This happens in about 5% of observed stacks and can
-    // easily be unwound by popping RSP and using it as the next frame's
-    // instruction pointer.
-    //
-    // Case 2 can occur anywhere on the stack, and attempting to unwind the
-    // stack will result in treating whatever value happens to be on the stack
-    // at RSP as the next frame's instruction pointer. This is certainly wrong
-    // and very likely to lead to crashing by deferencing invalid pointers in
-    // the next RtlVirtualUnwind call.
-    //
-    // If we see case 2 at a location not the last frame, and all the previous
-    // frame had valid unwind information, then this is definitely bad code.
-    // We blacklist the module as untrustable for unwinding if we encounter a
-    // function in it that doesn't have unwind information.
-
     if (at_top_frame_) {
       at_top_frame_ = false;
 
-      // We are at the end of the stack. It's very likely that we're in case 1
-      // since the vast majority of code adheres to the Microsoft x64 calling
-      // convention. But there's a small chance we might be unlucky and be in
-      // case 2. If this module is known to have bad code according to the
-      // leaf unwind blacklist, stop here, otherwise manually unwind.
-      if (LeafUnwindBlacklist::GetInstance()->IsBlacklisted(
-              reinterpret_cast<const void*>(image_base))) {
-        return false;
-      }
-
-      context->Rip = context->Rsp;
+      // This is a leaf function (i.e. a function that neither calls a function,
+      // nor allocates any stack space itself) so the return address is at RSP.
+      context->Rip = *reinterpret_cast<DWORD64*>(context->Rsp);
       context->Rsp += 8;
-      unwind_info_present_for_all_frames_ = false;
     } else {
-      // We're not at the end of the stack. This frame is untrustworthy and we
-      // can't safely unwind from here.
-      if (!image_base) {
-        // A null image_base means that the the last unwind produced an invalid
-        // instruction pointer. This has been observed where unwind information
-        // was present for a function but was inconsistent with the actual
-        // function code, in particular in BoringSSL. See
-        // https://crbug.com/542919.
-      } else if (unwind_info_present_for_all_frames_) {
-        // Unwind information was present for all previous frames, so we can
-        // be confident this is case 2. Record the module to be blacklisted.
-        LeafUnwindBlacklist::GetInstance()->BlacklistModule(
-            reinterpret_cast<const void *>(image_base));
-      } else {
-        // We started off on a function without unwind information. It's very
-        // likely that all frames up to this point have been good, and this
-        // frame is case 2. But it's possible that the initial frame was case
-        // 2 but hadn't been blacklisted yet, and we've started to go off into
-        // the weeds. Since we can't be sure, just bail out without
-        // blacklisting the module; chances are we'll later encounter the same
-        // function on a stack with full unwind information.
-      }
+      // In theory we shouldn't get here, as it means we've encountered a
+      // function without unwind information below the top of the stack, which
+      // is forbidden by the Microsoft x64 calling convention.
+      //
+      // The one known case in Chrome code that executes this path occurs
+      // because of BoringSSL unwind information inconsistent with the actual
+      // function code. See https://crbug.com/542919.
+      //
+      // Note that dodgy third-party generated code that otherwise would enter
+      // this path should be caught by the module check above, since the code
+      // typically is located outside of a module.
       return false;
     }
   }
@@ -286,7 +179,6 @@ bool Win32StackFrameUnwinder::TryUnwind(CONTEXT* context,
 Win32StackFrameUnwinder::Win32StackFrameUnwinder(
     scoped_ptr<UnwindFunctions> unwind_functions)
     : at_top_frame_(true),
-      unwind_info_present_for_all_frames_(true),
       unwind_functions_(std::move(unwind_functions)) {}
 
 }  // namespace base