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

components/history/core/browser/history_model_worker.cc

Index: components/history/core/browser/history_model_worker.cc
diff --git a/components/history/core/browser/history_model_worker.cc b/components/history/core/browser/history_model_worker.cc
index 72b843dc9ffc765938f73116d1f5a49c1f27f4cd..13f76dfc8b51d143ac6ab835c0b5870438488e50 100644
--- a/components/history/core/browser/history_model_worker.cc
+++ b/components/history/core/browser/history_model_worker.cc
@@ -6,22 +6,29 @@
 
 #include <utility>
 
-#include "base/synchronization/waitable_event.h"
+#include "base/memory/ptr_util.h"
 #include "components/history/core/browser/history_db_task.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/sync/base/scoped_event_signal.h"
 
 namespace browser_sync {
 
-class WorkerTask : public history::HistoryDBTask {
+class HistoryModelWorker::WorkerTask : public history::HistoryDBTask {
  public:
   WorkerTask(const syncer::WorkCallback& work,
+             scoped_refptr<HistoryModelWorker> history_model_worker,
              syncer::ScopedEventSignal scoped_event_signal,
              syncer::SyncerError* error)
       : work_(work),
+        history_model_worker_(std::move(history_model_worker)),
         scoped_event_signal_(std::move(scoped_event_signal)),
         error_(error) {}
 
+  ~WorkerTask() override {
+    // The event in |scoped_event_signal_| is signaled at the end of this
+    // scope if this is destroyed before RunOnDBThread runs.
+  }
+
   bool RunOnDBThread(history::HistoryBackend* backend,
                      history::HistoryDatabase* db) override {
     // Signal the completion event at the end of this scope.
@@ -38,88 +45,72 @@ class WorkerTask : public history::HistoryDBTask {
   void DoneRunOnMainThread() override {}
 
  protected:
-  ~WorkerTask() override {
-    // The event in |scoped_event_signal_| is signaled at the end of this
-    // scope if this is destroyed before RunOnDBThread runs.
-  }
-
   syncer::WorkCallback work_;
+  const scoped_refptr<HistoryModelWorker> history_model_worker_;

[maxbogue, 2017/03/20 19:55:50]

No longer needed I think? Or is this just to ensure the worker doesn't
disappear? If that's the case please add a comment.

[fdoray, 2017/03/21 17:21:31]

Added comment. This is required to make sure that the event in
|scoped_event_signal_| isn't deleted.

   syncer::ScopedEventSignal scoped_event_signal_;
   syncer::SyncerError* error_;
 };
 
-class AddDBThreadObserverTask : public history::HistoryDBTask {
- public:
-  explicit AddDBThreadObserverTask(base::Closure register_callback)
-     : register_callback_(register_callback) {}
-
-  bool RunOnDBThread(history::HistoryBackend* backend,
-                     history::HistoryDatabase* db) override {
-    register_callback_.Run();
-    return true;
-  }
-
-  void DoneRunOnMainThread() override {}
-
- private:
-  ~AddDBThreadObserverTask() override {}
-
-  base::Closure register_callback_;
-};
-
-namespace {
-
-// Post the work task on |history_service|'s DB thread from the UI
-// thread.
-void PostWorkerTask(
-    const base::WeakPtr<history::HistoryService>& history_service,
-    const syncer::WorkCallback& work,
-    syncer::ScopedEventSignal scoped_event_signal,
-    base::CancelableTaskTracker* cancelable_tracker,
-    syncer::SyncerError* error) {
-  if (history_service.get()) {
-    std::unique_ptr<history::HistoryDBTask> task(
-        new WorkerTask(work, std::move(scoped_event_signal), error));
-    history_service->ScheduleDBTask(std::move(task), cancelable_tracker);
-  } else {
-    *error = syncer::CANNOT_DO_WORK;
-    // The event in |scoped_event_signal| is signaled at the end of this
-    // scope.
-  }
-}
-
-}  // namespace
-
 HistoryModelWorker::HistoryModelWorker(
     const base::WeakPtr<history::HistoryService>& history_service,
     const scoped_refptr<base::SingleThreadTaskRunner>& ui_thread)
-    : history_service_(history_service), ui_thread_(ui_thread) {
+    : history_service_(history_service),
+      ui_thread_(ui_thread),
+      work_done_or_abandoned_(base::WaitableEvent::ResetPolicy::MANUAL,
+                              base::WaitableEvent::InitialState::NOT_SIGNALED) {
   CHECK(history_service.get());
   DCHECK(ui_thread_->BelongsToCurrentThread());
+  sequence_checker_.DetachFromSequence();
   cancelable_tracker_.reset(new base::CancelableTaskTracker);
 }
 
 syncer::SyncerError HistoryModelWorker::DoWorkAndWaitUntilDoneImpl(
     const syncer::WorkCallback& work) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
   syncer::SyncerError error = syncer::UNSET;
 
-  // Signaled after the task runs or when it is abandoned.
-  base::WaitableEvent work_done_or_abandoned(
-      base::WaitableEvent::ResetPolicy::AUTOMATIC,
-      base::WaitableEvent::InitialState::NOT_SIGNALED);
-
-  if (ui_thread_->PostTask(FROM_HERE,
-                           base::Bind(&PostWorkerTask, history_service_, work,
-                                      base::Passed(syncer::ScopedEventSignal(
-                                          &work_done_or_abandoned)),
-                                      cancelable_tracker_.get(), &error))) {
-    work_done_or_abandoned.Wait();
-  } else {
-    error = syncer::CANNOT_DO_WORK;
+  {
+    // If RequestStop() was called, return immediately. Otherwise, set a flag so
+    // that this method returns immediately if it is waiting for a UI task and
+    // RequestStop() is called (the UI thread doesn't run tasks after that).
+    base::AutoLock auto_lock(lock_);
+    if (IsStopped())

[maxbogue, 2017/03/20 19:55:50]

Is this needed? IsStopped() is checked immediately before this function is
called:

https://cs.chromium.org/chromium/src/components/sync/engine/model_safe_worker...

[fdoray, 2017/03/21 17:21:31]

Checking IsStopped() and resetting |work_done_or_abandoned_| must be done
atomically. Added comment.

+      return syncer::CANNOT_DO_WORK;
+    work_done_or_abandoned_.Reset();
+    DCHECK(!ui_task_pending_);
+    ui_task_pending_ = true;
+  }
+
+  // Post a task to the UI thread that itself posts |work| to the history DB
+  // thread.
+  if (!ui_thread_->PostTask(
+          FROM_HERE,
+          base::Bind(
+              &HistoryModelWorker::ScheduleHistoryDBTaskFromUIThread, this,
+              base::Passed(base::MakeUnique<WorkerTask>(
+                  work, this,
+                  syncer::ScopedEventSignal(&work_done_or_abandoned_), &error)),
+              base::Unretained(&error)))) {
+    return syncer::CANNOT_DO_WORK;
   }
+
+  work_done_or_abandoned_.Wait();
+
   return error;
 }
 
+void HistoryModelWorker::RequestStop() {
+  DCHECK(ui_thread_->BelongsToCurrentThread());
+
+  base::AutoLock auto_lock(lock_);
+  ModelSafeWorker::RequestStop();
+
+  // The UI thread doesn't run tasks after RequestStop(). Therefore, unblock
+  // DoWorkAndWaitUntilDoneImpl() if it is waiting for a UI task.
+  if (ui_task_pending_)
+    work_done_or_abandoned_.Signal();
+}
+
 syncer::ModelSafeGroup HistoryModelWorker::GetModelSafeGroup() {
   return syncer::GROUP_HISTORY;
 }
@@ -138,4 +129,19 @@ HistoryModelWorker::~HistoryModelWorker() {
   ui_thread_->DeleteSoon(FROM_HERE, cancelable_tracker_.release());
 }
 
+void HistoryModelWorker::ScheduleHistoryDBTaskFromUIThread(
+    std::unique_ptr<history::HistoryDBTask> worker_task,
+    syncer::SyncerError* error) {
+  if (history_service_) {
+    history_service_->ScheduleDBTask(std::move(worker_task),
+                                     cancelable_tracker_.get());
+  } else {
+    *error = syncer::CANNOT_DO_WORK;
+  }
+
+  base::AutoLock auto_lock(lock_);
+  DCHECK(ui_task_pending_);
+  ui_task_pending_ = false;
+}
+
 }  // namespace browser_sync