[Sync] Refactor ModelSafeWorker::DoWorkAndWaitUntilDone() to avoid code duplication.

[Sync] Refactor ModelSafeWorker::DoWorkAndWaitUntilDone() to avoid code duplication.

Before this CL, logic to wait for a WorkCallback to run or be
abandoned was duplicated in multiple
ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() implementations.

With this CL, ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() is
replaced with ModelSafeWorker::ScheduleWork(). Implementations
are merely responsible for scheduling the callback that they
receive; they don't have to wait for it to run or be abandoned.

The logic to wait for the callback to run or be abandoned is moved
into ModelSafeWorker::DoWorkAndWaitUntilDone().
ModelSafeWorker::DoWorkAndWaitUntilDone() calls
ModelSafeWorker::ScheduleWork(). Then, it waits until either the
callback that it passed to ModelSafeWorker::ScheduleWork() is deleted
(may or may not have run) or ModelSafeWorker::RequestStop() is called
before the callback has started to run. Additionnaly, calling
ModelSafeWorker::RequestStop() transforms into no-op further
invocations of callbacks passed to ModelSafeWorker::ScheduleWork().

Unblocking ModelSafeWorker::DoWorkAndWaitUntilDone() when
ModelSafeWorker::RequestStop() is called is important to
prevent deadlocks in workers that schedule work on the UI
thread. Indeed, in Chrome, no tasks are scheduled on the UI
thread after ModelSafeWorker::RequestStop() is called and
pending UI tasks are not scheduled until after the sync thread
is joined.

BUG=663600
Review-Url: https://codereview.chromium.org/2782573002
Cr-Original-Commit-Position: refs/heads/master@{#463413}
Committed: https://chromium.googlesource.com/chromium/src/+/316c08f3692150ac1c416c67391768622cc6a77f
Review-Url: https://codereview.chromium.org/2782573002
Cr-Commit-Position: refs/heads/master@{#465229}
Committed: https://chromium.googlesource.com/chromium/src/+/eeeb0d0ec28227aba875195e0f20076414fc4596

[fdoray, 2017-03-28 12:57:07]

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

[commit-bot: I haz the power, 2017-03-28 12:57:19]

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, 2017-03-28 13:05:08]

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

[commit-bot: I haz the power, 2017-03-28 13:05:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[fdoray, 2017-03-28 13:14:26]

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

[commit-bot: I haz the power, 2017-03-28 13:14:52]

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, 2017-03-28 13:58:18]

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

[commit-bot: I haz the power, 2017-03-28 13:58:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-03-29 13:40:47]

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

[commit-bot: I haz the power, 2017-03-29 13:41:03]

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, 2017-03-29 13:56:44]

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

[commit-bot: I haz the power, 2017-03-29 13:56:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-03-30 12:45:25]

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

[commit-bot: I haz the power, 2017-03-30 12:45:52]

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, 2017-03-30 12:56:18]

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

[commit-bot: I haz the power, 2017-03-30 12:56:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-03-30 13:15:19]

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

[commit-bot: I haz the power, 2017-03-30 13:15:42]

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

[fdoray, 2017-03-30 13:16:06]

Description was changed from

==========
[Sync] Unblock ModelSafeWorker::DoWorkAndWaitUntilDone() when RequestStop() is
called an no WorkCallback is running.

Before this CL, logic to wait for a WorkCallback to run or be
abandoned was duplicated in multiple
ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() implementations.

With this CL, ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() is
replaced with ModelSafeWorker::ScheduleWork(). Implementations
are merely responsible for scheduling the callback that they
receive; they don't have to wait for it to run or be abandoned.

The logic to wait for the callback to run or be abandoned is moved
into ModelSafeWorker::DoWorkAndWaitUntilDone().
ModelSafeWorker::DoWorkAndWaitUntilDone() calls
ModelSafeWorker::ScheduleWork(). Then, it waits until either the
callback that it passed to ModelSafeWorker::ScheduleWork() is deleted
(may or may not have run) or ModelSafeWorker::RequestStop() is called
before the callback has started to run. Additionnaly, calling
ModelSafeWorker::RequestStop() transforms into no-op further
invocations of callbacks passed to ModelSafeWorker::ScheduleWork().

