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

16 files changed, 40 insertions, 77 deletions

diff --git a/sync/engine/sync_scheduler.h b/sync/engine/sync_scheduler.h
index 1dd1018..8eb361e 100644
--- a/sync/engine/sync_scheduler.h
+++ b/sync/engine/sync_scheduler.h
@@ -79,7 +79,9 @@ class SYNC_EXPORT_PRIVATE SyncScheduler
   // cancel all scheduled tasks. This function can be called from any thread,
   // and should in fact be called from a thread that isn't the sync loop to
   // allow preempting ongoing sync cycles.
-  virtual void RequestStop() = 0;
+  // Invokes |callback| from the sync loop once syncer is idle and all tasks
+  // are cancelled.
+  virtual void RequestStop(const base::Closure& callback) = 0;
 
   // The meat and potatoes. All three of the following methods will post a
   // delayed task to attempt the actual nudge (see ScheduleNudgeImpl).
diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index 4a209e0..f837853 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -173,7 +173,7 @@ SyncSchedulerImpl::SyncSchedulerImpl(const std::string& name,
 
 SyncSchedulerImpl::~SyncSchedulerImpl() {
   DCHECK(CalledOnValidThread());
-  StopImpl();
+  StopImpl(base::Closure());
 }
 
 void SyncSchedulerImpl::OnCredentialsUpdated() {
@@ -650,15 +650,16 @@ void SyncSchedulerImpl::RestartWaiting() {
   }
 }
 
-void SyncSchedulerImpl::RequestStop() {
+void SyncSchedulerImpl::RequestStop(const base::Closure& callback) {
   syncer_->RequestEarlyExit();  // Safe to call from any thread.
   DCHECK(weak_handle_this_.IsInitialized());
   SDVLOG(3) << "Posting StopImpl";
   weak_handle_this_.Call(FROM_HERE,
-                         &SyncSchedulerImpl::StopImpl);
+                         &SyncSchedulerImpl::StopImpl,
+                         callback);
 }
 
-void SyncSchedulerImpl::StopImpl() {
+void SyncSchedulerImpl::StopImpl(const base::Closure& callback) {
   DCHECK(CalledOnValidThread());
   SDVLOG(2) << "StopImpl called";
 
@@ -671,6 +672,8 @@ void SyncSchedulerImpl::StopImpl() {
   pending_configure_params_.reset();
   if (started_)
     started_ = false;
+  if (!callback.is_null())
+    callback.Run();
 }
 
 // This is the only place where we invoke DoSyncSessionJob with canary
diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h
index 362d754..5d350a4 100644
--- a/sync/engine/sync_scheduler_impl.h
+++ b/sync/engine/sync_scheduler_impl.h
@@ -55,7 +55,7 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
   virtual void Start(Mode mode) OVERRIDE;
   virtual bool ScheduleConfiguration(
       const ConfigurationParams& params) OVERRIDE;
-  virtual void RequestStop() OVERRIDE;
+  virtual void RequestStop(const base::Closure& callback) OVERRIDE;
   virtual void ScheduleLocalNudge(
       const base::TimeDelta& desired_delay,
       ModelTypeSet types,
@@ -183,7 +183,7 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
   bool CanRunNudgeJobNow(JobPriority priority);
 
   // 'Impl' here refers to real implementation of public functions.
-  void StopImpl();
+  void StopImpl(const base::Closure& callback);
 
   // If the scheduler's current state supports it, this will create a job based
   // on the passed in parameters and coalesce it with any other pending jobs,
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 426f10f..4f2c56e 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -201,10 +201,8 @@ class SyncSchedulerTest : public testing::Test {
 
   // This stops the scheduler synchronously.
   void StopSyncScheduler() {
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(&SyncSchedulerTest::DoQuitLoopNow,
-                   weak_ptr_factory_.GetWeakPtr()));
+    scheduler()->RequestStop(base::Bind(&SyncSchedulerTest::DoQuitLoopNow,
+                             weak_ptr_factory_.GetWeakPtr()));
     RunLoop();
   }
 
@@ -234,8 +232,8 @@ class SyncSchedulerTest : public testing::Test {
     return dir_maker_.directory();
   }
 
-  base::MessageLoop loop_;
   base::WeakPtrFactory<SyncSchedulerTest> weak_ptr_factory_;
+  base::MessageLoop message_loop_;
   TestDirectorySetterUpper dir_maker_;
   scoped_ptr<MockConnectionManager> connection_;
   scoped_ptr<SyncSessionContext> context_;
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() {
diff --git a/sync/notifier/non_blocking_invalidator.cc b/sync/notifier/non_blocking_invalidator.cc
index d4c602b..2834f28 100644
--- a/sync/notifier/non_blocking_invalidator.cc
+++ b/sync/notifier/non_blocking_invalidator.cc
@@ -169,7 +169,7 @@ NonBlockingInvalidator::~NonBlockingInvalidator() {
           FROM_HERE,
           base::Bind(&NonBlockingInvalidator::Core::Teardown,
                      core_.get()))) {
-    DVLOG(1) << "Network thread stopped before invalidator is destroyed.";
+    NOTREACHED();
   }
 }
 
diff --git a/sync/test/engine/fake_sync_scheduler.cc b/sync/test/engine/fake_sync_scheduler.cc
index 585248e..200edb0 100644
--- a/sync/test/engine/fake_sync_scheduler.cc
+++ b/sync/test/engine/fake_sync_scheduler.cc
@@ -6,14 +6,16 @@
 
 namespace syncer {
 
-FakeSyncScheduler::FakeSyncScheduler() {}
+FakeSyncScheduler::FakeSyncScheduler()
+    : created_on_loop_(base::MessageLoop::current()) {}
 
 FakeSyncScheduler::~FakeSyncScheduler() {}
 
 void FakeSyncScheduler::Start(Mode mode) {
 }
 
-void FakeSyncScheduler::RequestStop() {
+void FakeSyncScheduler::RequestStop(const base::Closure& callback) {
+  created_on_loop_->PostTask(FROM_HERE, callback);
 }
 
 void FakeSyncScheduler::ScheduleLocalNudge(
diff --git a/sync/test/engine/fake_sync_scheduler.h b/sync/test/engine/fake_sync_scheduler.h
index f12e1df..96805f5 100644
--- a/sync/test/engine/fake_sync_scheduler.h
+++ b/sync/test/engine/fake_sync_scheduler.h
@@ -20,7 +20,7 @@ class FakeSyncScheduler : public SyncScheduler {
   virtual ~FakeSyncScheduler();
 
   virtual void Start(Mode mode) OVERRIDE;
-  virtual void RequestStop() OVERRIDE;
+  virtual void RequestStop(const base::Closure& callback) OVERRIDE;
   virtual void ScheduleLocalNudge(
       const base::TimeDelta& desired_delay,
       ModelTypeSet types,
@@ -58,6 +58,9 @@ class FakeSyncScheduler : public SyncScheduler {
   virtual void OnShouldStopSyncingPermanently() OVERRIDE;
   virtual void OnSyncProtocolError(
       const sessions::SyncSessionSnapshot& snapshot) OVERRIDE;
+
+ private:
+  base::MessageLoop* const created_on_loop_;
 };
 
 }  // namespace syncer