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

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

This prevents the module enumeration from running repeatedly if the user revisits chrome://conflicts.

BUG=617176
Committed: https://crrev.com/82dfb2421849dacb4dd315dbfc34b469b2dc3e79
Cr-Commit-Position: refs/heads/master@{#421311}

[chrisha, 2016-09-26 20:48:04]

chrisha@chromium.org changed reviewers:
+ gab@chromium.org

[chrisha, 2016-09-26 20:48:04]

Here's that other CL I mentioned :)

PTAL?

[chrisha, 2016-09-26 20:52:29]

The CQ bit was checked by chrisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 20:53:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 22:01:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 22:01:39]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2370053002/diff/1/chrome/browser/win/enumerat...
File chrome/browser/win/enumerate_modules_model.cc (right):

https://codereview.chromium.org/2370053002/diff/1/chrome/browser/win/enumerat...
chrome/browser/win/enumerate_modules_model.cc:734: if
(!enumerated_modules_.empty()) {
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]

https://codereview.chromium.org/2370053002/diff/1/chrome/browser/win/enumerat...
File chrome/browser/win/enumerate_modules_model.cc (right):

https://codereview.chromium.org/2370053002/diff/1/chrome/browser/win/enumerat...
chrome/browser/win/enumerate_modules_model.cc:734: if
(!enumerated_modules_.empty()) {
On 2016/09/27 14:27:11, gab wrote:
> 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).

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]

https://codereview.chromium.org/2370053002/diff/1/chrome/browser/win/enumerat...
File chrome/browser/win/enumerate_modules_model.cc (right):

https://codereview.chromium.org/2370053002/diff/1/chrome/browser/win/enumerat...
chrome/browser/win/enumerate_modules_model.cc:391: enumerated_modules_->clear();
DCHECK(enumerated_modules_->empty()); instead with a comment about how this
works?

https://codereview.chromium.org/2370053002/diff/1/chrome/browser/win/enumerat...
chrome/browser/win/enumerate_modules_model.cc:734: if
(!enumerated_modules_.empty()) {
On 2016/09/27 14:49:39, chrisha (slow) wrote:
> On 2016/09/27 14:27:11, gab wrote:
> > 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).
> 
> 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)

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?

> , 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.

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]

One last look?

https://codereview.chromium.org/2370053002/diff/1/chrome/browser/win/enumerat...
File chrome/browser/win/enumerate_modules_model.cc (right):

https://codereview.chromium.org/2370053002/diff/1/chrome/browser/win/enumerat...
chrome/browser/win/enumerate_modules_model.cc:391: enumerated_modules_->clear();
On 2016/09/27 15:11:24, gab wrote:
> DCHECK(enumerated_modules_->empty()); instead with a comment about how this
> works?

Great idea. Done.

https://codereview.chromium.org/2370053002/diff/1/chrome/browser/win/enumerat...
chrome/browser/win/enumerate_modules_model.cc:734: if
(!enumerated_modules_.empty()) {
On 2016/09/27 15:11:24, gab wrote:
> On 2016/09/27 14:49:39, chrisha (slow) wrote:
> > On 2016/09/27 14:27:11, gab wrote:
> > > 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).
> > 
> > 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)
> 
> 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?
> 
> > , 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.
> 
> 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.

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.

[gab, 2016-09-27 17:11:18]

lgtm

[chrisha, 2016-09-27 19:15:54]

The CQ bit was checked by chrisha@chromium.org

[commit-bot: I haz the power, 2016-09-27 19:16:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 20:10:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-27 20:12:04]

Description was changed from

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

This prevents the module enumeration from running repeatedly if the user
revisits chrome://conflicts.

BUG=617176
==========

to

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

This prevents the module enumeration from running repeatedly if the user
revisits chrome://conflicts.

BUG=617176

Committed: https://crrev.com/82dfb2421849dacb4dd315dbfc34b469b2dc3e79
Cr-Commit-Position: refs/heads/master@{#421311}
==========

[commit-bot: I haz the power, 2016-09-27 20:12:05]

Patchset 3 (id:??) landed as
https://crrev.com/82dfb2421849dacb4dd315dbfc34b469b2dc3e79
Cr-Commit-Position: refs/heads/master@{#421311}