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

This is a resubmit of the previous fix that has been reverted due to the
flaky unittest that would hang under the load:
https://codereview.chromium.org/637413003

Please see delta between patch sets 1 and 2 for the
unit test fix.
The test would hang due to a race condition between
MessageLoop::RunUntilIdle call for the main UI thread and
submitting a task on the UI thread from SyncBackendRegistrar running on the sync thread.
The new test implementation runs the UI thread loop continuously
until the expected result is reached so that eliminates the
race condition and makes the test simpler.

I verified the fix by running the test while running
Chromium build in background. Using this method I reliably
reproduced the hang with the first implementation of the test.
This version of the test works reliably under the same condition.

BUG=423078

Review URL: https://codereview.chromium.org/640853008

Cr-Commit-Position: refs/heads/master@{#301056}

3 files changed, 52 insertions, 22 deletions

diff --git a/sync/internal_api/public/engine/model_safe_worker.cc b/sync/internal_api/public/engine/model_safe_worker.cc
index eb3fc1f..ea8bd4b 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() {}
 
@@ -133,29 +133,54 @@ 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();
-}
-
-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::Callback<void(ModelSafeGroup)> unregister_done_callback;
 
   {
     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));
+    DCHECK(!working_loop_);
+
+    if (unregister_done_callback_.is_null()) {
+      // Expected case - UnregisterForLoopDestruction hasn't been called yet.
+      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. This method is posted unlike
+      // UnregisterForLoopDestruction - that's why they can end up being called
+      // out of order.
+      // In this case we skip the destruction observer registration
+      // and just invoke the callback stored at UnregisterForLoopDestruction.
+      DCHECK(stopped_);
+      unregister_done_callback = unregister_done_callback_;
+      unregister_done_callback_.Reset();
     }
   }
+
+  if (!unregister_done_callback.is_null()) {
+    unregister_done_callback.Run(GetModelSafeGroup());
+  }
+}
+
+void ModelSafeWorker::UnregisterForLoopDestruction(
+    base::Callback<void(ModelSafeGroup)> unregister_done_callback) {
+  base::AutoLock l(working_loop_lock_);
+  if (working_loop_ != NULL) {
+    // Normal case - observer registration has been already 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;
+  }
 }
 
 void ModelSafeWorker::UnregisterForLoopDestructionAsync(
diff --git a/sync/internal_api/public/engine/model_safe_worker.h b/sync/internal_api/public/engine/model_safe_worker.h
index 03970b9..ad84d49 100644
--- a/sync/internal_api/public/engine/model_safe_worker.h
+++ b/sync/internal_api/public/engine/model_safe_worker.h
@@ -133,7 +133,13 @@ class SYNC_EXPORT ModelSafeWorker
   // observation from sync thread when shutting down sync.
   base::Lock working_loop_lock_;
   base::MessageLoop* working_loop_;
-  base::WaitableEvent working_loop_set_wait_;
+
+  // Callback passed with UnregisterForLoopDestruction. Normally this
+  // remains unset/unused and is stored only if |working_loop_| isn't
+  // initialized by the time UnregisterForLoopDestruction is called.
+  // It is safe to copy and thread safe.
+  // See comments in model_safe_worker.cc for more details.
+  base::Callback<void(ModelSafeGroup)> unregister_done_callback_;
 };
 
 // A map that details which ModelSafeGroup each ModelType
diff --git a/sync/internal_api/public/engine/passive_model_worker.cc b/sync/internal_api/public/engine/passive_model_worker.cc
index 02036d5..3117194 100644
--- a/sync/internal_api/public/engine/passive_model_worker.cc
+++ b/sync/internal_api/public/engine/passive_model_worker.cc
@@ -18,7 +18,6 @@ PassiveModelWorker::~PassiveModelWorker() {
 }
 
 void PassiveModelWorker::RegisterForLoopDestruction() {
-  base::MessageLoop::current()->AddDestructionObserver(this);
   SetWorkingLoopToCurrent();
 }