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

This reverts commit 8e84bfcc1c33fc0b63392d48ee8288d73f4a7675.

Original CL: https://codereview.chromium.org/637413003
Reason for Revert:
New test SyncBackendRegistrarShutdownTest.BlockingShutdown fails repeatedly
on android and causes other tests to hang.
Also fails on other bots.
http://chromium-build-logs.appspot.com/gtest_query?gtest_query=SyncBackendRegistrarShutdownTest.BlockingShutdown
Failures are timeouts, and don't have any useful logs or stack traces (sorry).

Reason for hand-revert: Conflict with crrev.com/0cd9ff92b2dbc83fe8c3664b48041c7ced683c9c

NOTRY=true
TBR=stanisc@chromium.org
TBR=zea@chromium.org

-- Original CL Description --

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

This deadlock involves SyncBackendRegistrar running on the sync thread and at least two
workers - one of them (worker A) running on UI thread and
another (worker B) running on some other background thread.
In this particular case it was a password worker running on Password model thread.

The deadlock occurs when the SyncBackendRegistrar shutdown
code running in the sync thread blocks on a waitable event
set by worker B, at the same time the worker B's thread is
blocked for whatever reason waiting to make a synchronous
call into the UI thread, and at the same time the worker A
is blocked on a lock owned by the sync thread while it attempts
to unregister itself with SyncBackendRegistrar.

I think the main issue here is that the sync thread might
block on a worker. This is possible in only one situation -
when that worker hasn't have a chance to run any tasks because
its own thread was blocked doing something else.

The fix removes this dependency. The only reason this
blocking occurs is because ModelSafeWorker has a mechanism
for subscribing / unsubscribing to the MessageLoop descruction
which has to be done in the right thread context.
In order to unsubscribe the worker needs to remember the message loop it runs on which is done
in SetWorkingLoopToCurrent callback, which is called from a task
posted on the worker's thread. There is also a waitable
event which is signalled at that time. The even is used to
block ModelSafeWorker::UnregisterForLoopDestruction from
delegating the call on an uninitialized message loop which
is possible only when SetWorkingLoopToCurrent has never been called.

There is no need to block in UnregisterForLoopDestruction.
If the registration for even loop destruction hasn't been
been done yet (because the thread is blocked) then there
really no reason to wait for that and then immediately
unsubscribe. We can skip both subscription to the even loop
notification altogether and let the registrar know that
it is OK to remove the worker. That's essentially what the
fix does.

I added a test that verifies that SyncBackendRegistrar::Shutdown
doesn't block on a blocked model type thread anymore.

BUG=423078
Committed: https://crrev.com/8e84bfcc1c33fc0b63392d48ee8288d73f4a7675
Cr-Commit-Position: refs/heads/master@{#300350}

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

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

1 files changed, 18 insertions, 34 deletions

diff --git a/sync/internal_api/public/engine/model_safe_worker.cc b/sync/internal_api/public/engine/model_safe_worker.cc
index 2aea5cf..eb3fc1f 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_(NULL),
+      working_loop_set_wait_(true, false) {}
 
 ModelSafeWorker::~ModelSafeWorker() {}
 
@@ -135,42 +135,26 @@ void ModelSafeWorker::WillDestroyCurrentMessageLoop() {
 void ModelSafeWorker::SetWorkingLoopToCurrent() {
   base::AutoLock l(working_loop_lock_);
   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_.Run(GetModelSafeGroup());
-  }
+  working_loop_ = base::MessageLoop::current();
+  working_loop_set_wait_.Signal();
 }
 
 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;
+  // 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));
+    }
   }
 }