Unblocking ModelSafeWorker::DoWorkAndWaitUntilDone() when
ModelSafeWorker::RequestStop() is called is important to
prevent deadlocks in workers that schedule work on the UI
thread. Indeed, in Chrome, no tasks are scheduled on the UI
thread after ModelSafeWorker::RequestStop() is called and
pending UI tasks are not scheduled until after the sync thread
is joined.

BUG=663600
==========

to

==========
[Sync] Refactor ModelSafeWorker::DoWorkAndWaitUntilDone() to avoid code
duplication.

Before this CL, logic to wait for a WorkCallback to run or be
abandoned was duplicated in multiple
ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() implementations.

With this CL, ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() is
replaced with ModelSafeWorker::ScheduleWork(). Implementations
are merely responsible for scheduling the callback that they
receive; they don't have to wait for it to run or be abandoned.

The logic to wait for the callback to run or be abandoned is moved
into ModelSafeWorker::DoWorkAndWaitUntilDone().
ModelSafeWorker::DoWorkAndWaitUntilDone() calls
ModelSafeWorker::ScheduleWork(). Then, it waits until either the
callback that it passed to ModelSafeWorker::ScheduleWork() is deleted
(may or may not have run) or ModelSafeWorker::RequestStop() is called
before the callback has started to run. Additionnaly, calling
ModelSafeWorker::RequestStop() transforms into no-op further
invocations of callbacks passed to ModelSafeWorker::ScheduleWork().

Unblocking ModelSafeWorker::DoWorkAndWaitUntilDone() when
ModelSafeWorker::RequestStop() is called is important to
prevent deadlocks in workers that schedule work on the UI
thread. Indeed, in Chrome, no tasks are scheduled on the UI
thread after ModelSafeWorker::RequestStop() is called and
pending UI tasks are not scheduled until after the sync thread
is joined.

BUG=663600
==========

[commit-bot: I haz the power, 2017-03-30 15:40:25]

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

[commit-bot: I haz the power, 2017-03-30 15:40:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-03-31 18:38:55]

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

[commit-bot: I haz the power, 2017-03-31 18:45:28]

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, 2017-03-31 18:55:07]

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

[commit-bot: I haz the power, 2017-03-31 18:55:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fdoray, 2017-03-31 20:04:16]

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

[commit-bot: I haz the power, 2017-03-31 20:05:08]

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, 2017-03-31 22:47:40]

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

[commit-bot: I haz the power, 2017-03-31 22:47:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-04-03 14:59:12]

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

[commit-bot: I haz the power, 2017-04-03 14:59:41]

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, 2017-04-03 15:29:21]

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

[commit-bot: I haz the power, 2017-04-03 15:29:22]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[fdoray, 2017-04-03 17:56:57]

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

[commit-bot: I haz the power, 2017-04-03 17:57:57]

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, 2017-04-03 19:15:40]

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

[commit-bot: I haz the power, 2017-04-03 19:15:41]

Dry run: This issue passed the CQ dry run.

[fdoray, 2017-04-03 19:23:26]

fdoray@chromium.org changed reviewers:
+ brettw@chromium.org

[fdoray, 2017-04-03 19:23:27]

PTAL

This CL replaces https://codereview.chromium.org/2757193003/. It has less
complexity and less code duplication + better tests.

[brettw, 2017-04-05 23:28:09]

I don't have much experience with these sync classes. Is there a sync reviewer
you could ask and then have me do owners review for history?

[skym, 2017-04-06 00:34:53]

skym@chromium.org changed reviewers:
+ skym@chromium.org

[skym, 2017-04-06 00:34:55]

The refactor lgtm.

