Revert "Lock-free shutdown of profile sync service. Changes include:"

Introduced shutdown crashes. See bug crbug.com/259974 for discussion.

Original codereview at https://chromiumcodereview.appspot.com/16770005

This also reverts "Disconnect non-frontend processor using the processor in "
due to its depdency on the above patch.

Original codereview at https://chromiumcodereview.appspot.com/5878607637381120

BUG=19757,259974
TBR=haitaol@chromium.org

Review URL: https://codereview.chromium.org/18890004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@211506 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 16 insertions, 61 deletions

diff --git a/sync/internal_api/public/engine/model_safe_worker.cc b/sync/internal_api/public/engine/model_safe_worker.cc
index 7179ed5..44e0a24 100644
--- a/sync/internal_api/public/engine/model_safe_worker.cc
+++ b/sync/internal_api/public/engine/model_safe_worker.cc
@@ -4,7 +4,6 @@
 
 #include "sync/internal_api/public/engine/model_safe_worker.h"
 
-#include "base/bind.h"
 #include "base/json/json_writer.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/values.h"
@@ -85,9 +84,7 @@ std::string ModelSafeGroupToString(ModelSafeGroup group) {
 ModelSafeWorker::ModelSafeWorker(WorkerLoopDestructionObserver* observer)
     : stopped_(false),
       work_done_or_stopped_(false, false),
-      observer_(observer),
-      working_loop_(NULL),
-      working_loop_set_wait_(true, false) {}
+      observer_(observer) {}
 
 ModelSafeWorker::~ModelSafeWorker() {}
 
@@ -138,33 +135,4 @@ void ModelSafeWorker::WillDestroyCurrentMessageLoop() {
     observer_->OnWorkerLoopDestroyed(GetModelSafeGroup());
 }
 
-void ModelSafeWorker::SetWorkingLoopToCurrent() {
-  DCHECK(!working_loop_);
-  working_loop_ = base::MessageLoop::current();
-  working_loop_set_wait_.Signal();
-}
-
-void ModelSafeWorker::UnregisterForLoopDestruction(
-    base::Callback<void(ModelSafeGroup)> unregister_done_callback) {
-  // Ok to wait until |working_loop_| is set because this is called on sync
-  // loop.
-  working_loop_set_wait_.Wait();
-
-  // Should be called on sync loop.
-  DCHECK_NE(base::MessageLoop::current(), working_loop_);
-  DCHECK(working_loop_);
-  working_loop_->PostTask(
-      FROM_HERE,
-      base::Bind(&ModelSafeWorker::UnregisterForLoopDestructionAsync,
-                 this, unregister_done_callback));
-}
-
-void ModelSafeWorker::UnregisterForLoopDestructionAsync(
-    base::Callback<void(ModelSafeGroup)> unregister_done_callback) {
-  DCHECK(stopped_);
-  DCHECK_EQ(base::MessageLoop::current(), working_loop_);
-  base::MessageLoop::current()->RemoveDestructionObserver(this);
-  unregister_done_callback.Run(GetModelSafeGroup());
-}
-
 }  // namespace syncer
diff --git a/sync/internal_api/public/engine/model_safe_worker.h b/sync/internal_api/public/engine/model_safe_worker.h
index c7df5a5..aedf649 100644
--- a/sync/internal_api/public/engine/model_safe_worker.h
+++ b/sync/internal_api/public/engine/model_safe_worker.h
@@ -69,16 +69,9 @@ class SYNC_EXPORT ModelSafeWorker
       public base::MessageLoop::DestructionObserver {
  public:
   // Subclass should implement to observe destruction of the loop where
-  // it actually does work. Called on UI thread immediately after worker is
-  // created.
+  // it actually does work.
   virtual void RegisterForLoopDestruction() = 0;
 
-  // Called on sync loop from SyncBackendRegistrar::ShutDown(). Post task to
-  // working loop to stop observing loop destruction and invoke
-  // |unregister_done_callback|.
-  virtual void UnregisterForLoopDestruction(
-      base::Callback<void(ModelSafeGroup)> unregister_done_callback);
-
   // If not stopped, call DoWorkAndWaitUntilDoneImpl() to do work. Otherwise
   // return CANNOT_DO_WORK.
   SyncerError DoWorkAndWaitUntilDone(const WorkCallback& work);
@@ -110,14 +103,7 @@ class SYNC_EXPORT ModelSafeWorker
   // Return true if the worker was stopped. Thread safe.
   bool IsStopped();
 
-  // Subclass should call this in RegisterForLoopDestruction() from the loop
-  // where work is done.
-  void SetWorkingLoopToCurrent();
-
  private:
-  void UnregisterForLoopDestructionAsync(
-      base::Callback<void(ModelSafeGroup)> unregister_done_callback);
-
   // 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 stopped_lock_;
@@ -129,11 +115,6 @@ class SYNC_EXPORT ModelSafeWorker
 
   // Notified when working thread of the worker is to be destroyed.
   WorkerLoopDestructionObserver* observer_;
-
-  // Remember working loop for posting task to unregister destruction
-  // observation from sync thread when shutting down sync.
-  base::MessageLoop* working_loop_;
-  base::WaitableEvent working_loop_set_wait_;
 };
 
 // A map that details which ModelSafeGroup each ModelType
