Sync: Avoid deadlock in SyncBackendRegistrar / ModelSafeWorker on sync backend shutdown.

sync/internal_api/public/engine/model_safe_worker.cc

Index: sync/internal_api/public/engine/model_safe_worker.cc
diff --git a/sync/internal_api/public/engine/model_safe_worker.cc b/sync/internal_api/public/engine/model_safe_worker.cc
index eb3fc1f1c3ec286c961e743b23b05e77a7eae358..15f4d4cc897f0b4456065a23a4c07ead5aa26f32 100644
--- a/sync/internal_api/public/engine/model_safe_worker.cc
+++ b/sync/internal_api/public/engine/model_safe_worker.cc
@@ -75,8 +75,8 @@ ModelSafeWorker::ModelSafeWorker(WorkerLoopDestructionObserver* observer)
     : stopped_(false),
       work_done_or_stopped_(false, false),
       observer_(observer),
-      working_loop_(NULL),
-      working_loop_set_wait_(true, false) {}
+      working_loop_(NULL) {
+}
 
 ModelSafeWorker::~ModelSafeWorker() {}
 
@@ -135,26 +135,44 @@ void ModelSafeWorker::WillDestroyCurrentMessageLoop() {
 void ModelSafeWorker::SetWorkingLoopToCurrent() {
   base::AutoLock l(working_loop_lock_);
   DCHECK(!working_loop_);
-  working_loop_ = base::MessageLoop::current();
-  working_loop_set_wait_.Signal();
+
+  if (unregister_done_callback_.is_null()) {
+    // Normal case - UnregisterForLoopDestruction hasn't been
+    // called yet (which is possible because UnregisterForLoopDestruction

[Nicolas Zea, 2014/10/17 00:04:02]

The comment that this is possible "because UnregisterForLoopDestrcution is a
direct call" seems a bit confusing to me here. In my mind, that's more an
explanation for the "else" case, where UnregisterForLoopDestruction called, set
the unregister_done_callback, and finished, all before this ever ran (because
this method was posted). 

[stanisc, 2014/10/17 06:58:02]

OK. Clarified/simplified these two comments.

+    // is a direct call while this method comes from a posted call).
+    // Add the observer and remember the message loop to be used later
+    // to post the unregister callback from UnregisterForLoopDestructionAsync.
+    base::MessageLoop::current()->AddDestructionObserver(this);
+    working_loop_ = base::MessageLoop::current();
+  } else {
+    // Rare case which is possible when the model type thread remains
+    // blocked for the entire session and UnregisterForLoopDestruction ends
+    // up being called before this method.
+    // In this case we skip the destruction observer registration
+    // and just invoke the callback postponed at UnregisterForLoopDestruction.
+    DCHECK(stopped_);
+    unregister_done_callback_.Run(GetModelSafeGroup());
+  }
 }
 
 void ModelSafeWorker::UnregisterForLoopDestruction(
     base::Callback<void(ModelSafeGroup)> unregister_done_callback) {
-  // Ok to wait until |working_loop_| is set because this is called on sync
-  // loop.
-  working_loop_set_wait_.Wait();
-
-  {
-    base::AutoLock l(working_loop_lock_);
-    if (working_loop_ != NULL) {
-      // Should be called on sync loop.
-      DCHECK_NE(base::MessageLoop::current(), working_loop_);
-      working_loop_->PostTask(
-          FROM_HERE,
-          base::Bind(&ModelSafeWorker::UnregisterForLoopDestructionAsync,
-                     this, unregister_done_callback));
-    }
+  base::AutoLock l(working_loop_lock_);
+  if (working_loop_ != NULL) {
+    // Normal case - observer registratio has been already done.

[Nicolas Zea, 2014/10/17 00:04:01]

nit: registratio -> registration

[stanisc, 2014/10/17 06:58:02]

Done.

+    // Delegate to the sync thread to do the actual unregistration in
+    // UnregisterForLoopDestructionAsync.
+    DCHECK_NE(base::MessageLoop::current(), working_loop_);
+    working_loop_->PostTask(
+        FROM_HERE,
+        base::Bind(&ModelSafeWorker::UnregisterForLoopDestructionAsync,
+                   this,
+                   unregister_done_callback));
+  } else {
+    // The working loop is still unknown, probably because the model type
+    // thread is blocked. Store the callback to be called from
+    // SetWorkingLoopToCurrent.
+    unregister_done_callback_ = unregister_done_callback;
   }
 }