Make RunLoop::Quit() thread-safe.

Make RunLoop::Quit() thread-safe.

It was already safe to call it off-sequence as of r472835.

But making it thread-safe is key to enabling use cases like:

  RunLoop run_loop_;
  <post a task binding run_loop_.QuitClosure() to task scheduler>
  run_loop_.Run();

e.g. happens in https://codereview.chromium.org/2881143002/ without this CL

Run() and Quit() are racing in this situation as the posted task may run
at any point after being posted. FYI, RunLoop() already supports being Quit()
prior to being Run() so the race resolving either way is fine, it just needs
to be thread-safe.

BUG=722537
Review-Url: https://codereview.chromium.org/2892993002
Cr-Commit-Position: refs/heads/master@{#473378}
Committed: https://chromium.googlesource.com/chromium/src/+/cf5e4ce0c0eff930821b2459ee03d161ab4d6550

[gab, 2017-05-19 17:37:44]

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

[gab, 2017-05-19 17:37:55]

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

[gab, 2017-05-19 17:37:56]

Dana PTaL, thanks!

[commit-bot: I haz the power, 2017-05-19 17:38:37]

Dry run: CQ is trying da patch.

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

[gab, 2017-05-19 17:39:03]

Description was changed from

==========
Make RunLoop::Quit() thread-safe.

It was already safe to call it off-sequence as of r472835.

But making it thread-safe is key to enabling use cases like:

  RunLoop run_loop_;
  <post a task binding run_loop_.QuitClosure() to task scheduler>
  run_loop_.Run();

Run() and Quit() are racing in this situation as the posted task may run
at any point after being posted.

BUG=722537
==========

to

==========
Make RunLoop::Quit() thread-safe.

It was already safe to call it off-sequence as of r472835.

But making it thread-safe is key to enabling use cases like:

  RunLoop run_loop_;
  <post a task binding run_loop_.QuitClosure() to task scheduler>
  run_loop_.Run();

Run() and Quit() are racing in this situation as the posted task may run
at any point after being posted. FYI, RunLoop() already supports being Quit()
prior to being Run() so the race resolving either way is fine, it just needs
to be thread-safe.

BUG=722537
==========

[danakj, 2017-05-19 18:11:30]

https://codereview.chromium.org/2892993002/diff/1/base/run_loop.cc
File base/run_loop.cc (right):

https://codereview.chromium.org/2892993002/diff/1/base/run_loop.cc#newcode27
base/run_loop.cc:27: if (task_runner->RunsTasksInCurrentSequence()) {
This if (on thread) A else post A pattern always irks me a bit.

What do you think of a QuitSoon() that is thread safe, vs Quit() that is not.
And QuitSoon always posts? Then you get consistent behaviour about leaving the
current stack frame from each method.

[danakj, 2017-05-19 18:53:34]

https://codereview.chromium.org/2892993002/diff/1/base/run_loop_unittest.cc
File base/run_loop_unittest.cc (right):

https://codereview.chromium.org/2892993002/diff/1/base/run_loop_unittest.cc#n...
base/run_loop_unittest.cc:130: other_sequence->PostTask(FROM_HERE,
run_loop_.QuitClosure());
These tests all verify QuitClosure() but not Quit() directly. So they are
testing the thread hop in the proxy function only.

[gab, 2017-05-19 19:38:17]

Tests added PTanL

https://codereview.chromium.org/2892993002/diff/1/base/run_loop.cc
File base/run_loop.cc (right):

https://codereview.chromium.org/2892993002/diff/1/base/run_loop.cc#newcode27
base/run_loop.cc:27: if (task_runner->RunsTasksInCurrentSequence()) {
On 2017/05/19 18:11:30, danakj wrote:
> This if (on thread) A else post A pattern always irks me a bit.
> 
> What do you think of a QuitSoon() that is thread safe, vs Quit() that is not.
> And QuitSoon always posts? Then you get consistent behaviour about leaving the
> current stack frame from each method.

Discussed offline, won't do that because the goal of this CL is to make
migration from FILE thread to TaskScheduler as seamless as possible and
requiring to update tests unnecessarily doesn't fit that requirement.

https://codereview.chromium.org/2892993002/diff/1/base/run_loop_unittest.cc
File base/run_loop_unittest.cc (right):

https://codereview.chromium.org/2892993002/diff/1/base/run_loop_unittest.cc#n...
base/run_loop_unittest.cc:130: other_sequence->PostTask(FROM_HERE,
run_loop_.QuitClosure());
On 2017/05/19 18:53:34, danakj wrote:
> These tests all verify QuitClosure() but not Quit() directly. So they are
> testing the thread hop in the proxy function only.

Done.

[danakj, 2017-05-19 19:38:53]

LGTM

[gab, 2017-05-19 19:39:32]

Description was changed from

==========
Make RunLoop::Quit() thread-safe.

It was already safe to call it off-sequence as of r472835.

But making it thread-safe is key to enabling use cases like:

  RunLoop run_loop_;
  <post a task binding run_loop_.QuitClosure() to task scheduler>
  run_loop_.Run();

Run() and Quit() are racing in this situation as the posted task may run
at any point after being posted. FYI, RunLoop() already supports being Quit()
prior to being Run() so the race resolving either way is fine, it just needs
to be thread-safe.

BUG=722537
==========

to

==========
Make RunLoop::Quit() thread-safe.

It was already safe to call it off-sequence as of r472835.

But making it thread-safe is key to enabling use cases like:

  RunLoop run_loop_;
  <post a task binding run_loop_.QuitClosure() to task scheduler>
  run_loop_.Run();

e.g. happens in https://codereview.chromium.org/2881143002/ without this CL

Run() and Quit() are racing in this situation as the posted task may run
at any point after being posted. FYI, RunLoop() already supports being Quit()
prior to being Run() so the race resolving either way is fine, it just needs
to be thread-safe.

BUG=722537
==========

[gab, 2017-05-19 19:39:56]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2017-05-19 19:41:17]

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, 2017-05-19 22:57:00]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495222796473200,
"parent_rev": "eaf7cc9d2e262724f33be38d880fcb7eaa97100d", "commit_rev":
"cf5e4ce0c0eff930821b2459ee03d161ab4d6550"}

