Make module enumeration a low priority background task.

chrome/browser/win/enumerate_modules_model.cc

Index: chrome/browser/win/enumerate_modules_model.cc
diff --git a/chrome/browser/win/enumerate_modules_model.cc b/chrome/browser/win/enumerate_modules_model.cc
index 376e6b08bf18de878278b889079182c067c39854..9ccf53baffec64956a98827671dbcb1b51348dfa 100644
--- a/chrome/browser/win/enumerate_modules_model.cc
+++ b/chrome/browser/win/enumerate_modules_model.cc
@@ -60,6 +60,11 @@ static bool ModuleSort(const ModuleEnumerator::Module& a,
 
 namespace {
 
+// The default amount of time between module inspections. This prevents
+// certificate inspection and validation from using a large amount of CPU and
+// battery immediately after startup.
+const base::TimeDelta kDefaultPerModuleDelay = base::TimeDelta::FromSeconds(1);

[Patrick Monette, 2016/10/14 22:45:55]

constexpr

[chrisha, 2016/10/17 15:32:21]

Done.

+
 // A struct to help de-duping modules before adding them to the enumerated
 // modules vector.
 struct FindModule {
@@ -393,7 +398,8 @@ void ModuleEnumerator::NormalizeModule(Module* module) {
 
 ModuleEnumerator::ModuleEnumerator(EnumerateModulesModel* observer)
     : enumerated_modules_(nullptr),
-      observer_(observer) {
+      observer_(observer),
+      per_module_delay_(kDefaultPerModuleDelay) {
 }
 
 ModuleEnumerator::~ModuleEnumerator() {
@@ -408,12 +414,17 @@ void ModuleEnumerator::ScanNow(ModulesVector* list) {
   // scanning has not been finished before shutdown.
   BrowserThread::GetBlockingPool()->PostWorkerTaskWithShutdownBehavior(
       FROM_HERE,
-      base::Bind(&ModuleEnumerator::ScanImpl,
+      base::Bind(&ModuleEnumerator::ScanImplStart,
                  base::Unretained(this)),
       base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
 }
 
-void ModuleEnumerator::ScanImpl() {
+void ModuleEnumerator::SetPerModuleDelayToZero() {
+  // Set the delay to zero so the modules enumerate as quickly as possible.
+  per_module_delay_ = base::TimeDelta::FromSeconds(0);
+}
+
+void ModuleEnumerator::ScanImplStart() {
   base::TimeTicks start_time = base::TimeTicks::Now();
 
   // The provided destination for the enumerated modules should be empty, as it
@@ -444,6 +455,63 @@ void ModuleEnumerator::ScanImpl() {
   UMA_HISTOGRAM_TIMES("Conflicts.EnumerateWinsockModules",
                       checkpoint2 - checkpoint);
 
+  enumeration_total_time_ = base::TimeTicks::Now() - start_time;
+
+  // Post a delayed task to scan the first module. This forwards directly to
+  // ScanImplFinish if there are no modules to scan.
+  BrowserThread::GetBlockingPool()->PostDelayedWorkerTask(
+      FROM_HERE,
+      base::Bind(&ModuleEnumerator::ScanImplModule,
+                 base::Unretained(this),
+                 0),
+      per_module_delay_);
+}
+
+void ModuleEnumerator::ScanImplDelay(size_t index) {
+  // Bounce this over to a CONTINUE_ON_SHUTDOWN task in the same pool. This is
+  // necessary to prevent shutdown hangs while inspecting a module.
+  BrowserThread::GetBlockingPool()->PostWorkerTaskWithShutdownBehavior(
+      FROM_HERE,
+      base::Bind(&ModuleEnumerator::ScanImplModule,
+                 base::Unretained(this),
+                 index),
+      base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
+}
+
+void ModuleEnumerator::ScanImplModule(size_t index) {
+  while (index < enumerated_modules_->size()) {
+    base::TimeTicks start_time = base::TimeTicks::Now();
+    Module& entry = enumerated_modules_->at(index);
+    PopulateModuleInformation(&entry);
+    NormalizeModule(&entry);
+    CollapsePath(&entry);
+    base::TimeDelta elapsed = base::TimeTicks::Now() - start_time;
+    enumeration_inspection_time_ += elapsed;
+    enumeration_total_time_ += elapsed;
+
+    // If the delay has been set to zero then simply finish the rest of the
+    // enumeration in this already started task.
+    if (per_module_delay_.is_zero()) {

[Patrick Monette, 2016/10/14 22:45:55]

Reverse the conditional for simpler code? (Let you remove the else{} and
continue; statement.)

[chrisha, 2016/10/17 15:32:21]

Done.

+      ++index;
+      continue;
+    } else {
+      // Bounce back over to ScanImplDelay, which will bounce back to this
+      // function and inspect the next module.
+      BrowserThread::GetBlockingPool()->PostDelayedWorkerTask(
+          FROM_HERE,
+          base::Bind(&ModuleEnumerator::ScanImplDelay,
+                     base::Unretained(this),
+                     index + 1),
+          per_module_delay_);
+      return;
+    }
+  }
+
+  // Getting here means that all of the modules have been inspected.
+  return ScanImplFinish();
+}
+
+void ModuleEnumerator::ScanImplFinish() {
   // TODO(chrisha): Annotate any modules that are suspicious/bad.
 
   ReportThirdPartyMetrics();
@@ -451,8 +519,10 @@ void ModuleEnumerator::ScanImpl() {
   std::sort(enumerated_modules_->begin(),
             enumerated_modules_->end(), ModuleSort);
 
+  UMA_HISTOGRAM_TIMES("Conflicts.EnumerationInspectionTime",
+                      enumeration_inspection_time_);
   UMA_HISTOGRAM_TIMES("Conflicts.EnumerationTotalTime",
-                      base::TimeTicks::Now() - start_time);
+                      enumeration_total_time_);
 
   // Send a reply back on the UI thread. The |observer_| outlives this
   // enumerator, so posting a raw pointer is safe. This is done last as
@@ -482,10 +552,6 @@ void ModuleEnumerator::EnumerateLoadedModules() {
     Module entry;
     entry.type = LOADED_MODULE;
     entry.location = module.szExePath;
-    PopulateModuleInformation(&entry);
-
-    NormalizeModule(&entry);
-    CollapsePath(&entry);
     enumerated_modules_->push_back(entry);
   } while (::Module32Next(snap.Get(), &module));
 }
@@ -515,10 +581,6 @@ void ModuleEnumerator::ReadShellExtensions(HKEY parent) {
     Module entry;
     entry.type = SHELL_EXTENSION;
     entry.location = dll;
-    PopulateModuleInformation(&entry);
-
-    NormalizeModule(&entry);
-    CollapsePath(&entry);
     AddToListWithoutDuplicating(entry);
 
     ++registration;
@@ -542,13 +604,9 @@ void ModuleEnumerator::EnumerateWinsockModules() {
     wchar_t expanded[MAX_PATH];
     DWORD size = ExpandEnvironmentStrings(
         entry.location.c_str(), expanded, MAX_PATH);
-    if (size != 0 && size <= MAX_PATH) {
-      GetCertificateInfo(base::FilePath(expanded), &entry.cert_info);
-    }
+    if (size != 0 && size <= MAX_PATH)
+      entry.location = expanded;
     entry.version = base::IntToString16(layered_providers[i].version);
-
-    // Paths have already been collapsed.
-    NormalizeModule(&entry);
     AddToListWithoutDuplicating(entry);
   }
 }
@@ -756,20 +814,29 @@ void EnumerateModulesModel::MaybePostScanningTask() {
     BrowserThread::PostAfterStartupTask(
         FROM_HERE,
         BrowserThread::GetTaskRunnerForThread(BrowserThread::UI),
-        base::Bind(&EnumerateModulesModel::ScanNow, base::Unretained(this)));
+        base::Bind(&EnumerateModulesModel::ScanNow,
+                   base::Unretained(this),
+                   true));
     done = true;
   }
 }
 
-void EnumerateModulesModel::ScanNow() {
+void EnumerateModulesModel::ScanNow(bool background_mode) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // |module_enumerator_| is used as a lock to know whether or not there are
   // active/pending blocking pool tasks. If a module enumerator exists then a
   // scan is already underway. Otherwise, either no scan has been completed or
   // a scan has terminated.
-  if (module_enumerator_)
+  if (module_enumerator_) {
+    // If a scan is in progress and this request is for immediate results, then
+    // inform the background scan. This is done without any locks because the
+    // other thread only reads from the value that is being modified, and on
+    // Windows its an atomic write.

[Patrick Monette, 2016/10/14 22:45:55]

Is this really safe?

[chrisha, 2016/10/17 15:32:21]

Yeah, this is a standard trick. The variable is being used as a one-way switch.
Writing can only happen on one thread (UI), and reading can only happen on one
thread (a thread in the worker-pool). A 4-byte write/read is atomic, so the
reader will only ever see the original value, or the zero value, and nothing
else is possible.

(We'd require a lock if we had multiple threads doing read-update-write logic,
as they'd be racing with each other. It's even safe to have multiple writer
threads as long as the value they're writing doesn't depend on the current value
of the variable.)

[Patrick Monette, 2016/10/17 19:52:40]

Acknowledged.

+    if (!background_mode)
+      module_enumerator_->SetPerModuleDelayToZero();
     return;
+  }
 
   // Only allow a single scan per process lifetime. Immediately notify any
   // observers that the scan is complete. At this point |enumerated_modules_| is
@@ -781,6 +848,8 @@ void EnumerateModulesModel::ScanNow() {
 
   // ScanNow does not block, rather it simply schedules a task.
   module_enumerator_.reset(new ModuleEnumerator(this));
+  if (!background_mode)
+    module_enumerator_->SetPerModuleDelayToZero();
   module_enumerator_->ScanNow(&enumerated_modules_);
 }