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

chrome/browser/sync/glue/sync_backend_registrar_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/glue/sync_backend_registrar.h"
 
 #include "chrome/browser/sync/glue/ui_model_worker.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/sync_driver/change_processor_mock.h"
 #include "content/public/browser/browser_thread.h"
@@ skipping 238 matching lines @@
   done.Wait();
 
   registrar_->DeactivateDataType(AUTOFILL);
   ExpectRoutingInfo(registrar_.get(), syncer::ModelSafeRoutingInfo());
   ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet());
 
   // Should do nothing.
   TriggerChanges(registrar_.get(), AUTOFILL);
 }
 
+class SyncBackendRegistrarShutdownTest : public testing::Test {
+ public:
+  void DBThreadBlocker() {

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

nit: I think it would be clearer to call this method "BlockDBThread". I.e. as an
action

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

Done.

+    EXPECT_FALSE(ui_thread_lock_.Try());
+
+    blocked_.Signal();
+    base::AutoLock l(ui_thread_lock_);
+  }
+
+  // Wrap SyncBackendRegistrar so that we can monitor its lifetime.
+  class TestRegistrar : public SyncBackendRegistrar {

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

nit: The usual style is to avoid nested classes like this. I'd just declare it
outside the class and friend it.

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

Well, nested classes are supposed to be used for situations like this but I have
no problem moving this outside.
Done.

+   public:
+    explicit TestRegistrar(Profile* profile,
+                           SyncBackendRegistrarShutdownTest* test)
+        : SyncBackendRegistrar("test", profile, scoped_ptr<base::Thread>()),
+          test_(test) {}
+
+    virtual ~TestRegistrar() { test_->registrar_destroyed_.Signal(); }
+
+   private:
+    SyncBackendRegistrarShutdownTest* test_;
+  };
+
+ protected:
+  SyncBackendRegistrarShutdownTest()
+      : thread_bundle_(content::TestBrowserThreadBundle::REAL_DB_THREAD |
+                       content::TestBrowserThreadBundle::REAL_FILE_THREAD |
+                       content::TestBrowserThreadBundle::REAL_IO_THREAD),
+        blocked_(false, false),
+        registrar_destroyed_(false, false) {}
+
+  virtual ~SyncBackendRegistrarShutdownTest() {}
+
+  content::TestBrowserThreadBundle thread_bundle_;
+  base::WaitableEvent blocked_;

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

nit: perhaps name this db_thread_blocked_

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

Done.

+  base::Lock ui_thread_lock_;
+  base::WaitableEvent registrar_destroyed_;
+};
+
+TEST_F(SyncBackendRegistrarShutdownTest, BlockingShutdown) {
+  // Lock |ui_thread_lock_| so that the DB thread can't acquire it.

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

nit: because the ui_thread_lock_ is actually being used to block the db thread,
I think it might be cleaner to call it db_thread_lock_

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

Agree! Done.

+  ui_thread_lock_.Acquire();
+
+  // Block the DB thread by waiting on |ui_thread_lock_|.
+  BrowserThread::PostTask(
+      BrowserThread::DB,
+      FROM_HERE,
+      base::Bind(&SyncBackendRegistrarShutdownTest::DBThreadBlocker,
+                 base::Unretained(this)));
+
+  TestingProfile profile;
+  scoped_ptr<TestRegistrar> registrar(new TestRegistrar(&profile, this));
+  base::Thread* sync_thread = registrar->sync_thread();
+
+  // Stop here until the DB thread gets a chance to run and block on the lock.
+  // Please note that since the task above didn't finish, the task to
+  // initialize the worker on the DB thread hasn't had a chance to run yet too.
+  // Which means ModelSafeWorker::SetWorkingLoopToCurrent hasn't been called
+  // for the DB worker.
+  blocked_.Wait();
+
+  registrar->SetInitialTypes(ModelTypeSet());
+
+  // Start the shutdown.
+  registrar->RequestWorkerStopOnUIThread();
+  sync_thread->message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncBackendRegistrar::Shutdown,
+                 base::Unretained(registrar.release())));
+
+  // The test verifies that the sync thread doesn't block because
+  // of the blocked DB thread and can finish the shutdown.
+  sync_thread->message_loop()->RunUntilIdle();
+
+  ui_thread_lock_.Release();
+
+  // BrowserThread::UnsafeGetMessageLoopForThread(BrowserThread::DB)->
+  //    RunUntilIdle();

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

did you mean to keep this around?

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

I ended up not needing this. Removed.

+
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // This verifies that all workers have been removed and the registrar
+  // destroyed.
+  registrar_destroyed_.Wait();
+}
+
 }  // namespace
 
 }  // namespace browser_sync