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

components/history/core/browser/history_model_worker.cc

 // Copyright 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/history/core/browser/history_model_worker.h"
 
 #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;
   }
 
   // Since the DoWorkAndWaitUntilDone() is synchronous, we don't need to run
   // any code asynchronously on the main thread after completion.
   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;
+  work_done_or_abandoned_.Reset();
 
-  // 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);
+  // 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;
+  }
 
-  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;
-  }
+  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;
 }
 
 bool HistoryModelWorker::IsOnModelThread() {
   // Ideally HistoryService would expose a way to check whether this is the
   // history DB thread. Since it doesn't, just return true to bypass a CHECK in
   // the sync code.
   return true;
 }
 
 HistoryModelWorker::~HistoryModelWorker() {
   // The base::CancelableTaskTracker class is not thread-safe and must only be
   // used from a single thread but the current object may not be destroyed from
   // the UI thread, so delete it from the UI thread.
   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