[Sync] Do not deadlock when joining sync thread with a pending HistoryModelWorker task.

components/history/core/browser/history_model_worker_unittest.cc

Index: components/history/core/browser/history_model_worker_unittest.cc
diff --git a/components/history/core/browser/history_model_worker_unittest.cc b/components/history/core/browser/history_model_worker_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..cf67a27657153e0b1b54054f70316b50344c7b83
--- /dev/null
+++ b/components/history/core/browser/history_model_worker_unittest.cc
@@ -0,0 +1,172 @@
+// Copyright 2017 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/history/core/browser/history_model_worker.h"
+
+#include <memory>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
+#include "base/synchronization/atomic_flag.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/threading/platform_thread.h"
+#include "base/threading/thread.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "components/history/core/browser/history_db_task.h"
+#include "components/history/core/browser/history_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace browser_sync {
+namespace {
+
+class HistoryServiceMock : public history::HistoryService {
+ public:
+  HistoryServiceMock() = default;
+
+  base::CancelableTaskTracker::TaskId ScheduleDBTask(
+      std::unique_ptr<history::HistoryDBTask> task,
+      base::CancelableTaskTracker* tracker) override {
+    history::HistoryDBTask* task_raw = task.get();
+    task_runner_->PostTaskAndReply(
+        FROM_HERE,
+        base::Bind(base::IgnoreResult(&history::HistoryDBTask::RunOnDBThread),
+                   base::Unretained(task_raw), nullptr, nullptr),
+        base::Bind(&history::HistoryDBTask::DoneRunOnMainThread,
+                   base::Passed(std::move(task))));
+    return base::CancelableTaskTracker::kBadTaskId;  // Unused.
+  }
+
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner() const {
+    return task_runner_;
+  }
+
+ private:
+  const scoped_refptr<base::TestSimpleTaskRunner> task_runner_ =
+      new base::TestSimpleTaskRunner();
+
+  DISALLOW_COPY_AND_ASSIGN(HistoryServiceMock);
+};
+
+syncer::WorkCallback ClosureToWorkCallback(base::Closure work) {
+  return base::Bind(
+      [](scoped_refptr<base::SingleThreadTaskRunner> thread_verifier,
+         base::Closure work) {
+        EXPECT_TRUE(thread_verifier->BelongsToCurrentThread());
+        work.Run();
+        return syncer::SYNCER_OK;
+      },
+      base::ThreadTaskRunnerHandle::Get(), std::move(work));
+}
+
+class HistoryModelWorkerTest : public testing::Test {
+ public:
+  HistoryModelWorkerTest()
+      : thread_task_runner_handle_(ui_thread_task_runner_),
+        sync_thread_("SyncThreadForTest"),
+        history_service_factory_(&history_service_) {
+    sync_thread_.Start();
+    worker_ = new HistoryModelWorker(history_service_factory_.GetWeakPtr(),
+                                     ui_thread_task_runner_);
+  }
+
+ protected:
+  void DoWorkAndWaitUntilDoneOnSyncThread(base::Closure work) {
+    sync_thread_.task_runner()->PostTask(
+        FROM_HERE, base::Bind(base::IgnoreResult(
+                                  &HistoryModelWorker::DoWorkAndWaitUntilDone),
+                              worker_, ClosureToWorkCallback(work)));
+  }
+
+  scoped_refptr<base::TestSimpleTaskRunner> ui_thread_task_runner_ =
+      new base::TestSimpleTaskRunner();
+
+  scoped_refptr<HistoryModelWorker> worker_;
+
+  base::Thread sync_thread_;
+
+  base::ThreadTaskRunnerHandle thread_task_runner_handle_;

[maxbogue, 2017/03/20 16:52:37]

Does this overwrite base::ThreadTaskRunnerHandle::Get() for this thread? Could
you comment that if so?

[fdoray, 2017/03/20 18:54:54]

Done.

+
+  HistoryServiceMock history_service_;
+
+ private:
+  base::WeakPtrFactory<HistoryServiceMock> history_service_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(HistoryModelWorkerTest);
+};
+
+}  // namespace
+
+TEST_F(HistoryModelWorkerTest, JoinSyncThreadAfterUITaskDestroyed) {
+  // Post a DoWorkAndWaitUntilDone() task to the sync thread.
+  // DoWorkAndWaitUntilDone() posts a task to the UI thread that itself posts a
+  // task to the history DB thread. It blocks until either the history DB task
+  // runs or is abandoned.
+  DoWorkAndWaitUntilDoneOnSyncThread(base::Bind(&base::DoNothing));
+
+  // Wait until DoWorkAndWaitUntilDone() posts to the UI thread.
+  while (!ui_thread_task_runner_->HasPendingTask())
+    base::PlatformThread::YieldCurrentThread();
+
+  // Destroying the task posted to the UI thread should unblock
+  // DoWorkAndWaitUntilDone() on the sync thread.
+  ui_thread_task_runner_->ClearPendingTasks();
+  sync_thread_.Stop();
+}
+
+TEST_F(HistoryModelWorkerTest, JoinSyncThreadAfterRequestStop) {
+  // Post a DoWorkAndWaitUntilDone() task to the sync thread.
+  // DoWorkAndWaitUntilDone() posts a task to the UI thread that itself posts a
+  // task to the history DB thread. It blocks until either the history DB task
+  // runs or is abandoned.
+  DoWorkAndWaitUntilDoneOnSyncThread(base::Bind(&base::DoNothing));
+
+  // Wait until DoWorkAndWaitUntilDone() posts to the UI thread.
+  while (!ui_thread_task_runner_->HasPendingTask())
+    base::PlatformThread::YieldCurrentThread();
+
+  // Calling RequestStop() on HistoryModelWorker should unblock
+  // DoWorkAndWaitUntilDone() on the sync thread.
+  worker_->RequestStop();
+  sync_thread_.Stop();
+}
+
+TEST_F(HistoryModelWorkerTest, RequestStopAfterUITaskBeforeHistoryTask) {
+  base::AtomicFlag do_work_and_wait_until_done_unblocked;
+
+  // Post a DoWorkAndWaitUntilDone() task to the sync thread.
+  // DoWorkAndWaitUntilDone() posts a task to the UI thread that itself posts a
+  // task to the history DB thread. It blocks until either the history DB task
+  // runs or is abandoned.
+  DoWorkAndWaitUntilDoneOnSyncThread(base::Bind(
+      [](base::AtomicFlag* do_work_and_wait_until_done_unblocked) {
+        // DoWorkAndWaitUntilDone() should not return while a WorkCallback is
+        // running on the history DB thread.
+        EXPECT_FALSE(do_work_and_wait_until_done_unblocked->IsSet());
+      },
+      base::Unretained(&do_work_and_wait_until_done_unblocked)));
+  sync_thread_.task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&base::AtomicFlag::Set,
+                 base::Unretained(&do_work_and_wait_until_done_unblocked)));
+
+  // Wait until DoWorkAndWaitUntilDone() posts to the UI thread and run it.
+  while (!ui_thread_task_runner_->HasPendingTask())
+    base::PlatformThread::YieldCurrentThread();
+  ui_thread_task_runner_->RunUntilIdle();
+
+  // The UI thread should have posted a task to the history DB thread. Run it to
+  // unblock DoWorkAndWaitUntilDone() on the sync thread.
+  EXPECT_TRUE(history_service_.task_runner()->HasPendingTask());
+  history_service_.task_runner()->RunUntilIdle();
+
+  sync_thread_.Stop();

[maxbogue, 2017/03/20 16:52:37]

Is this guaranteed to run the task posted to it or do you need to pump it
beforehand? The docs are surprisingly vague on that point and I can't
remember...

[fdoray, 2017/03/20 18:54:54]

Done.

+
+  EXPECT_TRUE(do_work_and_wait_until_done_unblocked.IsSet());
+}
+
+}  // namespace browser_sync