[Sync] Signal UIModelWorker to abort on sync shutdown.

components/sync/driver/glue/ui_model_worker_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 "components/sync/driver/glue/ui_model_worker.h"
 
+#include <memory>
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
+#include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
+#include "base/test/test_timeouts.h"
+#include "base/threading/platform_thread.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace syncer {
 namespace {
 
 // Makes a Closure into a WorkCallback.
 // Does |work| and checks that we're on the |thread_verifier| thread.
 SyncerError DoWork(
     const scoped_refptr<base::SingleThreadTaskRunner>& thread_verifier,
     base::Closure work) {
   DCHECK(thread_verifier->BelongsToCurrentThread());
   work.Run();
   return SYNCER_OK;
 }
 
 // Converts |work| to a WorkCallback that will verify that it's run on the
 // thread it was constructed on.
 WorkCallback ClosureToWorkCallback(base::Closure work) {
   return base::Bind(&DoWork, base::ThreadTaskRunnerHandle::Get(), work);
 }
 
+// Increments |counter|.
+void IncrementCounter(int* counter) {
+  ++*counter;
+}
+
 class SyncUIModelWorkerTest : public testing::Test {
  public:
   SyncUIModelWorkerTest() : sync_thread_("SyncThreadForTest") {
     sync_thread_.Start();
     worker_ = new UIModelWorker(base::ThreadTaskRunnerHandle::Get());
   }
 
-  void PostWorkToSyncThread(WorkCallback work) {
+  void PostWorkToSyncThread(base::Closure work) {
     sync_thread_.task_runner()->PostTask(
         FROM_HERE,
         base::Bind(base::IgnoreResult(&UIModelWorker::DoWorkAndWaitUntilDone),
-                   worker_, work));
+                   worker_, ClosureToWorkCallback(work)));
   }
 
- private:
-  base::MessageLoop ui_loop_;
+ protected:
+  std::unique_ptr<base::MessageLoop> ui_loop_ =
+      base::MakeUnique<base::MessageLoop>();
   base::Thread sync_thread_;
   scoped_refptr<UIModelWorker> worker_;
 };
 
+}  // namespace
+
 TEST_F(SyncUIModelWorkerTest, ScheduledWorkRunsOnUILoop) {
   base::RunLoop run_loop;
-  PostWorkToSyncThread(ClosureToWorkCallback(run_loop.QuitClosure()));
+  PostWorkToSyncThread(run_loop.QuitClosure());
   // This won't quit until the QuitClosure is run.
   run_loop.Run();
 }
 
-}  // namespace
+TEST_F(SyncUIModelWorkerTest, MultipleDoWork) {
+  constexpr int kNumWorkCallbacks = 10;
+  int counter = 0;
+  for (int i = 0; i < kNumWorkCallbacks; ++i) {
+    PostWorkToSyncThread(
+        base::Bind(&IncrementCounter, base::Unretained(&counter)));
+  }
+
+  base::RunLoop run_loop;
+  PostWorkToSyncThread(run_loop.QuitClosure());
+  run_loop.Run();
+
+  EXPECT_EQ(kNumWorkCallbacks, counter);
+}
+
+TEST_F(SyncUIModelWorkerTest, JoinSyncThreadAfterUIMessageLoopDestruction) {
+  PostWorkToSyncThread(base::Bind(&base::DoNothing));
+
+  // Allow the sync thread to post the WorkCallback to the UI MessageLoop.
+  base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());

[maxbogue, 2016/11/18 18:26:51]

Ooo, interesting. Didn't know we had anything like this. This isn't racy?

[fdoray, 2016/11/21 16:57:56]

Yes, it's racy. I explained it in the comment.

Joining the sync thread within the callback like I asked you to do here
https://codereview.chromium.org/2496723003/diff/20001/components/sync/driver/...
doesn't work. It causes a write-after-free when |error_info| is accessed here
https://codereview.chromium.org/2505913003/diff/60001/components/sync/driver/...

+
+  // The sync thread shouldn't wait for the WorkCallback to run on the UI thread
+  // after the UI MessageLoop is gone.
+  ui_loop_.reset();
+  sync_thread_.Stop();
+}
+
+TEST_F(SyncUIModelWorkerTest, JoinSyncThreadAfterRequestStop) {
+  PostWorkToSyncThread(base::Bind(&base::DoNothing));
+
+  // Allow the sync thread to post the WorkCallback to the UI MessageLoop.
+  base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());
+
+  // The sync thread shouldn't wait for the WorkCallback to run on the UI thread
+  // after RequestStop() is called.
+  worker_->RequestStop();
+  sync_thread_.Stop();
+}
+
 }  // namespace syncer