Worker changes to prepare for lock-free shutdown.

Worker changes to prepare for lock-free shutdown.

* Make worker observe the destruction of the thread where it does work. 
* Make work done signal a memeber of worker and set it when worker's working
  thread is destroyed so that syncer won't be blocked indefinitely on work
  that's not gonna run.
* Ref-count worker in session context so that worker remains valid when syncer
  is still iterating through workers but registrar has erased it from its worker map.
* Add RequestStop() and Stopped() to allow worker to return early when it's 
  stopped while waiting to run work on its working thread.

This cl by itself should have no real impact because the added functions are not used.

BUG=19757
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202786

[haitaol1, 2013-04-24 17:16:42]

ping

[rlarocque, 2013-04-24 22:18:04]

I think I see what you're aiming for, but I'm not sure how it's supposed to
work.  

It looks like you want DoWorkAndWaitUntilDone() to return to its caller even if
the message loop dies before the posted task is run.  I think I understand that
part.  

What I don't understand is how it's possible to interact with services at that
point.  I assume that history's message loop won't be destroyed until all the
objects owned and managed by that thread are destroyed.  If the syncer were to
try to apply some Typed URL changes at that time (after all objects related to
typed URLs have been destroyed, but before the typed URL message loop is
destroyed), that would be a problem, right?

Have you considered using WeakPtrs here?  When an object that supports WeakPtrs
is destroyed, it ensures that all tasks that previously pointed at it are
effectively canceled.  In this case, just canceling the task wouldn't be enough;
we'd have to signal the WaitableEvent, too.  Implementing this would require
some work, but it should be possible.  

The advantage would be that it protects the syncer against (1) memory issues
when trying to post tasks to services that have been deleted *and* (2) the
syncer getting stuck if the message loop is destroyed.  It might also be simpler
and more test-able than listening for message loop destruction.

[haitaol1, 2013-04-25 20:14:11]

On 2013/04/24 22:18:04, rlarocque wrote:
> I think I see what you're aiming for, but I'm not sure how it's supposed to
> work.  
> 
> It looks like you want DoWorkAndWaitUntilDone() to return to its caller even
if
> the message loop dies before the posted task is run.  I think I understand
that
> part.  
> 
> What I don't understand is how it's possible to interact with services at that
> point.  I assume that history's message loop won't be destroyed until all the
> objects owned and managed by that thread are destroyed.  If the syncer were to
> try to apply some Typed URL changes at that time (after all objects related to
> typed URLs have been destroyed, but before the typed URL message loop is
> destroyed), that would be a problem, right?

In the case of history, this won't happen because syncer posts task to history
backend through UI. If task is posted before history service shutdown, it will
be run before history backend cleanup task. If it's posted after, it will wait
on UI loop indefinitely, which causes deadlock today. But lock-free shutdown
will break the deadlock when UI loop is destroyed.

> 
> Have you considered using WeakPtrs here?  When an object that supports
WeakPtrs
> is destroyed, it ensures that all tasks that previously pointed at it are
> effectively canceled.  In this case, just canceling the task wouldn't be
enough;
> we'd have to signal the WaitableEvent, too.  Implementing this would require
> some work, but it should be possible.  

I think it's only safe to invalidate weakptr on the thread where it's used.
You'll have racing problem when invalidating weakptr used on other thread. In
our case, we can only invalidate on UI loop because syncer is blocked on task
posted to browser thread.

> 
> The advantage would be that it protects the syncer against (1) memory issues
> when trying to post tasks to services that have been deleted *and* (2) the
> syncer getting stuck if the message loop is destroyed.  It might also be
simpler
> and more test-able than listening for message loop destruction.

[haitaol, 2013-04-25 20:31:46]

On Thu, Apr 25, 2013 at 1:14 PM, <haitaol@chromium.org> wrote:

> On 2013/04/24 22:18:04, rlarocque wrote:
>
>> I think I see what you're aiming for, but I'm not sure how it's supposed
>> to
>> work.
>>
>
>  It looks like you want DoWorkAndWaitUntilDone() to return to its caller
>> even
>>
> if
>
>> the message loop dies before the posted task is run.  I think I understand
>>
> that
>
>> part.
>>
>
>  What I don't understand is how it's possible to interact with services at
>> that
>> point.  I assume that history's message loop won't be destroyed until all
>> the
>> objects owned and managed by that thread are destroyed.  If the syncer
>> were to
>> try to apply some Typed URL changes at that time (after all objects
>> related to
>> typed URLs have been destroyed, but before the typed URL message loop is
>> destroyed), that would be a problem, right?
>>
>
> In the case of history, this won't happen because syncer posts task to
> history
> backend through UI. If task is posted before history service shutdown, it
> will
> be run before history backend cleanup task. If it's posted after, it will
> wait
> on UI loop indefinitely, which causes deadlock today. But lock-free
> shutdown
> will break the deadlock when UI loop is destroyed.



I should add that this won't happen in general case either in lock-free
shutdown, e.g. task posted directly to history backend
without going through UI. This is because sync manager nulls its user share
immediately on UI shutdown. So if a backend
syncer task holds a reference to user share, it must have acquired it
before ui shutdown and is currently running on backend thread,
so the cleanup of that backend always happens after syncer task.


>
>
>
>  Have you considered using WeakPtrs here?  When an object that supports
>>
> WeakPtrs
>
>> is destroyed, it ensures that all tasks that previously pointed at it are
>> effectively canceled.  In this case, just canceling the task wouldn't be
>>
> enough;
>
>> we'd have to signal the WaitableEvent, too.  Implementing this would
>> require
>> some work, but it should be possible.
>>
>
> I think it's only safe to invalidate weakptr on the thread where it's used.
> You'll have racing problem when invalidating weakptr used on other thread.
> In
> our case, we can only invalidate on UI loop because syncer is blocked on
> task
> posted to browser thread.
>
>
>
>  The advantage would be that it protects the syncer against (1) memory
>> issues
>> when trying to post tasks to services that have been deleted *and* (2) the
>> syncer getting stuck if the message loop is destroyed.  It might also be
>>
> simpler
>
>> and more test-able than listening for message loop destruction.
>>
>
>
>
>
https://codereview.chromium.**org/14046031/<https://codereview.chromium.org/1...
>

[haitaol1, 2013-05-10 16:48:34]

ping


On 2013/04/25 20:31:46, haitaol wrote:
> On Thu, Apr 25, 2013 at 1:14 PM, <mailto:haitaol@chromium.org> wrote:
> 
> > On 2013/04/24 22:18:04, rlarocque wrote:
> >
> >> I think I see what you're aiming for, but I'm not sure how it's supposed
> >> to
> >> work.
> >>
> >
> >  It looks like you want DoWorkAndWaitUntilDone() to return to its caller
> >> even
> >>
> > if
> >
> >> the message loop dies before the posted task is run.  I think I understand
> >>
> > that
> >
> >> part.
> >>
> >
> >  What I don't understand is how it's possible to interact with services at
> >> that
> >> point.  I assume that history's message loop won't be destroyed until all
> >> the
> >> objects owned and managed by that thread are destroyed.  If the syncer
> >> were to
> >> try to apply some Typed URL changes at that time (after all objects
> >> related to
> >> typed URLs have been destroyed, but before the typed URL message loop is
> >> destroyed), that would be a problem, right?
> >>
> >
> > In the case of history, this won't happen because syncer posts task to
> > history
> > backend through UI. If task is posted before history service shutdown, it
> > will
> > be run before history backend cleanup task. If it's posted after, it will
> > wait
> > on UI loop indefinitely, which causes deadlock today. But lock-free
> > shutdown
> > will break the deadlock when UI loop is destroyed.
> 
> 
> 
> I should add that this won't happen in general case either in lock-free
> shutdown, e.g. task posted directly to history backend
> without going through UI. This is because sync manager nulls its user share
> immediately on UI shutdown. So if a backend
> syncer task holds a reference to user share, it must have acquired it
> before ui shutdown and is currently running on backend thread,
> so the cleanup of that backend always happens after syncer task.
> 
> 
> >
> >
> >
> >  Have you considered using WeakPtrs here?  When an object that supports
> >>
> > WeakPtrs
> >
> >> is destroyed, it ensures that all tasks that previously pointed at it are
> >> effectively canceled.  In this case, just canceling the task wouldn't be
> >>
> > enough;
> >
> >> we'd have to signal the WaitableEvent, too.  Implementing this would
> >> require
> >> some work, but it should be possible.
> >>
> >
> > I think it's only safe to invalidate weakptr on the thread where it's used.
> > You'll have racing problem when invalidating weakptr used on other thread.
> > In
> > our case, we can only invalidate on UI loop because syncer is blocked on
> > task
> > posted to browser thread.
> >
> >
> >
> >  The advantage would be that it protects the syncer against (1) memory
> >> issues
> >> when trying to post tasks to services that have been deleted *and* (2) the
> >> syncer getting stuck if the message loop is destroyed.  It might also be
> >>
> > simpler
> >
> >> and more test-able than listening for message loop destruction.
> >>
> >
> >
> >
> >
>
https://codereview.chromium.**org/14046031/%3Chttps://codereview.chromium.org...>
> >