But reading this CLs comment, it isn't clear to me why this is fixing the
bug/crash. Do we even know which model type/worker implementation was
crashing/faulty? Looking at the a random crash I see the history worker is also
active. Were you able to write a unit tests against the old history
implementation that hit the issue?

https://codereview.chromium.org/2782573002/diff/160001/components/history/cor...
File components/history/core/browser/history_model_worker.cc (right):

https://codereview.chromium.org/2782573002/diff/160001/components/history/cor...
components/history/core/browser/history_model_worker.cc:82: FROM_HERE,
base::Bind(&PostWorkerTask, history_service_, std::move(work),
Is the Closure implicitly converted to a OnceClosure here? How does that work?

https://codereview.chromium.org/2782573002/diff/160001/components/history/cor...
File components/history/core/browser/history_model_worker.h (right):

https://codereview.chromium.org/2782573002/diff/160001/components/history/cor...
components/history/core/browser/history_model_worker.h:38: void
ScheduleWork(base::Closure work) override;
In the past we've been passing base::Callbacks to functions as const&s. Is there
any point to this? Should we be simply passing them like this? I've never really
understood the difference.

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
File components/sync/engine/browser_thread_model_worker.cc (right):

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
components/sync/engine/browser_thread_model_worker.cc:20: return;
What do you think of an else clause instead of having an early return?

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
components/sync/engine/browser_thread_model_worker.cc:23:
runner_->PostTask(FROM_HERE, std::move(work));
You're very consistent with your std::move(work). This would compile without the
moves? Is there a reason you're not passing OnceCallbacks here?

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
File components/sync/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
components/sync/engine/model_safe_worker.cc:77: class
ModelSafeWorker::ScopedSignalWorkDoneOrAbandoned {
This feels like a generic problem that is being solved with very use case
specific code. Would be cool if you had like an object that took a closure and
ran it in it's destructor. Feel free to disregard if there's a reason this is
tricky.

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
components/sync/engine/model_safe_worker.cc:88: if (outer_)
Why's this if check needed?

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
File components/sync/engine/model_safe_worker.h (right):

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
components/sync/engine/model_safe_worker.h:84: const WorkCallback& work,
The const& is back!

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
File components/sync/engine/ui_model_worker.cc (left):

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
components/sync/engine/ui_model_worker.cc:92: }
Woo! So much simpler!

[fdoray, 2017-04-06 16:13:06]

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

[fdoray, 2017-04-06 16:27:03]

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

[commit-bot: I haz the power, 2017-04-06 16:33:10]

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, 2017-04-06 17:44:13]

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

[commit-bot: I haz the power, 2017-04-06 17:44:14]

Dry run: This issue passed the CQ dry run.

[fdoray, 2017-04-06 18:10:54]

fdoray@chromium.org changed reviewers:
+ dvadym@chromium.org, sky@chromium.org

[fdoray, 2017-04-06 18:10:55]

sky@: Please review changes in components/history/
dvadym@: Please review changes in components/password_manager/

HistoryModelWorkerTest.DoWorkAndWaitUntilDoneRequestStopBeforeUITaskRun is the
test that reproduces https://crbug.com/663600. It fails when run on old code.

https://codereview.chromium.org/2782573002/diff/160001/components/history/cor...
File components/history/core/browser/history_model_worker.cc (right):

