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

components/sync/engine/model_safe_worker.cc

Index: components/sync/engine/model_safe_worker.cc
diff --git a/components/sync/engine/model_safe_worker.cc b/components/sync/engine/model_safe_worker.cc
index 530f206599673cf1b91239e5d40aeff463b0406c..3c8616e0e927b445701d6d141001a1778ae05cc9 100644
--- a/components/sync/engine/model_safe_worker.cc
+++ b/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,106 @@ std::string ModelSafeGroupToString(ModelSafeGroup group) {
   }
 }
 
-ModelSafeWorker::ModelSafeWorker() {}
+// An object which signals the |work_done_or_abandoned_| event of a
+// ModelSafeWorker when it is deleted.
+class ModelSafeWorker::ScopedSignalWorkDoneOrAbandoned {

[skym, 2017/04/06 00:34:54]

This feels like a generic problem that is being solved with very use case
specific code. Would be cool if you had like an object that took a closure and
ran it in it's destructor. Feel free to disregard if there's a reason this is
tricky.

[fdoray, 2017/04/06 18:10:55]

Done.

+ public:
+  explicit ScopedSignalWorkDoneOrAbandoned(scoped_refptr<ModelSafeWorker> outer)
+      : outer_(std::move(outer)) {}
+
+  ScopedSignalWorkDoneOrAbandoned(ScopedSignalWorkDoneOrAbandoned&& other) =
+      default;
+  ScopedSignalWorkDoneOrAbandoned& operator=(
+      ScopedSignalWorkDoneOrAbandoned&& other) = default;
+
+  ~ScopedSignalWorkDoneOrAbandoned() {
+    if (outer_)

[skym, 2017/04/06 00:34:54]

Why's this if check needed?

[fdoray, 2017/04/06 18:10:55]

n/a

+      outer_->work_done_or_abandoned_.Signal();
+  }
+
+ private:
+  scoped_refptr<ModelSafeWorker> outer_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedSignalWorkDoneOrAbandoned);
+};
+
+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);
+  {
+    // 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::Bind(&ModelSafeWorker::DoWork, this, work,
+                          base::Passed(ScopedSignalWorkDoneOrAbandoned(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(
+    const WorkCallback& work,
+    ScopedSignalWorkDoneOrAbandoned scoped_signal_work_done_or_abandoned,
+    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 the work is running.
+    DCHECK(!is_work_running_);
+    is_work_running_ = true;
+  }
+
+  *error = work.Run();
+  *did_run = true;
+
+  {
+    base::AutoLock auto_lock(lock_);
+    DCHECK(is_work_running_);
+    is_work_running_ = false;
+  }
 }
 
 }  // namespace syncer