[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..94fc4aa4cd9fadd01b9bd1ddf2fa3e84a892dd8f 100644
--- a/components/history/core/browser/history_model_worker.cc
+++ b/components/history/core/browser/history_model_worker.cc
@@ -6,29 +6,41 @@
 
 #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,
              syncer::ScopedEventSignal scoped_event_signal,
+             scoped_refptr<HistoryModelWorker> history_model_worker,
              syncer::SyncerError* error)
       : work_(work),
         scoped_event_signal_(std::move(scoped_event_signal)),
+        history_model_worker_(std::move(history_model_worker)),
         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.
     auto scoped_event_signal = std::move(scoped_event_signal_);
 
-    // Run the task.
-    *error_ = work_.Run();
+    // Run the task if RequestStop() hasn't been called.
+    if (history_model_worker_->WillRunWorkCallback()) {
+      *error_ = work_.Run();
+      history_model_worker_->DidRunWorkCallback();
+    } else {
+      *error_ = syncer::CANNOT_DO_WORK;
+    }
 
     return true;
   }
@@ -38,88 +50,84 @@ 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_;
   syncer::ScopedEventSignal scoped_event_signal_;
+  const scoped_refptr<HistoryModelWorker> history_model_worker_;
   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),
+      stop_requested_(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 {
+  work_done_or_abandoned_.Reset();
+
+  // Post a task to the UI thread that itself posts |work| to the history DB
+  // thread.
+  if (!ui_thread_->PostTask(
+          FROM_HERE,
+          base::Bind(
+              [](const base::WeakPtr<history::HistoryService>& history_service,

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

Optional: I love lambdas as much as anyone, but with four parameters with names
that are an underscore off from others in the surrounding scope, I think this is
actually less readable than the anonymous function that was here before...

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

Done.

+                 std::unique_ptr<history::HistoryDBTask> worker_task,
+                 base::CancelableTaskTracker* cancelable_tracker,
+                 syncer::SyncerError* error) {
+                if (history_service.get()) {

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

nit: do you need .get() here? WeakPtr implements the bool operator.

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

Done.

+                  history_service->ScheduleDBTask(std::move(worker_task),
+                                                  cancelable_tracker);
+                } else {
+                  *error = syncer::CANNOT_DO_WORK;
+                }
+              },
+              history_service_,
+              base::Passed(base::MakeUnique<WorkerTask>(
+                  work, syncer::ScopedEventSignal(&work_done_or_abandoned_),
+                  this, &error)),
+              base::Unretained(cancelable_tracker_.get()),
+              base::Unretained(&error)))) {
     error = syncer::CANNOT_DO_WORK;
+    return error;
   }
+
+  base::WaitableEvent* events[] = {&work_done_or_abandoned_, &stop_requested_};
+  base::WaitableEvent::WaitMany(events, arraysize(events));
+
   return error;
 }
 
+void HistoryModelWorker::RequestStop() {
+  DCHECK(ui_thread_->BelongsToCurrentThread());
+  ModelSafeWorker::RequestStop();
+
+  base::AutoLock auto_lock(lock_);
+
+  // If a WorkCallback is running on the history DB thread, do nothing.
+  // DoWorkAndWaitUntilDoneImpl() will return when the WorkCallback completes.
+  //
+  // Note: Returning from DoWorkAndWaitUntilDoneImpl() when a WorkCallback is
+  // running could result in invalid memory accesses.
+  if (work_callback_running_)
+    return;
+
+  // Else, signal |stop_requested_| to unblock DoWorkAndWaitUntilDoneImpl() and
+  // prevent any WorkCallback from being scheduled in the future.
+  stop_requested_.Signal();

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

"and prevent any WorkCallback from being scheduled in the future."

This makes me concerned that nothing tracks that stop was requested if it
happens while the work is running... do you need a bool that gets set regardless
and signals in DidRunWorkCallback()?

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

Done.

+}
+
 syncer::ModelSafeGroup HistoryModelWorker::GetModelSafeGroup() {
   return syncer::GROUP_HISTORY;
 }
@@ -138,4 +146,20 @@ HistoryModelWorker::~HistoryModelWorker() {
   ui_thread_->DeleteSoon(FROM_HERE, cancelable_tracker_.release());
 }
 
+bool HistoryModelWorker::WillRunWorkCallback() {
+  base::AutoLock auto_lock(lock_);
+
+  if (stop_requested_.IsSignaled())
+    return false;
+
+  work_callback_running_ = true;
+  return true;
+}
+
+void HistoryModelWorker::DidRunWorkCallback() {
+  base::AutoLock auto_lock(lock_);
+  DCHECK(work_callback_running_);
+  work_callback_running_ = false;
+}
+
 }  // namespace browser_sync