Worker changes to prepare for lock-free shutdown.

chrome/browser/sync/glue/ui_model_worker.cc

Index: chrome/browser/sync/glue/ui_model_worker.cc
diff --git a/chrome/browser/sync/glue/ui_model_worker.cc b/chrome/browser/sync/glue/ui_model_worker.cc
index c0336ba2ed8994b746bd6dfa766b010d1f10d2b9..beac8f84727e1100c03c2f76ca5d86b6fbdb25e3 100644
--- a/chrome/browser/sync/glue/ui_model_worker.cc
+++ b/chrome/browser/sync/glue/ui_model_worker.cc
@@ -16,41 +16,18 @@ using content::BrowserThread;
 
 namespace browser_sync {
 
-namespace {
-
-// A simple callback to signal a waitable event after running a closure.
-void CallDoWorkAndSignalCallback(const syncer::WorkCallback& work,
-                                 base::WaitableEvent* work_done,
-                                 UIModelWorker* const scheduler,
-                                 syncer::SyncerError* error_info) {
-  if (work.is_null()) {
-    // This can happen during tests or cases where there are more than just the
-    // default UIModelWorker in existence and it gets destroyed before
-    // the main UI loop has terminated.  There is no easy way to assert the
-    // loop is running / not running at the moment, so we just provide cancel
-    // semantics here and short-circuit.
-    // TODO(timsteele): Maybe we should have the message loop destruction
-    // observer fire when the loop has ended, just a bit before it
-    // actually gets destroyed.
-    return;
-  }
-
-  *error_info = work.Run();
-
-  // Notify the UIModelWorker that scheduled us that we have run
-  // successfully.
-  scheduler->OnTaskCompleted();
-  work_done->Signal();  // Unblock the syncer thread that scheduled us.
-}
-
-}  // namespace
-
-UIModelWorker::UIModelWorker()
-    : state_(WORKING),
+UIModelWorker::UIModelWorker(syncer::WorkerLoopDestructionObserver* observer)
+    : syncer::ModelSafeWorker(observer),
+      state_(WORKING),
       syncapi_has_shutdown_(false),
       syncapi_event_(&lock_) {
 }
 
+void UIModelWorker::RegisterForLoopDestruction() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  MessageLoop::current()->AddDestructionObserver(this);
+}
+
 void UIModelWorker::Stop() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -76,8 +53,8 @@ void UIModelWorker::Stop() {
   state_ = STOPPED;
 }
 
-syncer::SyncerError UIModelWorker::DoWorkAndWaitUntilDone(
-    const syncer::WorkCallback& work) {
+syncer::SyncerError UIModelWorker::DoWorkAndWaitUntilDoneImpl(
+    const syncer::WorkCallback& work, base::WaitableEvent* done) {
   // In most cases, this method is called in WORKING state. It is possible this
   // gets called when we are in the RUNNING_MANUAL_SHUTDOWN_PUMP state, because
   // the UI loop has initiated shutdown but the syncer hasn't got the memo yet.
@@ -92,16 +69,14 @@ syncer::SyncerError UIModelWorker::DoWorkAndWaitUntilDone(
     return work.Run();
   }
 
-  // Create an unsignaled event to wait on.
-  base::WaitableEvent work_done(false, false);
   {
     // We lock only to avoid PostTask'ing a NULL pending_work_ (because it
     // could get Run() in Stop() and call OnTaskCompleted before we post).
     // The task is owned by the message loop as per usual.
     base::AutoLock lock(lock_);
     DCHECK(pending_work_.is_null());
-    pending_work_ = base::Bind(&CallDoWorkAndSignalCallback, work, &work_done,
-                               base::Unretained(this), &error_info);
+    pending_work_ = base::Bind(&UIModelWorker::CallDoWorkAndSignalCallback,
+                               this, work, done, &error_info);
     if (!BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, pending_work_)) {
       DLOG(WARNING) << "Could not post work to UI loop.";
       error_info = syncer::CANNOT_DO_WORK;
@@ -111,10 +86,37 @@ syncer::SyncerError UIModelWorker::DoWorkAndWaitUntilDone(
     }
   }
   syncapi_event_.Signal();  // Notify that the syncapi produced work for us.
-  work_done.Wait();
+  done->Wait();

[tim (not reviewing), 2013/05/17 20:51:33]

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

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

Added protected accessor.

On 2013/05/17 20:51:33, timsteele wrote:

   return error_info;
 }
 
+void UIModelWorker::CallDoWorkAndSignalCallback(
+    const syncer::WorkCallback& work,
+    base::WaitableEvent* work_done,
+    syncer::SyncerError* error_info) {
+  if (work.is_null()) {
+    // This can happen during tests or cases where there are more than just the
+    // default UIModelWorker in existence and it gets destroyed before

[tim (not reviewing), 2013/05/17 20:51:33]

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

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

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:

+    // the main UI loop has terminated.  There is no easy way to assert the
+    // loop is running / not running at the moment, so we just provide cancel
+    // semantics here and short-circuit.
+    // TODO(timsteele): Maybe we should have the message loop destruction
+    // observer fire when the loop has ended, just a bit before it
+    // actually gets destroyed.
+    return;
+  }
+
+  if (!IsStopped())

[tim (not reviewing), 2013/05/17 20:51:33]

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

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

Reverted.

On 2013/05/17 20:51:33, timsteele wrote:

+    *error_info = work.Run();
+  else
+    *error_info = syncer::CANNOT_DO_WORK;
+
+  // Notify the UIModelWorker that scheduled us that we have run
+  // successfully.
+  OnTaskCompleted();

[tim (not reviewing), 2013/05/17 20:51:33]

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.  

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

Yes
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

+  work_done->Signal();  // Unblock the syncer thread that scheduled us.
+}
+
 syncer::ModelSafeGroup UIModelWorker::GetModelSafeGroup() {
   return syncer::GROUP_UI;
 }