[tim (not reviewing), 2013-05-14 04:14:29]

https://codereview.chromium.org/14046031/diff/2001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_registrar.cc (right):

https://codereview.chromium.org/14046031/diff/2001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_registrar.cc:62: ui_worker_(new
UIModelWorker(ALLOW_THIS_IN_INITIALIZER_LIST(this))),
See http://src.chromium.org/viewvc/chrome?revision=197671&view=revision.

https://codereview.chromium.org/14046031/diff/2001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_registrar.h (right):

https://codereview.chromium.org/14046031/diff/2001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_registrar.h:38: public
syncer::WorkerObserver {
WorkerDestructionObserver would be a better name for this, particularly since it
is just a relay of MessageLoop::DestructionObserver.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
File sync/internal_api/public/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.cc:90: void
ModelSafeWorker::RequestStop() {
Who calls RequestStop?

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.cc:132:
work_done_or_stopped_.Signal();
Still trying to get the hang of the new thread coordination... is it not
possible for HandleWork to have just executed past the synchronized block and we
context switch here to acquire the lock and signal?

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
File sync/internal_api/public/engine/model_safe_worker.h (right):

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:54: virtual void
OnWorkerDisabled(ModelSafeGroup group) = 0;
OnWorkerThreadDestroyed would be a better name.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:66: // work and will notify
its observer (SyncBackendRegistrar).
Don't mention 'browser' or components from glue like SyncBackendRegistrar in the
sync/ module.  As part of a component, comments here should not assume specifics
about the embedder.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:73: virtual void
RegisterForLoopDestruction() = 0;
Who calls this?  and when?

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:77: SyncerError
HandleWork(const WorkCallback& work);
DoWorkAndWaitUntilDone is a better, more descriptive name.  The fact that it can
fail doesn't need we need a more abstract name... most functions have a failure
mode :)  I think your comment + return value suffices.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:83: // Reset state when sync
is enabled.
When is this used?  Is this for when sync was disabled, but is now being
re-enabled, and the destruction observer hasn't fired? I'd rather be explicit
about what needs to happen than have a restart function here. e.g force the
caller to destroy (or put this MSW in a lame-duck mode and detach), and create a
new MSW.  

That moves the extra complexity for this case to the caller / client of
ModelSafeWorker, which seems right given that we want to remove that case
altogether from the higher layers. If we go with my suggestion we won't have to
make any changes to ModelSafeWorker when that switch happens, and we won't add
complexity here in the meantime.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:94:
ModelSafeWorker(WorkerObserver* worker_host);
Single argument constructors should be explicit.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:100: virtual SyncerError
DoWorkAndWaitUntilDone(
You can call this DoWorkAndWaitIUntilDoneImpl.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:104: bool Stopped();
IsStopped()

[haitaol1, 2013-05-15 23:39:21]

https://codereview.chromium.org/14046031/diff/2001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_registrar.cc (right):

https://codereview.chromium.org/14046031/diff/2001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_registrar.cc:62: ui_worker_(new
UIModelWorker(ALLOW_THIS_IN_INITIALIZER_LIST(this))),
On 2013/05/14 04:14:29, timsteele wrote:
> See http://src.chromium.org/viewvc/chrome?revision=197671&view=revision.

Done.

https://codereview.chromium.org/14046031/diff/2001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_registrar.h (right):

https://codereview.chromium.org/14046031/diff/2001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_registrar.h:38: public
syncer::WorkerObserver {
I think WorkerLoopDesctructionObserver is more accurate.

On 2013/05/14 04:14:29, timsteele wrote:
> WorkerDestructionObserver would be a better name for this, particularly since
it
> is just a relay of MessageLoop::DestructionObserver.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
File sync/internal_api/public/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.cc:90: void
ModelSafeWorker::RequestStop() {
SyncBackendRegistrar will call this from UI thread. If backend is iterating
through several workers, this allows pending workers to quit quickly when they
are called to do work.

On 2013/05/14 04:14:29, timsteele wrote:
> Who calls RequestStop?

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.cc:132:
work_done_or_stopped_.Signal();
Signal can be called before Wait. In that case, wait would just return without
blocking.

On 2013/05/14 04:14:29, timsteele wrote:
> Still trying to get the hang of the new thread coordination... is it not
> possible for HandleWork to have just executed past the synchronized block and
we
> context switch here to acquire the lock and signal?

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
File sync/internal_api/public/engine/model_safe_worker.h (right):

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:54: virtual void
OnWorkerDisabled(ModelSafeGroup group) = 0;
On 2013/05/14 04:14:29, timsteele wrote:
> OnWorkerThreadDestroyed would be a better name.

Done.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:66: // work and will notify
its observer (SyncBackendRegistrar).
On 2013/05/14 04:14:29, timsteele wrote:
> Don't mention 'browser' or components from glue like SyncBackendRegistrar in
the
> sync/ module.  As part of a component, comments here should not assume
specifics
> about the embedder.

Done.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:73: virtual void
RegisterForLoopDestruction() = 0;
On 2013/05/14 04:14:29, timsteele wrote:
> Who calls this?  and when?

SyncBackendHost will call this after creating workers.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:77: SyncerError
HandleWork(const WorkCallback& work);
On 2013/05/14 04:14:29, timsteele wrote:
> DoWorkAndWaitUntilDone is a better, more descriptive name.  The fact that it
can
> fail doesn't need we need a more abstract name... most functions have a
failure
> mode :)  I think your comment + return value suffices.

Done.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:83: // Reset state when sync
is enabled.
Removed. Worker will be destroyed when sync is disabled.

On 2013/05/14 04:14:29, timsteele wrote:
> When is this used?  Is this for when sync was disabled, but is now being
> re-enabled, and the destruction observer hasn't fired? I'd rather be explicit
> about what needs to happen than have a restart function here. e.g force the
> caller to destroy (or put this MSW in a lame-duck mode and detach), and create
a
> new MSW.  
> 
> That moves the extra complexity for this case to the caller / client of
> ModelSafeWorker, which seems right given that we want to remove that case
> altogether from the higher layers. If we go with my suggestion we won't have
to
> make any changes to ModelSafeWorker when that switch happens, and we won't add
> complexity here in the meantime.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:94:
ModelSafeWorker(WorkerObserver* worker_host);
On 2013/05/14 04:14:29, timsteele wrote:
> Single argument constructors should be explicit.

Done.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:100: virtual SyncerError
DoWorkAndWaitUntilDone(
On 2013/05/14 04:14:29, timsteele wrote:
> You can call this DoWorkAndWaitIUntilDoneImpl.

Done.

https://codereview.chromium.org/14046031/diff/2001/sync/internal_api/public/e...
sync/internal_api/public/engine/model_safe_worker.h:104: bool Stopped();
On 2013/05/14 04:14:29, timsteele wrote:
> IsStopped()

Done.

[tim (not reviewing), 2013-05-16 18:15:42]

https://codereview.chromium.org/14046031/diff/14001/sync/internal_api/public/...
File sync/internal_api/public/engine/model_safe_worker.h (right):

https://codereview.chromium.org/14046031/diff/14001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.h:81: void RequestStop();
If this is called from the UI thread, and we always create ModelSafeWorkers on
the UI thread in SyncBackendRegistrar, can't we replace lock_ and stopped_ with
a base::CancellationFlag?

https://codereview.chromium.org/14046031/diff/14001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.h:105: // when the worker's
working thread is to be destroyed.
Mention what |lock_| protects.  I think stopped_lock_ is a better name.

[haitaol1, 2013-05-17 16:24:59]

https://codereview.chromium.org/14046031/diff/14001/sync/internal_api/public/...
File sync/internal_api/public/engine/model_safe_worker.h (right):

https://codereview.chromium.org/14046031/diff/14001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.h:81: void RequestStop();
lock_ is also used to serialize signal/reset of work_done_or_stopped_, so can't
be replace. Then it's redundant to make stopped_ CancellationFlag.

On 2013/05/16 18:15:42, timsteele wrote:
> If this is called from the UI thread, and we always create ModelSafeWorkers on
> the UI thread in SyncBackendRegistrar, can't we replace lock_ and stopped_
with
> a base::CancellationFlag?

https://codereview.chromium.org/14046031/diff/14001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.h:105: // when the worker's
working thread is to be destroyed.
On 2013/05/16 18:15:42, timsteele wrote:
> Mention what |lock_| protects.  I think stopped_lock_ is a better name.

Done.

[tim (not reviewing), 2013-05-17 20:51:32]

https://codereview.chromium.org/14046031/diff/20001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/ui_model_worker.cc (right):

https://codereview.chromium.org/14046031/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/ui_model_worker.cc:89: done->Wait();
Do we need to pass the event around?  It's a member of this class, and having
multiple event*'s floating around really makes things complex to follow.  I'd
prefer a protected function, either returning a pointer or for waiting, e.g

work_done_or_stopped()->Wait()
WaitForWorkDoneOrStopped();

Although given the multiple possible signals and multiple layers of locks we're
using to protect waitable events (which is always something to question) I'm
wondering if it wouldn't be clearer using ConditionVariable,

while (work_is_pending_ && !stopped()) 
  not_processing_work_.Wait()

but given that I don't think |this|->lock_ will be necessary in a subsequent CL
(assuming we can remove pending_work_ as I discussed in a comment @line 116),
maybe it isn't worth rocking the boat too much yet.

It's just that various levels of locks make it extremely difficult to review
from a perspective of ensuring no re-entrancy or re-acquisition issues (e.g
someone Signal()ing holding a lock that tries to get re-acquired).

https://codereview.chromium.org/14046031/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/ui_model_worker.cc:99: // default UIModelWorker in
existence and it gets destroyed before
You're not passing UIModelWorker* to a static function anymore -- this is a
member function, so you should not end up here if UIModelWorker was destroyed.  

I also think we're about to run into another downside with ref-counted objects. 
Making this a member function of a ref-counted class means your base::Bind
invocation will implicitly keep the UIModelWorker alive while the posted task
lives.  This is logically wrong, but it will work technically due to
refcounting.  Instead of that, if you want to make this a member function, then
the UIModelWorker should have a weak_ptr_factory_ and vend a WeakPtr to Bind
(See SyncSchedulerImpl for a nearby example.)

https://codereview.chromium.org/14046031/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/ui_model_worker.cc:109: if (!IsStopped())
This is hard to follow because RequestStop sets stopped_ but doesn't transition
the state to STOPPED, which is what the UIModelWorker currently uses to
determine whether it is WORKING, STOPPED, or RUNNING_MANUAL_SHUTDOWN_PUMP. Also,
It's not clear from the naming when Stop() will be called relative to
RequestStop() (which actually has nothing to do with Stop(), despite the name),
or WillDestroyMessageLoop.  

Perhaps this clears up in subsequent patches, but I don't know if I can approve
this with two notions of Stop going on in the same class.  I'd like to try and
clear things up, or at the very least ensure one of the Stop-paths (methods and
state) are clearly annotated (so maybe Stop() -> StopNowDeprecated(), and a
comment when we assign pending_work_ that this is only temporarily needed while
we remove the MANUAL_SHUTDOWN_PUMP state, for starters).

https://codereview.chromium.org/14046031/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/ui_model_worker.cc:116: OnTaskCompleted();
OnTaskCompleted is now notifying |this| instead of "the UIModelWorker that
scheduled us") and should really be inlined, all it does is reset pending_work_.

e.g Are you planning to remove pending_work_ altogether in a follow up CL,
because we don't need to manually flush it? What should happen if UIModelWorker
is destroyed with a dangling CallDoWork? WeakPtrs as proposed above would
gracefully "cancel" the task, because the bound object will no longer exist when
the callback code tries to invoke us.

https://codereview.chromium.org/14046031/diff/20001/sync/internal_api/public/...
File sync/internal_api/public/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/14046031/diff/20001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.cc:106:
work_done_or_stopped_.Reset();
Why are we using manual reset for this?

[haitaol1, 2013-05-17 22:06:57]

https://codereview.chromium.org/14046031/diff/20001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/ui_model_worker.cc (right):

https://codereview.chromium.org/14046031/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/ui_model_worker.cc:89: done->Wait();
Added protected accessor.

On 2013/05/17 20:51:33, timsteele wrote:
> Do we need to pass the event around?  It's a member of this class, and having
> multiple event*'s floating around really makes things complex to follow.  I'd
> prefer a protected function, either returning a pointer or for waiting, e.g
> 
> work_done_or_stopped()->Wait()
> WaitForWorkDoneOrStopped();
> 
> Although given the multiple possible signals and multiple layers of locks
we're
> using to protect waitable events (which is always something to question) I'm
> wondering if it wouldn't be clearer using ConditionVariable,
> 
> while (work_is_pending_ && !stopped()) 
>   not_processing_work_.Wait()
> 
> but given that I don't think |this|->lock_ will be necessary in a subsequent
CL
> (assuming we can remove pending_work_ as I discussed in a comment @line 116),
> maybe it isn't worth rocking the boat too much yet.
> 
> It's just that various levels of locks make it extremely difficult to review
> from a perspective of ensuring no re-entrancy or re-acquisition issues (e.g
> someone Signal()ing holding a lock that tries to get re-acquired).

https://codereview.chromium.org/14046031/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/ui_model_worker.cc:99: // default UIModelWorker in
existence and it gets destroyed before
The UI worker (or any worker) will not be destroyed until it returns from the
work posted to it. So explicit reference is held by registrar. The ref-counting
of worker already exists, which seems unnecessary.


On 2013/05/17 20:51:33, timsteele wrote:
> You're not passing UIModelWorker* to a static function anymore -- this is a
> member function, so you should not end up here if UIModelWorker was destroyed.
 
> 
> I also think we're about to run into another downside with ref-counted
objects. 
> Making this a member function of a ref-counted class means your base::Bind
> invocation will implicitly keep the UIModelWorker alive while the posted task
> lives.  This is logically wrong, but it will work technically due to
> refcounting.  Instead of that, if you want to make this a member function,
then
> the UIModelWorker should have a weak_ptr_factory_ and vend a WeakPtr to Bind
> (See SyncSchedulerImpl for a nearby example.)

https://codereview.chromium.org/14046031/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/ui_model_worker.cc:109: if (!IsStopped())
Reverted.

On 2013/05/17 20:51:33, timsteele wrote:
> This is hard to follow because RequestStop sets stopped_ but doesn't
transition
> the state to STOPPED, which is what the UIModelWorker currently uses to
> determine whether it is WORKING, STOPPED, or RUNNING_MANUAL_SHUTDOWN_PUMP.
Also,
> It's not clear from the naming when Stop() will be called relative to
> RequestStop() (which actually has nothing to do with Stop(), despite the
name),
> or WillDestroyMessageLoop.  
> 
> Perhaps this clears up in subsequent patches, but I don't know if I can
approve
> this with two notions of Stop going on in the same class.  I'd like to try and
> clear things up, or at the very least ensure one of the Stop-paths (methods
and
> state) are clearly annotated (so maybe Stop() -> StopNowDeprecated(), and a
> comment when we assign pending_work_ that this is only temporarily needed
while
> we remove the MANUAL_SHUTDOWN_PUMP state, for starters).

https://codereview.chromium.org/14046031/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/ui_model_worker.cc:116: OnTaskCompleted();
On 2013/05/17 20:51:33, timsteele wrote:
> OnTaskCompleted is now notifying |this| instead of "the UIModelWorker that
> scheduled us") and should really be inlined, all it does is reset
pending_work_.
> 
> e.g Are you planning to remove pending_work_ altogether in a follow up CL,

Yes

> because we don't need to manually flush it? What should happen if
UIModelWorker
> is destroyed with a dangling CallDoWork? 

That shouldn't happen because syncer is blocked on worker and only unblocked
when work finish or worker loop is destroyed. Worker can only destroyed after
syncer stops.

WeakPtrs as proposed above would
> gracefully "cancel" the task, because the bound object will no longer exist
when
> the callback code tries to invoke us.

https://codereview.chromium.org/14046031/diff/20001/sync/internal_api/public/...
File sync/internal_api/public/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/14046031/diff/20001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.cc:106:
work_done_or_stopped_.Reset();
It should be unnecessary if worker is not reused, so once it's stopped, it will
be destroyed later. Add a CHECK to make sure.

On 2013/05/17 20:51:33, timsteele wrote:
> Why are we using manual reset for this?

[tim (not reviewing), 2013-05-20 17:22:38]

Thanks, that made it easier to review.  Getting there, have one more significant
comment.

https://codereview.chromium.org/14046031/diff/20001/sync/internal_api/public/...
File sync/internal_api/public/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/14046031/diff/20001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.cc:106:
work_done_or_stopped_.Reset();
On 2013/05/17 22:06:57, haitaol1 wrote:
> It should be unnecessary if worker is not reused, so once it's stopped, it
will
> be destroyed later. Add a CHECK to make sure.
> 
> On 2013/05/17 20:51:33, timsteele wrote:
> > Why are we using manual reset for this?
> 

My question was phrased a bit incorrectly; I was wondering why we were calling
Reset() despite having *not* passed |true| to manual_reset in the WaitableEvent
ctor.  I agree the Reset shouldn't be necessary, but I guess manual_reset=true
is still necessary since work_done will alternate.

https://codereview.chromium.org/14046031/diff/25001/sync/internal_api/public/...
File sync/internal_api/public/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/14046031/diff/25001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.cc:95: // living on sync
thread.
Add to comment:  "This prevents any *further* tasks from being posted to worker
threads (see DoWorkAndWaitUntilDone below), but note that one may already be
posted."

https://codereview.chromium.org/14046031/diff/25001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.cc:124: // the signal is set.
This is a subtle expectation.  For example, it's not true in the case of
UIModelWorker because it manually pumps the final task, meaning if we Signal()
it we won't run the code that does OnTaskCompleted(), for example.  However,
this winds up being OK because by the time WillDestroyCurrentMessageLoop is
called for the UI thread, we should have already called UIModelWorker::Stop().

Rather than just leaving a live task out there with now garbage pointers and
hoping the interactions remain "correct", the better thing to do would be to
make the tasks the workers post to their backend threads (the
"CallDoWorkAndSignal" variants) member functions of the ModelSafeWorker like you
did for UIModelWorker in the previous patchset, but then additionally base::Bind
should be passed a WeakPtr to the worker and then right here in
MSW::WillDestroyCurrentMessageLoop we should call 

weak_ptr_factory_.InvalidateWeakPtrs();

This would be a more graceful way of making the fact that those tasks will not
be executed 100% explicit in the code for all workers, current and future.

[tim (not reviewing), 2013-05-23 01:21:16]

We discussed my most lengthy comment offline and decided it's probably
unnecessary to add WeakPtrs since Haitao mentioned pending tasks will have been
destroyed by the time WillDestroyCurrentMessageLoop is called.

However the other two comments still stand, but provided you address those,
LGTM.

[haitaol1, 2013-05-23 16:07:29]

https://codereview.chromium.org/14046031/diff/20001/sync/internal_api/public/...
File sync/internal_api/public/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/14046031/diff/20001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.cc:106:
work_done_or_stopped_.Reset();
With manual_reset=true, Reset() becomes necessary because the signal is not
cleared when waking up from done work. So next time, signal needs to be cleared
so that worker can block on task posted to browser thread.

On 2013/05/20 17:22:38, timsteele wrote:
> On 2013/05/17 22:06:57, haitaol1 wrote:
> > It should be unnecessary if worker is not reused, so once it's stopped, it
> will
> > be destroyed later. Add a CHECK to make sure.
> > 
> > On 2013/05/17 20:51:33, timsteele wrote:
> > > Why are we using manual reset for this?
> > 
> 
> My question was phrased a bit incorrectly; I was wondering why we were calling
> Reset() despite having *not* passed |true| to manual_reset in the
WaitableEvent
> ctor.  I agree the Reset shouldn't be necessary, but I guess manual_reset=true
> is still necessary since work_done will alternate.

https://codereview.chromium.org/14046031/diff/25001/sync/internal_api/public/...
File sync/internal_api/public/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/14046031/diff/25001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.cc:95: // living on sync
thread.
On 2013/05/20 17:22:39, timsteele wrote:
> Add to comment:  "This prevents any *further* tasks from being posted to
worker
> threads (see DoWorkAndWaitUntilDone below), but note that one may already be
> posted."

Done.

https://codereview.chromium.org/14046031/diff/25001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.cc:124: // the signal is set.
Resolved per offline discussion. Updated comments.

On 2013/05/20 17:22:39, timsteele wrote:
> This is a subtle expectation.  For example, it's not true in the case of
> UIModelWorker because it manually pumps the final task, meaning if we Signal()
> it we won't run the code that does OnTaskCompleted(), for example.  However,
> this winds up being OK because by the time WillDestroyCurrentMessageLoop is
> called for the UI thread, we should have already called UIModelWorker::Stop().
> 
> Rather than just leaving a live task out there with now garbage pointers and
> hoping the interactions remain "correct", the better thing to do would be to
> make the tasks the workers post to their backend threads (the
> "CallDoWorkAndSignal" variants) member functions of the ModelSafeWorker like
you
> did for UIModelWorker in the previous patchset, but then additionally
base::Bind
> should be passed a WeakPtr to the worker and then right here in
> MSW::WillDestroyCurrentMessageLoop we should call 
> 
> weak_ptr_factory_.InvalidateWeakPtrs();
> 
> This would be a more graceful way of making the fact that those tasks will not
> be executed 100% explicit in the code for all workers, current and future.

[tim (not reviewing), 2013-05-23 18:10:36]

https://codereview.chromium.org/14046031/diff/20001/sync/internal_api/public/...
File sync/internal_api/public/engine/model_safe_worker.cc (right):

https://codereview.chromium.org/14046031/diff/20001/sync/internal_api/public/...
sync/internal_api/public/engine/model_safe_worker.cc:106:
work_done_or_stopped_.Reset();
On 2013/05/23 16:07:29, haitaol1 wrote:
> With manual_reset=true, Reset() becomes necessary because the signal is not
> cleared when waking up from done work. So next time, signal needs to be
cleared
> so that worker can block on task posted to browser thread.
> 
> On 2013/05/20 17:22:38, timsteele wrote:
> > On 2013/05/17 22:06:57, haitaol1 wrote:
> > > It should be unnecessary if worker is not reused, so once it's stopped, it
> > will
> > > be destroyed later. Add a CHECK to make sure.
> > > 
> > > On 2013/05/17 20:51:33, timsteele wrote:
> > > > Why are we using manual reset for this?
> > > 
> > 
> > My question was phrased a bit incorrectly; I was wondering why we were
calling
> > Reset() despite having *not* passed |true| to manual_reset in the
> WaitableEvent
> > ctor.  I agree the Reset shouldn't be necessary, but I guess
manual_reset=true
> > is still necessary since work_done will alternate.
> 

Right, but you never passed true for manual_reset in this patch, hence my
confusion.  Or I missed it somewhere...

[haitaol1, 2013-05-23 18:23:43]

On 2013/05/23 18:10:36, timsteele wrote:
>
https://codereview.chromium.org/14046031/diff/20001/sync/internal_api/public/...
> File sync/internal_api/public/engine/model_safe_worker.cc (right):
> 
>
https://codereview.chromium.org/14046031/diff/20001/sync/internal_api/public/...
> sync/internal_api/public/engine/model_safe_worker.cc:106:
> work_done_or_stopped_.Reset();
> On 2013/05/23 16:07:29, haitaol1 wrote:
> > With manual_reset=true, Reset() becomes necessary because the signal is not
> > cleared when waking up from done work. So next time, signal needs to be
> cleared
> > so that worker can block on task posted to browser thread.
> > 
> > On 2013/05/20 17:22:38, timsteele wrote:
> > > On 2013/05/17 22:06:57, haitaol1 wrote:
> > > > It should be unnecessary if worker is not reused, so once it's stopped,
it
> > > will
> > > > be destroyed later. Add a CHECK to make sure.
> > > > 
> > > > On 2013/05/17 20:51:33, timsteele wrote:
> > > > > Why are we using manual reset for this?
> > > > 
> > > 
> > > My question was phrased a bit incorrectly; I was wondering why we were
> calling
> > > Reset() despite having *not* passed |true| to manual_reset in the
> > WaitableEvent
> > > ctor.  I agree the Reset shouldn't be necessary, but I guess
> manual_reset=true
> > > is still necessary since work_done will alternate.
> > 
> 
> Right, but you never passed true for manual_reset in this patch, hence my
> confusion.  Or I missed it somewhere...

My point is that manual_reset should be false. Otherwise Reset() is required.

[commit-bot: I haz the power, 2013-05-28 22:26:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haitaol@chromium.org/14046031/31001

[commit-bot: I haz the power, 2013-05-29 06:50:46]

Change committed as 202786