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..a6de8f97bb136a968e05db31f647143bbe663f31 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,74 @@ enum ModelSafeGroup {
 
 SYNC_EXPORT std::string ModelSafeGroupToString(ModelSafeGroup group);
 
+// WorkerObserver is notified when worker can't do more work because the thread
+// it works on is going to be destroyed.
+class WorkerObserver {
+ public:
+  virtual void OnWorkerDisabled(ModelSafeGroup group) = 0;

[tim (not reviewing), 2013/05/14 04:14:29]

OnWorkerThreadDestroyed would be a better name.

[haitaol1, 2013/05/15 23:39:21]

Done.

+};
+
 // 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 browser 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 (SyncBackendRegistrar).

[tim (not reviewing), 2013/05/14 04:14:29]

Don't mention 'browser' or components from glue like SyncBackendRegistrar in the
sync/ module.  As part of a component, comments here should not assume specifics
about the embedder.

[haitaol1, 2013/05/15 23:39:21]

Done.

 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 observer destruction of the loop where
+  // it actually does work.
+  virtual void RegisterForLoopDestruction() = 0;

[tim (not reviewing), 2013/05/14 04:14:29]

Who calls this?  and when?

[haitaol1, 2013/05/15 23:39:21]

SyncBackendHost will call this after creating workers.

+
+  // If not stopped, call DoWorkAndWaitUntilDone() to do work. Otherwise
+  // return CANNOT_DO_WORK.
+  SyncerError HandleWork(const WorkCallback& work);

[tim (not reviewing), 2013/05/14 04:14:29]

DoWorkAndWaitUntilDone is a better, more descriptive name.  The fact that it can
fail doesn't need we need a more abstract name... most functions have a failure
mode :)  I think your comment + return value suffices.

[haitaol1, 2013/05/15 23:39:21]

Done.

+
+  // Soft stop worker by setting stopped_ flag. Called when sync is disabled
+  // or browser is shutting down.
+  void RequestStop();
+
+  // Reset state when sync is enabled.

[tim (not reviewing), 2013/05/14 04:14:29]

When is this used?  Is this for when sync was disabled, but is now being
re-enabled, and the destruction observer hasn't fired? I'd rather be explicit
about what needs to happen than have a restart function here. e.g force the
caller to destroy (or put this MSW in a lame-duck mode and detach), and create a
new MSW.  

That moves the extra complexity for this case to the caller / client of
ModelSafeWorker, which seems right given that we want to remove that case
altogether from the higher layers. If we go with my suggestion we won't have to
make any changes to ModelSafeWorker when that switch happens, and we won't add
complexity here in the meantime.

[haitaol1, 2013/05/15 23:39:21]

Removed. Worker will be destroyed when sync is disabled.

On 2013/05/14 04:14:29, timsteele wrote:

+  void Restart();
 
   virtual ModelSafeGroup GetModelSafeGroup() = 0;
 
+  // MessageLoop::DestructionObserver implementation.
+  virtual void WillDestroyCurrentMessageLoop() OVERRIDE;
+
  protected:
+  friend class base::RefCountedThreadSafe<ModelSafeWorker>;
+
+  ModelSafeWorker(WorkerObserver* worker_host);

[tim (not reviewing), 2013/05/14 04:14:29]

Single argument constructors should be explicit.

[haitaol1, 2013/05/15 23:39:21]

Done.

   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 DoWorkAndWaitUntilDone(

[tim (not reviewing), 2013/05/14 04:14:29]

You can call this DoWorkAndWaitIUntilDoneImpl.

[haitaol1, 2013/05/15 23:39:21]

Done.

+      const WorkCallback& work, base::WaitableEvent* done_or_stopped) = 0;
+
+  // Return true if the worker was stopped. Thread safe.
+  bool Stopped();

[tim (not reviewing), 2013/05/14 04:14:29]

IsStopped()

[haitaol1, 2013/05/15 23:39:21]

Done.

+
  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.
+  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.
+  WorkerObserver* observer_;
 };
 
 // A map that details which ModelSafeGroup each ModelType