[win] Make conflict module enumeration occur at most once.

chrome/browser/win/enumerate_modules_model.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/win/enumerate_modules_model.h"
 
 #include <softpub.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <tlhelp32.h>
@@ skipping 370 matching lines @@
   BrowserThread::GetBlockingPool()->PostWorkerTaskWithShutdownBehavior(
       FROM_HERE,
       base::Bind(&ModuleEnumerator::ScanImpl,
                  base::Unretained(this)),
       base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
 }
 
 void ModuleEnumerator::ScanImpl() {
   base::TimeTicks start_time = base::TimeTicks::Now();
 
   enumerated_modules_->clear();

[gab, 2016/09/27 15:11:24]

DCHECK(enumerated_modules_->empty()); instead with a comment about how this
works?

[chrisha, 2016/09/27 15:29:07]

Great idea. Done.

 
   // Make sure the path mapping vector is setup so we can collapse paths.
   PreparePathMappings();
 
   // Enumerating loaded modules must happen first since the other types of
   // modules check for duplication against the loaded modules.
   base::TimeTicks checkpoint = base::TimeTicks::Now();
   EnumerateLoadedModules();
   base::TimeTicks checkpoint2 = base::TimeTicks::Now();
   UMA_HISTOGRAM_TIMES("Conflicts.EnumerateLoadedModules",
@@ skipping 315 matching lines @@
         FROM_HERE,
         BrowserThread::GetTaskRunnerForThread(BrowserThread::UI),
         base::Bind(&EnumerateModulesModel::ScanNow, base::Unretained(this)));
     done = true;
   }
 }
 
 void EnumerateModulesModel::ScanNow() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  // If a module enumerator exists then a scan is already underway.
+  // If a module enumerator exists then a scan is already underway. The client
+  // will receive a notification from that thread.
   if (module_enumerator_)
     return;
 
+  // Only allow a single scan per process lifetime. Immediately notify any
+  // observers that the scan is complete.
+  if (!enumerated_modules_.empty()) {

[gab, 2016/09/27 14:27:11]

I'm not convinced usage of |enumerated_modules_| is thread-safe at the moment
and this addition makes me doubt this further.

It's currently passed as a raw pointer to the BlockingPool which accesses it
from a task running on the BlockingPool. There is some sequencing through the
ScanNow() -> DoneScanning() post task sequence which *may* make this safe, but
even then I don't think it was safe (prior to this CL) if ScanNow() is called
multiple times (the ScanNow() tasks are posted in parallel and therefore race
with each other and tasks on the UI thread).

The way to do this right IMO would be to have |enumerated_modules_| owned and
used on the UI thread only. The BlockingPool should return the computed list via
std::unique_ptr (handing ownership via the DoneScanning() callback).

[chrisha, 2016/09/27 14:49:39]

Greg and I had extensive discussions on the thread safety in the original
review. I had used locks to make it plainly obvious that things were threadsafe,
but he convinced me to remove them :)

The sequencing through ScanNow/DoneScanning doesn't *maybe* make it safe, it
guarantees safety.

module_enumerator_ acts as a lock for enumerated_modules_, effectively. It is
only modified/read on the UI thread, so it doesn't need a lock itself.

When module_enumerator_ is non-null, this means that a blocking pool worker is
pending/active, and that it is unsafe to access enumerated_modules_. If
module_enumerator_ is null, then no such worker is pending/active, and
enumerated_modules_ is safe to be accessed on the UI thread.

Multiple tasks posted on the UI thread can't "race" with each other, as exactly
one of them will run first (it's a single thread, after all), and allocate a
module_enumerator_. The others will then find a non-null module_enumerator_ and
bail out without looking at enumerated_modules_. "DoneScanning" will be called
exactly once, and module_enuemrator_ will then be set back to null. Any
subsequent or pending calls to ScanNow will then find a non-empty
enumerated_modules_ and bail out early as well.

[gab, 2016/09/27 15:11:24]

Indeed, I meant the BlockingPool tasks "racing each other" not tasks on the UI
thread :-), I realize now that this isn't possible because of the null check on
the module_enumerator_. This isn't obvious to a reader though, should we add
stronger comments to highlight this?
Got it, thanks for the explanation.

FWIW, I think it would be just as efficient to return via unique_ptr ownership
in the callback (no copies just passing ownership) instead of sharing the data
structure like this and would be more readable but I won't block this CL on
this.

[chrisha, 2016/09/27 15:29:07]

There's some other cleanup I want to do as well (move ModuleEnumerator to its
own .h/.cc), so I'll defer on that for now. Agreed that the passing ownership is
more clear.

Added more comments in ScanNow.

+    FOR_EACH_OBSERVER(Observer, observers_, OnScanCompleted());
+    return;
+  }
+
   // ScanNow does not block, rather it simply schedules a task.
   module_enumerator_.reset(new ModuleEnumerator(this));
   module_enumerator_->ScanNow(&enumerated_modules_);
 }
 
 base::ListValue* EnumerateModulesModel::GetModuleList() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // If a |module_enumerator_| is still around then scanning has not yet
   // completed, and it is unsafe to read from |enumerated_modules_|.
@@ skipping 120 matching lines @@
   module_enumerator_.reset();
 
   UMA_HISTOGRAM_COUNTS_100("Conflicts.SuspectedBadModules",
                            suspected_bad_modules_detected_);
   UMA_HISTOGRAM_COUNTS_100("Conflicts.ConfirmedBadModules",
                            confirmed_bad_modules_detected_);
 
   // Forward the callback to any registered observers.
   FOR_EACH_OBSERVER(Observer, observers_, OnScanCompleted());
 }