From df486a3cbdad2025fff98fa497edb1261e83435c Mon Sep 17 00:00:00 2001
From: stanisc <stanisc@chromium.org>
Date: Thu, 23 Oct 2014 22:17:44 -0700
Subject: 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}
---
 .../public/engine/model_safe_worker.cc             | 65 +++++++++++++++-------
 1 file changed, 45 insertions(+), 20 deletions(-)

(limited to '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 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(
-- 
cgit v1.1