[Sync] Do not deadlock when joining sync thread with a pending HistoryModelWorker task.

[Sync] Do not deadlock when joining sync thread with a pending HistoryModelWorker task.

Currently, this scenario results in a deadlock:

Thread  Action
Sync    HistoryModelWorker::DoWorkAndWaitUntilDone
          Posts to the UI thread and waits.
UI      HistoryModelWorker::RequestStop
UI      Join the sync thread.
           Deadlock because the sync thread is waiting
           for a task to run on the UI thread.

With this CL, RequestStop() unblocks DoWorkAndWaitUntilDone()
on the sync thread when it is waiting for a UI task.

This change is similar to https://codereview.chromium.org/2505913003

BUG=663600

[fdoray, 2017-03-20 14:50:12]

Description was changed from

==========
Do not deadlock when joining sync thread with a pending HistoryModelWorker task.

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

to

==========
Do not deadlock when joining sync thread with a pending HistoryModelWorker task.

Currently, this scenario results in a deadlock:
Thread  Action
Sync    HistoryModelWorker::DoWorkAndWaitUntilDone
          Posts to the UI thread and waits.
UI      HistoryModelWorker::RequestStop
UI      Join the sync thread.
           Deadlock because the sync thread is waiting
           for a task to run on the UI thread.

With this CL, RequestStop() unblocks DoWorkAndWaitUntilDone()
on the sync thread when it is waiting for a task to run on
the UI thread.

This change is similar to https://codereview.chromium.org/2505913003

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

[fdoray, 2017-03-20 14:50:26]

Description was changed from

==========
Do not deadlock when joining sync thread with a pending HistoryModelWorker task.

Currently, this scenario results in a deadlock:
Thread  Action
Sync    HistoryModelWorker::DoWorkAndWaitUntilDone
          Posts to the UI thread and waits.
UI      HistoryModelWorker::RequestStop
UI      Join the sync thread.
           Deadlock because the sync thread is waiting
           for a task to run on the UI thread.

With this CL, RequestStop() unblocks DoWorkAndWaitUntilDone()
on the sync thread when it is waiting for a task to run on
the UI thread.

This change is similar to https://codereview.chromium.org/2505913003

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

to

==========
[Sync] Do not deadlock when joining sync thread with a pending
HistoryModelWorker task.

Currently, this scenario results in a deadlock:
Thread  Action
Sync    HistoryModelWorker::DoWorkAndWaitUntilDone
          Posts to the UI thread and waits.
UI      HistoryModelWorker::RequestStop
UI      Join the sync thread.
           Deadlock because the sync thread is waiting
           for a task to run on the UI thread.

With this CL, RequestStop() unblocks DoWorkAndWaitUntilDone()
on the sync thread when it is waiting for a task to run on
the UI thread.

This change is similar to https://codereview.chromium.org/2505913003

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

[fdoray, 2017-03-20 14:50:43]

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

[fdoray, 2017-03-20 14:50:44]

PTAL

[maxbogue, 2017-03-20 16:52:37]

This is an unfortunate amount of state to have to track but I don't see a better
way of doing it if you can't risk the sync thread unblocking during the work
callback... I guess it makes sense that it would be complicated since there are
three threads involved.

Mostly lg with some questions/nits.

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
File components/history/core/browser/history_model_worker.cc (right):

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
components/history/core/browser/history_model_worker.cc:85: [](const
base::WeakPtr<history::HistoryService>& history_service,
Optional: I love lambdas as much as anyone, but with four parameters with names
that are an underscore off from others in the surrounding scope, I think this is
actually less readable than the anonymous function that was here before...

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
components/history/core/browser/history_model_worker.cc:89: if
(history_service.get()) {
nit: do you need .get() here? WeakPtr implements the bool operator.

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
components/history/core/browser/history_model_worker.cc:128:
stop_requested_.Signal();
"and prevent any WorkCallback from being scheduled in the future."

This makes me concerned that nothing tracks that stop was requested if it
happens while the work is running... do you need a bool that gets set regardless
and signals in DidRunWorkCallback()?

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
File components/history/core/browser/history_model_worker.h (right):

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
components/history/core/browser/history_model_worker.h:47: // Called before a
WorkCallback starts running. If this returns false, the
nit: mention the side effect of this function somewhere in the comment

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
File components/history/core/browser/history_model_worker_unittest.cc (right):

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
components/history/core/browser/history_model_worker_unittest.cc:92:
base::ThreadTaskRunnerHandle thread_task_runner_handle_;
Does this overwrite base::ThreadTaskRunnerHandle::Get() for this thread? Could
you comment that if so?

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
components/history/core/browser/history_model_worker_unittest.cc:167:
sync_thread_.Stop();
Is this guaranteed to run the task posted to it or do you need to pump it
beforehand? The docs are surprisingly vague on that point and I can't
remember...

[fdoray, 2017-03-20 18:04:42]

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

[commit-bot: I haz the power, 2017-03-20 18:05:23]

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-20 18:16:24]

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

[commit-bot: I haz the power, 2017-03-20 18:16:25]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  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-20 18:54:54]

