[Sync] Refactor ModelSafeWorker::DoWorkAndWaitUntilDone() to avoid code duplication.

Before this CL, logic to wait for a WorkCallback to run or be
abandoned was duplicated in multiple
ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() implementations.

With this CL, ModelSafeWorker::DoWorkAndWaitUntilDoneImpl() is
replaced with ModelSafeWorker::ScheduleWork(). Implementations
are merely responsible for scheduling the callback that they
receive; they don't have to wait for it to run or be abandoned.

The logic to wait for the callback to run or be abandoned is moved
into ModelSafeWorker::DoWorkAndWaitUntilDone().
ModelSafeWorker::DoWorkAndWaitUntilDone() calls
ModelSafeWorker::ScheduleWork(). Then, it waits until either the
callback that it passed to ModelSafeWorker::ScheduleWork() is deleted
(may or may not have run) or ModelSafeWorker::RequestStop() is called
before the callback has started to run. Additionnaly, calling
ModelSafeWorker::RequestStop() transforms into no-op further
invocations of callbacks passed to ModelSafeWorker::ScheduleWork().

Unblocking ModelSafeWorker::DoWorkAndWaitUntilDone() when
ModelSafeWorker::RequestStop() is called is important to
prevent deadlocks in workers that schedule work on the UI
thread. Indeed, in Chrome, no tasks are scheduled on the UI
thread after ModelSafeWorker::RequestStop() is called and
pending UI tasks are not scheduled until after the sync thread
is joined.

BUG=663600

Review-Url: https://codereview.chromium.org/2782573002
Cr-Commit-Position: refs/heads/master@{#463413}

components/history/core/browser/BUILD.gn

@@ -192,6 +192,7 @@ source_set("unit_tests") {
     "history_backend_db_unittest.cc",
     "history_backend_unittest.cc",
     "history_database_unittest.cc",
+    "history_model_worker_unittest.cc",
     "history_querying_unittest.cc",
     "history_service_unittest.cc",
     "history_types_unittest.cc",

components/history/core/browser/history_model_worker.cc

@@ -6,30 +6,19 @@
 
 #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 {
  public:
-  WorkerTask(const syncer::WorkCallback& work,
-             syncer::ScopedEventSignal scoped_event_signal,
-             syncer::SyncerError* error)
-      : work_(work),
-        scoped_event_signal_(std::move(scoped_event_signal)),
-        error_(error) {}
+  WorkerTask(base::OnceClosure work) : work_(std::move(work)) {}
 
   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();
-
+    std::move(work_).Run();
     return true;
   }
 
@@ -37,34 +26,12 @@ class WorkerTask : public history::HistoryDBTask {
   // 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_;
-  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 {}
+  // A OnceClosure is deleted right after it runs. This is important to unblock
+  // DoWorkAndWaitUntilDone() right after the task runs.
+  base::OnceClosure work_;
 
-  base::Closure register_callback_;
+  DISALLOW_COPY_AND_ASSIGN(WorkerTask);
 };
 
 namespace {
@@ -73,18 +40,11 @@ namespace {
 // 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) {
+    base::OnceClosure work,
+    base::CancelableTaskTracker* cancelable_tracker) {
   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.
+    history_service->ScheduleDBTask(
+        base::MakeUnique<WorkerTask>(std::move(work)), cancelable_tracker);
   }
 }
 
@@ -99,27 +59,6 @@ HistoryModelWorker::HistoryModelWorker(
   cancelable_tracker_.reset(new base::CancelableTaskTracker);
 }
 
-syncer::SyncerError HistoryModelWorker::DoWorkAndWaitUntilDoneImpl(
-    const syncer::WorkCallback& work) {
-  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;
-  }
-  return error;
-}
-
 syncer::ModelSafeGroup HistoryModelWorker::GetModelSafeGroup() {
   return syncer::GROUP_HISTORY;
 }
@@ -138,4 +77,10 @@ HistoryModelWorker::~HistoryModelWorker() {
   ui_thread_->DeleteSoon(FROM_HERE, cancelable_tracker_.release());
 }
 
+void HistoryModelWorker::ScheduleWork(base::OnceClosure work) {
+  ui_thread_->PostTask(FROM_HERE, base::Bind(&PostWorkerTask, history_service_,
+                                             base::Passed(std::move(work)),
+                                             cancelable_tracker_.get()));
+}
+
 }  // namespace browser_sync

components/history/core/browser/history_model_worker.h

