Worker changes to prepare for lock-free shutdown.

sync/internal_api/public/engine/model_safe_worker.h

Index: sync/internal_api/public/engine/model_safe_worker.h
diff --git a/sync/internal_api/public/engine/model_safe_worker.h b/sync/internal_api/public/engine/model_safe_worker.h
index fbec6b17a080afb1e590cd15babaa103ec672ff8..45ea37587806f6764533b5c0cd8aecd5506d8b82 100644
--- a/sync/internal_api/public/engine/model_safe_worker.h
+++ b/sync/internal_api/public/engine/model_safe_worker.h
@@ -11,6 +11,9 @@
 
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
+#include "base/message_loop.h"
+#include "base/synchronization/lock.h"
+#include "base/synchronization/waitable_event.h"
 #include "sync/base/sync_export.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/base/model_type_invalidation_map.h"
@@ -44,28 +47,71 @@ enum ModelSafeGroup {
 
 SYNC_EXPORT std::string ModelSafeGroupToString(ModelSafeGroup group);
 
+// WorkerLoopDestructionObserver is notified when the thread where it works
+// is going to be destroyed.
+class WorkerLoopDestructionObserver {
+ public:
+  virtual void OnWorkerLoopDestroyed(ModelSafeGroup group) = 0;
+};
+
 // The Syncer uses a ModelSafeWorker for all tasks that could potentially
 // modify syncable entries (e.g under a WriteTransaction). The ModelSafeWorker
 // only knows how to do one thing, and that is take some work (in a fully
 // pre-bound callback) and have it performed (as in Run()) from a thread which
 // is guaranteed to be "model-safe", where "safe" refers to not allowing us to
 // cause an embedding application model to fall out of sync with the
-// syncable::Directory due to a race.
+// syncable::Directory due to a race. Each ModelSafeWorker is affiliated with
+// a thread and does actual work on that thread. On the destruction of that
+// thread, the affiliated worker is effectively disabled to do more
+// work and will notify its observer.
 class SYNC_EXPORT ModelSafeWorker
-    : public base::RefCountedThreadSafe<ModelSafeWorker> {
+    : public base::RefCountedThreadSafe<ModelSafeWorker>,
+      public MessageLoop::DestructionObserver {
  public:
-  // 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 DoWorkAndWaitUntilDone(const WorkCallback& work) = 0;
+  // Subclass should implement to observe destruction of the loop where
+  // it actually does work.
+  virtual void RegisterForLoopDestruction() = 0;
+
+  // If not stopped, call DoWorkAndWaitUntilDoneImpl() to do work. Otherwise
+  // return CANNOT_DO_WORK.
+  SyncerError DoWorkAndWaitUntilDone(const WorkCallback& work);
+
+  // Soft stop worker by setting stopped_ flag. Called when sync is disabled
+  // or browser is shutting down.
+  void RequestStop();

[tim (not reviewing), 2013/05/16 18:15:42]

If this is called from the UI thread, and we always create ModelSafeWorkers on
the UI thread in SyncBackendRegistrar, can't we replace lock_ and stopped_ with
a base::CancellationFlag?

[haitaol1, 2013/05/17 16:24:59]

lock_ is also used to serialize signal/reset of work_done_or_stopped_, so can't
be replace. Then it's redundant to make stopped_ CancellationFlag.

On 2013/05/16 18:15:42, timsteele wrote:

 
   virtual ModelSafeGroup GetModelSafeGroup() = 0;
 
+  // MessageLoop::DestructionObserver implementation.
+  virtual void WillDestroyCurrentMessageLoop() OVERRIDE;
+
  protected:
+  friend class base::RefCountedThreadSafe<ModelSafeWorker>;
+
+  explicit ModelSafeWorker(WorkerLoopDestructionObserver* observer);
   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, base::WaitableEvent* done_or_stopped) = 0;
+
+  // Return true if the worker was stopped. Thread safe.
+  bool IsStopped();
+
  private:
-  friend class base::RefCountedThreadSafe<ModelSafeWorker>;
+  // Whether the worker should/can do more work. Set when sync is disabled or
+  // when the worker's working thread is to be destroyed.

[tim (not reviewing), 2013/05/16 18:15:42]

Mention what |lock_| protects.  I think stopped_lock_ is a better name.

[haitaol1, 2013/05/17 16:24:59]

Done.

+  base::Lock lock_;
+  bool stopped_;
+
+  // Signal set when work on native thread is finished or when native thread
+  // is to be destroyed so no more work can be done.
+  base::WaitableEvent work_done_or_stopped_;
+
+  // Notified when working thread of the worker is to be destroyed.
+  WorkerLoopDestructionObserver* observer_;
 };
 
 // A map that details which ModelSafeGroup each ModelType