[Sync] Signal UIModelWorker to abort on sync shutdown.

components/sync/driver/glue/ui_model_worker_unittest.cc

Index: components/sync/driver/glue/ui_model_worker_unittest.cc
diff --git a/components/sync/driver/glue/ui_model_worker_unittest.cc b/components/sync/driver/glue/ui_model_worker_unittest.cc
index d091f317a5c60f2896c0a05ec84abbea9095a730..ce401bf1684dbbf5f6bf38c24534b3c51b61573b 100644
--- a/components/sync/driver/glue/ui_model_worker_unittest.cc
+++ b/components/sync/driver/glue/ui_model_worker_unittest.cc
@@ -32,6 +32,15 @@ WorkCallback ClosureToWorkCallback(base::Closure work) {
   return base::Bind(&DoWork, base::ThreadTaskRunnerHandle::Get(), work);
 }
 
+// This function runs on the sync thread.
+void SignalAndDoWork(scoped_refptr<UIModelWorker> worker,
+                     WorkCallback work,
+                     base::WaitableEvent* event) {
+  if (event)

[fdoray, 2016/11/14 14:24:34]

EXPECT_TRUE(event) since we don't expect |event| to be nullptr

[maxbogue, 2016/11/14 20:53:21]

n/a, event is no longer needed.

+    event->Signal();
+  worker->DoWorkAndWaitUntilDone(work);
+}
+
 class SyncUIModelWorkerTest : public testing::Test {
  public:
   SyncUIModelWorkerTest() : sync_thread_("SyncThreadForTest") {
@@ -39,25 +48,54 @@ class SyncUIModelWorkerTest : public testing::Test {
     worker_ = new UIModelWorker(base::ThreadTaskRunnerHandle::Get());
   }
 
-  void PostWorkToSyncThread(WorkCallback work) {
+  void PostWorkToSyncThread(base::Closure work,
+                            base::WaitableEvent* task_on_sync_thread_event) {
     sync_thread_.task_runner()->PostTask(
         FROM_HERE,
-        base::Bind(base::IgnoreResult(&UIModelWorker::DoWorkAndWaitUntilDone),
-                   worker_, work));
+        base::Bind(&SignalAndDoWork, worker_, ClosureToWorkCallback(work),
+                   task_on_sync_thread_event));
   }
 
+  void StopSyncThread() { sync_thread_.Stop(); }
+
+  UIModelWorker* worker() { return worker_.get(); }
+
  private:
+  // Worker must outlive message loop.
+  scoped_refptr<UIModelWorker> worker_;
   base::MessageLoop ui_loop_;
   base::Thread sync_thread_;
-  scoped_refptr<UIModelWorker> worker_;
 };
 
 TEST_F(SyncUIModelWorkerTest, ScheduledWorkRunsOnUILoop) {
   base::RunLoop run_loop;
-  PostWorkToSyncThread(ClosureToWorkCallback(run_loop.QuitClosure()));
+  PostWorkToSyncThread(run_loop.QuitClosure(), nullptr);
   // This won't quit until the QuitClosure is run.
   run_loop.Run();
 }
 
+TEST_F(SyncUIModelWorkerTest, ShutdownNoDeadlock) {
+  // We need some way to wait until the sync thread has our task.
+  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::AUTOMATIC,
+                            base::WaitableEvent::InitialState::NOT_SIGNALED);
+
+  PostWorkToSyncThread(base::Bind(&SyncUIModelWorkerTest::StopSyncThread,
+                                  base::Unretained(this)),
+                       &event);
+
+  // Wait until the sync thread has started our task.
+  event.Wait();
+
+  // This is a little racy. We're banking on this stop signal to occur after the
+  // sync thread has progressed past the stopped check. However, if it doesn't,
+  // we just get a false positive (no flakes).

[fdoray, 2016/11/14 14:24:34]

I think the test below avoids false positives (the test hangs unless it goes
through the stopped_.IsSet() check in ModelSafeWorker::DoWorkAndWaitUntilDone,
posts the WorkCallback to the UI thread, starts waiting for the WorkCallback and
stops waiting when RequestStop is invoked).

class SyncUIModelWorkerTest : public testing::Test {
  ...

  void StopSyncThread(Closure quit_closure) {
    // This should cause the sync thread to stop waiting for
    // this WorkCallback.
    worker()->RequestStop();

    // This should not deadlock since the sync thread stopped
    // waiting for this WorkCallback.
    sync_thread_.Stop();

    // This should unblock the run_loop.Run() call in the test body.
    quit_closure.Run();
  }

  ...
};

TEST_F(SyncUIModelWorkerTest, ShutdownNoDeadlock) {
  base::RunLoop run_loop;

  PostWorkToSyncThread(base::Bind(&SyncUIModelWorkerTest::StopSyncThread,
                                  base::Unretained(this),
                                  run_loop.QuitClosure()));

  // This blocks until run_loop.QuitClosure() is invoked
  // by the WorkCallback above.
  run_loop.Run();  
}

[maxbogue, 2016/11/14 20:53:21]

Of course, just do everything inside the work closure. Clever, thanks!

+  worker()->RequestStop();
+
+  // Now we yield control of the UI thread. The stop signal has unblocked the
+  // sync thread, so the UI thread task can join the sync thread without
+  // deadlocking.
+  base::RunLoop().RunUntilIdle();
+}
+
 }  // namespace
 }  // namespace syncer