diff --git a/sync/internal_api/public/engine/passive_model_worker.cc b/sync/internal_api/public/engine/passive_model_worker.cc
index eed9fb9..8b1a4e3 100644
--- a/sync/internal_api/public/engine/passive_model_worker.cc
+++ b/sync/internal_api/public/engine/passive_model_worker.cc
@@ -18,8 +18,7 @@ PassiveModelWorker::~PassiveModelWorker() {
 }
 
 void PassiveModelWorker::RegisterForLoopDestruction() {
-  base::MessageLoop::current()->AddDestructionObserver(this);
-  SetWorkingLoopToCurrent();
+  NOTREACHED();
 }
 
 SyncerError PassiveModelWorker::DoWorkAndWaitUntilDoneImpl(
diff --git a/sync/internal_api/public/engine/passive_model_worker.h b/sync/internal_api/public/engine/passive_model_worker.h
index 783731b..f834c83 100644
--- a/sync/internal_api/public/engine/passive_model_worker.h
+++ b/sync/internal_api/public/engine/passive_model_worker.h
@@ -11,6 +11,10 @@
 #include "sync/internal_api/public/engine/model_safe_worker.h"
 #include "sync/internal_api/public/util/syncer_error.h"
 
+namespace base {
+class MessageLoop;
+}
+
 namespace syncer {
 
 // Implementation of ModelSafeWorker for passive types.  All work is
diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h
index 2c2c02d..bc6a137 100644
--- a/sync/internal_api/public/sync_manager.h
+++ b/sync/internal_api/public/sync_manager.h
@@ -401,7 +401,7 @@ class SYNC_EXPORT SyncManager : public syncer::InvalidationHandler {
   // If no scheduler exists, the callback is run immediately (from the loop
   // this was created on, which is the sync loop), as sync is effectively
   // stopped.
-  virtual void StopSyncingForShutdown() = 0;
+  virtual void StopSyncingForShutdown(const base::Closure& callback) = 0;
 
   // Issue a final SaveChanges, and close sqlite handles.
   virtual void ShutdownOnSyncThread() = 0;
diff --git a/sync/internal_api/public/test/fake_sync_manager.h b/sync/internal_api/public/test/fake_sync_manager.h
index 38c3d87..ed69a52 100644
--- a/sync/internal_api/public/test/fake_sync_manager.h
+++ b/sync/internal_api/public/test/fake_sync_manager.h
@@ -115,7 +115,7 @@ class FakeSyncManager : public SyncManager {
   virtual void RemoveObserver(Observer* observer) OVERRIDE;
   virtual SyncStatus GetDetailedStatus() const OVERRIDE;
   virtual void SaveChanges() OVERRIDE;
-  virtual void StopSyncingForShutdown() OVERRIDE;
+  virtual void StopSyncingForShutdown(const base::Closure& callback) OVERRIDE;
   virtual void ShutdownOnSyncThread() OVERRIDE;
   virtual UserShare* GetUserShare() OVERRIDE;
   virtual const std::string cache_guid() OVERRIDE;
diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc
index 0825317..e9eeea1 100644
--- a/sync/internal_api/sync_manager_impl.cc
+++ b/sync/internal_api/sync_manager_impl.cc
@@ -620,9 +620,9 @@ void SyncManagerImpl::RemoveObserver(SyncManager::Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-void SyncManagerImpl::StopSyncingForShutdown() {
+void SyncManagerImpl::StopSyncingForShutdown(const base::Closure& callback) {
   DVLOG(2) << "StopSyncingForShutdown";
-  scheduler_->RequestStop();
+  scheduler_->RequestStop(callback);
   if (connection_manager_)
     connection_manager_->TerminateAllIO();
 }
diff --git a/sync/internal_api/sync_manager_impl.h b/sync/internal_api/sync_manager_impl.h
index 1a97d07..153df81 100644
--- a/sync/internal_api/sync_manager_impl.h
+++ b/sync/internal_api/sync_manager_impl.h
@@ -106,7 +106,7 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl :
   virtual void RemoveObserver(SyncManager::Observer* observer) OVERRIDE;
   virtual SyncStatus GetDetailedStatus() const OVERRIDE;
   virtual void SaveChanges() OVERRIDE;
-  virtual void StopSyncingForShutdown() OVERRIDE;
+  virtual void StopSyncingForShutdown(const base::Closure& callback) OVERRIDE;
   virtual void ShutdownOnSyncThread() OVERRIDE;
   virtual UserShare* GetUserShare() OVERRIDE;
   virtual const std::string cache_guid() OVERRIDE;
diff --git a/sync/internal_api/test/fake_sync_manager.cc b/sync/internal_api/test/fake_sync_manager.cc
index 24c228c..7a6d6a4 100644
--- a/sync/internal_api/test/fake_sync_manager.cc
+++ b/sync/internal_api/test/fake_sync_manager.cc
@@ -210,7 +210,10 @@ void FakeSyncManager::SaveChanges() {
   // Do nothing.
 }
 
-void FakeSyncManager::StopSyncingForShutdown() {
+void FakeSyncManager::StopSyncingForShutdown(const base::Closure& callback) {
+  if (!sync_task_runner_->PostTask(FROM_HERE, callback)) {
+    NOTREACHED();
+  }
 }
 
 void FakeSyncManager::ShutdownOnSyncThread() {