PTAnL

I changed many things to address your comments and reduce complexity.

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
File components/history/core/browser/history_model_worker.cc (right):

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
components/history/core/browser/history_model_worker.cc:85: [](const
base::WeakPtr<history::HistoryService>& history_service,
On 2017/03/20 16:52:37, maxbogue wrote:
> Optional: I love lambdas as much as anyone, but with four parameters with
names
> that are an underscore off from others in the surrounding scope, I think this
is
> actually less readable than the anonymous function that was here before...

Done.

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
components/history/core/browser/history_model_worker.cc:89: if
(history_service.get()) {
On 2017/03/20 16:52:37, maxbogue wrote:
> nit: do you need .get() here? WeakPtr implements the bool operator.

Done.

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
components/history/core/browser/history_model_worker.cc:128:
stop_requested_.Signal();
On 2017/03/20 16:52:37, maxbogue wrote:
> "and prevent any WorkCallback from being scheduled in the future."
> 
> This makes me concerned that nothing tracks that stop was requested if it
> happens while the work is running... do you need a bool that gets set
regardless
> and signals in DidRunWorkCallback()?

Done.

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
File components/history/core/browser/history_model_worker.h (right):

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
components/history/core/browser/history_model_worker.h:47: // Called before a
WorkCallback starts running. If this returns false, the
On 2017/03/20 16:52:37, maxbogue wrote:
> nit: mention the side effect of this function somewhere in the comment

n/a

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
File components/history/core/browser/history_model_worker_unittest.cc (right):

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
components/history/core/browser/history_model_worker_unittest.cc:92:
base::ThreadTaskRunnerHandle thread_task_runner_handle_;
On 2017/03/20 16:52:37, maxbogue wrote:
> Does this overwrite base::ThreadTaskRunnerHandle::Get() for this thread? Could
> you comment that if so?

Done.

https://codereview.chromium.org/2757193003/diff/20001/components/history/core...
components/history/core/browser/history_model_worker_unittest.cc:167:
sync_thread_.Stop();
On 2017/03/20 16:52:37, maxbogue wrote:
> Is this guaranteed to run the task posted to it or do you need to pump it
> beforehand? The docs are surprisingly vague on that point and I can't
> remember...

Done.

[maxbogue, 2017-03-20 19:55:50]

Ah, interesting new approach. The key is that if the task has been posted to the
DB thread, we're safe because it's the UI thread that's joining the sync thread,
right?

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

https://codereview.chromium.org/2757193003/diff/120001/components/history/cor...
components/history/core/browser/history_model_worker.cc:49: const
scoped_refptr<HistoryModelWorker> history_model_worker_;
No longer needed I think? Or is this just to ensure the worker doesn't
disappear? If that's the case please add a comment.

https://codereview.chromium.org/2757193003/diff/120001/components/history/cor...
components/history/core/browser/history_model_worker.cc:77: if (IsStopped())
Is this needed? IsStopped() is checked immediately before this function is
called:

https://cs.chromium.org/chromium/src/components/sync/engine/model_safe_worker...

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

https://codereview.chromium.org/2757193003/diff/120001/components/history/cor...
components/history/core/browser/history_model_worker.h:44: class WorkerTask;
Is this still necessary?

https://codereview.chromium.org/2757193003/diff/120001/components/history/cor...
components/history/core/browser/history_model_worker.h:66: //
work_done_or_abandoned_.Reset() and all accesses to |ui_task_pending_|.
I think this is really just for ui_task_pending_... all the others already use
thread-safe things don't they? You may be able to change ui_task_pending_ to an
AtomicFlag and get rid of this.

[fdoray, 2017-03-21 17:21:31]

PTAnL

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

https://codereview.chromium.org/2757193003/diff/120001/components/history/cor...
components/history/core/browser/history_model_worker.cc:49: const
scoped_refptr<HistoryModelWorker> history_model_worker_;
On 2017/03/20 19:55:50, maxbogue wrote:
> No longer needed I think? Or is this just to ensure the worker doesn't
> disappear? If that's the case please add a comment.

Added comment. This is required to make sure that the event in
|scoped_event_signal_| isn't deleted.

https://codereview.chromium.org/2757193003/diff/120001/components/history/cor...
components/history/core/browser/history_model_worker.cc:77: if (IsStopped())
On 2017/03/20 19:55:50, maxbogue wrote:
> Is this needed? IsStopped() is checked immediately before this function is
> called:
> 
>
https://cs.chromium.org/chromium/src/components/sync/engine/model_safe_worker...

Checking IsStopped() and resetting |work_done_or_abandoned_| must be done
atomically. Added comment.

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

https://codereview.chromium.org/2757193003/diff/120001/components/history/cor...
components/history/core/browser/history_model_worker.h:44: class WorkerTask;
On 2017/03/20 19:55:50, maxbogue wrote:
> Is this still necessary?

No. Removed it.

https://codereview.chromium.org/2757193003/diff/120001/components/history/cor...
components/history/core/browser/history_model_worker.h:66: //
work_done_or_abandoned_.Reset() and all accesses to |ui_task_pending_|.
On 2017/03/20 19:55:50, maxbogue wrote:
> I think this is really just for ui_task_pending_... all the others already use
> thread-safe things don't they? You may be able to change ui_task_pending_ to
an
> AtomicFlag and get rid of this.

Added comment about potential deadlock at the top of
HistoryModelWorker::DoWorkAndWaitUntilDoneImpl() in the .cc file.

[maxbogue, 2017-03-21 18:35:40]

lgtm!

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

https://codereview.chromium.org/2757193003/diff/140001/components/history/cor...
components/history/core/browser/history_model_worker.cc:88: //   UI    
DoWorkAndWaitUntilDoneImpl() Waits on |work_done_or_abandoned_|
s/UI/Sync/, I think.

[fdoray, 2017-03-22 12:24:52]

Description was changed from

==========
[Sync] Do not deadlock when joining sync thread with a pending
HistoryModelWorker task.

Currently, this scenario results in a deadlock:
Thread  Action
Sync    HistoryModelWorker::DoWorkAndWaitUntilDone
          Posts to the UI thread and waits.
UI      HistoryModelWorker::RequestStop
UI      Join the sync thread.
           Deadlock because the sync thread is waiting
           for a task to run on the UI thread.

With this CL, RequestStop() unblocks DoWorkAndWaitUntilDone()
on the sync thread when it is waiting for a task to run on
the UI thread.

This change is similar to https://codereview.chromium.org/2505913003

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

to

==========
[Sync] Do not deadlock when joining sync thread with a pending
HistoryModelWorker task.

Currently, this scenario results in a deadlock:

Thread  Action
Sync    HistoryModelWorker::DoWorkAndWaitUntilDone
          Posts to the UI thread and waits.
UI      HistoryModelWorker::RequestStop
UI      Join the sync thread.
           Deadlock because the sync thread is waiting
           for a task to run on the UI thread.

With this CL, RequestStop() unblocks DoWorkAndWaitUntilDone()
on the sync thread when it is waiting for a UI task.

This change is similar to https://codereview.chromium.org/2505913003

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

[fdoray, 2017-03-22 12:25:03]

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

[fdoray, 2017-03-22 12:25:04]

sky@: PTAL

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

https://codereview.chromium.org/2757193003/diff/140001/components/history/cor...
components/history/core/browser/history_model_worker.cc:88: //   UI    
DoWorkAndWaitUntilDoneImpl() Waits on |work_done_or_abandoned_|
On 2017/03/21 18:35:39, maxbogue wrote:
> s/UI/Sync/, I think.

Done.

[sky, 2017-03-22 16:08:06]

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

[sky, 2017-03-22 16:08:07]

sky->brettw

[brettw, 2017-03-23 22:26:23]

This class has gone to having kind of mish-mash of locked threadsafe variables
and non-threadsafe ones.

Can the threadsafe portion of this class's variables be segregated in some inner
class or something?

I'd also like the comment above the HistoryModelWorker declaration to have a
discussion on the threading model and protection guarantees of it.