https://codereview.chromium.org/2782573002/diff/160001/components/history/cor...
components/history/core/browser/history_model_worker.cc:82: FROM_HERE,
base::Bind(&PostWorkerTask, history_service_, std::move(work),
On 2017/04/06 00:34:54, skym wrote:
> Is the Closure implicitly converted to a OnceClosure here? How does that work?

Yes, a Closure can be implicitly converted to OnceClosure, but not the reverse.
A OnceClosure DCHECKs if run more than once.

Before, I could not use OnceClosure as argument to ScheduleWork() because
TaskRunner::PostTask() took a Closure as argument. Thanks to
https://codereview.chromium.org/2637843002, I can now use OnceClosure
everywhere.

https://codereview.chromium.org/2782573002/diff/160001/components/history/cor...
File components/history/core/browser/history_model_worker.h (right):

https://codereview.chromium.org/2782573002/diff/160001/components/history/cor...
components/history/core/browser/history_model_worker.h:38: void
ScheduleWork(base::Closure work) override;
On 2017/04/06 00:34:54, skym wrote:
> In the past we've been passing base::Callbacks to functions as const&s. Is
there
> any point to this? Should we be simply passing them like this? I've never
really
> understood the difference.

Closure is a simple class that holds a reference to a BindStateBase. Copying a
Closure increments the reference count of this BindStateBase. Moving the Closure
prevents that. 

void ScheduleWork(const base::Closure& work) {
  task_runner->PostTask(FROM_HERE, work); <-- bump ref count
}

vs.

void ScheduleWork(base::Closure work) {
  task_runner->PostTask(FROM_HERE, std::move(work)); <-- ref count unchanged
} 

I'm currently writing documentation about this, so it's normal that you're not
aware of it.

Note: Bumping the ref count costs CPU cycles, but also creates non-determinism
(who will release the last reference?).

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
File components/sync/engine/browser_thread_model_worker.cc (right):

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
components/sync/engine/browser_thread_model_worker.cc:20: return;
On 2017/04/06 00:34:54, skym wrote:
> What do you think of an else clause instead of having an early return?

Done.

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
components/sync/engine/browser_thread_model_worker.cc:23:
runner_->PostTask(FROM_HERE, std::move(work));
On 2017/04/06 00:34:54, skym wrote:
> You're very consistent with your std::move(work). This would compile without
the
> moves? Is there a reason you're not passing OnceCallbacks here?

std::move() avoids ref count churn (see previous comment).

I wasn't passing OnceCalbacks because TaskRunner::PostTask didn't accept it, but
it does now.

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
File components/sync/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
components/sync/engine/model_safe_worker.cc:77: class
ModelSafeWorker::ScopedSignalWorkDoneOrAbandoned {
On 2017/04/06 00:34:54, skym wrote:
> This feels like a generic problem that is being solved with very use case
> specific code. Would be cool if you had like an object that took a closure and
> ran it in it's destructor. Feel free to disregard if there's a reason this is
> tricky.

Done.

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
components/sync/engine/model_safe_worker.cc:88: if (outer_)
On 2017/04/06 00:34:54, skym wrote:
> Why's this if check needed?

n/a

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
File components/sync/engine/model_safe_worker.h (right):

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
components/sync/engine/model_safe_worker.h:84: const WorkCallback& work,
On 2017/04/06 00:34:54, skym wrote:
> The const& is back!

Done.

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
File components/sync/engine/ui_model_worker.cc (left):

https://codereview.chromium.org/2782573002/diff/160001/components/sync/engine...
components/sync/engine/ui_model_worker.cc:92: }
On 2017/04/06 00:34:54, skym wrote:
> Woo! So much simpler!

Acknowledged.

[sky, 2017-04-06 20:33:27]

sky@chromium.org changed reviewers:
- sky@chromium.org

[sky, 2017-04-06 20:33:28]

My understanding is Brett is willing to do an owners review of
components/history, butg wanted a sync expert to look it all over. I'm removing
myself as a reviewer. If I'm wrong there and you really need me, add me back.

[fdoray, 2017-04-06 20:44:37]

On 2017/04/06 20:33:28, sky wrote:
> My understanding is Brett is willing to do an owners review of
> components/history, butg wanted a sync expert to look it all over. I'm
removing
> myself as a reviewer. If I'm wrong there and you really need me, add me back.

brettw@: Please review changes in components/history/ and
components/password_manager/.

[dvadym, 2017-04-07 08:27:10]

components/password_manager/ LGTM

[brettw, 2017-04-10 19:38:21]

lgtm

[fdoray, 2017-04-10 19:41:15]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-04-10 19:41:15]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2782573002/#ps200001
(title: "self-review")