[commit-bot: I haz the power, 2017-05-19 22:57:15]

Description was changed from

==========
Make RunLoop::Quit() thread-safe.

It was already safe to call it off-sequence as of r472835.

But making it thread-safe is key to enabling use cases like:

  RunLoop run_loop_;
  <post a task binding run_loop_.QuitClosure() to task scheduler>
  run_loop_.Run();

e.g. happens in https://codereview.chromium.org/2881143002/ without this CL

Run() and Quit() are racing in this situation as the posted task may run
at any point after being posted. FYI, RunLoop() already supports being Quit()
prior to being Run() so the race resolving either way is fine, it just needs
to be thread-safe.

BUG=722537
==========

to

==========
Make RunLoop::Quit() thread-safe.

It was already safe to call it off-sequence as of r472835.

But making it thread-safe is key to enabling use cases like:

  RunLoop run_loop_;
  <post a task binding run_loop_.QuitClosure() to task scheduler>
  run_loop_.Run();

e.g. happens in https://codereview.chromium.org/2881143002/ without this CL

Run() and Quit() are racing in this situation as the posted task may run
at any point after being posted. FYI, RunLoop() already supports being Quit()
prior to being Run() so the race resolving either way is fine, it just needs
to be thread-safe.

BUG=722537

Review-Url: https://codereview.chromium.org/2892993002
Cr-Commit-Position: refs/heads/master@{#473378}
Committed:
https://chromium.googlesource.com/chromium/src/+/cf5e4ce0c0eff930821b2459ee03...
==========

[commit-bot: I haz the power, 2017-05-19 22:57:16]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/cf5e4ce0c0eff930821b2459ee03...