Revert "Dump the process when a DLL is loaded in a background sequence"

This reverts commit f1191d7f.

Reason for revert: We're collected enough crash reports

Original change's description:
> Dump the process when a DLL is loaded in a background sequence
> 
> This is a temporary check that creates a crash report when a DLL
> is loaded on a thread with background priority. Once a sufficient
> number of crashes have been reported, this CL will be reverted.
> 
> Bug: 973868
> Change-Id: Ibb777eb64ada04454f50b080a54cb6d2fc371c9b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2067625
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: François Doray <fdoray@chromium.org>
> Commit-Queue: Patrick Monette <pmonette@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#744351}

TBR=avi@chromium.org,fdoray@chromium.org,pmonette@chromium.org

Change-Id: I4b707488bd7f21ca9252a0fbf5f4441585082d14
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 973868
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2075297Reviewed-by: Patrick Monette <pmonette@chromium.org>
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744700}

chrome/browser/chrome_browser_main_win.cc

@@ -427,9 +427,7 @@ void SetupModuleDatabase(std::unique_ptr<ModuleWatcher>* module_watcher) {
       FROM_HERE, base::BindOnce(&InitializeModuleDatabase,
                                 third_party_blocking_policy_enabled));
 
-  *module_watcher =
-      ModuleWatcher::Create(base::BindRepeating(&OnModuleEvent),
-                            /* report_background_loaded_modules = */ true);
+  *module_watcher = ModuleWatcher::Create(base::BindRepeating(&OnModuleEvent));
 }
 
 void ShowCloseBrowserFirstMessageBox() {

chrome/common/conflicts/module_watcher_win.cc

@@ -12,18 +12,15 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/debug/dump_without_crashing.h"
 #include "base/lazy_instance.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
-#include "base/rand_util.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
-#include "base/threading/platform_thread.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/win/scoped_handle.h"
 
@@ -114,11 +111,6 @@ constexpr wchar_t kNtDll[] = L"ntdll.dll";
 constexpr char kLdrRegisterDllNotification[] = "LdrRegisterDllNotification";
 constexpr char kLdrUnregisterDllNotification[] = "LdrUnregisterDllNotification";
 
-// It is currently estimated that around 10 DLLs are loaded in a background
-// sequence in Chrome. This number was chosen so that on average 1% of launches
-// will cause a process dump.
-constexpr int kMaxBackgroundLoadedDllCount = 1000;
-
 // Helper function for converting a UNICODE_STRING to a FilePath.
 base::FilePath ToFilePath(const UNICODE_STRING* str) {
   return base::FilePath(
@@ -139,15 +131,13 @@ void OnModuleEvent(ModuleWatcher::ModuleEventType event_type,
 
 // static
 std::unique_ptr<ModuleWatcher> ModuleWatcher::Create(
-    OnModuleEventCallback callback,
-    bool report_background_loaded_modules) {
+    OnModuleEventCallback callback) {
   {
     base::AutoLock lock(g_module_watcher_lock.Get());
     // If a ModuleWatcher already exists then bail out.
     if (g_module_watcher_instance)
       return nullptr;
-    g_module_watcher_instance =
-        new ModuleWatcher(report_background_loaded_modules);
+    g_module_watcher_instance = new ModuleWatcher();
   }
 
   // Initialization mustn't occur while holding |g_module_watcher_lock|.
@@ -156,8 +146,6 @@ std::unique_ptr<ModuleWatcher> ModuleWatcher::Create(
 }
 
 ModuleWatcher::~ModuleWatcher() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
   // Done before acquiring |g_module_watcher_lock|.
   UnregisterDllNotificationCallback();
 
@@ -168,17 +156,10 @@ ModuleWatcher::~ModuleWatcher() {
   g_module_watcher_instance = nullptr;
 }
 
-ModuleWatcher::ModuleWatcher(bool report_background_loaded_modules)
-    : report_background_loaded_modules_(report_background_loaded_modules),
-      background_loaded_dll_count_(0),
-      num_background_loaded_dll_report_(
-          base::RandInt(0, kMaxBackgroundLoadedDllCount)),
-      weak_ptr_factory_(this) {}
+ModuleWatcher::ModuleWatcher() {}
 
 // Initializes the ModuleWatcher instance.
 void ModuleWatcher::Initialize(OnModuleEventCallback callback) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
   callback_ = std::move(callback);
   RegisterDllNotificationCallback();
 
@@ -245,18 +226,13 @@ void ModuleWatcher::EnumerateAlreadyLoadedModules(
   }
 }
 
-void ModuleWatcher::DumpOnBackgroundLoadedModule() {
-  if (!report_background_loaded_modules_ ||
-      base::PlatformThread::GetCurrentThreadPriority() !=
-          base::ThreadPriority::BACKGROUND) {
-    return;
-  }
-
-  if (background_loaded_dll_count_++ == num_background_loaded_dll_report_) {
-    // DLL loaded on a thread with background priority. This can cause jank on
-    // the UI thread if it tries to acquire the loader lock.
-    base::debug::DumpWithoutCrashing();
-  }
+// static
+ModuleWatcher::OnModuleEventCallback ModuleWatcher::GetCallbackForContext(
+    void* context) {
+  base::AutoLock lock(g_module_watcher_lock.Get());
+  if (context != g_module_watcher_instance)
+    return OnModuleEventCallback();
+  return g_module_watcher_instance->callback_;
 }
 
 // static
@@ -264,16 +240,7 @@ void __stdcall ModuleWatcher::LoaderNotificationCallback(
     unsigned long notification_reason,
     const LDR_DLL_NOTIFICATION_DATA* notification_data,
     void* context) {
-  OnModuleEventCallback callback;
-  {
-    base::AutoLock lock(g_module_watcher_lock.Get());
-    if (context == g_module_watcher_instance) {
-      callback = g_module_watcher_instance->callback_;
-
-      g_module_watcher_instance->DumpOnBackgroundLoadedModule();
-    }
-  }
-
+  auto callback = GetCallbackForContext(context);
   if (!callback)
     return;
 
@@ -295,7 +262,5 @@ void __stdcall ModuleWatcher::LoaderNotificationCallback(
 }
 
 void ModuleWatcher::RunCallback(const ModuleEvent& event) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
   callback_.Run(event);
 }

chrome/common/conflicts/module_watcher_win.h

@@ -11,7 +11,6 @@
 #include "base/files/file_path.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
-#include "base/sequence_checker.h"
 
 class ModuleWatcherTest;
 
@@ -90,9 +89,7 @@ class ModuleWatcher {
   //
   // Only a single instance of a watcher may exist at any moment. This will
   // return nullptr when trying to create a second watcher.
-  static std::unique_ptr<ModuleWatcher> Create(
-      OnModuleEventCallback callback,
-      bool report_background_loaded_modules);
+  static std::unique_ptr<ModuleWatcher> Create(OnModuleEventCallback callback);
 
   // This can be called on any thread. After destruction the |callback|
   // provided to the constructor will no longer be invoked with module events.
@@ -103,7 +100,7 @@ class ModuleWatcher {
   friend class ModuleWatcherTest;
 
   // Private to enforce Singleton semantics. See Create above.
-  explicit ModuleWatcher(bool report_background_loaded_modules);
+  ModuleWatcher();
 
   // Initializes the ModuleWatcher instance.
   void Initialize(OnModuleEventCallback callback);
@@ -122,9 +119,9 @@ class ModuleWatcher {
       scoped_refptr<base::SequencedTaskRunner> task_runner,
       OnModuleEventCallback callback);
 
-  // Dumps the process if executed in a background sequence and
-  // |report_background_loaded_modules_| is true.
-  void DumpOnBackgroundLoadedModule();
+  // Helper function for retrieving the callback associated with a given
+  // LdrNotification context.
+  static OnModuleEventCallback GetCallbackForContext(void* context);
 
   // The loader notification callback. This is actually
   // void CALLBACK LoaderNotificationCallback(
@@ -144,17 +141,6 @@ class ModuleWatcher {
   // Used by the DllNotification mechanism.
   void* dll_notification_cookie_ = nullptr;
 
-  // Indicates if modules loaded in a background sequence should be reported.
-  const bool report_background_loaded_modules_;
-
-  // The count of DLL that were loaded in a background sequence.
-  int background_loaded_dll_count_;
-
-  // The number of background loaded DLL that will cause a process dump.
-  const int num_background_loaded_dll_report_;
-
-  SEQUENCE_CHECKER(sequence_checker_);
-
   base::WeakPtrFactory<ModuleWatcher> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(ModuleWatcher);

chrome/common/conflicts/module_watcher_win_unittest.cc

@@ -59,8 +59,7 @@ class ModuleWatcherTest : public testing::Test {
 
   std::unique_ptr<ModuleWatcher> Create() {
     return ModuleWatcher::Create(
-        base::Bind(&ModuleWatcherTest::OnModuleEvent, base::Unretained(this)),
-        /* report_background_loaded_modules = */ false);
+        base::Bind(&ModuleWatcherTest::OnModuleEvent, base::Unretained(this)));
   }
 
   base::test::TaskEnvironment task_environment_;

chrome/common/conflicts/remote_module_watcher_win.cc

@@ -58,12 +58,10 @@ void RemoteModuleWatcher::InitializeOnTaskRunner(
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
   module_event_sink_.Bind(std::move(remote_sink));
-  module_watcher_ = ModuleWatcher::Create(
-      base::BindRepeating(
-          &OnModuleEvent, task_runner_,
-          base::BindRepeating(&RemoteModuleWatcher::HandleModuleEvent,
-                              weak_ptr_factory_.GetWeakPtr())),
-      /*report_background_loaded_modules=*/false);
+  module_watcher_ = ModuleWatcher::Create(base::BindRepeating(
+      &OnModuleEvent, task_runner_,
+      base::BindRepeating(&RemoteModuleWatcher::HandleModuleEvent,
+                          weak_ptr_factory_.GetWeakPtr())));
 }
 
 void RemoteModuleWatcher::HandleModuleEvent(