[commit-bot: I haz the power, 2017-04-10 19:41:53]

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-04-10 22:05:35]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491853275073790,
"parent_rev": "fcc16efe4623f2452944d08d0a4d672dffafef00", "commit_rev":
"316c08f3692150ac1c416c67391768622cc6a77f"}

[commit-bot: I haz the power, 2017-04-10 22:06:31]

Description was changed from

==========
[Sync] Refactor ModelSafeWorker::DoWorkAndWaitUntilDone() to avoid code
duplication.

Before this CL, logic to wait for a WorkCallback to run or be
abandoned was duplicated in multiple
ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() implementations.

With this CL, ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() is
replaced with ModelSafeWorker::ScheduleWork(). Implementations
are merely responsible for scheduling the callback that they
receive; they don't have to wait for it to run or be abandoned.

The logic to wait for the callback to run or be abandoned is moved
into ModelSafeWorker::DoWorkAndWaitUntilDone().
ModelSafeWorker::DoWorkAndWaitUntilDone() calls
ModelSafeWorker::ScheduleWork(). Then, it waits until either the
callback that it passed to ModelSafeWorker::ScheduleWork() is deleted
(may or may not have run) or ModelSafeWorker::RequestStop() is called
before the callback has started to run. Additionnaly, calling
ModelSafeWorker::RequestStop() transforms into no-op further
invocations of callbacks passed to ModelSafeWorker::ScheduleWork().

Unblocking ModelSafeWorker::DoWorkAndWaitUntilDone() when
ModelSafeWorker::RequestStop() is called is important to
prevent deadlocks in workers that schedule work on the UI
thread. Indeed, in Chrome, no tasks are scheduled on the UI
thread after ModelSafeWorker::RequestStop() is called and
pending UI tasks are not scheduled until after the sync thread
is joined.

BUG=663600
==========

to

==========
[Sync] Refactor ModelSafeWorker::DoWorkAndWaitUntilDone() to avoid code
duplication.

Before this CL, logic to wait for a WorkCallback to run or be
abandoned was duplicated in multiple
ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() implementations.

With this CL, ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() is
replaced with ModelSafeWorker::ScheduleWork(). Implementations
are merely responsible for scheduling the callback that they
receive; they don't have to wait for it to run or be abandoned.

The logic to wait for the callback to run or be abandoned is moved
into ModelSafeWorker::DoWorkAndWaitUntilDone().
ModelSafeWorker::DoWorkAndWaitUntilDone() calls
ModelSafeWorker::ScheduleWork(). Then, it waits until either the
callback that it passed to ModelSafeWorker::ScheduleWork() is deleted
(may or may not have run) or ModelSafeWorker::RequestStop() is called
before the callback has started to run. Additionnaly, calling
ModelSafeWorker::RequestStop() transforms into no-op further
invocations of callbacks passed to ModelSafeWorker::ScheduleWork().

Unblocking ModelSafeWorker::DoWorkAndWaitUntilDone() when
ModelSafeWorker::RequestStop() is called is important to
prevent deadlocks in workers that schedule work on the UI
thread. Indeed, in Chrome, no tasks are scheduled on the UI
thread after ModelSafeWorker::RequestStop() is called and
pending UI tasks are not scheduled until after the sync thread
is joined.

BUG=663600

