diff options
author | zea@chromium.org <zea@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-07-13 00:58:14 +0000 |
---|---|---|
committer | zea@chromium.org <zea@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-07-13 00:58:14 +0000 |
commit | 7df2d35dd54de39672acd39ee8e7590479c89b95 (patch) | |
tree | 11b4127dc3bd3a292d17dd00bafafd043061d90f /sync | |
parent | 66bfba6684d26e1eeaed4eacdb5a7e77f11c6725 (diff) | |
download | chromium_src-7df2d35dd54de39672acd39ee8e7590479c89b95.zip chromium_src-7df2d35dd54de39672acd39ee8e7590479c89b95.tar.gz chromium_src-7df2d35dd54de39672acd39ee8e7590479c89b95.tar.bz2 |
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
Diffstat (limited to 'sync')
-rw-r--r-- | sync/engine/sync_scheduler.h | 4 | ||||
-rw-r--r-- | sync/engine/sync_scheduler_impl.cc | 11 | ||||
-rw-r--r-- | sync/engine/sync_scheduler_impl.h | 4 | ||||
-rw-r--r-- | sync/engine/sync_scheduler_unittest.cc | 8 | ||||
-rw-r--r-- | sync/internal_api/public/engine/model_safe_worker.cc | 34 | ||||
-rw-r--r-- | sync/internal_api/public/engine/model_safe_worker.h | 21 | ||||
-rw-r--r-- | sync/internal_api/public/engine/passive_model_worker.cc | 3 | ||||
-rw-r--r-- | sync/internal_api/public/engine/passive_model_worker.h | 4 | ||||
-rw-r--r-- | sync/internal_api/public/sync_manager.h | 2 | ||||
-rw-r--r-- | sync/internal_api/public/test/fake_sync_manager.h | 2 | ||||
-rw-r--r-- | sync/internal_api/sync_manager_impl.cc | 4 | ||||
-rw-r--r-- | sync/internal_api/sync_manager_impl.h | 2 | ||||
-rw-r--r-- | sync/internal_api/test/fake_sync_manager.cc | 5 | ||||
-rw-r--r-- | sync/notifier/non_blocking_invalidator.cc | 2 | ||||
-rw-r--r-- | sync/test/engine/fake_sync_scheduler.cc | 6 | ||||
-rw-r--r-- | sync/test/engine/fake_sync_scheduler.h | 5 |
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 |