diff options
52 files changed, 1064 insertions, 992 deletions
diff --git a/chrome/browser/password_manager/password_store.h b/chrome/browser/password_manager/password_store.h index 9ae6bf3..4d6079f 100644 --- a/chrome/browser/password_manager/password_store.h +++ b/chrome/browser/password_manager/password_store.h @@ -21,7 +21,6 @@ class PasswordStoreConsumer; class Task; namespace browser_sync { -class PasswordChangeProcessor; class PasswordDataTypeController; class PasswordModelAssociator; class PasswordModelWorker; @@ -133,7 +132,6 @@ class PasswordStore protected: friend class base::RefCountedThreadSafe<PasswordStore>; - friend class browser_sync::PasswordChangeProcessor; friend class browser_sync::PasswordDataTypeController; friend class browser_sync::PasswordModelAssociator; friend class browser_sync::PasswordModelWorker; diff --git a/chrome/browser/sync/glue/browser_thread_model_worker.cc b/chrome/browser/sync/glue/browser_thread_model_worker.cc index 7f4024a..57e4b9d 100644 --- a/chrome/browser/sync/glue/browser_thread_model_worker.cc +++ b/chrome/browser/sync/glue/browser_thread_model_worker.cc @@ -51,7 +51,6 @@ BrowserThreadModelWorker::~BrowserThreadModelWorker() {} void BrowserThreadModelWorker::RegisterForLoopDestruction() { if (BrowserThread::CurrentlyOn(thread_)) { base::MessageLoop::current()->AddDestructionObserver(this); - SetWorkingLoopToCurrent(); } else { BrowserThread::PostTask( thread_, FROM_HERE, diff --git a/chrome/browser/sync/glue/history_model_worker.cc b/chrome/browser/sync/glue/history_model_worker.cc index ae05d45..13b5217 100644 --- a/chrome/browser/sync/glue/history_model_worker.cc +++ b/chrome/browser/sync/glue/history_model_worker.cc @@ -43,12 +43,12 @@ class WorkerTask : public history::HistoryDBTask { class AddDBThreadObserverTask : public history::HistoryDBTask { public: - explicit AddDBThreadObserverTask(base::Closure register_callback) - : register_callback_(register_callback) {} + explicit AddDBThreadObserverTask(HistoryModelWorker* history_worker) + : history_worker_(history_worker) {} virtual bool RunOnDBThread(history::HistoryBackend* backend, history::HistoryDatabase* db) OVERRIDE { - register_callback_.Run(); + base::MessageLoop::current()->AddDestructionObserver(history_worker_.get()); return true; } @@ -57,7 +57,7 @@ class AddDBThreadObserverTask : public history::HistoryDBTask { private: virtual ~AddDBThreadObserverTask() {} - base::Closure register_callback_; + scoped_refptr<HistoryModelWorker> history_worker_; }; namespace { @@ -91,15 +91,8 @@ HistoryModelWorker::HistoryModelWorker( void HistoryModelWorker::RegisterForLoopDestruction() { CHECK(history_service_.get()); - history_service_->ScheduleDBTask( - new AddDBThreadObserverTask( - base::Bind(&HistoryModelWorker::RegisterOnDBThread, this)), - &cancelable_consumer_); -} - -void HistoryModelWorker::RegisterOnDBThread() { - base::MessageLoop::current()->AddDestructionObserver(this); - SetWorkingLoopToCurrent(); + history_service_->ScheduleDBTask(new AddDBThreadObserverTask(this), + &cancelable_consumer_); } syncer::SyncerError HistoryModelWorker::DoWorkAndWaitUntilDoneImpl( diff --git a/chrome/browser/sync/glue/history_model_worker.h b/chrome/browser/sync/glue/history_model_worker.h index 0f55569..93112ab 100644 --- a/chrome/browser/sync/glue/history_model_worker.h +++ b/chrome/browser/sync/glue/history_model_worker.h @@ -32,10 +32,6 @@ class HistoryModelWorker : public syncer::ModelSafeWorker { virtual void RegisterForLoopDestruction() OVERRIDE; virtual syncer::ModelSafeGroup GetModelSafeGroup() OVERRIDE; - // Called on history DB thread to register HistoryModelWorker to observe - // destruction of history backend loop. - void RegisterOnDBThread(); - protected: virtual syncer::SyncerError DoWorkAndWaitUntilDoneImpl( const syncer::WorkCallback& work) OVERRIDE; diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller.cc b/chrome/browser/sync/glue/non_frontend_data_type_controller.cc index 88fee95..6d80bef 100644 --- a/chrome/browser/sync/glue/non_frontend_data_type_controller.cc +++ b/chrome/browser/sync/glue/non_frontend_data_type_controller.cc @@ -7,6 +7,7 @@ #include "base/bind.h" #include "base/callback.h" #include "base/logging.h" +#include "base/threading/thread_restrictions.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/sync/glue/change_processor.h" #include "chrome/browser/sync/glue/chrome_report_unrecoverable_error.h" @@ -16,146 +17,12 @@ #include "content/public/browser/browser_thread.h" #include "sync/api/sync_error.h" #include "sync/internal_api/public/base/model_type.h" -#include "sync/internal_api/public/util/weak_handle.h" #include "sync/util/data_type_histogram.h" using content::BrowserThread; namespace browser_sync { -class NonFrontendDataTypeController::BackendComponentsContainer { - public: - explicit BackendComponentsContainer( - NonFrontendDataTypeController* controller); - ~BackendComponentsContainer(); - void Run(); - void Disconnect(); - - private: - bool CreateComponents(); - void Associate(); - - // For creating components. - NonFrontendDataTypeController* controller_; - base::Lock controller_lock_; - - syncer::ModelType type_; - - // For returning association results to controller on UI. - syncer::WeakHandle<NonFrontendDataTypeController> controller_handle_; - - scoped_ptr<AssociatorInterface> model_associator_; - scoped_ptr<ChangeProcessor> change_processor_; -}; - -NonFrontendDataTypeController:: -BackendComponentsContainer::BackendComponentsContainer( - NonFrontendDataTypeController* controller) - : controller_(controller), - type_(controller->type()) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - controller_handle_ = - syncer::MakeWeakHandle(controller_->weak_ptr_factory_.GetWeakPtr()); -} - -NonFrontendDataTypeController:: -BackendComponentsContainer::~BackendComponentsContainer() { - if (model_associator_) - model_associator_->DisassociateModels(); -} - -void NonFrontendDataTypeController::BackendComponentsContainer::Run() { - DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); - if (CreateComponents()) - Associate(); -} - -bool -NonFrontendDataTypeController::BackendComponentsContainer::CreateComponents() { - base::AutoLock al(controller_lock_); - if (!controller_) { - DVLOG(1) << "Controller was stopped before sync components are created."; - return false; - } - - ProfileSyncComponentsFactory::SyncComponents sync_components = - controller_->CreateSyncComponents(); - model_associator_.reset(sync_components.model_associator); - change_processor_.reset(sync_components.change_processor); - return true; -} - -void NonFrontendDataTypeController::BackendComponentsContainer::Associate() { - CHECK(model_associator_); - - bool succeeded = false; - - browser_sync::NonFrontendDataTypeController::AssociationResult result(type_); - if (!model_associator_->CryptoReadyIfNecessary()) { - result.needs_crypto = true; - } else { - base::TimeTicks start_time = base::TimeTicks::Now(); - - if (!model_associator_->SyncModelHasUserCreatedNodes( - &result.sync_has_nodes)) { - result.unrecoverable_error = true; - result.error = syncer::SyncError(FROM_HERE, - syncer::SyncError::UNRECOVERABLE_ERROR, - "Failed to load sync nodes", - type_); - } else { - result.error = model_associator_->AssociateModels( - &result.local_merge_result, &result.syncer_merge_result); - - // Return components to frontend when no error. - if (!result.error.IsSet()) { - succeeded = true; - result.change_processor = change_processor_.get(); - result.model_associator = model_associator_.get(); - } - } - result.association_time = base::TimeTicks::Now() - start_time; - } - result.local_merge_result.set_error(result.error); - - // Destroy processor/associator on backend on failure. - if (!succeeded) { - base::AutoLock al(controller_lock_); - model_associator_->DisassociateModels(); - change_processor_.reset(); - model_associator_.reset(); - } - - controller_handle_.Call( - FROM_HERE, - &browser_sync::NonFrontendDataTypeController::AssociationCallback, - result); -} - -void NonFrontendDataTypeController::BackendComponentsContainer::Disconnect() { - base::AutoLock al(controller_lock_); - CHECK(controller_); - - if (change_processor_) - controller_->DisconnectProcessor(change_processor_.get()); - if (model_associator_) - model_associator_->AbortAssociation(); - - controller_ = NULL; -} - -NonFrontendDataTypeController::AssociationResult::AssociationResult( - syncer::ModelType type) - : needs_crypto(false), - unrecoverable_error(false), - sync_has_nodes(false), - local_merge_result(type), - syncer_merge_result(type), - change_processor(NULL), - model_associator(NULL) {} - -NonFrontendDataTypeController::AssociationResult::~AssociationResult() {} - NonFrontendDataTypeController::NonFrontendDataTypeController( ProfileSyncComponentsFactory* profile_sync_factory, Profile* profile, @@ -164,9 +31,10 @@ NonFrontendDataTypeController::NonFrontendDataTypeController( profile_sync_factory_(profile_sync_factory), profile_(profile), profile_sync_service_(sync_service), - model_associator_(NULL), - change_processor_(NULL), - weak_ptr_factory_(this) { + abort_association_(false), + abort_association_complete_(false, false), + start_association_called_(true, false), + start_models_failed_(false) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(profile_sync_factory_); DCHECK(profile_); @@ -177,6 +45,8 @@ void NonFrontendDataTypeController::LoadModels( const ModelLoadCallback& model_load_callback) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(!model_load_callback.is_null()); + start_association_called_.Reset(); + start_models_failed_ = false; if (state_ != NOT_RUNNING) { model_load_callback.Run(type(), syncer::SyncError(FROM_HERE, @@ -188,6 +58,7 @@ void NonFrontendDataTypeController::LoadModels( state_ = MODEL_STARTING; if (!StartModels()) { + start_models_failed_ = true; // We failed to start the models. There is no point in waiting. // Note: This code is deprecated. The only 2 datatypes here, // passwords and typed urls, dont have any special loading. So if we @@ -214,69 +85,131 @@ void NonFrontendDataTypeController::StartAssociating( const StartCallback& start_callback) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(!start_callback.is_null()); - DCHECK(!components_container_); DCHECK_EQ(state_, MODEL_LOADED); // Kick off association on the thread the datatype resides on. state_ = ASSOCIATING; start_callback_ = start_callback; - - components_container_.reset(new BackendComponentsContainer(this)); - - if (!PostTaskOnBackendThread( - FROM_HERE, - base::Bind(&BackendComponentsContainer::Run, - base::Unretained(components_container_.get())))) { + if (!StartAssociationAsync()) { syncer::SyncError error( FROM_HERE, syncer::SyncError::DATATYPE_ERROR, "Failed to post StartAssociation", type()); syncer::SyncMergeResult local_merge_result(type()); local_merge_result.set_error(error); - StartDone(ASSOCIATION_FAILED, - local_merge_result, - syncer::SyncMergeResult(type())); + StartDoneImpl(ASSOCIATION_FAILED, + DISABLED, + local_merge_result, + syncer::SyncMergeResult(type())); } } -void DestroyComponentsInBackend( - NonFrontendDataTypeController::BackendComponentsContainer *containter) { - delete containter; +void NonFrontendDataTypeController::StopWhileAssociating() { + state_ = STOPPING; + { + base::AutoLock lock(abort_association_lock_); + abort_association_ = true; + if (model_associator_) + model_associator_->AbortAssociation(); + if (!start_association_called_.IsSignaled()) { + StartDoneImpl(ABORTED, + NOT_RUNNING, + syncer::SyncMergeResult(type()), + syncer::SyncMergeResult(type())); + return; // There is nothing more for us to do. + } + } + + // Wait for the model association to abort. + if (start_association_called_.IsSignaled()) { + LOG(INFO) << "Stopping after |StartAssocation| is called."; + if (start_models_failed_) { + LOG(INFO) << "Start models failed"; + abort_association_complete_.Wait(); + } else { + LOG(INFO) << "Start models succeeded"; + abort_association_complete_.Wait(); + } + } else { + LOG(INFO) << "Stopping before |StartAssocation| is called."; + if (start_models_failed_) { + LOG(INFO) << "Start models failed"; + abort_association_complete_.Wait(); + } else { + LOG(INFO) << "Start models succeeded"; + abort_association_complete_.Wait(); + } + + } + + StartDoneImpl(ABORTED, + STOPPING, + syncer::SyncMergeResult(type()), + syncer::SyncMergeResult(type())); } +namespace { +// Helper function that signals the UI thread once the StopAssociation task +// has finished completing (this task is queued after the StopAssociation task). +void SignalCallback(base::WaitableEvent* wait_event) { + wait_event->Signal(); +} +} // namespace + void NonFrontendDataTypeController::Stop() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK_NE(state_, NOT_RUNNING); - // Deactivate the date type on the UI thread first to stop processing - // sync server changes. This needs to happen before posting task to destroy - // processor and associator on backend. Otherwise it could crash if syncer - // post work to backend after destruction task and that work is run before - // deactivation. - profile_sync_service()->DeactivateDataType(type()); - - // Ignore association callback. - weak_ptr_factory_.InvalidateWeakPtrs(); - - // Disconnect on UI and post task to destroy on backend. - if (components_container_) { - components_container_->Disconnect(); - PostTaskOnBackendThread( - FROM_HERE, - base::Bind(&DestroyComponentsInBackend, - components_container_.release())); - model_associator_ = NULL; - change_processor_ = NULL; + // TODO(sync): Blocking the UI thread at shutdown is bad. The new API avoids + // this. Once all non-frontend datatypes use the new API, we can get rid of + // this locking (see implementation in AutofillProfileDataTypeController). + // http://crbug.com/19757 + base::ThreadRestrictions::ScopedAllowWait allow_wait; + + // If Stop() is called while Start() is waiting for association to + // complete, we need to abort the association and wait for the DB + // thread to finish the StartImpl() task. + switch (state_) { + case ASSOCIATING: + StopWhileAssociating(); + + // TODO(sync) : This should be cleaned up. Once we move to the new api + // this should not be a problem. + if (!start_association_called_.IsSignaled()) { + // If datatype's thread has not even picked up executing it is safe + // to bail out now. We have no more state cleanups to do. + // The risk of waiting is that the datatype thread might not respond. + return; + } + break; + case MODEL_STARTING: + // It is possible for a model to fail to start. For example, if the + // password store on a machine is unavailable, the password model will not + // start. In such cases, we should stop the model instead of crashing. + state_ = STOPPING; + StopModels(); + return; + case DISABLED: + state_ = NOT_RUNNING; + StopModels(); + return; + default: + DCHECK_EQ(state_, RUNNING); + state_ = STOPPING; + StopModels(); + break; } + DCHECK(start_callback_.is_null()); - // Call start callback if waiting for association. - if (state_ == ASSOCIATING) { - StartDone(ABORTED, - syncer::SyncMergeResult(type()), - syncer::SyncMergeResult(type())); + // Deactivate the change processor on the UI thread. We dont want to listen + // for any more changes or process them from server. + profile_sync_service_->DeactivateDataType(type()); + if (!StopAssociationAsync()) { + // We do DFATAL here because this will eventually lead to a failed CHECK + // when the change processor gets destroyed on the wrong thread. + LOG(DFATAL) << "Failed to destroy datatype " << name(); } - state_ = NOT_RUNNING; } @@ -306,15 +239,13 @@ NonFrontendDataTypeController::NonFrontendDataTypeController() profile_sync_factory_(NULL), profile_(NULL), profile_sync_service_(NULL), - model_associator_(NULL), - change_processor_(NULL), - weak_ptr_factory_(this) { + abort_association_(false), + abort_association_complete_(false, false), + start_association_called_(true, false) { } NonFrontendDataTypeController::~NonFrontendDataTypeController() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - DCHECK(!change_processor_); - DCHECK(!model_associator_); } bool NonFrontendDataTypeController::StartModels() { @@ -329,7 +260,7 @@ void NonFrontendDataTypeController::StartDone( DataTypeController::StartResult start_result, const syncer::SyncMergeResult& local_merge_result, const syncer::SyncMergeResult& syncer_merge_result) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); DataTypeController::State new_state; if (IsSuccessfulResult(start_result)) { @@ -338,10 +269,20 @@ void NonFrontendDataTypeController::StartDone( new_state = (start_result == ASSOCIATION_FAILED ? DISABLED : NOT_RUNNING); if (IsUnrecoverableResult(start_result)) RecordUnrecoverableError(FROM_HERE, "StartFailed"); + StopAssociation(); } - StartDoneImpl(start_result, new_state, local_merge_result, - syncer_merge_result); + abort_association_complete_.Signal(); + base::AutoLock lock(abort_association_lock_); + if (!abort_association_) { + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, + base::Bind(&NonFrontendDataTypeController::StartDoneImpl, + this, + start_result, + new_state, + local_merge_result, + syncer_merge_result)); + } } void NonFrontendDataTypeController::StartDoneImpl( @@ -350,14 +291,21 @@ void NonFrontendDataTypeController::StartDoneImpl( const syncer::SyncMergeResult& local_merge_result, const syncer::SyncMergeResult& syncer_merge_result) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + // It's possible to have StartDoneImpl called first from the UI thread + // (due to Stop being called) and then posted from the non-UI thread. In + // this case, we drop the second call because we've already been stopped. + if (state_ == NOT_RUNNING) { + DCHECK(start_callback_.is_null()); + return; + } state_ = new_state; if (state_ != RUNNING) { // Start failed. + StopModels(); RecordStartFailure(start_result); } - DCHECK(!start_callback_.is_null()); // We have to release the callback before we call it, since it's possible // invoking the callback will trigger a call to STOP(), which will get // confused by the non-NULL start_callback_. @@ -366,6 +314,13 @@ void NonFrontendDataTypeController::StartDoneImpl( callback.Run(start_result, local_merge_result, syncer_merge_result); } +void NonFrontendDataTypeController::StopModels() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(state_ == STOPPING || state_ == NOT_RUNNING || state_ == DISABLED); + DVLOG(1) << "NonFrontendDataTypeController::StopModels(): State = " << state_; + // Do nothing by default. +} + void NonFrontendDataTypeController::DisableImpl( const tracked_objects::Location& from_here, const std::string& message) { @@ -375,7 +330,7 @@ void NonFrontendDataTypeController::DisableImpl( void NonFrontendDataTypeController::RecordAssociationTime( base::TimeDelta time) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); #define PER_DATA_TYPE_MACRO(type_str) \ UMA_HISTOGRAM_TIMES("Sync." type_str "AssociationTime", time); SYNC_DATA_TYPE_HISTOGRAM(type()); @@ -394,6 +349,14 @@ void NonFrontendDataTypeController::RecordStartFailure(StartResult result) { #undef PER_DATA_TYPE_MACRO } +bool NonFrontendDataTypeController::StartAssociationAsync() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK_EQ(state(), ASSOCIATING); + return PostTaskOnBackendThread( + FROM_HERE, + base::Bind(&NonFrontendDataTypeController::StartAssociation, this)); +} + ProfileSyncComponentsFactory* NonFrontendDataTypeController::profile_sync_factory() const { return profile_sync_factory_; @@ -418,49 +381,117 @@ void NonFrontendDataTypeController::set_state(State state) { } AssociatorInterface* NonFrontendDataTypeController::associator() const { - return model_associator_; + return model_associator_.get(); +} + +void NonFrontendDataTypeController::set_model_associator( + AssociatorInterface* associator) { + model_associator_.reset(associator); } ChangeProcessor* NonFrontendDataTypeController::change_processor() const { - return change_processor_; + return change_processor_.get(); } -void NonFrontendDataTypeController::AssociationCallback( - AssociationResult result) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); +void NonFrontendDataTypeController::set_change_processor( + ChangeProcessor* change_processor) { + change_processor_.reset(change_processor); +} + +void NonFrontendDataTypeController::StartAssociation() { + DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); + syncer::SyncMergeResult local_merge_result(type()); + syncer::SyncMergeResult syncer_merge_result(type()); + + { + base::AutoLock lock(abort_association_lock_); + if (abort_association_) { + abort_association_complete_.Signal(); + return; + } + start_association_called_.Signal(); + CreateSyncComponents(); + } - if (result.needs_crypto) { + DCHECK_EQ(state_, ASSOCIATING); + + if (!model_associator_->CryptoReadyIfNecessary()) { StartDone(NEEDS_CRYPTO, - result.local_merge_result, - result.syncer_merge_result); + local_merge_result, + syncer_merge_result); return; } - if (result.unrecoverable_error) { + bool sync_has_nodes = false; + if (!model_associator_->SyncModelHasUserCreatedNodes(&sync_has_nodes)) { + syncer::SyncError error(FROM_HERE, + syncer::SyncError::UNRECOVERABLE_ERROR, + "Failed to load sync nodes", + type()); + local_merge_result.set_error(error); StartDone(UNRECOVERABLE_ERROR, - result.local_merge_result, - result.syncer_merge_result); + local_merge_result, + syncer_merge_result); return; } - RecordAssociationTime(result.association_time); - if (result.error.IsSet()) { + // TODO(zea): Have AssociateModels fill the local and syncer merge results. + base::TimeTicks start_time = base::TimeTicks::Now(); + syncer::SyncError error = model_associator_->AssociateModels( + &local_merge_result, + &syncer_merge_result); + // TODO(lipalani): crbug.com/122690 - handle abort. + RecordAssociationTime(base::TimeTicks::Now() - start_time); + if (error.IsSet()) { + local_merge_result.set_error(error); StartDone(ASSOCIATION_FAILED, - result.local_merge_result, - result.syncer_merge_result); + local_merge_result, + syncer_merge_result); return; } - CHECK(result.change_processor); - CHECK(result.model_associator); - change_processor_ = result.change_processor; - model_associator_ = result.model_associator; - profile_sync_service_->ActivateDataType(type(), model_safe_group(), change_processor()); - StartDone(!result.sync_has_nodes ? OK_FIRST_RUN : OK, - result.local_merge_result, - result.syncer_merge_result); + StartDone(!sync_has_nodes ? OK_FIRST_RUN : OK, + local_merge_result, + syncer_merge_result); +} + +bool NonFrontendDataTypeController::StopAssociationAsync() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK_EQ(state(), STOPPING); + if (PostTaskOnBackendThread( + FROM_HERE, + base::Bind( + &NonFrontendDataTypeController::StopAssociation, this))) { + // The remote thread will hold on to a reference to this object until + // the StopAssociation task finishes running. We want to make sure that we + // do not return from this routine until there are no more references to + // this object on the remote thread, so we queue up the SignalCallback + // task below - this task does not maintain a reference to the DTC, so + // when it signals this thread, we know that the previous task has executed + // and there are no more lingering remote references to the DTC. + // This fixes the race described in http://crbug.com/127706. + base::WaitableEvent datatype_stopped(false, false); + if (PostTaskOnBackendThread( + FROM_HERE, + base::Bind(&SignalCallback, &datatype_stopped))) { + datatype_stopped.Wait(); + return true; + } + } + return false; +} + +void NonFrontendDataTypeController::StopAssociation() { + DCHECK(!HasOneRef()); + DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); + if (model_associator_) { + syncer::SyncError error; // Not used. + error = model_associator_->DisassociateModels(); + } + model_associator_.reset(); + change_processor_.reset(); } } // namespace browser_sync diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller.h b/chrome/browser/sync/glue/non_frontend_data_type_controller.h index 35cacb0..8a67b12c 100644 --- a/chrome/browser/sync/glue/non_frontend_data_type_controller.h +++ b/chrome/browser/sync/glue/non_frontend_data_type_controller.h @@ -15,7 +15,6 @@ #include "base/synchronization/waitable_event.h" #include "chrome/browser/sync/glue/data_type_controller.h" #include "chrome/browser/sync/glue/data_type_error_handler.h" -#include "chrome/browser/sync/profile_sync_components_factory.h" class Profile; class ProfileSyncService; @@ -37,16 +36,13 @@ class ChangeProcessor; // Implementation for datatypes that do not reside on the frontend thread // (UI thread). This is the same thread we perform initialization // on, so we don't have to worry about thread safety. The main start/stop -// functionality is implemented by default. Derived classes must implement: +// funtionality is implemented by default. Derived classes must implement: // type() // model_safe_group() // PostTaskOnBackendThread() // CreateSyncComponents() class NonFrontendDataTypeController : public DataTypeController { public: - // For creating non-frontend processor/associator and associating on backend. - class BackendComponentsContainer; - NonFrontendDataTypeController( ProfileSyncComponentsFactory* profile_sync_factory, Profile* profile, @@ -68,22 +64,6 @@ class NonFrontendDataTypeController : public DataTypeController { const tracked_objects::Location& from_here, const std::string& message) OVERRIDE; - // Callback to receive background association results. - struct AssociationResult { - explicit AssociationResult(syncer::ModelType type); - ~AssociationResult(); - bool needs_crypto; - bool unrecoverable_error; - bool sync_has_nodes; - syncer::SyncError error; - syncer::SyncMergeResult local_merge_result; - syncer::SyncMergeResult syncer_merge_result; - base::TimeDelta association_time; - ChangeProcessor* change_processor; - AssociatorInterface* model_associator; - }; - void AssociationCallback(AssociationResult result); - protected: // For testing only. NonFrontendDataTypeController(); @@ -114,12 +94,7 @@ class NonFrontendDataTypeController : public DataTypeController { // Datatype specific creation of sync components. // Note: this is performed on the datatype's thread. - virtual ProfileSyncComponentsFactory::SyncComponents - CreateSyncComponents() = 0; - - // Called on UI thread during shutdown to effectively disable processing - // any changes. - virtual void DisconnectProcessor(ChangeProcessor* processor) = 0; + virtual void CreateSyncComponents() = 0; // Start up complete, update the state and invoke the callback. // Note: this is performed on the datatype's thread. @@ -135,6 +110,11 @@ class NonFrontendDataTypeController : public DataTypeController { const syncer::SyncMergeResult& local_merge_result, const syncer::SyncMergeResult& syncer_merge_result); + // Perform any DataType controller specific state cleanup before stopping + // the datatype controller. The default implementation is a no-op. + // Note: this is performed on the frontend (UI) thread. + virtual void StopModels(); + // The actual implementation of Disabling the datatype. This happens // on the UI thread. virtual void DisableImpl(const tracked_objects::Location& from_here, @@ -153,26 +133,60 @@ class NonFrontendDataTypeController : public DataTypeController { void set_state(State state); virtual AssociatorInterface* associator() const; + virtual void set_model_associator(AssociatorInterface* associator); virtual ChangeProcessor* change_processor() const; + virtual void set_change_processor(ChangeProcessor* change_processor); State state_; StartCallback start_callback_; ModelLoadCallback model_load_callback_; private: - friend class BackendComponentsContainer; + // Post the association task to the thread the datatype lives on. + // Note: this is performed on the frontend (UI) thread. + // Return value: True if task posted successfully, False otherwise. + // + // TODO(akalin): Callers handle false return values inconsistently; + // some set the state to NOT_RUNNING, and some set the state to + // DISABLED. Move the error handling inside this function to be + // consistent. + virtual bool StartAssociationAsync(); + + // Build sync components and associate models. + // Note: this is performed on the datatype's thread. + void StartAssociation(); + + // Helper method to stop associating. + void StopWhileAssociating(); + + // Post the StopAssociation task to the thread the datatype lives on. + // Note: this is performed on the frontend (UI) thread. + // Return value: True if task posted successfully, False otherwise. + bool StopAssociationAsync(); + + // Disassociate the models and destroy the sync components. + // Note: this is performed on the datatype's thread. + void StopAssociation(); + ProfileSyncComponentsFactory* const profile_sync_factory_; Profile* const profile_; ProfileSyncService* const profile_sync_service_; - // Created on UI thread and passed to backend to create processor/associator - // and associate model. Released on backend. - scoped_ptr<BackendComponentsContainer> components_container_; + scoped_ptr<AssociatorInterface> model_associator_; + scoped_ptr<ChangeProcessor> change_processor_; + + // Locks/Barriers for aborting association early. + base::Lock abort_association_lock_; + bool abort_association_; + base::WaitableEvent abort_association_complete_; - AssociatorInterface* model_associator_; - ChangeProcessor* change_processor_; + // This is added for debugging purpose. + // TODO(lipalani): Remove this after debugging. + base::WaitableEvent start_association_called_; - base::WeakPtrFactory<NonFrontendDataTypeController> weak_ptr_factory_; + // This is added for debugging purpose. + // TODO(lipalani): Remove after debugging. + bool start_models_failed_; DISALLOW_COPY_AND_ASSIGN(NonFrontendDataTypeController); }; diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller_mock.h b/chrome/browser/sync/glue/non_frontend_data_type_controller_mock.h index 6d3adc4..a9cbfaf 100644 --- a/chrome/browser/sync/glue/non_frontend_data_type_controller_mock.h +++ b/chrome/browser/sync/glue/non_frontend_data_type_controller_mock.h @@ -36,8 +36,7 @@ class NonFrontendDataTypeControllerMock : public NonFrontendDataTypeController { bool(const tracked_objects::Location&, const base::Closure&)); MOCK_METHOD0(StartAssociation, void()); - MOCK_METHOD0(CreateSyncComponents, - ProfileSyncComponentsFactory::SyncComponents()); + MOCK_METHOD0(CreateSyncComponents, void()); MOCK_METHOD3(StartDone, void(DataTypeController::StartResult result, const syncer::SyncMergeResult& local_merge_result, @@ -47,7 +46,8 @@ class NonFrontendDataTypeControllerMock : public NonFrontendDataTypeController { DataTypeController::State new_state, const syncer::SyncMergeResult& local_merge_result, const syncer::SyncMergeResult& syncer_merge_result)); - MOCK_METHOD1(DisconnectProcessor, void(ChangeProcessor*)); + MOCK_METHOD0(StopModels, void()); + MOCK_METHOD0(StopAssociation, void()); MOCK_METHOD2(OnUnrecoverableErrorImpl, void(const tracked_objects::Location&, const std::string&)); MOCK_METHOD2(RecordUnrecoverableError, void(const tracked_objects::Location&, diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc b/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc index 103335e..2c6a1d6 100644 --- a/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc @@ -69,10 +69,12 @@ class NonFrontendDataTypeControllerFake : public NonFrontendDataTypeController { private: virtual ~NonFrontendDataTypeControllerFake() {} - virtual ProfileSyncComponentsFactory::SyncComponents - CreateSyncComponents() OVERRIDE { - return profile_sync_factory()-> + virtual void CreateSyncComponents() OVERRIDE { + ProfileSyncComponentsFactory::SyncComponents sync_components = + profile_sync_factory()-> CreateBookmarkSyncComponents(profile_sync_service(), this); + set_model_associator(sync_components.model_associator); + set_change_processor(sync_components.change_processor); } virtual bool PostTaskOnBackendThread( @@ -86,6 +88,9 @@ class NonFrontendDataTypeControllerFake : public NonFrontendDataTypeController { virtual bool StartModels() OVERRIDE { return mock_->StartModels(); } + virtual void StopModels() OVERRIDE { + mock_->StopModels(); + } virtual void RecordUnrecoverableError( const tracked_objects::Location& from_here, const std::string& message) OVERRIDE { @@ -98,11 +103,6 @@ class NonFrontendDataTypeControllerFake : public NonFrontendDataTypeController { DataTypeController::StartResult result) OVERRIDE { mock_->RecordStartFailure(result); } - virtual void DisconnectProcessor( - browser_sync::ChangeProcessor* processor) OVERRIDE{ - mock_->DisconnectProcessor(processor); - } - private: NonFrontendDataTypeControllerMock* mock_; }; @@ -130,10 +130,6 @@ class SyncNonFrontendDataTypeControllerTest : public testing::Test { } virtual void TearDown() { - if (non_frontend_dtc_->state() != - NonFrontendDataTypeController::NOT_RUNNING) { - non_frontend_dtc_->Stop(); - } db_thread_.Stop(); } @@ -164,13 +160,14 @@ class SyncNonFrontendDataTypeControllerTest : public testing::Test { } void SetStopExpectations() { - EXPECT_CALL(*dtc_mock_.get(), DisconnectProcessor(_)); + EXPECT_CALL(*dtc_mock_.get(), StopModels()); EXPECT_CALL(service_, DeactivateDataType(_)); EXPECT_CALL(*model_associator_, DisassociateModels()). WillOnce(Return(syncer::SyncError())); } void SetStartFailExpectations(DataTypeController::StartResult result) { + EXPECT_CALL(*dtc_mock_.get(), StopModels()); if (DataTypeController::IsUnrecoverableResult(result)) EXPECT_CALL(*dtc_mock_.get(), RecordUnrecoverableError(_, _)); if (model_associator_) { @@ -223,7 +220,6 @@ TEST_F(SyncNonFrontendDataTypeControllerTest, StartOk) { SetStartExpectations(); SetAssociateExpectations(); SetActivateExpectations(DataTypeController::OK); - SetStopExpectations(); EXPECT_EQ(DataTypeController::NOT_RUNNING, non_frontend_dtc_->state()); Start(); WaitForDTC(); @@ -240,7 +236,6 @@ TEST_F(SyncNonFrontendDataTypeControllerTest, StartFirstRun) { WillOnce(Return(syncer::SyncError())); EXPECT_CALL(*dtc_mock_.get(), RecordAssociationTime(_)); SetActivateExpectations(DataTypeController::OK_FIRST_RUN); - SetStopExpectations(); EXPECT_EQ(DataTypeController::NOT_RUNNING, non_frontend_dtc_->state()); Start(); WaitForDTC(); @@ -314,6 +309,8 @@ TEST_F(SyncNonFrontendDataTypeControllerTest, AbortDuringAssociationInactive) { SignalEvent(&pause_db_thread)); EXPECT_CALL(*model_associator_, AssociateModels(_, _)). WillOnce(Return(syncer::SyncError())); + EXPECT_CALL(*dtc_mock_.get(), RecordAssociationTime(_)); + EXPECT_CALL(service_, ActivateDataType(_, _, _)); EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED,_,_)); EXPECT_CALL(*dtc_mock_.get(), RecordStartFailure(DataTypeController::ABORTED)); @@ -328,8 +325,8 @@ TEST_F(SyncNonFrontendDataTypeControllerTest, AbortDuringAssociationInactive) { // Same as above but abort during the Activate call. TEST_F(SyncNonFrontendDataTypeControllerTest, AbortDuringAssociationActivated) { - WaitableEvent wait_for_association_starts(false, false); - WaitableEvent wait_for_dtc_stop(false, false); + WaitableEvent wait_for_db_thread_pause(false, false); + WaitableEvent pause_db_thread(false, false); SetStartExpectations(); EXPECT_CALL(*model_associator_, CryptoReadyIfNecessary()). @@ -338,21 +335,21 @@ TEST_F(SyncNonFrontendDataTypeControllerTest, AbortDuringAssociationActivated) { WillOnce(DoAll( SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AbortAssociation()); + EXPECT_CALL(*model_associator_, AbortAssociation()).WillOnce( + SignalEvent(&pause_db_thread)); EXPECT_CALL(*model_associator_, AssociateModels(_, _)). - WillOnce(DoAll( - SignalEvent(&wait_for_association_starts), - WaitOnEvent(&wait_for_dtc_stop), - Return(syncer::SyncError()))); + WillOnce(Return(syncer::SyncError())); + EXPECT_CALL(*dtc_mock_.get(), RecordAssociationTime(_)); + EXPECT_CALL(service_, ActivateDataType(_, _, _)).WillOnce(DoAll( + SignalEvent(&wait_for_db_thread_pause), WaitOnEvent(&pause_db_thread))); EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED,_,_)); EXPECT_CALL(*dtc_mock_.get(), RecordStartFailure(DataTypeController::ABORTED)); SetStopExpectations(); EXPECT_EQ(DataTypeController::NOT_RUNNING, non_frontend_dtc_->state()); Start(); - wait_for_association_starts.Wait(); + wait_for_db_thread_pause.Wait(); non_frontend_dtc_->Stop(); - wait_for_dtc_stop.Signal(); WaitForDTC(); EXPECT_EQ(DataTypeController::NOT_RUNNING, non_frontend_dtc_->state()); } diff --git a/chrome/browser/sync/glue/password_change_processor.cc b/chrome/browser/sync/glue/password_change_processor.cc index e6969b7..83b8cb5 100644 --- a/chrome/browser/sync/glue/password_change_processor.cc +++ b/chrome/browser/sync/glue/password_change_processor.cc @@ -35,8 +35,7 @@ PasswordChangeProcessor::PasswordChangeProcessor( : ChangeProcessor(error_handler), model_associator_(model_associator), password_store_(password_store), - expected_loop_(base::MessageLoop::current()), - disconnected_(false) { + expected_loop_(base::MessageLoop::current()) { DCHECK(model_associator); DCHECK(error_handler); #if defined(OS_MACOSX) @@ -57,10 +56,6 @@ void PasswordChangeProcessor::Observe( DCHECK(expected_loop_ == base::MessageLoop::current()); DCHECK(chrome::NOTIFICATION_LOGINS_CHANGED == type); - base::AutoLock lock(disconnect_lock_); - if (disconnected_) - return; - syncer::WriteTransaction trans(FROM_HERE, share_handle()); syncer::ReadNode password_root(&trans); @@ -167,9 +162,6 @@ void PasswordChangeProcessor::ApplyChangesFromSyncModel( int64 model_version, const syncer::ImmutableChangeRecordList& changes) { DCHECK(expected_loop_ == base::MessageLoop::current()); - base::AutoLock lock(disconnect_lock_); - if (disconnected_) - return; syncer::ReadNode password_root(trans); if (password_root.InitByTagLookup(kPasswordTag) != @@ -228,10 +220,6 @@ void PasswordChangeProcessor::ApplyChangesFromSyncModel( void PasswordChangeProcessor::CommitChangesFromSyncModel() { DCHECK(expected_loop_ == base::MessageLoop::current()); - base::AutoLock lock(disconnect_lock_); - if (disconnected_) - return; - ScopedStopObserving<PasswordChangeProcessor> stop_observing(this); syncer::SyncError error = model_associator_->WriteToPasswordStore( @@ -249,17 +237,9 @@ void PasswordChangeProcessor::CommitChangesFromSyncModel() { updated_passwords_.clear(); } -void PasswordChangeProcessor::Disconnect() { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - base::AutoLock lock(disconnect_lock_); - disconnected_ = true; -} - void PasswordChangeProcessor::StartImpl(Profile* profile) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - password_store_->ScheduleTask( - base::Bind(&PasswordChangeProcessor::StartObserving, - base::Unretained(this))); + DCHECK(expected_loop_ == base::MessageLoop::current()); + StartObserving(); } void PasswordChangeProcessor::StartObserving() { diff --git a/chrome/browser/sync/glue/password_change_processor.h b/chrome/browser/sync/glue/password_change_processor.h index 11e4372..ed4b54c 100644 --- a/chrome/browser/sync/glue/password_change_processor.h +++ b/chrome/browser/sync/glue/password_change_processor.h @@ -56,9 +56,6 @@ class PasswordChangeProcessor : public ChangeProcessor, // thread (http://crbug.com/70658). virtual void CommitChangesFromSyncModel() OVERRIDE; - // Stop processing changes and wait for being destroyed. - void Disconnect(); - protected: virtual void StartImpl(Profile* profile) OVERRIDE; @@ -85,11 +82,6 @@ class PasswordChangeProcessor : public ChangeProcessor, base::MessageLoop* expected_loop_; - // If disconnected is true, local/sync changes are dropped without modifying - // sync/local models. - bool disconnected_; - base::Lock disconnect_lock_; - DISALLOW_COPY_AND_ASSIGN(PasswordChangeProcessor); }; diff --git a/chrome/browser/sync/glue/password_data_type_controller.cc b/chrome/browser/sync/glue/password_data_type_controller.cc index 688c2a6..db8df21 100644 --- a/chrome/browser/sync/glue/password_data_type_controller.cc +++ b/chrome/browser/sync/glue/password_data_type_controller.cc @@ -9,7 +9,7 @@ #include "chrome/browser/password_manager/password_store.h" #include "chrome/browser/password_manager/password_store_factory.h" #include "chrome/browser/profiles/profile.h" -#include "chrome/browser/sync/glue/password_change_processor.h" +#include "chrome/browser/sync/profile_sync_components_factory.h" #include "chrome/browser/sync/profile_sync_service.h" #include "content/public/browser/browser_thread.h" #include "sync/api/sync_error.h" @@ -42,8 +42,7 @@ bool PasswordDataTypeController::PostTaskOnBackendThread( const tracked_objects::Location& from_here, const base::Closure& task) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - if (!password_store_) - return false; + DCHECK(password_store_.get()); return password_store_->ScheduleTask(task); } @@ -55,19 +54,16 @@ bool PasswordDataTypeController::StartModels() { return password_store_.get() != NULL; } -ProfileSyncComponentsFactory::SyncComponents -PasswordDataTypeController::CreateSyncComponents() { +void PasswordDataTypeController::CreateSyncComponents() { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK_EQ(state(), ASSOCIATING); - return profile_sync_factory()->CreatePasswordSyncComponents( - profile_sync_service(), - password_store_.get(), - this); -} - -void PasswordDataTypeController::DisconnectProcessor( - ChangeProcessor* processor) { - static_cast<PasswordChangeProcessor*>(processor)->Disconnect(); + ProfileSyncComponentsFactory::SyncComponents sync_components = + profile_sync_factory()->CreatePasswordSyncComponents( + profile_sync_service(), + password_store_.get(), + this); + set_model_associator(sync_components.model_associator); + set_change_processor(sync_components.change_processor); } } // namespace browser_sync diff --git a/chrome/browser/sync/glue/password_data_type_controller.h b/chrome/browser/sync/glue/password_data_type_controller.h index d9bcdf0..75bbf4b 100644 --- a/chrome/browser/sync/glue/password_data_type_controller.h +++ b/chrome/browser/sync/glue/password_data_type_controller.h @@ -35,9 +35,7 @@ class PasswordDataTypeController : public NonFrontendDataTypeController { const tracked_objects::Location& from_here, const base::Closure& task) OVERRIDE; virtual bool StartModels() OVERRIDE; - virtual ProfileSyncComponentsFactory::SyncComponents CreateSyncComponents() - OVERRIDE; - virtual void DisconnectProcessor(ChangeProcessor* processor) OVERRIDE; + virtual void CreateSyncComponents() OVERRIDE; private: scoped_refptr<PasswordStore> password_store_; diff --git a/chrome/browser/sync/glue/password_model_associator.cc b/chrome/browser/sync/glue/password_model_associator.cc index 96416783..f034c47 100644 --- a/chrome/browser/sync/glue/password_model_associator.cc +++ b/chrome/browser/sync/glue/password_model_associator.cc @@ -34,7 +34,7 @@ PasswordModelAssociator::PasswordModelAssociator( : sync_service_(sync_service), password_store_(password_store), password_node_id_(syncer::kInvalidId), - abort_association_requested_(false), + abort_association_pending_(false), expected_loop_(base::MessageLoop::current()), error_handler_(error_handler) { DCHECK(sync_service_); @@ -45,14 +45,17 @@ PasswordModelAssociator::PasswordModelAssociator( #endif } -PasswordModelAssociator::~PasswordModelAssociator() { - DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); -} +PasswordModelAssociator::~PasswordModelAssociator() {} syncer::SyncError PasswordModelAssociator::AssociateModels( syncer::SyncMergeResult* local_merge_result, syncer::SyncMergeResult* syncer_merge_result) { + syncer::SyncError error; DCHECK(expected_loop_ == base::MessageLoop::current()); + { + base::AutoLock lock(abort_association_pending_lock_); + abort_association_pending_ = false; + } // We must not be holding a transaction when we interact with the password // store, as it can post tasks to the UI thread which can itself be blocked @@ -73,14 +76,10 @@ syncer::SyncError PasswordModelAssociator::AssociateModels( model_type()); } + std::set<std::string> current_passwords; PasswordVector new_passwords; PasswordVector updated_passwords; { - base::AutoLock lock(association_lock_); - if (abort_association_requested_) - return syncer::SyncError(); - - std::set<std::string> current_passwords; syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare()); syncer::ReadNode password_root(&trans); if (password_root.InitByTagLookup(kPasswordTag) != @@ -95,6 +94,9 @@ syncer::SyncError PasswordModelAssociator::AssociateModels( for (std::vector<content::PasswordForm*>::iterator ix = passwords.begin(); ix != passwords.end(); ++ix) { + if (IsAbortPending()) { + return syncer::SyncError(); + } std::string tag = MakeTag(**ix); syncer::ReadNode node(&trans); @@ -174,9 +176,14 @@ syncer::SyncError PasswordModelAssociator::AssociateModels( // We must not be holding a transaction when we interact with the password // store, as it can post tasks to the UI thread which can itself be blocked // on our transaction, resulting in deadlock. (http://crbug.com/70658) - return WriteToPasswordStore(&new_passwords, - &updated_passwords, - NULL); + error = WriteToPasswordStore(&new_passwords, + &updated_passwords, + NULL); + if (error.IsSet()) { + return error; + } + + return error; } bool PasswordModelAssociator::DeleteAllNodes( @@ -231,8 +238,8 @@ bool PasswordModelAssociator::SyncModelHasUserCreatedNodes(bool* has_nodes) { void PasswordModelAssociator::AbortAssociation() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - base::AutoLock lock(association_lock_); - abort_association_requested_ = true; + base::AutoLock lock(abort_association_pending_lock_); + abort_association_pending_ = true; } bool PasswordModelAssociator::CryptoReadyIfNecessary() { @@ -253,6 +260,11 @@ bool PasswordModelAssociator::InitSyncNodeFromChromeId( return false; } +bool PasswordModelAssociator::IsAbortPending() { + base::AutoLock lock(abort_association_pending_lock_); + return abort_association_pending_; +} + int64 PasswordModelAssociator::GetSyncIdFromChromeId( const std::string& password) { PasswordToSyncIdMap::const_iterator iter = id_map_.find(password); diff --git a/chrome/browser/sync/glue/password_model_associator.h b/chrome/browser/sync/glue/password_model_associator.h index 38effd6..c1e2cf3 100644 --- a/chrome/browser/sync/glue/password_model_associator.h +++ b/chrome/browser/sync/glue/password_model_associator.h @@ -118,6 +118,10 @@ class PasswordModelAssociator static void WriteToSyncNode(const content::PasswordForm& password_form, syncer::WriteNode* node); + // Called at various points in model association to determine if the + // user requested an abort. + bool IsAbortPending(); + private: typedef std::map<std::string, int64> PasswordToSyncIdMap; typedef std::map<int64, std::string> SyncIdToPasswordMap; @@ -126,9 +130,11 @@ class PasswordModelAssociator PasswordStore* password_store_; int64 password_node_id_; - // Set true by AbortAssociation. - bool abort_association_requested_; - base::Lock association_lock_; + // Abort association pending flag and lock. If this is set to true + // (via the AbortAssociation method), return from the + // AssociateModels method as soon as possible. + base::Lock abort_association_pending_lock_; + bool abort_association_pending_; base::MessageLoop* expected_loop_; diff --git a/chrome/browser/sync/glue/password_model_worker.cc b/chrome/browser/sync/glue/password_model_worker.cc index beaf5b15..a3a3037 100644 --- a/chrome/browser/sync/glue/password_model_worker.cc +++ b/chrome/browser/sync/glue/password_model_worker.cc @@ -56,7 +56,6 @@ void PasswordModelWorker::CallDoWorkAndSignalTask( void PasswordModelWorker::RegisterForPasswordLoopDestruction() { base::MessageLoop::current()->AddDestructionObserver(this); - SetWorkingLoopToCurrent(); } } // namespace browser_sync diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc index d1f61a1..e1186a5 100644 --- a/chrome/browser/sync/glue/sync_backend_host.cc +++ b/chrome/browser/sync/glue/sync_backend_host.cc @@ -202,11 +202,16 @@ class SyncBackendHost::Core void DoFinishInitialProcessControlTypes(); // The shutdown order is a bit complicated: - // 1) Call DoStopSyncManagerForShutdown() from |frontend_loop_| to request - // sync manager to stop as soon as possible. - // 2) Post DoShutdown() to sync loop to clean up backend state, save - // directory and destroy sync manager. - void DoStopSyncManagerForShutdown(); + // 1) From |sync_thread_|, invoke the syncapi Shutdown call to do + // a final SaveChanges, and close sqlite handles. + // 2) Then, from |frontend_loop_|, halt the sync_thread_ (which is + // a blocking call). This causes syncapi thread-exit handlers + // to run and make use of cached pointers to various components + // owned implicitly by us. + // 3) Destroy this Core. That will delete syncapi components in a + // safe order because the thread that was using them has exited + // (in step 2). + void DoStopSyncManagerForShutdown(const base::Closure& closure); void DoShutdown(bool stopping_sync); void DoDestroySyncManager(); @@ -249,14 +254,15 @@ class SyncBackendHost::Core // Invoked when initialization of syncapi is complete and we can start // our timer. // This must be called from the thread on which SaveChanges is intended to - // be run on; the host's |registrar_->sync_thread()|. + // be run on; the host's |sync_thread_|. void StartSavingChanges(); // Invoked periodically to tell the syncapi to persist its state // by writing to disk. - // This is called from the thread we were created on (which is sync thread), - // using a repeating timer that is kicked off as soon as the SyncManager - // tells us it completed initialization. + // This is called from the thread we were created on (which is the + // SyncBackendHost |sync_thread_|), using a repeating timer that is kicked + // off as soon as the SyncManager tells us it completed + // initialization. void SaveChanges(); // Name used for debugging. @@ -269,7 +275,7 @@ class SyncBackendHost::Core syncer::WeakHandle<SyncBackendHost> host_; // The loop where all the sync backend operations happen. - // Non-NULL only between calls to DoInitialize() and ~Core(). + // Non-NULL only between calls to DoInitialize() and DoShutdown(). base::MessageLoop* sync_loop_; // Our parent's registrar (not owned). Non-NULL only between @@ -288,8 +294,6 @@ class SyncBackendHost::Core // The top-level syncapi entry point. Lives on the sync thread. scoped_ptr<syncer::SyncManager> sync_manager_; - base::WeakPtrFactory<Core> weak_ptr_factory_; - DISALLOW_COPY_AND_ASSIGN(Core); }; @@ -298,10 +302,11 @@ SyncBackendHost::SyncBackendHost( Profile* profile, const base::WeakPtr<SyncPrefs>& sync_prefs) : weak_ptr_factory_(this), + sync_thread_("Chrome_SyncThread"), frontend_loop_(base::MessageLoop::current()), profile_(profile), name_(name), - core_(new Core(name_, profile_->GetPath().Append(kSyncDataFolderName), + core_(new Core(name, profile_->GetPath().Append(kSyncDataFolderName), weak_ptr_factory_.GetWeakPtr())), initialization_state_(NOT_ATTEMPTED), sync_prefs_(sync_prefs), @@ -314,6 +319,7 @@ SyncBackendHost::SyncBackendHost( SyncBackendHost::SyncBackendHost(Profile* profile) : weak_ptr_factory_(this), + sync_thread_("Chrome_SyncThread"), frontend_loop_(base::MessageLoop::current()), profile_(profile), name_("Unknown"), @@ -345,7 +351,6 @@ scoped_ptr<syncer::HttpPostProviderFactory> MakeHttpBridgeFactory( void SyncBackendHost::Initialize( SyncFrontend* frontend, - scoped_ptr<base::Thread> sync_thread, const syncer::WeakHandle<syncer::JsEventHandler>& event_handler, const GURL& sync_service_url, const SyncCredentials& credentials, @@ -354,14 +359,15 @@ void SyncBackendHost::Initialize( syncer::UnrecoverableErrorHandler* unrecoverable_error_handler, syncer::ReportUnrecoverableErrorFunction report_unrecoverable_error_function) { - registrar_.reset(new SyncBackendRegistrar(name_, - profile_, - sync_thread.Pass())); - CHECK(registrar_->sync_thread()); + if (!sync_thread_.Start()) + return; frontend_ = frontend; DCHECK(frontend); + registrar_.reset(new SyncBackendRegistrar(name_, + profile_, + sync_thread_.message_loop())); syncer::ModelSafeRoutingInfo routing_info; std::vector<syncer::ModelSafeWorker*> workers; registrar_->GetModelSafeRoutingInfo(&routing_info); @@ -380,7 +386,7 @@ void SyncBackendHost::Initialize( initialization_state_ = CREATING_SYNC_MANAGER; InitCore(DoInitializeOptions( - registrar_->sync_thread()->message_loop(), + sync_thread_.message_loop(), registrar_.get(), routing_info, workers, @@ -403,10 +409,9 @@ void SyncBackendHost::Initialize( } void SyncBackendHost::UpdateCredentials(const SyncCredentials& credentials) { - DCHECK(registrar_->sync_thread()->IsRunning()); - registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, - base::Bind(&SyncBackendHost::Core::DoUpdateCredentials, - core_.get(), + DCHECK(sync_thread_.IsRunning()); + sync_thread_.message_loop()->PostTask(FROM_HERE, + base::Bind(&SyncBackendHost::Core::DoUpdateCredentials, core_.get(), credentials)); } @@ -416,14 +421,14 @@ void SyncBackendHost::StartSyncingWithServer() { syncer::ModelSafeRoutingInfo routing_info; registrar_->GetModelSafeRoutingInfo(&routing_info); - registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, + sync_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(&SyncBackendHost::Core::DoStartSyncing, core_.get(), routing_info)); } void SyncBackendHost::SetEncryptionPassphrase(const std::string& passphrase, bool is_explicit) { - DCHECK(registrar_->sync_thread()->IsRunning()); + DCHECK(sync_thread_.IsRunning()); if (!IsNigoriEnabled()) { NOTREACHED() << "SetEncryptionPassphrase must never be called when nigori" " is disabled."; @@ -442,9 +447,8 @@ void SyncBackendHost::SetEncryptionPassphrase(const std::string& passphrase, cached_passphrase_type_ == syncer::IMPLICIT_PASSPHRASE); // Post an encryption task on the syncer thread. - registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, - base::Bind(&SyncBackendHost::Core::DoSetEncryptionPassphrase, - core_.get(), + sync_thread_.message_loop()->PostTask(FROM_HERE, + base::Bind(&SyncBackendHost::Core::DoSetEncryptionPassphrase, core_.get(), passphrase, is_explicit)); } @@ -471,9 +475,8 @@ bool SyncBackendHost::SetDecryptionPassphrase(const std::string& passphrase) { return false; // Post a decryption task on the syncer thread. - registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, - base::Bind(&SyncBackendHost::Core::DoSetDecryptionPassphrase, - core_.get(), + sync_thread_.message_loop()->PostTask(FROM_HERE, + base::Bind(&SyncBackendHost::Core::DoSetDecryptionPassphrase, core_.get(), passphrase)); // Since we were able to decrypt the cached pending keys with the passphrase @@ -489,19 +492,22 @@ bool SyncBackendHost::SetDecryptionPassphrase(const std::string& passphrase) { return true; } -void SyncBackendHost::StopSyncManagerForShutdown() { +void SyncBackendHost::StopSyncManagerForShutdown( + const base::Closure& closure) { DCHECK_GT(initialization_state_, NOT_ATTEMPTED); if (initialization_state_ == CREATING_SYNC_MANAGER) { // We post here to implicitly wait for the SyncManager to be created, // if needed. We have to wait, since we need to shutdown immediately, // and we need to tell the SyncManager so it can abort any activity // (net I/O, data application). - DCHECK(registrar_->sync_thread()->IsRunning()); - registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, - base::Bind(&SyncBackendHost::Core::DoStopSyncManagerForShutdown, - core_.get())); + DCHECK(sync_thread_.IsRunning()); + sync_thread_.message_loop()->PostTask(FROM_HERE, + base::Bind( + &SyncBackendHost::Core::DoStopSyncManagerForShutdown, + core_.get(), + closure)); } else { - core_->DoStopSyncManagerForShutdown(); + core_->DoStopSyncManagerForShutdown(closure); } } @@ -514,18 +520,44 @@ void SyncBackendHost::StopSyncingForShutdown() { // Stop listening for and forwarding locally-triggered sync refresh requests. notification_registrar_.RemoveAll(); - DCHECK(registrar_->sync_thread()->IsRunning()); - - registrar_->RequestWorkerStopOnUIThread(); - - StopSyncManagerForShutdown(); + // Thread shutdown should occur in the following order: + // - Sync Thread + // - UI Thread (stops some time after we return from this call). + // + // In order to achieve this, we first shutdown components from the UI thread + // and send signals to abort components that may be busy on the sync thread. + // The callback (OnSyncerShutdownComplete) will happen on the sync thread, + // after which we'll shutdown components on the sync thread, and then be + // able to stop the sync loop. + if (sync_thread_.IsRunning()) { + StopSyncManagerForShutdown( + base::Bind(&SyncBackendRegistrar::OnSyncerShutdownComplete, + base::Unretained(registrar_.get()))); + + // Before joining the sync_thread_, we wait for the UIModelWorker to + // give us the green light that it is not depending on the frontend_loop_ + // to process any more tasks. Stop() blocks until this termination + // condition is true. + base::Time stop_registrar_start_time = base::Time::Now(); + if (registrar_) + registrar_->StopOnUIThread(); + base::TimeDelta stop_registrar_time = base::Time::Now() - + stop_registrar_start_time; + UMA_HISTOGRAM_TIMES("Sync.Shutdown.StopRegistrarTime", + stop_registrar_time); + } else { + // If the sync thread isn't running, then the syncer is effectively + // stopped. Moreover, it implies that we never attempted initialization, + // so the registrar won't need stopping either. + DCHECK_EQ(initialization_state_, NOT_ATTEMPTED); + DCHECK(!registrar_.get()); + } } -scoped_ptr<base::Thread> SyncBackendHost::Shutdown(bool sync_disabled) { +void SyncBackendHost::Shutdown(bool sync_disabled) { // StopSyncingForShutdown() (which nulls out |frontend_|) should be // called first. DCHECK(!frontend_); - DCHECK(registrar_->sync_thread()->IsRunning()); if (invalidation_handler_registered_) { if (sync_disabled) { @@ -538,25 +570,37 @@ scoped_ptr<base::Thread> SyncBackendHost::Shutdown(bool sync_disabled) { } invalidation_handler_registered_ = false; - // Shut down and destroy sync manager. - registrar_->sync_thread()->message_loop()->PostTask( - FROM_HERE, - base::Bind(&SyncBackendHost::Core::DoShutdown, - core_.get(), sync_disabled)); - core_ = NULL; + // TODO(tim): DCHECK(registrar_->StoppedOnUIThread()) would be nice. + if (sync_thread_.IsRunning()) { + sync_thread_.message_loop()->PostTask(FROM_HERE, + base::Bind(&SyncBackendHost::Core::DoShutdown, core_.get(), + sync_disabled)); + } - // Worker cleanup. - SyncBackendRegistrar* detached_registrar = registrar_.release(); - detached_registrar->sync_thread()->message_loop()->PostTask( - FROM_HERE, - base::Bind(&SyncBackendRegistrar::Shutdown, - base::Unretained(detached_registrar))); + // Stop will return once the thread exits, which will be after DoShutdown + // runs. DoShutdown needs to run from sync_thread_ because the sync backend + // requires any thread that opened sqlite handles to relinquish them + // personally. We need to join threads, because otherwise the main Chrome + // thread (ui loop) can exit before DoShutdown finishes, at which point + // virtually anything the sync backend does (or the post-back to + // frontend_loop_ by our Core) will epically fail because the CRT won't be + // initialized. + // Since we are blocking the UI thread here, we need to turn ourselves in + // with the ThreadRestriction police. For sentencing and how we plan to fix + // this, see bug 19757. + base::Time stop_thread_start_time = base::Time::Now(); + { + base::ThreadRestrictions::ScopedAllowIO allow_io; + sync_thread_.Stop(); + } + base::TimeDelta stop_sync_thread_time = base::Time::Now() - + stop_thread_start_time; + UMA_HISTOGRAM_TIMES("Sync.Shutdown.StopSyncThreadTime", + stop_sync_thread_time); + registrar_.reset(); js_backend_.Reset(); - if (sync_disabled) - return detached_registrar->ReleaseSyncThread(); - else - return scoped_ptr<base::Thread>(); + core_ = NULL; // Releases reference to core_. } void SyncBackendHost::ConfigureDataTypes( @@ -673,7 +717,7 @@ void SyncBackendHost::ConfigureDataTypes( } void SyncBackendHost::EnableEncryptEverything() { - registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, + sync_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(&SyncBackendHost::Core::DoEnableEncryptEverything, core_.get())); } @@ -740,7 +784,7 @@ SyncedDeviceTracker* SyncBackendHost::GetSyncedDeviceTracker() const { } void SyncBackendHost::InitCore(const DoInitializeOptions& options) { - registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, + sync_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(&SyncBackendHost::Core::DoInitialize, core_.get(), options)); } @@ -760,7 +804,7 @@ void SyncBackendHost::RequestConfigureSyncer( config_types.to_purge = to_purge; config_types.to_journal = to_journal; config_types.to_unapply = to_unapply; - registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, + sync_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(&SyncBackendHost::Core::DoConfigureSyncer, core_.get(), reason, @@ -822,7 +866,7 @@ void SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop( // Kick off the next step in SyncBackendHost initialization by downloading // any necessary control types. - registrar_->sync_thread()->message_loop()->PostTask( + sync_thread_.message_loop()->PostTask( FROM_HERE, base::Bind(&SyncBackendHost::Core::DoDownloadControlTypes, core_.get(), @@ -838,7 +882,7 @@ void SyncBackendHost::Observe( content::Details<const syncer::ModelTypeSet> state_details(details); const syncer::ModelTypeSet& types = *(state_details.ptr()); - registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, + sync_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(&SyncBackendHost::Core::DoRefreshTypes, core_.get(), types)); } @@ -893,13 +937,13 @@ SyncBackendHost::Core::Core(const std::string& name, sync_data_folder_path_(sync_data_folder_path), host_(backend), sync_loop_(NULL), - registrar_(NULL), - weak_ptr_factory_(this) { + registrar_(NULL) { DCHECK(backend.get()); } SyncBackendHost::Core::~Core() { DCHECK(!sync_manager_.get()); + DCHECK(!sync_loop_); } void SyncBackendHost::Core::OnSyncCycleCompleted( @@ -936,9 +980,9 @@ void SyncBackendHost::Core::DoDownloadControlTypes( syncer::ModelTypeSet(), routing_info, base::Bind(&SyncBackendHost::Core::DoInitialProcessControlTypes, - weak_ptr_factory_.GetWeakPtr()), + this), base::Bind(&SyncBackendHost::Core::OnControlTypesDownloadRetry, - weak_ptr_factory_.GetWeakPtr())); + this)); } void SyncBackendHost::Core::DoRefreshTypes(syncer::ModelTypeSet types) { @@ -976,8 +1020,7 @@ void SyncBackendHost::Core::OnInitializationComplete( // Sync manager initialization is complete, so we can schedule recurring // SaveChanges. sync_loop_->PostTask(FROM_HERE, - base::Bind(&Core::StartSavingChanges, - weak_ptr_factory_.GetWeakPtr())); + base::Bind(&Core::StartSavingChanges, this)); host_.Call(FROM_HERE, &SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop, @@ -1229,7 +1272,7 @@ void SyncBackendHost::Core::DoInitialProcessControlTypes() { sync_manager_->cache_guid())); synced_device_tracker_->InitLocalDeviceInfo( base::Bind(&SyncBackendHost::Core::DoFinishInitialProcessControlTypes, - weak_ptr_factory_.GetWeakPtr())); + this)); } void SyncBackendHost::Core::DoFinishInitialProcessControlTypes() { @@ -1256,9 +1299,13 @@ void SyncBackendHost::Core::DoEnableEncryptEverything() { sync_manager_->GetEncryptionHandler()->EnableEncryptEverything(); } -void SyncBackendHost::Core::DoStopSyncManagerForShutdown() { - if (sync_manager_) - sync_manager_->StopSyncingForShutdown(); +void SyncBackendHost::Core::DoStopSyncManagerForShutdown( + const base::Closure& closure) { + if (sync_manager_) { + sync_manager_->StopSyncingForShutdown(closure); + } else { + sync_loop_->PostTask(FROM_HERE, closure); + } } void SyncBackendHost::Core::DoShutdown(bool sync_disabled) { @@ -1274,8 +1321,9 @@ void SyncBackendHost::Core::DoShutdown(bool sync_disabled) { if (sync_disabled) DeleteSyncDataFolder(); + sync_loop_ = NULL; + host_.Reset(); - weak_ptr_factory_.InvalidateWeakPtrs(); } void SyncBackendHost::Core::DoDestroySyncManager() { @@ -1304,11 +1352,11 @@ void SyncBackendHost::Core::DoConfigureSyncer( config_types.to_unapply, routing_info, base::Bind(&SyncBackendHost::Core::DoFinishConfigureDataTypes, - weak_ptr_factory_.GetWeakPtr(), + this, config_types.to_download, ready_task), base::Bind(&SyncBackendHost::Core::DoRetryConfiguration, - weak_ptr_factory_.GetWeakPtr(), + this, retry_callback)); } @@ -1463,7 +1511,7 @@ void SyncBackendHost::HandleActionableErrorEventOnFrontendLoop( } void SyncBackendHost::OnInvalidatorStateChange(syncer::InvalidatorState state) { - registrar_->sync_thread()->message_loop()->PostTask( + sync_thread_.message_loop()->PostTask( FROM_HERE, base::Bind(&SyncBackendHost::Core::DoOnInvalidatorStateChange, core_.get(), @@ -1481,7 +1529,7 @@ void SyncBackendHost::OnIncomingInvalidation( invalidator_->AcknowledgeInvalidation(it->first, it->second.ack_handle); } - registrar_->sync_thread()->message_loop()->PostTask( + sync_thread_.message_loop()->PostTask( FROM_HERE, base::Bind(&SyncBackendHost::Core::DoOnIncomingInvalidation, core_.get(), @@ -1581,10 +1629,6 @@ void SyncBackendHost::HandleConnectionStatusChangeOnFrontendLoop( frontend_->OnConnectionStatusChange(status); } -base::MessageLoop* SyncBackendHost::GetSyncLoopForTesting() { - return registrar_->sync_thread()->message_loop(); -} - #undef SDVLOG #undef SLOG diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h index 0aa9be7..9498ece 100644 --- a/chrome/browser/sync/glue/sync_backend_host.h +++ b/chrome/browser/sync/glue/sync_backend_host.h @@ -176,7 +176,6 @@ class SyncBackendHost // Note: |unrecoverable_error_handler| may be invoked from any thread. void Initialize( SyncFrontend* frontend, - scoped_ptr<base::Thread> sync_thread, const syncer::WeakHandle<syncer::JsEventHandler>& event_handler, const GURL& service_url, const syncer::SyncCredentials& credentials, @@ -227,10 +226,7 @@ class SyncBackendHost // |sync_disabled| indicates if syncing is being disabled or not. // See the implementation and Core::DoShutdown for details. // Must be called *after* StopSyncingForShutdown. - // If |sync_disabled| is false, return sync thread to caller, i.e. PSS, - // which will be responsible to stop sync thread. Otherwise, backend will - // stop sync thread after backend shutdown finishes. - scoped_ptr<base::Thread> Shutdown(bool sync_disabled); + void Shutdown(bool sync_disabled); // Changes the set of data types that are currently being synced. // The ready_task will be run when configuration is done with the @@ -293,8 +289,6 @@ class SyncBackendHost // Fetches the DeviceInfo tracker. virtual SyncedDeviceTracker* GetSyncedDeviceTracker() const; - base::MessageLoop* GetSyncLoopForTesting(); - protected: // The types and functions below are protected so that test // subclasses can use them. @@ -512,12 +506,15 @@ class SyncBackendHost // Handles stopping the core's SyncManager, accounting for whether // initialization is done yet. - void StopSyncManagerForShutdown(); + void StopSyncManagerForShutdown(const base::Closure& closure); base::WeakPtrFactory<SyncBackendHost> weak_ptr_factory_; content::NotificationRegistrar notification_registrar_; + // A thread where all the sync operations happen. + base::Thread sync_thread_; + // A reference to the MessageLoop used to construct |this|, so we know how // to safely talk back to the SyncFrontend. base::MessageLoop* const frontend_loop_; @@ -527,9 +524,7 @@ class SyncBackendHost // Name used for debugging (set from profile_->GetDebugName()). const std::string name_; - // Our core, which communicates directly to the syncapi. Use refptr instead - // of WeakHandle because |core_| is created on UI loop but released on - // sync loop. + // Our core, which communicates directly to the syncapi. scoped_refptr<Core> core_; InitializationState initialization_state_; diff --git a/chrome/browser/sync/glue/sync_backend_host_unittest.cc b/chrome/browser/sync/glue/sync_backend_host_unittest.cc index 47eddbd..0753ca5 100644 --- a/chrome/browser/sync/glue/sync_backend_host_unittest.cc +++ b/chrome/browser/sync/glue/sync_backend_host_unittest.cc @@ -98,6 +98,7 @@ class FakeSyncManagerFactory : public syncer::SyncManagerFactory { // SyncManagerFactory implementation. Called on the sync thread. virtual scoped_ptr<SyncManager> CreateSyncManager( std::string name) OVERRIDE { + DCHECK(!fake_manager_); fake_manager_ = new FakeSyncManager(initial_sync_ended_types_, progress_marker_types_, configure_fail_types_); @@ -135,8 +136,6 @@ class SyncBackendHostTest : public testing::Test { protected: SyncBackendHostTest() : ui_thread_(BrowserThread::UI, &ui_loop_), - db_thread_(BrowserThread::DB), - file_thread_(BrowserThread::FILE), io_thread_(BrowserThread::IO), fake_manager_(NULL) {} @@ -144,8 +143,6 @@ class SyncBackendHostTest : public testing::Test { virtual void SetUp() OVERRIDE { io_thread_.StartIOThread(); - db_thread_.Start(); - file_thread_.Start(); profile_.reset(new TestingProfile()); profile_->CreateRequestContext(); sync_prefs_.reset(new SyncPrefs(profile_->GetPrefs())); @@ -183,8 +180,6 @@ class SyncBackendHostTest : public testing::Test { // posting on the IO thread). ui_loop_.RunUntilIdle(); io_thread_.Stop(); - file_thread_.Stop(); - db_thread_.Stop(); // Pump any messages posted by the IO thread. ui_loop_.RunUntilIdle(); } @@ -194,7 +189,6 @@ class SyncBackendHostTest : public testing::Test { EXPECT_CALL(mock_frontend_, OnBackendInitialized(_, _, expect_success)). WillOnce(InvokeWithoutArgs(QuitMessageLoop)); backend_->Initialize(&mock_frontend_, - scoped_ptr<base::Thread>(), syncer::WeakHandle<syncer::JsEventHandler>(), GURL(std::string()), credentials_, @@ -258,8 +252,6 @@ class SyncBackendHostTest : public testing::Test { base::MessageLoop ui_loop_; content::TestBrowserThread ui_thread_; - content::TestBrowserThread db_thread_; - content::TestBrowserThread file_thread_; content::TestBrowserThread io_thread_; StrictMock<MockSyncFrontend> mock_frontend_; syncer::SyncCredentials credentials_; diff --git a/chrome/browser/sync/glue/sync_backend_registrar.cc b/chrome/browser/sync/glue/sync_backend_registrar.cc index 4f11589..69898b1 100644 --- a/chrome/browser/sync/glue/sync_backend_registrar.cc +++ b/chrome/browser/sync/glue/sync_backend_registrar.cc @@ -20,7 +20,6 @@ #include "chrome/browser/sync/glue/ui_model_worker.h" #include "content/public/browser/browser_thread.h" #include "sync/internal_api/public/engine/passive_model_worker.h" -#include "sync/internal_api/public/user_share.h" using content::BrowserThread; @@ -55,43 +54,27 @@ bool IsOnThreadForGroup(syncer::ModelType type, syncer::ModelSafeGroup group) { } // namespace SyncBackendRegistrar::SyncBackendRegistrar( - const std::string& name, - Profile* profile, - scoped_ptr<base::Thread> sync_thread) : + const std::string& name, Profile* profile, + base::MessageLoop* sync_loop) : name_(name), - profile_(profile) { + profile_(profile), + sync_loop_(sync_loop), + ui_worker_(new UIModelWorker(this)), + stopped_on_ui_thread_(false) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); CHECK(profile_); - - sync_thread_ = sync_thread.Pass(); - if (!sync_thread_) { - sync_thread_.reset(new base::Thread("Chrome_SyncThread")); - CHECK(sync_thread_->Start()); - } - + DCHECK(sync_loop_); workers_[syncer::GROUP_DB] = new DatabaseModelWorker(this); - workers_[syncer::GROUP_DB]->RegisterForLoopDestruction(); - workers_[syncer::GROUP_FILE] = new FileModelWorker(this); - workers_[syncer::GROUP_FILE]->RegisterForLoopDestruction(); - - workers_[syncer::GROUP_UI] = new UIModelWorker(this); - workers_[syncer::GROUP_UI]->RegisterForLoopDestruction(); - - // GROUP_PASSIVE worker does work on sync_loop_. But sync_loop_ is not - // stopped until all workers have stopped. To break the cycle, use UI loop - // instead. - workers_[syncer::GROUP_PASSIVE] = - new syncer::PassiveModelWorker(sync_thread_->message_loop(), this); - workers_[syncer::GROUP_PASSIVE]->RegisterForLoopDestruction(); + workers_[syncer::GROUP_UI] = ui_worker_; + workers_[syncer::GROUP_PASSIVE] = new syncer::PassiveModelWorker(sync_loop_, + this); HistoryService* history_service = HistoryServiceFactory::GetForProfile(profile, Profile::IMPLICIT_ACCESS); if (history_service) { workers_[syncer::GROUP_HISTORY] = new HistoryModelWorker(history_service->AsWeakPtr(), this); - workers_[syncer::GROUP_HISTORY]->RegisterForLoopDestruction(); - } scoped_refptr<PasswordStore> password_store = @@ -99,17 +82,20 @@ SyncBackendRegistrar::SyncBackendRegistrar( if (password_store.get()) { workers_[syncer::GROUP_PASSWORD] = new PasswordModelWorker(password_store, this); - workers_[syncer::GROUP_PASSWORD]->RegisterForLoopDestruction(); } } +SyncBackendRegistrar::~SyncBackendRegistrar() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(stopped_on_ui_thread_); +} + void SyncBackendRegistrar::SetInitialTypes(syncer::ModelTypeSet initial_types) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); base::AutoLock lock(lock_); - - // This function should be called only once, shortly after construction. The - // routing info at that point is expected to be empty. + // This function should be called only once, shortly after construction. The + // routing info at that point is expected to be emtpy. DCHECK(routing_info_.empty()); // Set our initial state to reflect the current status of the sync directory. @@ -190,13 +176,16 @@ syncer::ModelTypeSet SyncBackendRegistrar::GetLastConfiguredTypes() const { return last_configured_types_; } -void SyncBackendRegistrar::RequestWorkerStopOnUIThread() { +void SyncBackendRegistrar::StopOnUIThread() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - base::AutoLock lock(lock_); - for (WorkerMap::const_iterator it = workers_.begin(); - it != workers_.end(); ++it) { - it->second->RequestStop(); - } + DCHECK(!stopped_on_ui_thread_); + ui_worker_->Stop(); + stopped_on_ui_thread_ = true; +} + +void SyncBackendRegistrar::OnSyncerShutdownComplete() { + DCHECK_EQ(base::MessageLoop::current(), sync_loop_); + ui_worker_->OnSyncerShutdownComplete(); } void SyncBackendRegistrar::ActivateDataType( @@ -204,8 +193,6 @@ void SyncBackendRegistrar::ActivateDataType( syncer::ModelSafeGroup group, ChangeProcessor* change_processor, syncer::UserShare* user_share) { - DVLOG(1) << "Activate: " << syncer::ModelTypeToString(type); - CHECK(IsOnThreadForGroup(type, group)); base::AutoLock lock(lock_); // Ensure that the given data type is in the PASSIVE group. @@ -226,8 +213,6 @@ void SyncBackendRegistrar::ActivateDataType( } void SyncBackendRegistrar::DeactivateDataType(syncer::ModelType type) { - DVLOG(1) << "Deactivate: " << syncer::ModelTypeToString(type); - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) || IsControlType(type)); base::AutoLock lock(lock_); @@ -297,8 +282,8 @@ ChangeProcessor* SyncBackendRegistrar::GetProcessor( ChangeProcessor* SyncBackendRegistrar::GetProcessorUnsafe( syncer::ModelType type) const { lock_.AssertAcquired(); - std::map<syncer::ModelType, ChangeProcessor*>::const_iterator - it = processors_.find(type); + std::map<syncer::ModelType, ChangeProcessor*>::const_iterator it = + processors_.find(type); // Until model association happens for a datatype, it will not // appear in the processors list. During this time, it is OK to @@ -319,58 +304,8 @@ bool SyncBackendRegistrar::IsCurrentThreadSafeForModel( GetGroupForModelType(model_type, routing_info_)); } -SyncBackendRegistrar::~SyncBackendRegistrar() { - DCHECK(workers_.empty()); -} - void SyncBackendRegistrar::OnWorkerLoopDestroyed(syncer::ModelSafeGroup group) { - RemoveWorker(group); -} - -void SyncBackendRegistrar::OnWorkerUnregistrationDone( - syncer::ModelSafeGroup group) { - RemoveWorker(group); -} - -void SyncBackendRegistrar::RemoveWorker(syncer::ModelSafeGroup group) { - bool last_worker = false; - { - base::AutoLock al(lock_); - WorkerMap::iterator it = workers_.find(group); - CHECK(it != workers_.end()); - stopped_workers_.push_back(it->second); - workers_.erase(it); - last_worker = workers_.empty(); - } - - if (last_worker) { - // Self-destruction after last worker. - DVLOG(1) << "Destroy registrar on loop of " - << ModelSafeGroupToString(group); - delete this; - } -} - -scoped_ptr<base::Thread> SyncBackendRegistrar::ReleaseSyncThread() { - return sync_thread_.Pass(); -} - -void SyncBackendRegistrar::Shutdown() { - // All data types should have been deactivated by now. - DCHECK(processors_.empty()); - - // Unregister worker from observing loop destruction. - base::AutoLock al(lock_); - for (WorkerMap::iterator it = workers_.begin(); - it != workers_.end(); ++it) { - it->second->UnregisterForLoopDestruction( - base::Bind(&SyncBackendRegistrar::OnWorkerUnregistrationDone, - base::Unretained(this))); - } -} - -base::Thread* SyncBackendRegistrar::sync_thread() { - return sync_thread_.get(); + // Do nothing for now. } } // namespace browser_sync diff --git a/chrome/browser/sync/glue/sync_backend_registrar.h b/chrome/browser/sync/glue/sync_backend_registrar.h index 91d1e9a..f7ea1fb 100644 --- a/chrome/browser/sync/glue/sync_backend_registrar.h +++ b/chrome/browser/sync/glue/sync_backend_registrar.h @@ -12,7 +12,6 @@ #include "base/compiler_specific.h" #include "base/memory/ref_counted.h" #include "base/synchronization/lock.h" -#include "base/threading/thread.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/engine/model_safe_worker.h" #include "sync/internal_api/public/sync_manager.h" @@ -42,19 +41,19 @@ class SyncBackendRegistrar : public syncer::SyncManager::ChangeDelegate, // |sync_loop|. Must be created on the UI thread. SyncBackendRegistrar(const std::string& name, Profile* profile, - scoped_ptr<base::Thread> sync_thread); + base::MessageLoop* sync_loop); // SyncBackendRegistrar must be destroyed as follows: // - // 1) On the UI thread, call RequestWorkerStopOnUIThread(). - // 2) UI posts task to shut down syncer on sync thread. - // 3) If sync is disabled, call ReleaseSyncThread() on the UI thread. - // 3) UI posts SyncBackendRegistrar::ShutDown() on sync thread to - // unregister workers from observing destruction of their working loops. - // 4) Workers notify registrar when unregistration finishes or working - // loops are destroyed. Registrar destroys itself on last worker - // notification. Sync thread will be stopped if ownership was not - // released. + // 1) On the sync thread, call OnSyncerShutdownComplete() after + // the syncer is shutdown. + // 2) Meanwhile, on the UI thread, call StopOnUIThread(), which + // blocks until OnSyncerShutdownComplete() is called. + // 3) Destroy the SyncBackendRegistrar. + // + // This is to handle the complicated shutdown requirements of the + // UIModelWorker (since the UI thread is both the main thread and a + // thread which the syncer pushes changes to). virtual ~SyncBackendRegistrar(); // Informs the SyncBackendRegistrar of the currently enabled set of types. @@ -81,7 +80,10 @@ class SyncBackendRegistrar : public syncer::SyncManager::ChangeDelegate, syncer::ModelTypeSet GetLastConfiguredTypes() const; // Must be called from the UI thread. (See destructor comment.) - void RequestWorkerStopOnUIThread(); + void StopOnUIThread(); + + // Must be called from the sync thread. (See destructor comment.) + void OnSyncerShutdownComplete(); // Activates the given data type (which should belong to the given // group) and starts the given change processor. Must be called @@ -115,25 +117,9 @@ class SyncBackendRegistrar : public syncer::SyncManager::ChangeDelegate, // syncer::WorkerLoopDestructionObserver implementation. virtual void OnWorkerLoopDestroyed(syncer::ModelSafeGroup group) OVERRIDE; - // Release ownership of |sync_thread_|. Called when sync is disabled. - scoped_ptr<base::Thread> ReleaseSyncThread(); - - // Unregister workers from loop destruction observation. - void Shutdown(); - - base::Thread* sync_thread(); - private: typedef std::map<syncer::ModelSafeGroup, - scoped_refptr<syncer::ModelSafeWorker> > WorkerMap; - typedef std::map<syncer::ModelType, ChangeProcessor*> - ProcessorMap; - - // Callback after workers unregister from observing destruction of their - // working loops. - void OnWorkerUnregistrationDone(syncer::ModelSafeGroup group); - - void RemoveWorker(syncer::ModelSafeGroup group); + scoped_refptr<syncer::ModelSafeWorker> > WorkerMap; // Returns the change processor for the given model, or NULL if none // exists. Must be called from |group|'s native thread. @@ -154,6 +140,12 @@ class SyncBackendRegistrar : public syncer::SyncManager::ChangeDelegate, Profile* const profile_; + base::MessageLoop* const sync_loop_; + + const scoped_refptr<UIModelWorker> ui_worker_; + + bool stopped_on_ui_thread_; + // Protects all variables below. mutable base::Lock lock_; @@ -171,19 +163,12 @@ class SyncBackendRegistrar : public syncer::SyncManager::ChangeDelegate, syncer::ModelSafeRoutingInfo routing_info_; // The change processors that handle the different data types. - ProcessorMap processors_; + std::map<syncer::ModelType, ChangeProcessor*> processors_; // The types that were enabled as of the last configuration. Updated on each // call to ConfigureDataTypes as well as SetInitialTypes. syncer::ModelTypeSet last_configured_types_; - // Declare |sync_thread_| after |lock_| so it's destroyed first because task - // on |sync_thread_|, e.g. Shutdown(), depends on |lock_|. - scoped_ptr<base::Thread> sync_thread_; - - // Parks stopped workers because they may still be referenced by syncer. - std::vector<scoped_refptr<syncer::ModelSafeWorker> > stopped_workers_; - DISALLOW_COPY_AND_ASSIGN(SyncBackendRegistrar); }; diff --git a/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc b/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc index eed132c..979f9a5 100644 --- a/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc +++ b/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc @@ -34,59 +34,18 @@ using syncer::ModelTypeSet; using syncer::ModelType; using syncer::ModelTypeFromInt; -void TriggerChanges(SyncBackendRegistrar* registrar, ModelType type) { - registrar->OnChangesApplied(type, 0, NULL, - syncer::ImmutableChangeRecordList()); - registrar->OnChangesComplete(type); -} - class SyncBackendRegistrarTest : public testing::Test { - public: - void TestNonUIDataTypeActivationAsync(ChangeProcessor* processor, - base::WaitableEvent* done) { - registrar_->ActivateDataType(AUTOFILL, - syncer::GROUP_DB, - processor, - test_user_share_.user_share()); - syncer::ModelSafeRoutingInfo expected_routing_info; - expected_routing_info[AUTOFILL] = syncer::GROUP_DB; - ExpectRoutingInfo(registrar_.get(), expected_routing_info); - ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet(AUTOFILL)); - TriggerChanges(registrar_.get(), AUTOFILL); - done->Signal(); - } - protected: - SyncBackendRegistrarTest() : - sync_thread_(NULL), - ui_thread_(BrowserThread::UI, &ui_loop_), - db_thread_(BrowserThread::DB), - file_thread_(BrowserThread::FILE), - io_thread_(BrowserThread::IO) {} + SyncBackendRegistrarTest() : ui_thread_(BrowserThread::UI, &loop_) {} virtual ~SyncBackendRegistrarTest() {} virtual void SetUp() { - db_thread_.Start(); - file_thread_.Start(); - io_thread_.Start(); test_user_share_.SetUp(); - registrar_.reset(new SyncBackendRegistrar("test", &profile_, - scoped_ptr<base::Thread>())); - sync_thread_ = registrar_->sync_thread(); } virtual void TearDown() { - registrar_->RequestWorkerStopOnUIThread(); test_user_share_.TearDown(); - sync_thread_->message_loop()->PostTask( - FROM_HERE, - base::Bind(&SyncBackendRegistrar::Shutdown, - base::Unretained(registrar_.release()))); - sync_thread_->message_loop()->RunUntilIdle(); - io_thread_.Stop(); - file_thread_.Stop(); - db_thread_.Stop(); } void ExpectRoutingInfo( @@ -102,41 +61,42 @@ class SyncBackendRegistrarTest : public testing::Test { for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) { ModelType model_type = ModelTypeFromInt(i); EXPECT_EQ(types.Has(model_type), - registrar_->IsTypeActivatedForTest(model_type)); + registrar.IsTypeActivatedForTest(model_type)); } } - base::MessageLoop ui_loop_; + base::MessageLoop loop_; syncer::TestUserShare test_user_share_; - TestingProfile profile_; - scoped_ptr<SyncBackendRegistrar> registrar_; - base::Thread* sync_thread_; + private: content::TestBrowserThread ui_thread_; - content::TestBrowserThread db_thread_; - content::TestBrowserThread file_thread_; - content::TestBrowserThread io_thread_; }; TEST_F(SyncBackendRegistrarTest, ConstructorEmpty) { - registrar_->SetInitialTypes(ModelTypeSet()); - EXPECT_FALSE(registrar_->IsNigoriEnabled()); + TestingProfile profile; + SyncBackendRegistrar registrar("test", &profile, &loop_); + registrar.SetInitialTypes(ModelTypeSet()); + EXPECT_FALSE(registrar.IsNigoriEnabled()); { std::vector<syncer::ModelSafeWorker*> workers; - registrar_->GetWorkers(&workers); + registrar.GetWorkers(&workers); EXPECT_EQ(4u, workers.size()); } - ExpectRoutingInfo(registrar_.get(), syncer::ModelSafeRoutingInfo()); - ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet()); + ExpectRoutingInfo(®istrar, syncer::ModelSafeRoutingInfo()); + ExpectHasProcessorsForTypes(registrar, ModelTypeSet()); + registrar.OnSyncerShutdownComplete(); + registrar.StopOnUIThread(); } TEST_F(SyncBackendRegistrarTest, ConstructorNonEmpty) { + TestingProfile profile; const ModelTypeSet initial_types(BOOKMARKS, NIGORI, PASSWORDS); - registrar_->SetInitialTypes(initial_types); - EXPECT_TRUE(registrar_->IsNigoriEnabled()); + SyncBackendRegistrar registrar("test", &profile, &loop_); + registrar.SetInitialTypes(initial_types); + EXPECT_TRUE(registrar.IsNigoriEnabled()); { std::vector<syncer::ModelSafeWorker*> workers; - registrar_->GetWorkers(&workers); + registrar.GetWorkers(&workers); EXPECT_EQ(4u, workers.size()); } { @@ -144,56 +104,71 @@ TEST_F(SyncBackendRegistrarTest, ConstructorNonEmpty) { expected_routing_info[BOOKMARKS] = syncer::GROUP_PASSIVE; expected_routing_info[NIGORI] = syncer::GROUP_PASSIVE; // Passwords dropped because of no password store. - ExpectRoutingInfo(registrar_.get(), expected_routing_info); + ExpectRoutingInfo(®istrar, expected_routing_info); } - ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet()); + ExpectHasProcessorsForTypes(registrar, ModelTypeSet()); + registrar.OnSyncerShutdownComplete(); + registrar.StopOnUIThread(); } TEST_F(SyncBackendRegistrarTest, ConfigureDataTypes) { - registrar_->SetInitialTypes(ModelTypeSet()); + TestingProfile profile; + SyncBackendRegistrar registrar("test", &profile, &loop_); + registrar.SetInitialTypes(ModelTypeSet()); // Add. const ModelTypeSet types1(BOOKMARKS, NIGORI, AUTOFILL); EXPECT_TRUE( - registrar_->ConfigureDataTypes(types1, ModelTypeSet()).Equals(types1)); + registrar.ConfigureDataTypes(types1, ModelTypeSet()).Equals(types1)); { syncer::ModelSafeRoutingInfo expected_routing_info; expected_routing_info[BOOKMARKS] = syncer::GROUP_PASSIVE; expected_routing_info[NIGORI] = syncer::GROUP_PASSIVE; expected_routing_info[AUTOFILL] = syncer::GROUP_PASSIVE; - ExpectRoutingInfo(registrar_.get(), expected_routing_info); + ExpectRoutingInfo(®istrar, expected_routing_info); } - ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet()); - EXPECT_TRUE(types1.Equals(registrar_->GetLastConfiguredTypes())); + ExpectHasProcessorsForTypes(registrar, ModelTypeSet()); + EXPECT_TRUE(types1.Equals(registrar.GetLastConfiguredTypes())); // Add and remove. const ModelTypeSet types2(PREFERENCES, THEMES); - EXPECT_TRUE(registrar_->ConfigureDataTypes(types2, types1).Equals(types2)); + EXPECT_TRUE(registrar.ConfigureDataTypes(types2, types1).Equals(types2)); { syncer::ModelSafeRoutingInfo expected_routing_info; expected_routing_info[PREFERENCES] = syncer::GROUP_PASSIVE; expected_routing_info[THEMES] = syncer::GROUP_PASSIVE; - ExpectRoutingInfo(registrar_.get(), expected_routing_info); + ExpectRoutingInfo(®istrar, expected_routing_info); } - ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet()); - EXPECT_TRUE(types2.Equals(registrar_->GetLastConfiguredTypes())); + ExpectHasProcessorsForTypes(registrar, ModelTypeSet()); + EXPECT_TRUE(types2.Equals(registrar.GetLastConfiguredTypes())); // Remove. - EXPECT_TRUE(registrar_->ConfigureDataTypes(ModelTypeSet(), types2).Empty()); - ExpectRoutingInfo(registrar_.get(), syncer::ModelSafeRoutingInfo()); - ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet()); - EXPECT_TRUE(ModelTypeSet().Equals(registrar_->GetLastConfiguredTypes())); + EXPECT_TRUE(registrar.ConfigureDataTypes(ModelTypeSet(), types2).Empty()); + ExpectRoutingInfo(®istrar, syncer::ModelSafeRoutingInfo()); + ExpectHasProcessorsForTypes(registrar, ModelTypeSet()); + EXPECT_TRUE(ModelTypeSet().Equals(registrar.GetLastConfiguredTypes())); + + registrar.OnSyncerShutdownComplete(); + registrar.StopOnUIThread(); +} + +void TriggerChanges(SyncBackendRegistrar* registrar, ModelType type) { + registrar->OnChangesApplied(type, 0, NULL, + syncer::ImmutableChangeRecordList()); + registrar->OnChangesComplete(type); } TEST_F(SyncBackendRegistrarTest, ActivateDeactivateUIDataType) { InSequence in_sequence; - registrar_->SetInitialTypes(ModelTypeSet()); + TestingProfile profile; + SyncBackendRegistrar registrar("test", &profile, &loop_); + registrar.SetInitialTypes(ModelTypeSet()); // Should do nothing. - TriggerChanges(registrar_.get(), BOOKMARKS); + TriggerChanges(®istrar, BOOKMARKS); StrictMock<ChangeProcessorMock> change_processor_mock; - EXPECT_CALL(change_processor_mock, StartImpl(&profile_)); + EXPECT_CALL(change_processor_mock, StartImpl(&profile)); EXPECT_CALL(change_processor_mock, IsRunning()) .WillRepeatedly(Return(true)); EXPECT_CALL(change_processor_mock, ApplyChangesFromSyncModel(NULL, _, _)); @@ -205,40 +180,42 @@ TEST_F(SyncBackendRegistrarTest, ActivateDeactivateUIDataType) { const ModelTypeSet types(BOOKMARKS); EXPECT_TRUE( - registrar_->ConfigureDataTypes(types, ModelTypeSet()).Equals(types)); - registrar_->ActivateDataType(BOOKMARKS, syncer::GROUP_UI, + registrar.ConfigureDataTypes(types, ModelTypeSet()).Equals(types)); + registrar.ActivateDataType(BOOKMARKS, syncer::GROUP_UI, &change_processor_mock, test_user_share_.user_share()); { syncer::ModelSafeRoutingInfo expected_routing_info; expected_routing_info[BOOKMARKS] = syncer::GROUP_UI; - ExpectRoutingInfo(registrar_.get(), expected_routing_info); + ExpectRoutingInfo(®istrar, expected_routing_info); } - ExpectHasProcessorsForTypes(*registrar_, types); + ExpectHasProcessorsForTypes(registrar, types); - TriggerChanges(registrar_.get(), BOOKMARKS); + TriggerChanges(®istrar, BOOKMARKS); - registrar_->DeactivateDataType(BOOKMARKS); - ExpectRoutingInfo(registrar_.get(), syncer::ModelSafeRoutingInfo()); - ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet()); + registrar.DeactivateDataType(BOOKMARKS); + ExpectRoutingInfo(®istrar, syncer::ModelSafeRoutingInfo()); + ExpectHasProcessorsForTypes(registrar, ModelTypeSet()); // Should do nothing. - TriggerChanges(registrar_.get(), BOOKMARKS); -} + TriggerChanges(®istrar, BOOKMARKS); -void ActiviateDoneOnDb(base::WaitableEvent* done) { - done->Signal(); + registrar.OnSyncerShutdownComplete(); + registrar.StopOnUIThread(); } TEST_F(SyncBackendRegistrarTest, ActivateDeactivateNonUIDataType) { + content::TestBrowserThread db_thread(BrowserThread::DB, &loop_); InSequence in_sequence; - registrar_->SetInitialTypes(ModelTypeSet()); + TestingProfile profile; + SyncBackendRegistrar registrar("test", &profile, &loop_); + registrar.SetInitialTypes(ModelTypeSet()); // Should do nothing. - TriggerChanges(registrar_.get(), AUTOFILL); + TriggerChanges(®istrar, AUTOFILL); StrictMock<ChangeProcessorMock> change_processor_mock; - EXPECT_CALL(change_processor_mock, StartImpl(&profile_)); + EXPECT_CALL(change_processor_mock, StartImpl(&profile)); EXPECT_CALL(change_processor_mock, IsRunning()) .WillRepeatedly(Return(true)); EXPECT_CALL(change_processor_mock, ApplyChangesFromSyncModel(NULL, _, _)); @@ -250,24 +227,28 @@ TEST_F(SyncBackendRegistrarTest, ActivateDeactivateNonUIDataType) { const ModelTypeSet types(AUTOFILL); EXPECT_TRUE( - registrar_->ConfigureDataTypes(types, ModelTypeSet()).Equals(types)); - - base::WaitableEvent done(false, false); - BrowserThread::PostTask( - BrowserThread::DB, - FROM_HERE, - base::Bind(&SyncBackendRegistrarTest::TestNonUIDataTypeActivationAsync, - base::Unretained(this), - &change_processor_mock, - &done)); - done.Wait(); - - registrar_->DeactivateDataType(AUTOFILL); - ExpectRoutingInfo(registrar_.get(), syncer::ModelSafeRoutingInfo()); - ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet()); + registrar.ConfigureDataTypes(types, ModelTypeSet()).Equals(types)); + registrar.ActivateDataType(AUTOFILL, syncer::GROUP_DB, + &change_processor_mock, + test_user_share_.user_share()); + { + syncer::ModelSafeRoutingInfo expected_routing_info; + expected_routing_info[AUTOFILL] = syncer::GROUP_DB; + ExpectRoutingInfo(®istrar, expected_routing_info); + } + ExpectHasProcessorsForTypes(registrar, types); + + TriggerChanges(®istrar, AUTOFILL); + + registrar.DeactivateDataType(AUTOFILL); + ExpectRoutingInfo(®istrar, syncer::ModelSafeRoutingInfo()); + ExpectHasProcessorsForTypes(registrar, ModelTypeSet()); // Should do nothing. - TriggerChanges(registrar_.get(), AUTOFILL); + TriggerChanges(®istrar, AUTOFILL); + + registrar.OnSyncerShutdownComplete(); + registrar.StopOnUIThread(); } } // namespace diff --git a/chrome/browser/sync/glue/typed_url_change_processor.cc b/chrome/browser/sync/glue/typed_url_change_processor.cc index ca97e32..1a86800 100644 --- a/chrome/browser/sync/glue/typed_url_change_processor.cc +++ b/chrome/browser/sync/glue/typed_url_change_processor.cc @@ -43,8 +43,7 @@ TypedUrlChangeProcessor::TypedUrlChangeProcessor( profile_(profile), model_associator_(model_associator), history_backend_(history_backend), - backend_loop_(base::MessageLoop::current()), - disconnected_(false) { + expected_loop_(base::MessageLoop::current()) { DCHECK(model_associator); DCHECK(history_backend); DCHECK(error_handler); @@ -56,18 +55,14 @@ TypedUrlChangeProcessor::TypedUrlChangeProcessor( } TypedUrlChangeProcessor::~TypedUrlChangeProcessor() { - DCHECK(backend_loop_ == base::MessageLoop::current()); + DCHECK(expected_loop_ == base::MessageLoop::current()); } void TypedUrlChangeProcessor::Observe( int type, const content::NotificationSource& source, const content::NotificationDetails& details) { - DCHECK(backend_loop_ == base::MessageLoop::current()); - - base::AutoLock al(disconnect_lock_); - if (disconnected_) - return; + DCHECK(expected_loop_ == base::MessageLoop::current()); DVLOG(1) << "Observed typed_url change."; if (type == chrome::NOTIFICATION_HISTORY_URLS_MODIFIED) { @@ -244,11 +239,7 @@ void TypedUrlChangeProcessor::ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, int64 model_version, const syncer::ImmutableChangeRecordList& changes) { - DCHECK(backend_loop_ == base::MessageLoop::current()); - - base::AutoLock al(disconnect_lock_); - if (disconnected_) - return; + DCHECK(expected_loop_ == base::MessageLoop::current()); syncer::ReadNode typed_url_root(trans); if (typed_url_root.InitByTagLookup(kTypedUrlTag) != @@ -304,11 +295,7 @@ void TypedUrlChangeProcessor::ApplyChangesFromSyncModel( } void TypedUrlChangeProcessor::CommitChangesFromSyncModel() { - DCHECK(backend_loop_ == base::MessageLoop::current()); - - base::AutoLock al(disconnect_lock_); - if (disconnected_) - return; + DCHECK(expected_loop_ == base::MessageLoop::current()); // Make sure we stop listening for changes while we're modifying the backend, // so we don't try to re-apply these changes to the sync DB. @@ -330,23 +317,14 @@ void TypedUrlChangeProcessor::CommitChangesFromSyncModel() { model_associator_->GetErrorPercentage()); } -void TypedUrlChangeProcessor::Disconnect() { - base::AutoLock al(disconnect_lock_); - disconnected_ = true; -} - void TypedUrlChangeProcessor::StartImpl(Profile* profile) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(expected_loop_ == base::MessageLoop::current()); DCHECK_EQ(profile, profile_); - DCHECK(history_backend_); - DCHECK(backend_loop_); - backend_loop_->PostTask(FROM_HERE, - base::Bind(&TypedUrlChangeProcessor::StartObserving, - base::Unretained(this))); + StartObserving(); } void TypedUrlChangeProcessor::StartObserving() { - DCHECK(backend_loop_ == base::MessageLoop::current()); + DCHECK(expected_loop_ == base::MessageLoop::current()); DCHECK(profile_); notification_registrar_.Add( this, chrome::NOTIFICATION_HISTORY_URLS_MODIFIED, @@ -360,7 +338,7 @@ void TypedUrlChangeProcessor::StartObserving() { } void TypedUrlChangeProcessor::StopObserving() { - DCHECK(backend_loop_ == base::MessageLoop::current()); + DCHECK(expected_loop_ == base::MessageLoop::current()); DCHECK(profile_); notification_registrar_.Remove( this, chrome::NOTIFICATION_HISTORY_URLS_MODIFIED, diff --git a/chrome/browser/sync/glue/typed_url_change_processor.h b/chrome/browser/sync/glue/typed_url_change_processor.h index a5e447e..823c45c 100644 --- a/chrome/browser/sync/glue/typed_url_change_processor.h +++ b/chrome/browser/sync/glue/typed_url_change_processor.h @@ -67,9 +67,6 @@ class TypedUrlChangeProcessor : public ChangeProcessor, // jank. virtual void CommitChangesFromSyncModel() OVERRIDE; - // Stop processing changes and wait for being destroyed. - void Disconnect(); - protected: virtual void StartImpl(Profile* profile) OVERRIDE; @@ -104,10 +101,11 @@ class TypedUrlChangeProcessor : public ChangeProcessor, // WebDataService which is kept alive by our data type controller // holding a reference. history::HistoryBackend* history_backend_; - base::MessageLoop* backend_loop_; content::NotificationRegistrar notification_registrar_; + base::MessageLoop* expected_loop_; + scoped_ptr<content::NotificationService> notification_service_; // The set of pending changes that will be written out on the next @@ -118,9 +116,6 @@ class TypedUrlChangeProcessor : public ChangeProcessor, TypedUrlModelAssociator::TypedUrlVisitVector pending_new_visits_; history::VisitVector pending_deleted_visits_; - bool disconnected_; - base::Lock disconnect_lock_; - DISALLOW_COPY_AND_ASSIGN(TypedUrlChangeProcessor); }; diff --git a/chrome/browser/sync/glue/typed_url_data_type_controller.cc b/chrome/browser/sync/glue/typed_url_data_type_controller.cc index fe7d137..6a5d4a7 100644 --- a/chrome/browser/sync/glue/typed_url_data_type_controller.cc +++ b/chrome/browser/sync/glue/typed_url_data_type_controller.cc @@ -13,7 +13,6 @@ #include "chrome/browser/history/history_service.h" #include "chrome/browser/history/history_service_factory.h" #include "chrome/browser/profiles/profile.h" -#include "chrome/browser/sync/glue/typed_url_change_processor.h" #include "chrome/browser/sync/profile_sync_components_factory.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/common/pref_names.h" @@ -126,20 +125,23 @@ bool TypedUrlDataTypeController::PostTaskOnBackendThread( } } -ProfileSyncComponentsFactory::SyncComponents -TypedUrlDataTypeController::CreateSyncComponents() { +void TypedUrlDataTypeController::CreateSyncComponents() { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK_EQ(state(), ASSOCIATING); DCHECK(backend_); - return profile_sync_factory()->CreateTypedUrlSyncComponents( - profile_sync_service(), - backend_, - this); + ProfileSyncComponentsFactory::SyncComponents sync_components = + profile_sync_factory()->CreateTypedUrlSyncComponents( + profile_sync_service(), + backend_, + this); + set_model_associator(sync_components.model_associator); + set_change_processor(sync_components.change_processor); } -void TypedUrlDataTypeController::DisconnectProcessor( - ChangeProcessor* processor) { - static_cast<TypedUrlChangeProcessor*>(processor)->Disconnect(); +void TypedUrlDataTypeController::StopModels() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(state() == STOPPING || state() == NOT_RUNNING || state() == DISABLED); + DVLOG(1) << "TypedUrlDataTypeController::StopModels(): State = " << state(); } TypedUrlDataTypeController::~TypedUrlDataTypeController() {} diff --git a/chrome/browser/sync/glue/typed_url_data_type_controller.h b/chrome/browser/sync/glue/typed_url_data_type_controller.h index 95b087b..9afd3e0 100644 --- a/chrome/browser/sync/glue/typed_url_data_type_controller.h +++ b/chrome/browser/sync/glue/typed_url_data_type_controller.h @@ -44,9 +44,8 @@ class TypedUrlDataTypeController : public NonFrontendDataTypeController { virtual bool PostTaskOnBackendThread( const tracked_objects::Location& from_here, const base::Closure& task) OVERRIDE; - virtual ProfileSyncComponentsFactory::SyncComponents CreateSyncComponents() - OVERRIDE; - virtual void DisconnectProcessor(ChangeProcessor* processor) OVERRIDE; + virtual void CreateSyncComponents() OVERRIDE; + virtual void StopModels() OVERRIDE; private: virtual ~TypedUrlDataTypeController(); diff --git a/chrome/browser/sync/glue/typed_url_model_associator.cc b/chrome/browser/sync/glue/typed_url_model_associator.cc index b4cabc0..89f0e33 100644 --- a/chrome/browser/sync/glue/typed_url_model_associator.cc +++ b/chrome/browser/sync/glue/typed_url_model_associator.cc @@ -65,7 +65,7 @@ TypedUrlModelAssociator::TypedUrlModelAssociator( : sync_service_(sync_service), history_backend_(history_backend), expected_loop_(base::MessageLoop::current()), - abort_requested_(false), + pending_abort_(false), error_handler_(error_handler), num_db_accesses_(0), num_db_errors_(0) { @@ -173,50 +173,43 @@ int TypedUrlModelAssociator::GetErrorPercentage() const { syncer::SyncError TypedUrlModelAssociator::DoAssociateModels() { DVLOG(1) << "Associating TypedUrl Models"; + syncer::SyncError error; DCHECK(expected_loop_ == base::MessageLoop::current()); - + if (IsAbortPending()) + return syncer::SyncError(); history::URLRows typed_urls; ++num_db_accesses_; - bool query_succeeded = - history_backend_ && history_backend_->GetAllTypedURLs(&typed_urls); + if (!history_backend_->GetAllTypedURLs(&typed_urls)) { + ++num_db_errors_; + return error_handler_->CreateAndUploadError( + FROM_HERE, + "Could not get the typed_url entries.", + model_type()); + } + + // Get all the visits. + std::map<history::URLID, history::VisitVector> visit_vectors; + for (history::URLRows::iterator ix = typed_urls.begin(); + ix != typed_urls.end();) { + if (IsAbortPending()) + return syncer::SyncError(); + DCHECK_EQ(0U, visit_vectors.count(ix->id())); + if (!FixupURLAndGetVisits(&(*ix), &(visit_vectors[ix->id()])) || + ShouldIgnoreUrl(ix->url()) || + ShouldIgnoreVisits(visit_vectors[ix->id()])) { + // Ignore this URL if we couldn't load the visits or if there's some + // other problem with it (it was empty, or imported and never visited). + ix = typed_urls.erase(ix); + } else { + ++ix; + } + } history::URLRows new_urls; TypedUrlVisitVector new_visits; TypedUrlUpdateVector updated_urls; - { - base::AutoLock au(abort_lock_); - if (abort_requested_) { - return syncer::SyncError(FROM_HERE, - syncer::SyncError::DATATYPE_ERROR, - "Association was aborted.", - model_type()); - } - - // Must lock and check first to make sure |error_handler_| is valid. - if (!query_succeeded) { - ++num_db_errors_; - return error_handler_->CreateAndUploadError( - FROM_HERE, - "Could not get the typed_url entries.", - model_type()); - } - - // Get all the visits. - std::map<history::URLID, history::VisitVector> visit_vectors; - for (history::URLRows::iterator ix = typed_urls.begin(); - ix != typed_urls.end();) { - DCHECK_EQ(0U, visit_vectors.count(ix->id())); - if (!FixupURLAndGetVisits(&(*ix), &(visit_vectors[ix->id()])) || - ShouldIgnoreUrl(ix->url()) || - ShouldIgnoreVisits(visit_vectors[ix->id()])) { - // Ignore this URL if we couldn't load the visits or if there's some - // other problem with it (it was empty, or imported and never visited). - ix = typed_urls.erase(ix); - } else { - ++ix; - } - } + { syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare()); syncer::ReadNode typed_url_root(&trans); if (typed_url_root.InitByTagLookup(kTypedUrlTag) != @@ -231,6 +224,8 @@ syncer::SyncError TypedUrlModelAssociator::DoAssociateModels() { std::set<std::string> current_urls; for (history::URLRows::iterator ix = typed_urls.begin(); ix != typed_urls.end(); ++ix) { + if (IsAbortPending()) + return syncer::SyncError(); std::string tag = ix->url().spec(); // Empty URLs should be filtered out by ShouldIgnoreUrl() previously. DCHECK(!tag.empty()); @@ -316,6 +311,8 @@ syncer::SyncError TypedUrlModelAssociator::DoAssociateModels() { std::vector<int64> obsolete_nodes; int64 sync_child_id = typed_url_root.GetFirstChildId(); while (sync_child_id != syncer::kInvalidId) { + if (IsAbortPending()) + return syncer::SyncError(); syncer::ReadNode sync_child_node(&trans); if (sync_child_node.InitByIdLookup(sync_child_id) != syncer::BaseNode::INIT_OK) { @@ -375,6 +372,8 @@ syncer::SyncError TypedUrlModelAssociator::DoAssociateModels() { for (std::vector<int64>::const_iterator it = obsolete_nodes.begin(); it != obsolete_nodes.end(); ++it) { + if (IsAbortPending()) + return syncer::SyncError(); syncer::WriteNode sync_node(&trans); if (sync_node.InitByIdLookup(*it) != syncer::BaseNode::INIT_OK) { return error_handler_->CreateAndUploadError( @@ -393,7 +392,7 @@ syncer::SyncError TypedUrlModelAssociator::DoAssociateModels() { // to worry about the sync model getting out of sync, because changes are // propagated to the ChangeProcessor on this thread. WriteToHistoryBackend(&new_urls, &updated_urls, &new_visits, NULL); - return syncer::SyncError(); + return error; } void TypedUrlModelAssociator::UpdateFromSyncDB( @@ -481,8 +480,13 @@ syncer::SyncError TypedUrlModelAssociator::DisassociateModels() { } void TypedUrlModelAssociator::AbortAssociation() { - base::AutoLock lock(abort_lock_); - abort_requested_ = true; + base::AutoLock lock(pending_abort_lock_); + pending_abort_ = true; +} + +bool TypedUrlModelAssociator::IsAbortPending() { + base::AutoLock lock(pending_abort_lock_); + return pending_abort_; } bool TypedUrlModelAssociator::SyncModelHasUserCreatedNodes(bool* has_nodes) { diff --git a/chrome/browser/sync/glue/typed_url_model_associator.h b/chrome/browser/sync/glue/typed_url_model_associator.h index 5f9cd38..269ec03 100644 --- a/chrome/browser/sync/glue/typed_url_model_associator.h +++ b/chrome/browser/sync/glue/typed_url_model_associator.h @@ -170,6 +170,9 @@ class TypedUrlModelAssociator : public AssociatorInterface { bool ShouldIgnoreUrl(const GURL& url); protected: + // Returns true if pending_abort_ is true. Overridable by tests. + virtual bool IsAbortPending(); + // Helper function that clears our error counters (used to reset stats after // model association so we can track model association errors separately). // Overridden by tests. @@ -189,8 +192,11 @@ class TypedUrlModelAssociator : public AssociatorInterface { base::MessageLoop* expected_loop_; - bool abort_requested_; - base::Lock abort_lock_; + // Lock to ensure exclusive access to the pending_abort_ flag. + base::Lock pending_abort_lock_; + + // Set to true if there's a pending abort. + bool pending_abort_; DataTypeErrorHandler* error_handler_; // Guaranteed to outlive datatypes. diff --git a/chrome/browser/sync/glue/typed_url_model_associator_unittest.cc b/chrome/browser/sync/glue/typed_url_model_associator_unittest.cc index c777163..39d3de2 100644 --- a/chrome/browser/sync/glue/typed_url_model_associator_unittest.cc +++ b/chrome/browser/sync/glue/typed_url_model_associator_unittest.cc @@ -67,27 +67,36 @@ class SyncTypedUrlModelAssociatorTest : public testing::Test { class TestTypedUrlModelAssociator : public TypedUrlModelAssociator { public: - TestTypedUrlModelAssociator() - : TypedUrlModelAssociator(&mock_, NULL, NULL) {} + TestTypedUrlModelAssociator(base::WaitableEvent* startup, + base::WaitableEvent* aborted) + : TypedUrlModelAssociator(&mock_, NULL, NULL), + startup_(startup), + aborted_(aborted) {} + virtual bool IsAbortPending() OVERRIDE { + // Let the main thread know that we've been started up, and block until + // they've called Abort(). + startup_->Signal(); + EXPECT_TRUE(aborted_->TimedWait(TestTimeouts::action_timeout())); + return TypedUrlModelAssociator::IsAbortPending(); + } private: ProfileSyncServiceMock mock_; + base::WaitableEvent* startup_; + base::WaitableEvent* aborted_; }; -static void CreateModelAssociatorAsync(base::WaitableEvent* startup, - base::WaitableEvent* aborted, - base::WaitableEvent* done, - TypedUrlModelAssociator** associator) { +static void CreateModelAssociator(base::WaitableEvent* startup, + base::WaitableEvent* aborted, + base::WaitableEvent* done, + TypedUrlModelAssociator** associator) { // Grab the done lock - when we exit, this will be released and allow the // test to finish. - *associator = new TestTypedUrlModelAssociator(); - - // Signal frontend to call AbortAssociation and proceed after it's called. - startup->Signal(); - aborted->Wait(); + *associator = new TestTypedUrlModelAssociator(startup, aborted); + // AssociateModels should be aborted and should return false. syncer::SyncError error = (*associator)->AssociateModels(NULL, NULL); - EXPECT_TRUE(error.IsSet()); - EXPECT_EQ("datatype error was encountered: Association was aborted.", - error.message()); + + // TODO(lipalani): crbug.com/122690 fix this when fixing abort. + // EXPECT_TRUE(error.IsSet()); delete *associator; done->Signal(); } @@ -424,7 +433,7 @@ TEST_F(SyncTypedUrlModelAssociatorTest, TestAbort) { // model association. db_thread.Start(); base::Closure callback = base::Bind( - &CreateModelAssociatorAsync, &startup, &aborted, &done, &associator); + &CreateModelAssociator, &startup, &aborted, &done, &associator); BrowserThread::PostTask(BrowserThread::DB, FROM_HERE, callback); // Wait for the model associator to get created and start assocation. ASSERT_TRUE(startup.TimedWait(TestTimeouts::action_timeout())); diff --git a/chrome/browser/sync/glue/ui_model_worker.cc b/chrome/browser/sync/glue/ui_model_worker.cc index 970c6be..1105c97 100644 --- a/chrome/browser/sync/glue/ui_model_worker.cc +++ b/chrome/browser/sync/glue/ui_model_worker.cc @@ -21,6 +21,7 @@ namespace { // A simple callback to signal a waitable event after running a closure. void CallDoWorkAndSignalCallback(const syncer::WorkCallback& work, base::WaitableEvent* work_done, + UIModelWorker* const scheduler, syncer::SyncerError* error_info) { if (work.is_null()) { // This can happen during tests or cases where there are more than just the @@ -36,23 +37,60 @@ void CallDoWorkAndSignalCallback(const syncer::WorkCallback& work, *error_info = work.Run(); + // Notify the UIModelWorker that scheduled us that we have run + // successfully. + scheduler->OnTaskCompleted(); work_done->Signal(); // Unblock the syncer thread that scheduled us. } } // namespace UIModelWorker::UIModelWorker(syncer::WorkerLoopDestructionObserver* observer) - : syncer::ModelSafeWorker(observer) { + : syncer::ModelSafeWorker(observer), + state_(WORKING), + syncapi_has_shutdown_(false), + syncapi_event_(&lock_) { +} + +void UIModelWorker::Stop() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + + base::AutoLock lock(lock_); + DCHECK_EQ(state_, WORKING); + + // We're on our own now, the beloved UI MessageLoop is no longer running. + // Any tasks scheduled or to be scheduled on the UI MessageLoop will not run. + state_ = RUNNING_MANUAL_SHUTDOWN_PUMP; + + // Drain any final tasks manually until the SyncerThread tells us it has + // totally finished. There should only ever be 0 or 1 tasks Run() here. + while (!syncapi_has_shutdown_) { + if (!pending_work_.is_null()) + pending_work_.Run(); // OnTaskCompleted will set reset |pending_work_|. + + // http://crbug.com/19757 + base::ThreadRestrictions::ScopedAllowWait allow_wait; + // Wait for either a new task or SyncerThread termination. + syncapi_event_.Wait(); + } + + state_ = STOPPED; } void UIModelWorker::RegisterForLoopDestruction() { CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); base::MessageLoop::current()->AddDestructionObserver(this); - SetWorkingLoopToCurrent(); } syncer::SyncerError UIModelWorker::DoWorkAndWaitUntilDoneImpl( const syncer::WorkCallback& work) { + // In most cases, this method is called in WORKING state. It is possible this + // gets called when we are in the RUNNING_MANUAL_SHUTDOWN_PUMP state, because + // the UI loop has initiated shutdown but the syncer hasn't got the memo yet. + // This is fine, the work will get scheduled and run normally or run by our + // code handling this case in Stop(). Note there _no_ way we can be in here + // with state_ = STOPPED, so it is safe to read / compare in this case. + CHECK_NE(ANNOTATE_UNPROTECTED_READ(state_), STOPPED); syncer::SyncerError error_info; if (BrowserThread::CurrentlyOn(BrowserThread::UI)) { DLOG(WARNING) << "DoWorkAndWaitUntilDone called from " @@ -60,16 +98,25 @@ syncer::SyncerError UIModelWorker::DoWorkAndWaitUntilDoneImpl( return work.Run(); } - if (!BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - base::Bind(&CallDoWorkAndSignalCallback, - work, work_done_or_stopped(), &error_info))) { - DLOG(WARNING) << "Could not post work to UI loop."; - error_info = syncer::CANNOT_DO_WORK; - return error_info; + { + // We lock only to avoid PostTask'ing a NULL pending_work_ (because it + // could get Run() in Stop() and call OnTaskCompleted before we post). + // The task is owned by the message loop as per usual. + base::AutoLock lock(lock_); + DCHECK(pending_work_.is_null()); + pending_work_ = base::Bind(&CallDoWorkAndSignalCallback, work, + work_done_or_stopped(), + base::Unretained(this), &error_info); + if (!BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, pending_work_)) { + DLOG(WARNING) << "Could not post work to UI loop."; + error_info = syncer::CANNOT_DO_WORK; + pending_work_.Reset(); + syncapi_event_.Signal(); + return error_info; + } } + syncapi_event_.Signal(); // Notify that the syncapi produced work for us. work_done_or_stopped()->Wait(); - return error_info; } @@ -77,7 +124,21 @@ syncer::ModelSafeGroup UIModelWorker::GetModelSafeGroup() { return syncer::GROUP_UI; } +void UIModelWorker::OnSyncerShutdownComplete() { + base::AutoLock lock(lock_); + // The SyncerThread has terminated and we are no longer needed by syncapi. + // The UI loop initiated shutdown and is (or will be) waiting in Stop(). + // We could either be WORKING or RUNNING_MANUAL_SHUTDOWN_PUMP, depending + // on where we timeslice the UI thread in Stop; but we can't be STOPPED, + // because that would imply OnSyncerShutdownComplete already signaled. + DCHECK_NE(state_, STOPPED); + + syncapi_has_shutdown_ = true; + syncapi_event_.Signal(); +} + UIModelWorker::~UIModelWorker() { + DCHECK_EQ(state_, STOPPED); } } // namespace browser_sync diff --git a/chrome/browser/sync/glue/ui_model_worker.h b/chrome/browser/sync/glue/ui_model_worker.h index 02b8cd3..8f71433 100644 --- a/chrome/browser/sync/glue/ui_model_worker.h +++ b/chrome/browser/sync/glue/ui_model_worker.h @@ -17,21 +17,86 @@ namespace browser_sync { // A syncer::ModelSafeWorker for UI models (e.g. bookmarks) that // accepts work requests from the syncapi that need to be fulfilled // from the MessageLoop home to the native model. +// +// Lifetime note: Instances of this class will generally be owned by the +// SyncerThread. When the SyncerThread _object_ is destroyed, the +// UIModelWorker will be destroyed. The SyncerThread object is destroyed +// after the actual syncer pthread has exited. class UIModelWorker : public syncer::ModelSafeWorker { public: explicit UIModelWorker(syncer::WorkerLoopDestructionObserver* observer); + // Called by the UI thread on shutdown of the sync service. Blocks until + // the UIModelWorker has safely met termination conditions, namely that + // no task scheduled by CallDoWorkFromModelSafeThreadAndWait remains un- + // processed and that syncapi will not schedule any further work for us to do. + void Stop(); + // syncer::ModelSafeWorker implementation. Called on syncapi SyncerThread. virtual void RegisterForLoopDestruction() OVERRIDE; virtual syncer::ModelSafeGroup GetModelSafeGroup() OVERRIDE; + // Upon receiving this idempotent call, the syncer::ModelSafeWorker can + // assume no work will ever be scheduled again from now on. If it has any work + // that it has not yet completed, it must make sure to run it as soon as + // possible as the Syncer is trying to shut down. Called from the CoreThread. + void OnSyncerShutdownComplete(); + + // Callback from |pending_work_| to notify us that it has been run. + // Called on ui loop. + void OnTaskCompleted() { pending_work_.Reset(); } + protected: virtual syncer::SyncerError DoWorkAndWaitUntilDoneImpl( const syncer::WorkCallback& work) OVERRIDE; private: + // The life-cycle of a UIModelWorker in three states. + enum State { + // We hit the ground running in this state and remain until + // the UI loop calls Stop(). + WORKING, + // Stop() sequence has been initiated, but we have not received word that + // the SyncerThread has terminated and doesn't need us anymore. Since the + // UI MessageLoop is not running at this point, we manually process any + // last pending_task_ that the Syncer throws at us, effectively dedicating + // the UI thread to terminating the Syncer. + RUNNING_MANUAL_SHUTDOWN_PUMP, + // We have come to a complete stop, no scheduled work remains, and no work + // will be scheduled from now until our destruction. + STOPPED, + }; + virtual ~UIModelWorker(); + // This is set by the UI thread, but is not explicitly thread safe, so only + // read this value from other threads when you know it is absolutely safe. + State state_; + + // We keep a reference to any task we have scheduled so we can gracefully + // force them to run if the syncer is trying to shutdown. + base::Closure pending_work_; + + // Set by the SyncCoreThread when Syncapi shutdown has completed and the + // SyncerThread has terminated, so no more work will be scheduled. Read by + // the UI thread in Stop(). + bool syncapi_has_shutdown_; + + // We use a Lock for all data members and a ConditionVariable to synchronize. + // We do this instead of using a WaitableEvent and a bool condition in order + // to guard against races that could arise due to the fact that the lack of a + // barrier permits instructions to be reordered by compiler optimizations. + // Possible or not, that route makes for very fragile code due to existence + // of theoretical races. + base::Lock lock_; + + // Used as a barrier at shutdown to ensure the SyncerThread terminates before + // we allow the UI thread to return from Stop(). This gets signalled whenever + // one of two events occur: a new pending_work_ task was scheduled, or the + // SyncerThread has terminated. We only care about (1) when we are in Stop(), + // because we have to manually Run() the task. + base::ConditionVariable syncapi_event_; + DISALLOW_COPY_AND_ASSIGN(UIModelWorker); }; diff --git a/chrome/browser/sync/glue/ui_model_worker_unittest.cc b/chrome/browser/sync/glue/ui_model_worker_unittest.cc index ab3fc64..2b61faf 100644 --- a/chrome/browser/sync/glue/ui_model_worker_unittest.cc +++ b/chrome/browser/sync/glue/ui_model_worker_unittest.cc @@ -18,6 +18,8 @@ using browser_sync::UIModelWorker; using syncer::SyncerError; using content::BrowserThread; +// Various boilerplate, primarily for the StopWithPendingWork test. + class UIModelWorkerVisitor { public: UIModelWorkerVisitor(base::WaitableEvent* was_run, @@ -57,6 +59,27 @@ class Syncer { DISALLOW_COPY_AND_ASSIGN(Syncer); }; +// A callback run from the CoreThread to simulate terminating syncapi. +void FakeSyncapiShutdownCallback(base::Thread* syncer_thread, + UIModelWorker* worker, + base::WaitableEvent** jobs, + size_t job_count) { + base::WaitableEvent all_jobs_done(false, false); + + // In real life, we would try and close a sync directory, which would + // result in the syncer calling it's own destructor, which results in + // the SyncerThread::HaltSyncer being called, which sets the + // syncer in RequestEarlyExit mode and waits until the Syncer finishes + // SyncShare to remove the syncer from its watch. Here we just manually + // wait until all outstanding jobs are done to simulate what happens in + // SyncerThread::HaltSyncer. + all_jobs_done.WaitMany(jobs, job_count); + + // These two calls are made from SyncBackendHost::Core::DoShutdown. + syncer_thread->Stop(); + worker->OnSyncerShutdownComplete(); +} + class SyncUIModelWorkerTest : public testing::Test { public: SyncUIModelWorkerTest() : faux_syncer_thread_("FauxSyncerThread"), @@ -94,5 +117,96 @@ TEST_F(SyncUIModelWorkerTest, ScheduledWorkRunsOnUILoop) { // We are on the UI thread, so run our loop to process the // (hopefully) scheduled task from a SyncShare invocation. base::MessageLoop::current()->Run(); + + bmw()->OnSyncerShutdownComplete(); + bmw()->Stop(); syncer_thread()->Stop(); } + +TEST_F(SyncUIModelWorkerTest, StopWithPendingWork) { + // What we want to set up is the following: + // ("ui_thread" is the thread we are currently executing on) + // 1 - simulate the user shutting down the browser, and the ui thread needing + // to terminate the core thread. + // 2 - the core thread is where the syncapi is accessed from, and so it needs + // to shut down the SyncerThread. + // 3 - the syncer is waiting on the UIModelWorker to + // perform a task for it. + // The UIModelWorker's manual shutdown pump will save the day, as the + // UI thread is not actually trying to join() the core thread, it is merely + // waiting for the SyncerThread to give it work or to finish. After that, it + // will join the core thread which should succeed as the SyncerThread has left + // the building. Unfortunately this test as written is not provably decidable, + // as it will always halt on success, but it may not on failure (namely if + // the task scheduled by the Syncer is _never_ run). + core_thread()->Start(); + base::WaitableEvent v_ran(false, false); + scoped_ptr<UIModelWorkerVisitor> v(new UIModelWorkerVisitor( + &v_ran, false)); + base::WaitableEvent* jobs[] = { &v_ran }; + + // The current message loop is not running, so queue a task to cause + // UIModelWorker::Stop() to play a crucial role. See comment below. + syncer_thread()->message_loop()->PostTask(FROM_HERE, + base::Bind(&Syncer::SyncShare, base::Unretained(syncer()), v.get())); + + // This is what gets the core_thread blocked on the syncer_thread. + core_thread()->message_loop()->PostTask(FROM_HERE, + base::Bind(&FakeSyncapiShutdownCallback, syncer_thread(), + base::Unretained(bmw()), + static_cast<base::WaitableEvent**>(jobs), 1)); + + // This is what gets the UI thread blocked until NotifyExitRequested, + // which is called when FakeSyncapiShutdownCallback runs and deletes the + // syncer. + bmw()->Stop(); + + EXPECT_FALSE(syncer_thread()->IsRunning()); + core_thread()->Stop(); +} + +TEST_F(SyncUIModelWorkerTest, HypotheticalManualPumpFlooding) { + // This situation should not happen in real life because the Syncer should + // never send more than one CallDoWork notification after early_exit_requested + // has been set, but our UIModelWorker is built to handle this case + // nonetheless. It may be needed in the future, and since we support it and + // it is not actually exercised in the wild this test is essential. + // It is identical to above except we schedule more than one visitor. + core_thread()->Start(); + + // Our ammunition. + base::WaitableEvent fox1_ran(false, false); + scoped_ptr<UIModelWorkerVisitor> fox1(new UIModelWorkerVisitor( + &fox1_ran, false)); + base::WaitableEvent fox2_ran(false, false); + scoped_ptr<UIModelWorkerVisitor> fox2(new UIModelWorkerVisitor( + &fox2_ran, false)); + base::WaitableEvent fox3_ran(false, false); + scoped_ptr<UIModelWorkerVisitor> fox3(new UIModelWorkerVisitor( + &fox3_ran, false)); + base::WaitableEvent* jobs[] = { &fox1_ran, &fox2_ran, &fox3_ran }; + + // The current message loop is not running, so queue a task to cause + // UIModelWorker::Stop() to play a crucial role. See comment below. + syncer_thread()->message_loop()->PostTask(FROM_HERE, + base::Bind(&Syncer::SyncShare, base::Unretained(syncer()), fox1.get())); + syncer_thread()->message_loop()->PostTask(FROM_HERE, + base::Bind(&Syncer::SyncShare, base::Unretained(syncer()), fox2.get())); + + // This is what gets the core_thread blocked on the syncer_thread. + core_thread()->message_loop()->PostTask(FROM_HERE, + base::Bind(&FakeSyncapiShutdownCallback, syncer_thread(), + base::Unretained(bmw()), + static_cast<base::WaitableEvent**>(jobs), 3)); + syncer_thread()->message_loop()->PostTask(FROM_HERE, + base::Bind(&Syncer::SyncShare, base::Unretained(syncer()), fox3.get())); + + // This is what gets the UI thread blocked until NotifyExitRequested, + // which is called when FakeSyncapiShutdownCallback runs and deletes the + // syncer. + bmw()->Stop(); + + // Was the thread killed? + EXPECT_FALSE(syncer_thread()->IsRunning()); + core_thread()->Stop(); +} diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc index 6af44ca..5902452 100644 --- a/chrome/browser/sync/profile_sync_service.cc +++ b/chrome/browser/sync/profile_sync_service.cc @@ -480,7 +480,6 @@ void ProfileSyncService::InitializeBackend(bool delete_stale_data) { backend_->Initialize( this, - sync_thread_.Pass(), MakeWeakHandle(sync_js_controller_.AsWeakPtr()), sync_service_url_, credentials, @@ -687,19 +686,17 @@ void ProfileSyncService::Shutdown() { SigninGlobalError::GetForProfile(profile_)->RemoveProvider(this); ShutdownImpl(false); - - if (sync_thread_) - sync_thread_->Stop(); } void ProfileSyncService::ShutdownImpl(bool sync_disabled) { - if (!backend_) - return; - - // First, we spin down the backend to stop change processing as soon as - // possible. + // First, we spin down the backend and wait for it to stop syncing completely + // before we Stop the data type manager. This is to avoid a late sync cycle + // applying changes to the sync db that wouldn't get applied via + // ChangeProcessors, leading to back-from-the-dead bugs. base::Time shutdown_start_time = base::Time::Now(); - backend_->StopSyncingForShutdown(); + if (backend_) { + backend_->StopSyncingForShutdown(); + } // Stop all data type controllers, if needed. Note that until Stop // completes, it is possible in theory to have a ChangeProcessor apply a @@ -725,11 +722,8 @@ void ProfileSyncService::ShutdownImpl(bool sync_disabled) { // shutting it down. scoped_ptr<SyncBackendHost> doomed_backend(backend_.release()); if (doomed_backend) { - sync_thread_ = doomed_backend->Shutdown(sync_disabled); - // When sync is disabled, backend should pass sync thread to PSS. Otherwise - // defer join()ing on backend until Chrome shutdown and avoid blocking UI - // right now. - DCHECK(!sync_disabled || sync_thread_.get()); + doomed_backend->Shutdown(sync_disabled); + doomed_backend.reset(); } base::TimeDelta shutdown_time = base::Time::Now() - shutdown_start_time; diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h index 3726d2b..18dc129 100644 --- a/chrome/browser/sync/profile_sync_service.h +++ b/chrome/browser/sync/profile_sync_service.h @@ -907,13 +907,6 @@ class ProfileSyncService : public ProfileSyncServiceBase, // and association information. syncer::WeakHandle<syncer::DataTypeDebugInfoListener> debug_info_listener_; - // A thread where all the sync operations happen. - // OWNERSHIP Notes: - // * Created when backend starts for the first time. - // * If sync is disabled, PSS claims ownership from backend. - // * If sync is reenabled, PSS passes ownership to new backend. - scoped_ptr<base::Thread> sync_thread_; - // Specifies whenever to use oauth2 access token or ClientLogin token in // communications with sync and xmpp servers. // TODO(pavely): Remove once android is converted to oauth2 tokens. diff --git a/chrome/browser/sync/profile_sync_service_startup_unittest.cc b/chrome/browser/sync/profile_sync_service_startup_unittest.cc index 1f463f9..18a0a72 100644 --- a/chrome/browser/sync/profile_sync_service_startup_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_startup_unittest.cc @@ -84,7 +84,6 @@ class ProfileSyncServiceStartupTest : public testing::Test { virtual void SetUp() { file_thread_.Start(); io_thread_.StartIOThread(); - db_thread_.Start(); profile_->CreateRequestContext(); #if defined(OS_CHROMEOS) SigninManagerFactory::GetInstance()->SetTestingFactory( @@ -106,9 +105,8 @@ class ProfileSyncServiceStartupTest : public testing::Test { // Pump messages posted by the sync core thread (which may end up // posting on the IO thread). ui_loop_.RunUntilIdle(); - file_thread_.Stop(); - db_thread_.Stop(); io_thread_.Stop(); + file_thread_.Stop(); ui_loop_.RunUntilIdle(); } diff --git a/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc b/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc index 4db1776..e933442 100644 --- a/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc @@ -97,11 +97,6 @@ class HistoryServiceMock : public HistoryService { explicit HistoryServiceMock(Profile* profile) : HistoryService(profile) {} MOCK_METHOD2(ScheduleDBTask, void(history::HistoryDBTask*, CancelableRequestConsumerBase*)); - MOCK_METHOD0(Shutdown, void()); - - void ShutdownBaseService() { - HistoryService::Shutdown(); - } private: virtual ~HistoryServiceMock() {} @@ -140,11 +135,6 @@ ACTION_P2(RunTaskOnDBThread, thread, backend) { task)); } -ACTION_P2(ShutdownHistoryService, thread, service) { - service->ShutdownBaseService(); - delete thread; -} - ACTION_P6(MakeTypedUrlSyncComponents, profile, service, @@ -178,8 +168,8 @@ class ProfileSyncServiceTypedUrlTest : public AbstractProfileSyncServiceTest { } protected: - ProfileSyncServiceTypedUrlTest() { - history_thread_.reset(new Thread("history")); + ProfileSyncServiceTypedUrlTest() + : history_thread_("history") { } virtual void SetUp() { @@ -193,15 +183,17 @@ class ProfileSyncServiceTypedUrlTest : public AbstractProfileSyncServiceTest { HistoryServiceFactory::GetInstance()->SetTestingFactoryAndUse( profile_.get(), BuildHistoryService)); EXPECT_CALL((*history_service_), ScheduleDBTask(_, _)) - .WillRepeatedly(RunTaskOnDBThread(history_thread_.get(), + .WillRepeatedly(RunTaskOnDBThread(&history_thread_, history_backend_.get())); - history_thread_->Start(); + history_thread_.Start(); } virtual void TearDown() { - EXPECT_CALL((*history_service_), Shutdown()) - .WillOnce(ShutdownHistoryService(history_thread_.release(), - history_service_)); + history_backend_ = NULL; + history_service_ = NULL; + ProfileSyncServiceFactory::GetInstance()->SetTestingFactory( + profile_.get(), NULL); + history_thread_.Stop(); profile_->ResetRequestContext(); profile_.reset(); AbstractProfileSyncServiceTest::TearDown(); @@ -322,7 +314,7 @@ class ProfileSyncServiceTypedUrlTest : public AbstractProfileSyncServiceTest { return history_url; } - scoped_ptr<Thread> history_thread_; + Thread history_thread_; scoped_ptr<ProfileMock> profile_; scoped_refptr<HistoryBackendMock> history_backend_; @@ -572,8 +564,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeAdd) { history::URLsModifiedDetails details; details.changed_urls.push_back(added_entry); - scoped_refptr<ThreadNotifier> notifier( - new ThreadNotifier(history_thread_.get())); + scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED, content::Source<Profile>(profile_.get()), content::Details<history::URLsModifiedDetails>(&details)); @@ -603,8 +594,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeAddWithBlank) { history::URLsModifiedDetails details; details.changed_urls.push_back(empty_entry); details.changed_urls.push_back(added_entry); - scoped_refptr<ThreadNotifier> notifier( - new ThreadNotifier(history_thread_.get())); + scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED, content::Source<Profile>(profile_.get()), content::Details<history::URLsModifiedDetails>(&details)); @@ -641,8 +631,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeUpdate) { history::URLsModifiedDetails details; details.changed_urls.push_back(updated_entry); - scoped_refptr<ThreadNotifier> notifier( - new ThreadNotifier(history_thread_.get())); + scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED, content::Source<Profile>(profile_.get()), content::Details<history::URLsModifiedDetails>(&details)); @@ -670,8 +659,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeAddFromVisit) { history::URLVisitedDetails details; details.row = added_entry; details.transition = content::PAGE_TRANSITION_TYPED; - scoped_refptr<ThreadNotifier> notifier( - new ThreadNotifier(history_thread_.get())); + scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); notifier->Notify(chrome::NOTIFICATION_HISTORY_URL_VISITED, content::Source<Profile>(profile_.get()), content::Details<history::URLVisitedDetails>(&details)); @@ -709,8 +697,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeUpdateFromVisit) { history::URLVisitedDetails details; details.row = updated_entry; details.transition = content::PAGE_TRANSITION_TYPED; - scoped_refptr<ThreadNotifier> notifier( - new ThreadNotifier(history_thread_.get())); + scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); notifier->Notify(chrome::NOTIFICATION_HISTORY_URL_VISITED, content::Source<Profile>(profile_.get()), content::Details<history::URLVisitedDetails>(&details)); @@ -750,8 +737,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserIgnoreChangeUpdateFromVisit) { // Should ignore this change because it's not TYPED. details.transition = content::PAGE_TRANSITION_RELOAD; - scoped_refptr<ThreadNotifier> notifier( - new ThreadNotifier(history_thread_.get())); + scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); notifier->Notify(chrome::NOTIFICATION_HISTORY_URL_VISITED, content::Source<Profile>(profile_.get()), content::Details<history::URLVisitedDetails>(&details)); @@ -817,8 +803,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeRemove) { history::URLsDeletedDetails changes; changes.all_history = false; changes.rows.push_back(history::URLRow(GURL("http://mine.com"))); - scoped_refptr<ThreadNotifier> notifier( - new ThreadNotifier(history_thread_.get())); + scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_DELETED, content::Source<Profile>(profile_.get()), content::Details<history::URLsDeletedDetails>(&changes)); @@ -856,8 +841,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeRemoveArchive) { // Setting archived=true should cause the sync code to ignore this deletion. changes.archived = true; changes.rows.push_back(history::URLRow(GURL("http://mine.com"))); - scoped_refptr<ThreadNotifier> notifier( - new ThreadNotifier(history_thread_.get())); + scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_DELETED, content::Source<Profile>(profile_.get()), content::Details<history::URLsDeletedDetails>(&changes)); @@ -896,8 +880,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeRemoveAll) { history::URLsDeletedDetails changes; changes.all_history = true; - scoped_refptr<ThreadNotifier> notifier( - new ThreadNotifier(history_thread_.get())); + scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_DELETED, content::Source<Profile>(profile_.get()), content::Details<history::URLsDeletedDetails>(&changes)); @@ -976,7 +959,6 @@ TEST_F(ProfileSyncServiceTypedUrlTest, FailToGetTypedURLs) { // Can't check GetErrorPercentage(), because generating an unrecoverable // error will free the model associator. } - TEST_F(ProfileSyncServiceTypedUrlTest, IgnoreLocalFileURL) { history::VisitVector original_visits; // Create http and file url. @@ -1015,8 +997,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, IgnoreLocalFileURL) { details.changed_urls.push_back(updated_url_entry); details.changed_urls.push_back(updated_file_entry); details.changed_urls.push_back(new_file_entry); - scoped_refptr<ThreadNotifier> notifier( - new ThreadNotifier(history_thread_.get())); + scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED, content::Source<Profile>(profile_.get()), content::Details<history::URLsModifiedDetails>(&details)); @@ -1069,8 +1050,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, IgnoreLocalhostURL) { details.changed_urls.push_back(updated_url_entry); details.changed_urls.push_back(updated_localhost_entry); details.changed_urls.push_back(localhost_ip_entry); - scoped_refptr<ThreadNotifier> notifier( - new ThreadNotifier(history_thread_.get())); + scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED, content::Source<Profile>(profile_.get()), content::Details<history::URLsModifiedDetails>(&details)); diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc index f1ddbe7..c257449 100644 --- a/chrome/browser/sync/profile_sync_service_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_unittest.cc @@ -46,10 +46,6 @@ using testing::Mock; using testing::Return; using testing::StrictMock; -void SignalDone(base::WaitableEvent* done) { - done->Signal(); -} - class ProfileSyncServiceTestHarness { public: ProfileSyncServiceTestHarness() @@ -61,7 +57,6 @@ class ProfileSyncServiceTestHarness { ~ProfileSyncServiceTestHarness() {} void SetUp() { - db_thread_.Start(); file_thread_.Start(); io_thread_.StartIOThread(); profile.reset(new TestingProfile()); @@ -84,7 +79,6 @@ class ProfileSyncServiceTestHarness { ui_loop_.RunUntilIdle(); io_thread_.Stop(); file_thread_.Stop(); - db_thread_.Stop(); // Ensure that the sync objects destruct to avoid memory leaks. ui_loop_.RunUntilIdle(); } @@ -133,21 +127,6 @@ class ProfileSyncServiceTestHarness { } } - void WaitForBackendInitDone() { - for (int i = 0; i < 5; ++i) { - base::WaitableEvent done(false, false); - service->GetBackendForTest()->GetSyncLoopForTesting() - ->PostTask(FROM_HERE, - base::Bind(&SignalDone, &done)); - done.Wait(); - ui_loop_.RunUntilIdle(); - if (service->sync_initialized()) { - return; - } - } - LOG(ERROR) << "Backend not initialized."; - } - void IssueTestTokens() { TokenService* token_service = TokenServiceFactory::GetForProfile(profile.get()); @@ -373,7 +352,6 @@ TEST_F(ProfileSyncServiceTest, TEST_F(ProfileSyncServiceTest, JsControllerProcessJsMessageBasic) { harness_.StartSyncService(); - harness_.WaitForBackendInitDone(); StrictMock<syncer::MockJsReplyHandler> reply_handler; @@ -391,11 +369,6 @@ TEST_F(ProfileSyncServiceTest, JsControllerProcessJsMessageBasic) { } // This forces the sync thread to process the message and reply. - base::WaitableEvent done(false, false); - harness_.service->GetBackendForTest()->GetSyncLoopForTesting() - ->PostTask(FROM_HERE, - base::Bind(&SignalDone, &done)); - done.Wait(); harness_.TearDown(); } @@ -420,14 +393,9 @@ TEST_F(ProfileSyncServiceTest, } harness_.IssueTestTokens(); - harness_.WaitForBackendInitDone(); // This forces the sync thread to process the message and reply. - base::WaitableEvent done(false, false); - harness_.service->GetBackendForTest()->GetSyncLoopForTesting() - ->PostTask(FROM_HERE, - base::Bind(&SignalDone, &done)); - done.Wait(); harness_.TearDown(); + harness_.TearDown(); } // Make sure that things still work if sync is not enabled, but some old sync 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 |