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/+/2067625Reviewed-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}

chrome/browser/chrome_browser_main_win.cc

@@ -427,7 +427,9 @@ 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));
+  *module_watcher =
+      ModuleWatcher::Create(base::BindRepeating(&OnModuleEvent),
+                            /* report_background_loaded_modules = */ true);
 }
 
 void ShowCloseBrowserFirstMessageBox() {

chrome/common/conflicts/module_watcher_win.cc

@@ -12,15 +12,18 @@
 #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"
 
@@ -111,6 +114,11 @@ 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(
@@ -131,13 +139,15 @@ void OnModuleEvent(ModuleWatcher::ModuleEventType event_type,
 
 // static
 std::unique_ptr<ModuleWatcher> ModuleWatcher::Create(
-    OnModuleEventCallback callback) {
+    OnModuleEventCallback callback,
+    bool report_background_loaded_modules) {
   {
     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();
+    g_module_watcher_instance =
+        new ModuleWatcher(report_background_loaded_modules);
   }
 
   // Initialization mustn't occur while holding |g_module_watcher_lock|.
@@ -146,6 +156,8 @@ std::unique_ptr<ModuleWatcher> ModuleWatcher::Create(
 }
 
 ModuleWatcher::~ModuleWatcher() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   // Done before acquiring |g_module_watcher_lock|.
   UnregisterDllNotificationCallback();
 
@@ -156,10 +168,17 @@ ModuleWatcher::~ModuleWatcher() {
   g_module_watcher_instance = nullptr;
 }
 
-ModuleWatcher::ModuleWatcher() {}
+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) {}
 
 // Initializes the ModuleWatcher instance.
 void ModuleWatcher::Initialize(OnModuleEventCallback callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   callback_ = std::move(callback);
   RegisterDllNotificationCallback();
 
@@ -226,13 +245,18 @@ void ModuleWatcher::EnumerateAlreadyLoadedModules(
   }
 }
 
-// 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_;
+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
@@ -240,7 +264,16 @@ void __stdcall ModuleWatcher::LoaderNotificationCallback(
     unsigned long notification_reason,
     const LDR_DLL_NOTIFICATION_DATA* notification_data,
     void* context) {
-  auto callback = GetCallbackForContext(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();
+    }
+  }
+
   if (!callback)
     return;
 
@@ -262,5 +295,7 @@ 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,6 +11,7 @@
 #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;
 
@@ -89,7 +90,9 @@ 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);
+  static std::unique_ptr<ModuleWatcher> Create(
+      OnModuleEventCallback callback,
+      bool report_background_loaded_modules);
 
   // This can be called on any thread. After destruction the |callback|
   // provided to the constructor will no longer be invoked with module events.
@@ -100,7 +103,7 @@ class ModuleWatcher {
   friend class ModuleWatcherTest;
 
   // Private to enforce Singleton semantics. See Create above.
-  ModuleWatcher();
+  explicit ModuleWatcher(bool report_background_loaded_modules);
 
   // Initializes the ModuleWatcher instance.
   void Initialize(OnModuleEventCallback callback);
@@ -119,9 +122,9 @@ class ModuleWatcher {
       scoped_refptr<base::SequencedTaskRunner> task_runner,
       OnModuleEventCallback callback);
 
-  // Helper function for retrieving the callback associated with a given
-  // LdrNotification context.
-  static OnModuleEventCallback GetCallbackForContext(void* context);
+  // Dumps the process if executed in a background sequence and
+  // |report_background_loaded_modules_| is true.
+  void DumpOnBackgroundLoadedModule();
 
   // The loader notification callback. This is actually
   // void CALLBACK LoaderNotificationCallback(
@@ -141,6 +144,17 @@ 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,7 +59,8 @@ class ModuleWatcherTest : public testing::Test {
 
   std::unique_ptr<ModuleWatcher> Create() {
     return ModuleWatcher::Create(
-        base::Bind(&ModuleWatcherTest::OnModuleEvent, base::Unretained(this)));
+        base::Bind(&ModuleWatcherTest::OnModuleEvent, base::Unretained(this)),
+        /* report_background_loaded_modules = */ false);
   }
 
   base::test::TaskEnvironment task_environment_;

chrome/common/conflicts/remote_module_watcher_win.cc

@@ -58,10 +58,12 @@ 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())));
+  module_watcher_ = ModuleWatcher::Create(
+      base::BindRepeating(
+          &OnModuleEvent, task_runner_,
+          base::BindRepeating(&RemoteModuleWatcher::HandleModuleEvent,
+                              weak_ptr_factory_.GetWeakPtr())),
+      /*report_background_loaded_modules=*/false);
 }
 
 void RemoteModuleWatcher::HandleModuleEvent(