@@ -32,13 +32,11 @@ class HistoryModelWorker : public syncer::ModelSafeWorker {
   syncer::ModelSafeGroup GetModelSafeGroup() override;
   bool IsOnModelThread() override;
 
- protected:
-  syncer::SyncerError DoWorkAndWaitUntilDoneImpl(
-      const syncer::WorkCallback& work) override;
-
  private:
   ~HistoryModelWorker() override;
 
+  void ScheduleWork(base::OnceClosure work) override;
+
   const base::WeakPtr<history::HistoryService> history_service_;
 
   // A reference to the UI thread's task runner.

components/history/core/browser/history_model_worker_unittest.cc

+// 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/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/single_thread_task_runner.h"
+#include "base/synchronization/atomic_flag.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/test/test_timeouts.h"
+#include "base/threading/platform_thread.h"
+#include "base/threading/thread.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(scoped_refptr<base::SingleThreadTaskRunner> history_thread)
+      : history_thread_(std::move(history_thread)) {}
+
+  base::CancelableTaskTracker::TaskId ScheduleDBTask(
+      std::unique_ptr<history::HistoryDBTask> task,
+      base::CancelableTaskTracker* tracker) override {
+    history::HistoryDBTask* task_raw = task.get();
+    history_thread_->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.
+  }
+
+ private:
+  const scoped_refptr<base::SingleThreadTaskRunner> history_thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(HistoryServiceMock);
+};
+
+syncer::WorkCallback ClosureToWorkCallback(base::Closure work) {
+  return base::Bind(
+      [](base::Closure work) {
+        work.Run();
+        return syncer::SYNCER_OK;
+      },
+      std::move(work));
+}
+
+class HistoryModelWorkerTest : public testing::Test {
+ public:
+  HistoryModelWorkerTest()
+      : sync_thread_("SyncThreadForTest"),
+        history_service_(history_thread_),
+        history_service_factory_(&history_service_) {
+    sync_thread_.Start();
+    worker_ = new HistoryModelWorker(history_service_factory_.GetWeakPtr(),
+                                     ui_thread_);
+  }
+
+  ~HistoryModelWorkerTest() override {
+    // HistoryModelWorker posts a cleanup task to the UI thread in its
+    // destructor. Run it to prevent a leak.
+    worker_ = nullptr;
+    ui_thread_->RunUntilIdle();
+  }
+
+ protected:
+  void DoWorkAndWaitUntilDoneOnSyncThread(base::Closure work) {
+    sync_thread_.task_runner()->PostTask(
+        FROM_HERE,
+        base::Bind(
+            base::IgnoreResult(&HistoryModelWorker::DoWorkAndWaitUntilDone),
+            worker_, base::Passed(ClosureToWorkCallback(work))));
+    sync_thread_.task_runner()->PostTask(
+        FROM_HERE, base::Bind(&base::AtomicFlag::Set,
+                              base::Unretained(&sync_thread_unblocked_)));
+  }
+
+  const scoped_refptr<base::TestSimpleTaskRunner> ui_thread_ =
+      new base::TestSimpleTaskRunner();
+  scoped_refptr<base::TestSimpleTaskRunner> history_thread_ =
+      new base::TestSimpleTaskRunner();
+  base::AtomicFlag sync_thread_unblocked_;
+  base::Thread sync_thread_;
+  HistoryServiceMock history_service_;
+  scoped_refptr<HistoryModelWorker> worker_;
+
+ private:
+  base::WeakPtrFactory<HistoryServiceMock> history_service_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(HistoryModelWorkerTest);
+};
+
+}  // namespace
+
+TEST_F(HistoryModelWorkerTest, DoWorkAndWaitUntilDone) {
+  bool did_work = false;
+  DoWorkAndWaitUntilDoneOnSyncThread(base::Bind(
+      [](bool* did_work) { *did_work = true; }, base::Unretained(&did_work)));
+
+  EXPECT_FALSE(did_work);
+  EXPECT_FALSE(sync_thread_unblocked_.IsSet());
+
+  // Wait for a task to be posted to the UI thread and run it. Expect this task
+  // to post another task to the history DB thread and run it.
+  while (!ui_thread_->HasPendingTask())
+    base::PlatformThread::YieldCurrentThread();
+  ui_thread_->RunUntilIdle();
+  EXPECT_TRUE(history_thread_->HasPendingTask());
+  history_thread_->RunUntilIdle();
+
+  EXPECT_TRUE(did_work);
+
+  sync_thread_.Stop();
+  EXPECT_TRUE(sync_thread_unblocked_.IsSet());
+}
+
+TEST_F(HistoryModelWorkerTest, DoWorkAndWaitUntilDoneRequestStopBeforeRunWork) {
+  bool did_work = false;
+  DoWorkAndWaitUntilDoneOnSyncThread(base::Bind(
+      [](bool* did_work) { *did_work = true; }, base::Unretained(&did_work)));
+
+  EXPECT_FALSE(did_work);
+  EXPECT_FALSE(sync_thread_unblocked_.IsSet());
+
+  // Wait for a task to be posted to the UI thread and run it.
+  while (!ui_thread_->HasPendingTask())
+    base::PlatformThread::YieldCurrentThread();
+  ui_thread_->RunUntilIdle();
+
+  // Stop the worker.
+  worker_->RequestStop();
+
+  // The WorkCallback should not run on the history DB thread.
+  EXPECT_TRUE(history_thread_->HasPendingTask());
+  history_thread_->RunUntilIdle();
+  EXPECT_FALSE(did_work);
+
+  sync_thread_.Stop();
+  EXPECT_TRUE(sync_thread_unblocked_.IsSet());
+}
+
+TEST_F(HistoryModelWorkerTest,
+       DoWorkAndWaitUntilDoneRequestStopBeforeUITaskRun) {
+  bool did_work = false;
+  DoWorkAndWaitUntilDoneOnSyncThread(base::Bind(
+      [](bool* did_work) { *did_work = true; }, base::Unretained(&did_work)));
+
+  EXPECT_FALSE(did_work);
+  EXPECT_FALSE(sync_thread_unblocked_.IsSet());
+
+  // Wait for a task to be posted to the UI thread.
+  while (!ui_thread_->HasPendingTask())
+    base::PlatformThread::YieldCurrentThread();
+
+  // Stop the worker.
+  worker_->RequestStop();
+
+  // Stopping the worker should unblock the sync thread.
+  sync_thread_.Stop();
+  EXPECT_TRUE(sync_thread_unblocked_.IsSet());
+}
+
+TEST_F(HistoryModelWorkerTest, DoWorkAndWaitUntilDoneDeleteWorkBeforeRun) {
+  bool did_work = false;
+  DoWorkAndWaitUntilDoneOnSyncThread(base::Bind(
+      [](bool* did_work) { *did_work = true; }, base::Unretained(&did_work)));
+
+  EXPECT_FALSE(did_work);
+  EXPECT_FALSE(sync_thread_unblocked_.IsSet());
+
+  // Wait for a task to be posted to the UI thread. Delete it before it can run.
+  while (!ui_thread_->HasPendingTask())
+    base::PlatformThread::YieldCurrentThread();
+  ui_thread_->ClearPendingTasks();
+
+  EXPECT_FALSE(did_work);
+
+  // Deleting the task should have unblocked the sync thread.
+  sync_thread_.Stop();
+  EXPECT_TRUE(sync_thread_unblocked_.IsSet());
+}
+
+TEST_F(HistoryModelWorkerTest, DoWorkAndWaitUntilDoneRequestStopDuringRunWork) {
+  bool did_work = false;
+  DoWorkAndWaitUntilDoneOnSyncThread(base::Bind(
+      [](scoped_refptr<HistoryModelWorker> worker,
+         base::AtomicFlag* sync_thread_unblocked, bool* did_work) {
+        worker->RequestStop();
+        base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());
+
+        // The sync thread should not be unblocked while a WorkCallback is
+        // running.
+        EXPECT_FALSE(sync_thread_unblocked->IsSet());
+
+        *did_work = true;
+      },
+      worker_, base::Unretained(&sync_thread_unblocked_),
+      base::Unretained(&did_work)));
+  EXPECT_FALSE(did_work);
+  EXPECT_FALSE(sync_thread_unblocked_.IsSet());
+
+  // Wait for a task to be posted to the UI thread and run it.
+  while (!ui_thread_->HasPendingTask())
+    base::PlatformThread::YieldCurrentThread();
+  ui_thread_->RunUntilIdle();
+
+  // Expect a task to be posted to the history DB thread. Run it.
+  EXPECT_TRUE(history_thread_->HasPendingTask());
+  history_thread_->RunUntilIdle();
+  EXPECT_TRUE(did_work);
+
+  sync_thread_.Stop();
+  EXPECT_TRUE(sync_thread_unblocked_.IsSet());
+}
+
+}  // namespace browser_sync