Review-Url: https://codereview.chromium.org/2782573002
Cr-Commit-Position: refs/heads/master@{#463413}
Committed:
https://chromium.googlesource.com/chromium/src/+/316c08f3692150ac1c416c673917...
==========

[commit-bot: I haz the power, 2017-04-10 22:06:32]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/316c08f3692150ac1c416c673917...

[cmumford, 2017-04-13 23:00:49]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2820623002/ by cmumford@chromium.org.

The reason for reverting is: Causing build failure. More info at
https://bugs.chromium.org/p/chromium/issues/detail?id=711466..

[fdoray, 2017-04-18 12:54:12]

Description was changed from

==========
[Sync] Refactor ModelSafeWorker::DoWorkAndWaitUntilDone() to avoid code
duplication.

Before this CL, logic to wait for a WorkCallback to run or be
abandoned was duplicated in multiple
ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() implementations.

With this CL, ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() is
replaced with ModelSafeWorker::ScheduleWork(). Implementations
are merely responsible for scheduling the callback that they
receive; they don't have to wait for it to run or be abandoned.

The logic to wait for the callback to run or be abandoned is moved
into ModelSafeWorker::DoWorkAndWaitUntilDone().
ModelSafeWorker::DoWorkAndWaitUntilDone() calls
ModelSafeWorker::ScheduleWork(). Then, it waits until either the
callback that it passed to ModelSafeWorker::ScheduleWork() is deleted
(may or may not have run) or ModelSafeWorker::RequestStop() is called
before the callback has started to run. Additionnaly, calling
ModelSafeWorker::RequestStop() transforms into no-op further
invocations of callbacks passed to ModelSafeWorker::ScheduleWork().

Unblocking ModelSafeWorker::DoWorkAndWaitUntilDone() when
ModelSafeWorker::RequestStop() is called is important to
prevent deadlocks in workers that schedule work on the UI
thread. Indeed, in Chrome, no tasks are scheduled on the UI
thread after ModelSafeWorker::RequestStop() is called and
pending UI tasks are not scheduled until after the sync thread
is joined.

BUG=663600

Review-Url: https://codereview.chromium.org/2782573002
Cr-Commit-Position: refs/heads/master@{#463413}
Committed:
https://chromium.googlesource.com/chromium/src/+/316c08f3692150ac1c416c673917...
==========

to

==========
[Sync] Refactor ModelSafeWorker::DoWorkAndWaitUntilDone() to avoid code
duplication.

Before this CL, logic to wait for a WorkCallback to run or be
abandoned was duplicated in multiple
ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() implementations.

With this CL, ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() is
replaced with ModelSafeWorker::ScheduleWork(). Implementations
are merely responsible for scheduling the callback that they
receive; they don't have to wait for it to run or be abandoned.

The logic to wait for the callback to run or be abandoned is moved
into ModelSafeWorker::DoWorkAndWaitUntilDone().
ModelSafeWorker::DoWorkAndWaitUntilDone() calls
ModelSafeWorker::ScheduleWork(). Then, it waits until either the
callback that it passed to ModelSafeWorker::ScheduleWork() is deleted
(may or may not have run) or ModelSafeWorker::RequestStop() is called
before the callback has started to run. Additionnaly, calling
ModelSafeWorker::RequestStop() transforms into no-op further
invocations of callbacks passed to ModelSafeWorker::ScheduleWork().

Unblocking ModelSafeWorker::DoWorkAndWaitUntilDone() when
ModelSafeWorker::RequestStop() is called is important to
prevent deadlocks in workers that schedule work on the UI
thread. Indeed, in Chrome, no tasks are scheduled on the UI
thread after ModelSafeWorker::RequestStop() is called and
pending UI tasks are not scheduled until after the sync thread
is joined.

BUG=663600

Review-Url: https://codereview.chromium.org/2782573002
Cr-Commit-Position: refs/heads/master@{#463413}
Committed:
https://chromium.googlesource.com/chromium/src/+/316c08f3692150ac1c416c673917...
==========

[fdoray, 2017-04-18 12:56:26]

Relanding on trunk.

https://bugs.chromium.org/p/chromium/issues/detail?id=711466 is an M58 build
failure, which occurred because a CL on which this CL depends is missing on that
branch. AFAIK, there was no failure on trunk.

[fdoray, 2017-04-18 12:56:34]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-04-18 12:56:57]

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-04-18 14:26:43]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1492520194843990,
"parent_rev": "972d77b0c224be9055cc3d78993b574f0ece88ab", "commit_rev":
"eeeb0d0ec28227aba875195e0f20076414fc4596"}

