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

chrome/browser/sync/glue/sync_backend_registrar_unittest.cc

Index: chrome/browser/sync/glue/sync_backend_registrar_unittest.cc
diff --git a/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc b/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc
index cd2d29ddf4edfdb88cc9ecefb6feab6cab9e8586..c455772944fbd81eb144f56af7ae42a7888b50c2 100644
--- a/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc
+++ b/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/sync/glue/sync_backend_registrar.h"
 
+#include "base/run_loop.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"
@@ -256,6 +257,101 @@ TEST_F(SyncBackendRegistrarTest, ActivateDeactivateNonUIDataType) {
   TriggerChanges(registrar_.get(), AUTOFILL);
 }
 
+class SyncBackendRegistrarShutdownTest : public testing::Test {
+ public:
+  void BlockDBThread() {
+    EXPECT_FALSE(lock_.Try());
+
+    db_thread_blocked_.Signal();
+    base::AutoLock l(lock_);
+  }
+
+ protected:
+  friend class TestRegistrar;
+
+  SyncBackendRegistrarShutdownTest()
+      : thread_bundle_(content::TestBrowserThreadBundle::REAL_DB_THREAD |
+                       content::TestBrowserThreadBundle::REAL_FILE_THREAD |
+                       content::TestBrowserThreadBundle::REAL_IO_THREAD),
+        db_thread_blocked_(false, false) {}
+
+  virtual ~SyncBackendRegistrarShutdownTest() {}
+
+  void PostQuitOnUIMessageLoop() {
+    base::AutoLock l(lock_);

[Nicolas Zea, 2014/10/23 18:07:16]

given that you control the lifetime of the registrar in the test (and this is
only triggered after destruction), it might be cleaner to just assert that the
lock is not acquired here, rather than attempting to acquire.

[stanisc, 2014/10/23 19:21:57]

I've done this based on the thread sanitizer output. I'll see if moving
quit_closure to the constructor would make this unnecessary.

+    EXPECT_FALSE(quit_closure_.is_null());
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, quit_closure_);
+  }
+
+  content::TestBrowserThreadBundle thread_bundle_;
+  TestingProfile profile_;
+  base::WaitableEvent db_thread_blocked_;
+  // The lock is used to block the DB thread and also to access quit_closure_
+  base::Lock lock_;

[Nicolas Zea, 2014/10/23 18:07:16]

nit: perhaps name this db_thread_lock_?

[stanisc, 2014/10/23 19:21:57]

I renamed it because its purpose got extended to protect quit_closure_ to make
thread sanitizer happy. I'll test test again whether is that necessary. If not,
I'll rename it back to db_thread_lock_.

+  base::RunLoop run_loop_;
+  base::Closure quit_closure_;
+};
+
+// Wrap SyncBackendRegistrar so that we can monitor its lifetime.
+class TestRegistrar : public SyncBackendRegistrar {
+ public:
+  explicit TestRegistrar(
+      Profile* profile, SyncBackendRegistrarShutdownTest* test)
+      : SyncBackendRegistrar("test", profile, scoped_ptr<base::Thread>()),
+        test_(test) {}
+
+  ~TestRegistrar() override {
+    test_->PostQuitOnUIMessageLoop();
+  }
+
+ private:
+  SyncBackendRegistrarShutdownTest* test_;
+};
+
+TEST_F(SyncBackendRegistrarShutdownTest, BlockingShutdown) {
+  // Take ownership of |lock_| so that the DB thread can't acquire it.
+  lock_.Acquire();
+
+  quit_closure_ = run_loop_.QuitClosure();

[Nicolas Zea, 2014/10/23 18:07:16]

nit: may as well just put this in the constructor for the test

[stanisc, 2014/10/23 19:21:57]

Done.

+
+  // This will block the DB thread by waiting on |lock_|.
+  BrowserThread::PostTask(
+      BrowserThread::DB,
+      FROM_HERE,
+      base::Bind(&SyncBackendRegistrarShutdownTest::BlockDBThread,
+                 base::Unretained(this)));
+
+  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.
+  db_thread_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();

[Nicolas Zea, 2014/10/23 18:07:16]

Could you sanity check that this is necessary? SyncBackendRegistrar is calling
Thread::Start, which means the thread is actually running in parallel. I don't
think you need to call RunUntilIdle here.

[stanisc, 2014/10/23 19:21:57]

I think this is needed to ensure that the task posted above (running Shutdown)
gets executed and finishes without blocking. With the old implementation
RunUntilIdle would block.

+
+  lock_.Release();
+
+  // Run the main thread loop until all workers have been removed and the
+  // registrar destroyed.
+  run_loop_.Run();
+}
+
 }  // namespace
 
 }  // namespace browser_sync