components/password_manager/sync/browser/password_model_worker.cc

@@ -4,60 +4,18 @@
 
 #include "components/password_manager/sync/browser/password_model_worker.h"
 
-#include "base/bind.h"
-#include "base/callback.h"
-#include "base/synchronization/waitable_event.h"
+#include <utility>
+
 #include "components/password_manager/core/browser/password_store.h"
-#include "components/sync/base/scoped_event_signal.h"
 
 namespace browser_sync {
 
-namespace {
-
-void CallDoWorkAndSignalEvent(const syncer::WorkCallback& work,
-                              syncer::ScopedEventSignal scoped_event_signal,
-                              syncer::SyncerError* error_info) {
-  *error_info = work.Run();
-  // The event in |scoped_event_signal| is signaled at the end of this scope.
-}
-
-}  // namespace
-
 PasswordModelWorker::PasswordModelWorker(
     const scoped_refptr<password_manager::PasswordStore>& password_store)
     : password_store_(password_store) {
   DCHECK(password_store.get());
 }
 
-syncer::SyncerError PasswordModelWorker::DoWorkAndWaitUntilDoneImpl(
-    const syncer::WorkCallback& work) {
-  syncer::SyncerError error = syncer::UNSET;
-
-  // Signaled when the task is deleted, i.e. after it runs or when it is
-  // abandoned.
-  base::WaitableEvent work_done_or_abandoned(
-      base::WaitableEvent::ResetPolicy::AUTOMATIC,
-      base::WaitableEvent::InitialState::NOT_SIGNALED);
-
-  bool scheduled = false;
-  {
-    base::AutoLock lock(password_store_lock_);
-    if (!password_store_.get())
-      return syncer::CANNOT_DO_WORK;
-
-    scheduled = password_store_->ScheduleTask(base::Bind(
-        &CallDoWorkAndSignalEvent, work,
-        base::Passed(syncer::ScopedEventSignal(&work_done_or_abandoned)),
-        &error));
-  }
-
-  if (scheduled)
-    work_done_or_abandoned.Wait();
-  else
-    error = syncer::CANNOT_DO_WORK;
-  return error;
-}
-
 syncer::ModelSafeGroup PasswordModelWorker::GetModelSafeGroup() {
   return syncer::GROUP_PASSWORD;
 }
@@ -71,6 +29,15 @@ bool PasswordModelWorker::IsOnModelThread() {
 
 PasswordModelWorker::~PasswordModelWorker() {}
 
+void PasswordModelWorker::ScheduleWork(base::OnceClosure work) {
+  base::AutoLock lock(password_store_lock_);
+  if (password_store_) {
+    password_store_->ScheduleTask(
+        base::Bind([](base::OnceClosure work) { std::move(work).Run(); },
+                   base::Passed(std::move(work))));
+  }
+}
+
 void PasswordModelWorker::RequestStop() {
   ModelSafeWorker::RequestStop();
 

components/password_manager/sync/browser/password_model_worker.h

@@ -28,13 +28,11 @@ class PasswordModelWorker : public syncer::ModelSafeWorker {
   bool IsOnModelThread() override;
   void RequestStop() override;
 
- protected:
-  syncer::SyncerError DoWorkAndWaitUntilDoneImpl(
-      const syncer::WorkCallback& work) override;
-
  private:
   ~PasswordModelWorker() override;
 
+  void ScheduleWork(base::OnceClosure work) override;
+
   // |password_store_| is used on password thread but released on UI thread.
   // Protected by |password_store_lock_|.
   base::Lock password_store_lock_;

components/sync/BUILD.gn

@@ -61,8 +61,6 @@ static_library("sync") {
     "base/proto_value_ptr.h",
     "base/report_unrecoverable_error.cc",
     "base/report_unrecoverable_error.h",
-    "base/scoped_event_signal.cc",
-    "base/scoped_event_signal.h",
     "base/stop_source.h",
     "base/sync_features.cc",
     "base/sync_features.h",
@@ -844,7 +842,6 @@ source_set("unit_tests") {
     "base/ordinal_unittest.cc",
     "base/proto_value_ptr_unittest.cc",
     "base/protobuf_unittest.cc",
-    "base/scoped_event_signal_unittest.cc",
     "base/sync_prefs_unittest.cc",
     "base/system_encryptor_unittest.cc",
     "base/unique_position_unittest.cc",

components/sync/base/scoped_event_signal.cc

-// Copyright 2016 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/base/scoped_event_signal.h"
-
-#include "base/logging.h"
-#include "base/synchronization/waitable_event.h"
-
-namespace syncer {
-
-ScopedEventSignal::ScopedEventSignal(base::WaitableEvent* event)
-    : event_(event) {
-  DCHECK(event_);
-}
-
-ScopedEventSignal::ScopedEventSignal(ScopedEventSignal&& other)
-    : event_(other.event_) {
-  other.event_ = nullptr;
-}
-
-ScopedEventSignal& ScopedEventSignal::operator=(ScopedEventSignal&& other) {
-  DCHECK(!event_);
-  event_ = other.event_;
-  other.event_ = nullptr;
-  return *this;
-}
-
-ScopedEventSignal::~ScopedEventSignal() {
-  if (event_)
-    event_->Signal();
-}
-
-}  // namespace syncer

components/sync/base/scoped_event_signal.h

-// Copyright 2016 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.
-
-#ifndef COMPONENTS_SYNC_BASE_SCOPED_EVENT_SIGNAL_H_
-#define COMPONENTS_SYNC_BASE_SCOPED_EVENT_SIGNAL_H_
-
-#include "base/macros.h"
-
-namespace base {
-class WaitableEvent;
-}
-
-namespace syncer {
-
-// An object which signals a WaitableEvent when it is deleted. Used to wait for
-// a task to run or be abandoned.
-class ScopedEventSignal {
- public:
-  // |event| will be signaled in the destructor.
-  explicit ScopedEventSignal(base::WaitableEvent* event);
-
-  // This ScopedEventSignal will signal |other|'s WaitableEvent in its
-  // destructor. |other| will not signal anything in its destructor. The
-  // assignment operator cannot be used if this ScopedEventSignal's
-  // WaitableEvent hasn't been moved to another ScopedEventSignal.
-  ScopedEventSignal(ScopedEventSignal&& other);
-  ScopedEventSignal& operator=(ScopedEventSignal&& other);
-
-  ~ScopedEventSignal();
-
- private:
-  base::WaitableEvent* event_;
-
-  DISALLOW_COPY_AND_ASSIGN(ScopedEventSignal);
-};
-
-}  // namespace syncer
-
-#endif  // COMPONENTS_SYNC_BASE_SCOPED_EVENT_SIGNAL_H_

components/sync/base/scoped_event_signal_unittest.cc

-// Copyright 2016 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/base/scoped_event_signal.h"
-
-#include <memory>
-#include <utility>
-
-#include "base/synchronization/waitable_event.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace syncer {
-
-TEST(ScopedEventSignalTest, SignalAtEndOfScope) {
-  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::MANUAL,
-                            base::WaitableEvent::InitialState::NOT_SIGNALED);
-
-  {
-    ScopedEventSignal scoped_event_signal(&event);
-    EXPECT_FALSE(event.IsSignaled());
-  }
-
-  EXPECT_TRUE(event.IsSignaled());
-}
-
-TEST(ScopedEventSignalTest, MoveConstructor) {
-  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::MANUAL,
-                            base::WaitableEvent::InitialState::NOT_SIGNALED);
-
-  {
-    ScopedEventSignal scoped_event_signal(&event);
-    EXPECT_FALSE(event.IsSignaled());
-
-    {
-      ScopedEventSignal other_scoped_event_signal(
-          std::move(scoped_event_signal));
-      EXPECT_FALSE(event.IsSignaled());
-    }
-
-    // |event| is signaled when |other_scoped_event_signal| is destroyed.
-    EXPECT_TRUE(event.IsSignaled());
-
-    event.Reset();
-  }
-
-  // |event| is not signaled when |scoped_signal_event| is destroyed.
-  EXPECT_FALSE(event.IsSignaled());
-}
-
-TEST(ScopedEventSignalTest, MoveAssignOperator) {
-  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::MANUAL,
-                            base::WaitableEvent::InitialState::NOT_SIGNALED);
-
-  {
-    ScopedEventSignal scoped_event_signal_a(&event);
-    EXPECT_FALSE(event.IsSignaled());
-
-    {
-      base::WaitableEvent other_event(
-          base::WaitableEvent::ResetPolicy::MANUAL,
-          base::WaitableEvent::InitialState::NOT_SIGNALED);
-      ScopedEventSignal scoped_event_signal_b(&other_event);
-
-      // Move |scoped_event_signal_b| to |scoped_event_signal_c| because the
-      // assignment operator cannot be used on a ScopedEventSignal which is
-      // already associated with an event.
-      ScopedEventSignal scoped_event_signal_c(std::move(scoped_event_signal_b));
-
-      scoped_event_signal_b = std::move(scoped_event_signal_a);
-      EXPECT_FALSE(event.IsSignaled());
-    }
-
-    // |event| is signaled when |scoped_event_signal_b| is destroyed.
-    EXPECT_TRUE(event.IsSignaled());
-
-    event.Reset();
-  }
-
-  // |event| is not signaled when |scoped_signal_event_a| is destroyed.
-  EXPECT_FALSE(event.IsSignaled());
-}
-
-}  // namespace syncer

components/sync/engine/browser_thread_model_worker.cc

@@ -4,45 +4,22 @@
 
 #include "components/sync/engine/browser_thread_model_worker.h"
 
-#include "base/bind.h"
-#include "base/callback.h"
-#include "base/synchronization/waitable_event.h"
-
-using base::SingleThreadTaskRunner;
+#include <utility>
 
 namespace syncer {
 
 BrowserThreadModelWorker::BrowserThreadModelWorker(
-    const scoped_refptr<SingleThreadTaskRunner>& runner,
+    const scoped_refptr<base::SingleThreadTaskRunner>& runner,
     ModelSafeGroup group)
     : runner_(runner), group_(group) {}
 
-SyncerError BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl(
-    const WorkCallback& work) {
-  SyncerError error = UNSET;
+void BrowserThreadModelWorker::ScheduleWork(base::OnceClosure work) {
   if (runner_->BelongsToCurrentThread()) {
     DLOG(WARNING) << "Already on thread " << runner_;
-    return work.Run();
-  }
-
-  // Signaled when the task is deleted, i.e. after it runs or when it is
-  // abandoned.
-  base::WaitableEvent work_done_or_abandoned(
-      base::WaitableEvent::ResetPolicy::AUTOMATIC,
-      base::WaitableEvent::InitialState::NOT_SIGNALED);
-
-  if (!runner_->PostTask(
-          FROM_HERE,
-          base::Bind(
-              &BrowserThreadModelWorker::CallDoWorkAndSignalTask, this, work,
-              base::Passed(syncer::ScopedEventSignal(&work_done_or_abandoned)),
-              &error))) {
-    DLOG(WARNING) << "Failed to post task to runner " << runner_;
-    error = CANNOT_DO_WORK;
-    return error;
+    std::move(work).Run();
+  } else {
+    runner_->PostTask(FROM_HERE, std::move(work));
   }
-  work_done_or_abandoned.Wait();
-  return error;
 }
 
 ModelSafeGroup BrowserThreadModelWorker::GetModelSafeGroup() {
@@ -55,14 +32,4 @@ bool BrowserThreadModelWorker::IsOnModelThread() {
 
 BrowserThreadModelWorker::~BrowserThreadModelWorker() {}
 
-void BrowserThreadModelWorker::CallDoWorkAndSignalTask(
-    const WorkCallback& work,
-    syncer::ScopedEventSignal scoped_event_signal,
-    SyncerError* error) {
-  DCHECK(runner_->BelongsToCurrentThread());
-  if (!IsStopped())
-    *error = work.Run();
-  // The event in |scoped_event_signal| is signaled at the end of this scope.
-}
-
 }  // namespace syncer

components/sync/engine/browser_thread_model_worker.h

@@ -8,8 +8,6 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/single_thread_task_runner.h"
-#include "components/sync/base/scoped_event_signal.h"
-#include "components/sync/base/syncer_error.h"
 #include "components/sync/engine/model_safe_worker.h"
 
 namespace syncer {
@@ -28,16 +26,11 @@ class BrowserThreadModelWorker : public ModelSafeWorker {
   ModelSafeGroup GetModelSafeGroup() override;
   bool IsOnModelThread() override;
 
- protected:
+ private:
   ~BrowserThreadModelWorker() override;
 
-  SyncerError DoWorkAndWaitUntilDoneImpl(const WorkCallback& work) override;
-
-  void CallDoWorkAndSignalTask(const WorkCallback& work,
-                               syncer::ScopedEventSignal scoped_event_signal,
-                               SyncerError* error);
+  void ScheduleWork(base::OnceClosure work) override;
 
- private:
   scoped_refptr<base::SingleThreadTaskRunner> runner_;
   ModelSafeGroup group_;
 

components/sync/engine/browser_thread_model_worker_unittest.cc

@@ -39,11 +39,10 @@ class SyncBrowserThreadModelWorkerTest : public testing::Test {
   // DoWork hasn't executed within action_timeout().
   void ScheduleWork() {
     // We wait until the callback is done. So it is safe to use unretained.
-    WorkCallback c = base::Bind(&SyncBrowserThreadModelWorkerTest::DoWork,
-                                base::Unretained(this));
     timer()->Start(FROM_HERE, TestTimeouts::action_timeout(), this,
                    &SyncBrowserThreadModelWorkerTest::Timeout);
-    worker()->DoWorkAndWaitUntilDone(c);
+    worker()->DoWorkAndWaitUntilDone(base::BindOnce(
+        &SyncBrowserThreadModelWorkerTest::DoWork, base::Unretained(this)));
   }
 
   // This is the work that will be scheduled to be done on the DB thread.

components/sync/engine/model_safe_worker.cc

@@ -4,6 +4,9 @@
 
 #include "components/sync/engine/model_safe_worker.h"
 
+#include <utility>
+
+#include "base/bind.h"
 #include "base/json/json_writer.h"
 #include "base/values.h"
 
@@ -69,24 +72,86 @@ std::string ModelSafeGroupToString(ModelSafeGroup group) {
   }
 }
 
-ModelSafeWorker::ModelSafeWorker() {}
+ModelSafeWorker::ModelSafeWorker()
+    : work_done_or_abandoned_(base::WaitableEvent::ResetPolicy::AUTOMATIC,
+                              base::WaitableEvent::InitialState::NOT_SIGNALED) {
+}
 ModelSafeWorker::~ModelSafeWorker() {}
 
 void ModelSafeWorker::RequestStop() {
-  // Set stop flag. This prevents any *further* tasks from being posted to
-  // worker threads (see DoWorkAndWaitUntilDone below), but note that one may
-  // already be posted.
-  stopped_.Set();
+  base::AutoLock auto_lock(lock_);
+
+  // Set stop flag to prevent any *further* WorkCallback from starting to run
+  // (note that one may alreay be running).
+  stopped_ = true;
+
+  // If no work is running, unblock DoWorkAndWaitUntilDone(). If work is
+  // running, it is unsafe to return from DoWorkAndWaitUntilDone().
+  // ScopedSignalWorkDoneOrAbandoned will take care of signaling the event when
+  // the work is done.
+  if (!is_work_running_)
+    work_done_or_abandoned_.Signal();
 }
 
-SyncerError ModelSafeWorker::DoWorkAndWaitUntilDone(const WorkCallback& work) {
-  if (stopped_.IsSet())
-    return CANNOT_DO_WORK;
-  return DoWorkAndWaitUntilDoneImpl(work);
+SyncerError ModelSafeWorker::DoWorkAndWaitUntilDone(WorkCallback work) {
+  {
+    // It is important to check |stopped_| and reset |work_done_or_abandoned_|
+    // atomically to prevent this race:
+    //
+    // Thread  Action
+    // Sync    Sees that |stopped_| is false.
+    // UI      Calls RequestStop(). Signals |work_done_or_abandoned_|.
+    // Sync    Resets |work_done_or_abandoned_|.
+    //         Waits on |work_done_or_abandoned_| forever since the task may not
+    //         run after RequestStop() is called.
+    base::AutoLock auto_lock(lock_);
+    if (stopped_)
+      return CANNOT_DO_WORK;
+    DCHECK(!is_work_running_);
+    work_done_or_abandoned_.Reset();
+  }
+
+  SyncerError error = UNSET;
+  bool did_run = false;
+  ScheduleWork(base::BindOnce(
+      &ModelSafeWorker::DoWork, this, base::Passed(std::move(work)),
+      base::Passed(base::ScopedClosureRunner(base::Bind(
+          [](scoped_refptr<ModelSafeWorker> worker) {
+            worker->work_done_or_abandoned_.Signal();
+          },
+          make_scoped_refptr(this)))),
+      base::Unretained(&error), base::Unretained(&did_run)));
+
+  // Unblocked when the task runs or is deleted or when RequestStop() is called
+  // before the task starts running.
+  work_done_or_abandoned_.Wait();
+
+  return did_run ? error : CANNOT_DO_WORK;
 }
 
-bool ModelSafeWorker::IsStopped() {
-  return stopped_.IsSet();
+void ModelSafeWorker::DoWork(WorkCallback work,
+                             base::ScopedClosureRunner scoped_closure_runner,
+                             SyncerError* error,
+                             bool* did_run) {
+  {
+    base::AutoLock auto_lock(lock_);
+    if (stopped_)
+      return;
+
+    // Set |is_work_running_| to make sure that DoWorkAndWaitUntilDone() doesn't
+    // return while |work| is running.
+    DCHECK(!is_work_running_);
+    is_work_running_ = true;
+  }
+
+  *error = std::move(work).Run();
+  *did_run = true;
+
+  {
+    base::AutoLock auto_lock(lock_);
+    DCHECK(is_work_running_);
+    is_work_running_ = false;
+  }
 }
 
 }  // namespace syncer

components/sync/engine/model_safe_worker.h

@@ -9,10 +9,12 @@
 #include <memory>
 #include <string>
 
-#include "base/callback_forward.h"
+#include "base/callback.h"
+#include "base/callback_helpers.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/synchronization/atomic_flag.h"
+#include "base/synchronization/lock.h"
+#include "base/synchronization/waitable_event.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/syncer_error.h"
 
@@ -22,7 +24,7 @@ class DictionaryValue;
 
 namespace syncer {
 
-using WorkCallback = base::Callback<enum SyncerError(void)>;
+using WorkCallback = base::OnceCallback<enum SyncerError(void)>;
 
 enum ModelSafeGroup {
   GROUP_PASSIVE = 0,   // Models that are just "passively" being synced; e.g.
@@ -54,17 +56,14 @@ std::string ModelSafeGroupToString(ModelSafeGroup group);
 // a thread and does actual work on that thread.
 class ModelSafeWorker : public base::RefCountedThreadSafe<ModelSafeWorker> {
  public:
-  // If not stopped, call DoWorkAndWaitUntilDoneImpl() to do work. Otherwise
-  // return CANNOT_DO_WORK.
-  SyncerError DoWorkAndWaitUntilDone(const WorkCallback& work);
+  // If not stopped, calls ScheduleWork() to schedule |work| and waits until it
+  // is done or abandoned. Otherwise, returns CANNOT_DO_WORK.
+  SyncerError DoWorkAndWaitUntilDone(WorkCallback work);
 
   // Soft stop worker by setting stopped_ flag. Called when sync is disabled
   // or browser is shutting down. Called on UI loop.
   virtual void RequestStop();
 
-  // Return true if the worker was stopped. Thread safe.
-  bool IsStopped();
-
   virtual ModelSafeGroup GetModelSafeGroup() = 0;
 
   // Returns true if called on the thread this worker works on.
@@ -74,16 +73,31 @@ class ModelSafeWorker : public base::RefCountedThreadSafe<ModelSafeWorker> {
   ModelSafeWorker();
   virtual ~ModelSafeWorker();
 
-  // Any time the Syncer performs model modifications (e.g employing a
-  // WriteTransaction), it should be done by this method to ensure it is done
-  // from a model-safe thread.
-  virtual SyncerError DoWorkAndWaitUntilDoneImpl(const WorkCallback& work) = 0;
-
  private:
   friend class base::RefCountedThreadSafe<ModelSafeWorker>;
 
-  // Whether the worker should do more work. Set when sync is disabled.
-  base::AtomicFlag stopped_;
+  // Schedules |work| on the appropriate thread.
+  virtual void ScheduleWork(base::OnceClosure work) = 0;
+
+  void DoWork(WorkCallback work,
+              base::ScopedClosureRunner scoped_closure_runner,
+              SyncerError* error,
+              bool* did_run);
+
+  // Synchronizes access to all members.
+  base::Lock lock_;
+
+  // Signaled when DoWorkAndWaitUntilDone() can return, either because the work
+  // is done, the work has been abandoned or RequestStop() was called while no
+  // work was running. Reset at the beginning of DoWorkAndWaitUntilDone().
+  base::WaitableEvent work_done_or_abandoned_;
+
+  // Whether a WorkCallback is currently running.
+  bool is_work_running_ = false;
+
+  // Whether the worker was stopped. No WorkCallback can start running when this
+  // is true.
+  bool stopped_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(ModelSafeWorker);
 };

components/sync/engine/model_safe_worker_unittest.cc

@@ -4,13 +4,84 @@
 
 #include "components/sync/engine/model_safe_worker.h"
 
+#include <utility>
+
+#include "base/bind.h"
+#include "base/macros.h"
+#include "base/synchronization/atomic_flag.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/test/test_timeouts.h"
+#include "base/threading/platform_thread.h"
+#include "base/threading/thread.h"
 #include "base/values.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace syncer {
 namespace {
 
-class ModelSafeWorkerTest : public ::testing::Test {};
+syncer::WorkCallback ClosureToWorkCallback(base::Closure work) {
+  return base::Bind(
+      [](base::Closure work) {
+        work.Run();
+        return syncer::SYNCER_OK;
+      },
+      std::move(work));
+}
+
+class MockModelSafeWorker : public ModelSafeWorker {
+ public:
+  MockModelSafeWorker() = default;
+
+  void ScheduleWork(base::OnceClosure work) override {
+    task_runner_->PostTask(FROM_HERE, std::move(work));
+  }
+
+  ModelSafeGroup GetModelSafeGroup() override { return GROUP_PASSIVE; }
+
+  bool IsOnModelThread() override {
+    return task_runner_->BelongsToCurrentThread();
+  }
+
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner() const {
+    return task_runner_;
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<MockModelSafeWorker>;
+
+  ~MockModelSafeWorker() override = default;
+
+  const scoped_refptr<base::TestSimpleTaskRunner> task_runner_ =
+      new base::TestSimpleTaskRunner();
+
+  DISALLOW_COPY_AND_ASSIGN(MockModelSafeWorker);
+};
+
+class ModelSafeWorkerTest : public ::testing::Test {
+ protected:
+  ModelSafeWorkerTest() : sync_thread_("SyncThreadForTest") {
+    sync_thread_.Start();
+  }
+
+  void DoWorkAndWaitUntilDoneOnSyncThread(base::Closure work) {
+    sync_thread_.task_runner()->PostTask(
+        FROM_HERE,
+        base::Bind(base::IgnoreResult(&ModelSafeWorker::DoWorkAndWaitUntilDone),
+                   worker_, base::Passed(ClosureToWorkCallback(work))));
+    sync_thread_.task_runner()->PostTask(
+        FROM_HERE, base::Bind(&base::AtomicFlag::Set,
+                              base::Unretained(&sync_thread_unblocked_)));
+  }
+
+  base::AtomicFlag sync_thread_unblocked_;
+  base::Thread sync_thread_;
+  const scoped_refptr<MockModelSafeWorker> worker_ = new MockModelSafeWorker();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ModelSafeWorkerTest);
+};
+
+}  // namespace
 
 TEST_F(ModelSafeWorkerTest, ModelSafeRoutingInfoToValue) {
   ModelSafeRoutingInfo routing_info;
@@ -49,5 +120,95 @@ TEST_F(ModelSafeWorkerTest, GetRoutingInfoTypes) {
   EXPECT_EQ(expected_types, GetRoutingInfoTypes(routing_info));
 }
 
-}  // namespace
+TEST_F(ModelSafeWorkerTest, DoWorkAndWaitUntilDone) {
+  bool did_work = false;
+  DoWorkAndWaitUntilDoneOnSyncThread(base::Bind(
+      [](bool* did_work) { *did_work = true; }, base::Unretained(&did_work)));
+
+  EXPECT_FALSE(did_work);
+  EXPECT_FALSE(sync_thread_unblocked_.IsSet());
+
+  // Wait for a task to be posted to |worker_|'s TaskRunner and run it.
+  while (!worker_->task_runner()->HasPendingTask())
+    base::PlatformThread::YieldCurrentThread();
+  worker_->task_runner()->RunUntilIdle();
+
+  EXPECT_TRUE(did_work);
+
+  sync_thread_.Stop();
+  EXPECT_TRUE(sync_thread_unblocked_.IsSet());
+}
+
+TEST_F(ModelSafeWorkerTest, DoWorkAndWaitUntilDoneRequestStopBeforeRunWork) {
+  bool did_work = false;
+  DoWorkAndWaitUntilDoneOnSyncThread(base::Bind(
+      [](bool* did_work) { *did_work = true; }, base::Unretained(&did_work)));
+
+  EXPECT_FALSE(did_work);
+  EXPECT_FALSE(sync_thread_unblocked_.IsSet());
+
+  // Wait for a task to be posted to |worker_|'s TaskRunner.
+  while (!worker_->task_runner()->HasPendingTask())
+    base::PlatformThread::YieldCurrentThread();
+
+  // Stop the worker.
+  worker_->RequestStop();
+
+  // The WorkCallback should not run.
+  worker_->task_runner()->RunUntilIdle();
+  EXPECT_FALSE(did_work);
+
+  sync_thread_.Stop();
+  EXPECT_TRUE(sync_thread_unblocked_.IsSet());
+}
+
+TEST_F(ModelSafeWorkerTest, DoWorkAndWaitUntilDoneDeleteWorkBeforeRun) {
+  bool did_work = false;
+  DoWorkAndWaitUntilDoneOnSyncThread(base::Bind(
+      [](bool* did_work) { *did_work = true; }, base::Unretained(&did_work)));
+
+  EXPECT_FALSE(did_work);
+  EXPECT_FALSE(sync_thread_unblocked_.IsSet());
+
+  // Wait for a task to be posted to |worker_|'s TaskRunner and delete it.
+  while (!worker_->task_runner()->HasPendingTask())
+    base::PlatformThread::YieldCurrentThread();
+  worker_->task_runner()->ClearPendingTasks();
+
+  EXPECT_FALSE(did_work);
+
+  // Deleting the task should have unblocked the sync thread.
+  sync_thread_.Stop();
+  EXPECT_TRUE(sync_thread_unblocked_.IsSet());
+}
+
+TEST_F(ModelSafeWorkerTest, DoWorkAndWaitUntilDoneRequestStopDuringRunWork) {
+  bool did_work = false;
+  DoWorkAndWaitUntilDoneOnSyncThread(base::Bind(
+      [](scoped_refptr<ModelSafeWorker> worker,
+         base::AtomicFlag* sync_thread_unblocked, bool* did_work) {
+        worker->RequestStop();
+        base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());
+
+        // The sync thread should not be unblocked while a WorkCallback is
+        // running.
+        EXPECT_FALSE(sync_thread_unblocked->IsSet());
+
+        *did_work = true;
+      },
+      worker_, base::Unretained(&sync_thread_unblocked_),
+      base::Unretained(&did_work)));
+  EXPECT_FALSE(did_work);
+  EXPECT_FALSE(sync_thread_unblocked_.IsSet());
+
+  // Wait for a task to be posted to |worker_|'s TaskRunner and run it.
+  while (!worker_->task_runner()->HasPendingTask())
+    base::PlatformThread::YieldCurrentThread();
+  worker_->task_runner()->RunUntilIdle();
+
+  EXPECT_TRUE(did_work);
+  sync_thread_.Stop();
+  EXPECT_TRUE(sync_thread_unblocked_.IsSet());
+}
+
 }  // namespace syncer

components/sync/engine/passive_model_worker.cc

@@ -4,7 +4,7 @@
 
 #include "components/sync/engine/passive_model_worker.h"
 
-#include "base/callback.h"
+#include <utility>
 
 namespace syncer {
 
@@ -12,10 +12,8 @@ PassiveModelWorker::PassiveModelWorker() = default;
 
 PassiveModelWorker::~PassiveModelWorker() {}
 
-SyncerError PassiveModelWorker::DoWorkAndWaitUntilDoneImpl(
-    const WorkCallback& work) {
-  // Simply do the work on the current thread.
-  return work.Run();
+void PassiveModelWorker::ScheduleWork(base::OnceClosure work) {
+  std::move(work).Run();
 }
 
 ModelSafeGroup PassiveModelWorker::GetModelSafeGroup() {

components/sync/engine/passive_model_worker.h

@@ -6,7 +6,6 @@
 #define COMPONENTS_SYNC_ENGINE_PASSIVE_MODEL_WORKER_H_
 
 #include "base/macros.h"
-#include "components/sync/base/syncer_error.h"
 #include "components/sync/engine/model_safe_worker.h"
 
 namespace syncer {
@@ -22,12 +21,11 @@ class PassiveModelWorker : public ModelSafeWorker {
   ModelSafeGroup GetModelSafeGroup() override;
   bool IsOnModelThread() override;
 
- protected:
-  SyncerError DoWorkAndWaitUntilDoneImpl(const WorkCallback& work) override;
-
  private:
   ~PassiveModelWorker() override;
 
+  void ScheduleWork(base::OnceClosure work) override;
+
   DISALLOW_COPY_AND_ASSIGN(PassiveModelWorker);
 };
 

components/sync/engine/ui_model_worker.cc

@@ -6,90 +6,11 @@
 
 #include <utility>
 
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/callback.h"
-#include "components/sync/base/scoped_event_signal.h"
-
 namespace syncer {
 
-namespace {
-
-class ScopedEventSignalWithWorker {
- public:
-  ScopedEventSignalWithWorker(scoped_refptr<UIModelWorker> ui_model_worker,
-                              base::WaitableEvent* event)
-      : ui_model_worker_(std::move(ui_model_worker)),
-        scoped_event_signal_(event) {}
-
-  ScopedEventSignalWithWorker(ScopedEventSignalWithWorker&&) = default;
-  ScopedEventSignalWithWorker& operator=(ScopedEventSignalWithWorker&&) =
-      default;
-
-  bool IsStopped() const { return ui_model_worker_->IsStopped(); }
-
- private:
-  // This reference prevents the event in |scoped_event_signal_| from being
-  // signaled after being deleted.
-  scoped_refptr<UIModelWorker> ui_model_worker_;
-
-  ScopedEventSignal scoped_event_signal_;
-
-  DISALLOW_COPY_AND_ASSIGN(ScopedEventSignalWithWorker);
-};
-
-void CallDoWorkAndSignalEvent(
-    const WorkCallback& work,
-    ScopedEventSignalWithWorker scoped_event_signal_with_worker,
-    SyncerError* error_info) {
-  if (!scoped_event_signal_with_worker.IsStopped())
-    *error_info = work.Run();
-  // The event in |scoped_event_signal_with_worker| is signaled at the end of
-  // this scope.
-}
-
-}  // namespace
-
 UIModelWorker::UIModelWorker(
     scoped_refptr<base::SingleThreadTaskRunner> ui_thread)
-    : ui_thread_(std::move(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) {
-  sequence_checker_.DetachFromSequence();
-}
-
-SyncerError UIModelWorker::DoWorkAndWaitUntilDoneImpl(
-    const WorkCallback& work) {
-  DCHECK(sequence_checker_.CalledOnValidSequence());
-  DCHECK(!ui_thread_->BelongsToCurrentThread());
-
-  SyncerError error_info;
-  work_done_or_abandoned_.Reset();
-
-  if (!ui_thread_->PostTask(
-          FROM_HERE,
-          base::Bind(&CallDoWorkAndSignalEvent, work,
-                     base::Passed(syncer::ScopedEventSignalWithWorker(
-                         this, &work_done_or_abandoned_)),
-                     &error_info))) {
-    DLOG(WARNING) << "Could not post work to UI loop.";
-    error_info = CANNOT_DO_WORK;
-    return error_info;
-  }
-
-  base::WaitableEvent* events[] = {&work_done_or_abandoned_, &stop_requested_};
-  base::WaitableEvent::WaitMany(events, arraysize(events));
-
-  return error_info;
-}
-
-void UIModelWorker::RequestStop() {
-  DCHECK(ui_thread_->BelongsToCurrentThread());
-  ModelSafeWorker::RequestStop();
-  stop_requested_.Signal();
-}
+    : ui_thread_(std::move(ui_thread)) {}
 
 ModelSafeGroup UIModelWorker::GetModelSafeGroup() {
   return GROUP_UI;
@@ -101,4 +22,8 @@ bool UIModelWorker::IsOnModelThread() {
 
 UIModelWorker::~UIModelWorker() {}
 
+void UIModelWorker::ScheduleWork(base::OnceClosure work) {
+  ui_thread_->PostTask(FROM_HERE, std::move(work));
+}
+
 }  // namespace syncer

components/sync/engine/ui_model_worker.h

@@ -7,9 +7,7 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/sequence_checker.h"
 #include "base/single_thread_task_runner.h"
-#include "base/synchronization/waitable_event.h"
 #include "components/sync/engine/model_safe_worker.h"
 
 namespace syncer {
@@ -22,33 +20,17 @@ class UIModelWorker : public ModelSafeWorker {
   explicit UIModelWorker(scoped_refptr<base::SingleThreadTaskRunner> ui_thread);
 
   // ModelSafeWorker implementation.
-  void RequestStop() override;
   ModelSafeGroup GetModelSafeGroup() override;
   bool IsOnModelThread() override;
 
- protected:
-  SyncerError DoWorkAndWaitUntilDoneImpl(const WorkCallback& work) override;
-
  private:
   ~UIModelWorker() override;
 
+  void ScheduleWork(base::OnceClosure work) override;
+
   // A reference to the UI thread's task runner.
   const scoped_refptr<base::SingleThreadTaskRunner> ui_thread_;
 
-  // Signaled when a task posted by DoWorkAndWaitUntilDoneImpl() is deleted,
-  // i.e. after it runs or when it is abandoned. Reset at the beginning of every
-  // DoWorkAndWaitUntilDoneImpl() call.
-  base::WaitableEvent work_done_or_abandoned_;
-
-  // Signaled from RequestStop(). When this is signaled,
-  // DoWorkAndWaitUntilDoneImpl() returns immediately. This is needed to prevent
-  // the UI thread from joining the sync thread while it is waiting for a
-  // WorkCallback to run on the UI thread. See crbug.com/663600.
-  base::WaitableEvent stop_requested_;
-
-  // Verifies that calls to DoWorkAndWaitUntilDoneImpl() are sequenced.
-  base::SequenceChecker sequence_checker_;
-
   DISALLOW_COPY_AND_ASSIGN(UIModelWorker);
 };
 

components/sync/engine/ui_model_worker_unittest.cc

@@ -52,7 +52,7 @@ class SyncUIModelWorkerTest : public testing::Test {
     sync_thread_.task_runner()->PostTask(
         FROM_HERE,
         base::Bind(base::IgnoreResult(&UIModelWorker::DoWorkAndWaitUntilDone),
-                   worker_, ClosureToWorkCallback(work)));
+                   worker_, base::Passed(ClosureToWorkCallback(work))));
   }
 
  protected:

components/sync/engine_impl/directory_update_handler.cc

@@ -6,6 +6,7 @@
 
 #include <stdint.h>
 
+#include <utility>
 #include <vector>
 
 #include "base/memory/ptr_util.h"
@@ -127,7 +128,7 @@ void DirectoryUpdateHandler::ApplyUpdates(StatusController* status) {
                    // We wait until the callback is executed.  We can safely use
                    // Unretained.
                    base::Unretained(this), base::Unretained(status));
-    worker_->DoWorkAndWaitUntilDone(c);
+    worker_->DoWorkAndWaitUntilDone(std::move(c));
 
     debug_info_emitter_->EmitUpdateCountersUpdate();
     debug_info_emitter_->EmitStatusCountersUpdate();

components/sync/test/engine/fake_model_worker.cc

@@ -4,7 +4,7 @@
 
 #include "components/sync/test/engine/fake_model_worker.h"
 
-#include "base/callback.h"
+#include <utility>
 
 namespace syncer {
 
@@ -18,11 +18,10 @@ FakeModelWorker::~FakeModelWorker() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
-SyncerError FakeModelWorker::DoWorkAndWaitUntilDoneImpl(
-    const WorkCallback& work) {
+void FakeModelWorker::ScheduleWork(base::OnceClosure work) {
   DCHECK(thread_checker_.CalledOnValidThread());
   // Simply do the work on the current thread.
-  return work.Run();
+  std::move(work).Run();
 }
 
 ModelSafeGroup FakeModelWorker::GetModelSafeGroup() {

components/sync/test/engine/fake_model_worker.h

@@ -22,12 +22,11 @@ class FakeModelWorker : public ModelSafeWorker {
   ModelSafeGroup GetModelSafeGroup() override;
   bool IsOnModelThread() override;
 
- protected:
-  SyncerError DoWorkAndWaitUntilDoneImpl(const WorkCallback& work) override;
-
  private:
   ~FakeModelWorker() override;
 
+  void ScheduleWork(base::OnceClosure work) override;
+
   const ModelSafeGroup group_;
   base::ThreadChecker thread_checker_;