[commit-bot: I haz the power, 2017-04-18 14:26:53]

Description was changed from

==========
[Sync] Refactor ModelSafeWorker::DoWorkAndWaitUntilDone() to avoid code
duplication.

Before this CL, logic to wait for a WorkCallback to run or be
abandoned was duplicated in multiple
ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() implementations.

With this CL, ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() is
replaced with ModelSafeWorker::ScheduleWork(). Implementations
are merely responsible for scheduling the callback that they
receive; they don't have to wait for it to run or be abandoned.

The logic to wait for the callback to run or be abandoned is moved
into ModelSafeWorker::DoWorkAndWaitUntilDone().
ModelSafeWorker::DoWorkAndWaitUntilDone() calls
ModelSafeWorker::ScheduleWork(). Then, it waits until either the
callback that it passed to ModelSafeWorker::ScheduleWork() is deleted
(may or may not have run) or ModelSafeWorker::RequestStop() is called
before the callback has started to run. Additionnaly, calling
ModelSafeWorker::RequestStop() transforms into no-op further
invocations of callbacks passed to ModelSafeWorker::ScheduleWork().

Unblocking ModelSafeWorker::DoWorkAndWaitUntilDone() when
ModelSafeWorker::RequestStop() is called is important to
prevent deadlocks in workers that schedule work on the UI
thread. Indeed, in Chrome, no tasks are scheduled on the UI
thread after ModelSafeWorker::RequestStop() is called and
pending UI tasks are not scheduled until after the sync thread
is joined.

BUG=663600

Review-Url: https://codereview.chromium.org/2782573002
Cr-Commit-Position: refs/heads/master@{#463413}
Committed:
https://chromium.googlesource.com/chromium/src/+/316c08f3692150ac1c416c673917...
==========

to

==========
[Sync] Refactor ModelSafeWorker::DoWorkAndWaitUntilDone() to avoid code
duplication.

Before this CL, logic to wait for a WorkCallback to run or be
abandoned was duplicated in multiple
ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() implementations.

With this CL, ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() is
replaced with ModelSafeWorker::ScheduleWork(). Implementations
are merely responsible for scheduling the callback that they
receive; they don't have to wait for it to run or be abandoned.

The logic to wait for the callback to run or be abandoned is moved
into ModelSafeWorker::DoWorkAndWaitUntilDone().
ModelSafeWorker::DoWorkAndWaitUntilDone() calls
ModelSafeWorker::ScheduleWork(). Then, it waits until either the
callback that it passed to ModelSafeWorker::ScheduleWork() is deleted
(may or may not have run) or ModelSafeWorker::RequestStop() is called
before the callback has started to run. Additionnaly, calling
ModelSafeWorker::RequestStop() transforms into no-op further
invocations of callbacks passed to ModelSafeWorker::ScheduleWork().

Unblocking ModelSafeWorker::DoWorkAndWaitUntilDone() when
ModelSafeWorker::RequestStop() is called is important to
prevent deadlocks in workers that schedule work on the UI
thread. Indeed, in Chrome, no tasks are scheduled on the UI
thread after ModelSafeWorker::RequestStop() is called and
pending UI tasks are not scheduled until after the sync thread
is joined.

BUG=663600

Review-Url: https://codereview.chromium.org/2782573002
Cr-Original-Commit-Position: refs/heads/master@{#463413}
Committed:
https://chromium.googlesource.com/chromium/src/+/316c08f3692150ac1c416c673917...
Review-Url: https://codereview.chromium.org/2782573002
Cr-Commit-Position: refs/heads/master@{#465229}
Committed:
https://chromium.googlesource.com/chromium/src/+/eeeb0d0ec28227aba875195e0f20...
==========

[commit-bot: I haz the power, 2017-04-18 14:26:56]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/eeeb0d0ec28227aba875195e0f20...