diff options
86 files changed, 1387 insertions, 1502 deletions
diff --git a/chrome/browser/password_manager/password_store.h b/chrome/browser/password_manager/password_store.h index 4d6079f..9ae6bf3 100644 --- a/chrome/browser/password_manager/password_store.h +++ b/chrome/browser/password_manager/password_store.h @@ -21,6 +21,7 @@ class PasswordStoreConsumer; class Task; namespace browser_sync { +class PasswordChangeProcessor; class PasswordDataTypeController; class PasswordModelAssociator; class PasswordModelWorker; @@ -132,6 +133,7 @@ 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 57e4b9d..7f4024a 100644 --- a/chrome/browser/sync/glue/browser_thread_model_worker.cc +++ b/chrome/browser/sync/glue/browser_thread_model_worker.cc @@ -51,6 +51,7 @@ 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/chrome_extensions_activity_monitor.h b/chrome/browser/sync/glue/chrome_extensions_activity_monitor.h deleted file mode 100644 index 969a3fe..0000000 --- a/chrome/browser/sync/glue/chrome_extensions_activity_monitor.h +++ /dev/null @@ -1,47 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef CHROME_BROWSER_SYNC_GLUE_CHROME_EXTENSIONS_ACTIVITY_MONITOR_H_ -#define CHROME_BROWSER_SYNC_GLUE_CHROME_EXTENSIONS_ACTIVITY_MONITOR_H_ - -#include "base/compiler_specific.h" -#include "base/synchronization/lock.h" -#include "content/public/browser/notification_observer.h" -#include "content/public/browser/notification_registrar.h" -#include "sync/util/extensions_activity_monitor.h" - -namespace browser_sync { - -// Chrome-specific implementation of syncer::ExtensionsActivityMonitor. -// -// As per the requirements of syncer::ExtensionsActivityMonitor, all -// overridden methods are thread-safe, although this class must be -// created and destroyed on the UI thread. -class ChromeExtensionsActivityMonitor - : public syncer::ExtensionsActivityMonitor, - public content::NotificationObserver { - public: - ChromeExtensionsActivityMonitor(); - virtual ~ChromeExtensionsActivityMonitor(); - - // syncer::ExtensionsActivityMonitor implementation. - virtual void GetAndClearRecords(Records* buffer) OVERRIDE; - virtual void PutRecords(const Records& records) OVERRIDE; - - // content::NotificationObserver implementation. - virtual void Observe(int type, - const content::NotificationSource& source, - const content::NotificationDetails& details) OVERRIDE; - - private: - Records records_; - mutable base::Lock records_lock_; - - // Used only on UI loop. - content::NotificationRegistrar registrar_; -}; - -} // namespace browser_sync - -#endif // CHROME_BROWSER_SYNC_GLUE_CHROME_EXTENSIONS_ACTIVITY_MONITOR_H_ diff --git a/chrome/browser/sync/glue/chrome_extensions_activity_monitor.cc b/chrome/browser/sync/glue/extensions_activity_monitor.cc index fbe3d2a..b8de3bb 100644 --- a/chrome/browser/sync/glue/chrome_extensions_activity_monitor.cc +++ b/chrome/browser/sync/glue/extensions_activity_monitor.cc @@ -1,8 +1,8 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Copyright 2013 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "chrome/browser/sync/glue/chrome_extensions_activity_monitor.h" +#include "chrome/browser/sync/glue/extensions_activity_monitor.h" #include "base/bind.h" #include "chrome/browser/chrome_notification_types.h" @@ -10,12 +10,14 @@ #include "chrome/common/extensions/extension.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/notification_service.h" +#include "sync/util/extensions_activity.h" using content::BrowserThread; namespace browser_sync { -ChromeExtensionsActivityMonitor::ChromeExtensionsActivityMonitor() { +ExtensionsActivityMonitor::ExtensionsActivityMonitor() + : extensions_activity_(new syncer::ExtensionsActivity()) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); // It would be nice if we could specify a Source for each specific function // we wanted to observe, but the actual function objects are allocated on @@ -27,29 +29,14 @@ ChromeExtensionsActivityMonitor::ChromeExtensionsActivityMonitor() { content::NotificationService::AllSources()); } -ChromeExtensionsActivityMonitor::~ChromeExtensionsActivityMonitor() { +ExtensionsActivityMonitor::~ExtensionsActivityMonitor() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); } -void ChromeExtensionsActivityMonitor::GetAndClearRecords(Records* buffer) { - base::AutoLock lock(records_lock_); - buffer->clear(); - buffer->swap(records_); -} - -void ChromeExtensionsActivityMonitor::PutRecords(const Records& records) { - base::AutoLock lock(records_lock_); - for (Records::const_iterator i = records.begin(); i != records.end(); ++i) { - records_[i->first].extension_id = i->second.extension_id; - records_[i->first].bookmark_write_count += i->second.bookmark_write_count; - } -} - -void ChromeExtensionsActivityMonitor::Observe( +void ExtensionsActivityMonitor::Observe( int type, const content::NotificationSource& source, const content::NotificationDetails& details) { - base::AutoLock lock(records_lock_); DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); const extensions::Extension* extension = content::Source<const extensions::Extension>(source).ptr(); @@ -60,10 +47,13 @@ void ChromeExtensionsActivityMonitor::Observe( f->name() == "bookmarks.create" || f->name() == "bookmarks.removeTree" || f->name() == "bookmarks.remove") { - Record& record = records_[extension->id()]; - record.extension_id = extension->id(); - record.bookmark_write_count++; + extensions_activity_->UpdateRecord(extension->id()); } } +const scoped_refptr<syncer::ExtensionsActivity>& +ExtensionsActivityMonitor::GetExtensionsActivity() { + return extensions_activity_; +} + } // namespace browser_sync diff --git a/chrome/browser/sync/glue/extensions_activity_monitor.h b/chrome/browser/sync/glue/extensions_activity_monitor.h new file mode 100644 index 0000000..4d5e741 --- /dev/null +++ b/chrome/browser/sync/glue/extensions_activity_monitor.h @@ -0,0 +1,41 @@ +// Copyright 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_SYNC_GLUE_EXTENSIONS_ACTIVITY_MONITOR_H_ +#define CHROME_BROWSER_SYNC_GLUE_EXTENSIONS_ACTIVITY_MONITOR_H_ + +#include "base/compiler_specific.h" +#include "base/memory/ref_counted.h" +#include "content/public/browser/notification_observer.h" +#include "content/public/browser/notification_registrar.h" + +namespace syncer { +class ExtensionsActivity; +} + +namespace browser_sync { + +// Observe and record usage of extension bookmark API. +class ExtensionsActivityMonitor : public content::NotificationObserver { + public: + ExtensionsActivityMonitor(); + virtual ~ExtensionsActivityMonitor(); + + // content::NotificationObserver implementation. + virtual void Observe(int type, + const content::NotificationSource& source, + const content::NotificationDetails& details) OVERRIDE; + + const scoped_refptr<syncer::ExtensionsActivity>& GetExtensionsActivity(); + + private: + scoped_refptr<syncer::ExtensionsActivity> extensions_activity_; + + // Used only on UI loop. + content::NotificationRegistrar registrar_; +}; + +} // namespace browser_sync + +#endif // CHROME_BROWSER_SYNC_GLUE_EXTENSIONS_ACTIVITY_MONITOR_H_ diff --git a/chrome/browser/sync/glue/chrome_extensions_activity_monitor_unittest.cc b/chrome/browser/sync/glue/extensions_activity_monitor_unittest.cc index eab034c7..bfeeb21 100644 --- a/chrome/browser/sync/glue/chrome_extensions_activity_monitor_unittest.cc +++ b/chrome/browser/sync/glue/extensions_activity_monitor_unittest.cc @@ -1,8 +1,8 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Copyright 2013 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "chrome/browser/sync/glue/chrome_extensions_activity_monitor.h" +#include "chrome/browser/sync/glue/extensions_activity_monitor.h" #include "base/files/file_path.h" #include "base/message_loop/message_loop.h" @@ -15,6 +15,7 @@ #include "chrome/common/extensions/extension_manifest_constants.h" #include "content/public/browser/notification_service.h" #include "content/public/test/test_browser_thread.h" +#include "sync/util/extensions_activity.h" #include "testing/gtest/include/gtest/gtest.h" using extensions::Extension; @@ -74,7 +75,7 @@ class SyncChromeExtensionsActivityMonitorTest : public testing::Test { content::TestBrowserThread ui_thread_; protected: - ChromeExtensionsActivityMonitor monitor_; + ExtensionsActivityMonitor monitor_; scoped_refptr<Extension> extension1_; scoped_refptr<Extension> extension2_; // IDs of |extension{1,2}_|. @@ -106,8 +107,8 @@ TEST_F(SyncChromeExtensionsActivityMonitorTest, DISABLED_Basic) { FireBookmarksApiEvent<extensions::BookmarksGetTreeFunction>(extension2_, 33); const uint32 writes_by_extension2 = 8; - syncer::ExtensionsActivityMonitor::Records results; - monitor_.GetAndClearRecords(&results); + syncer::ExtensionsActivity::Records results; + monitor_.GetExtensionsActivity()->GetAndClearRecords(&results); EXPECT_EQ(2U, results.size()); EXPECT_TRUE(results.find(id1_) != results.end()); @@ -124,8 +125,8 @@ TEST_F(SyncChromeExtensionsActivityMonitorTest, DISABLED_Put) { FireBookmarksApiEvent<extensions::BookmarksCreateFunction>(extension1_, 5); FireBookmarksApiEvent<extensions::BookmarksMoveFunction>(extension2_, 8); - syncer::ExtensionsActivityMonitor::Records results; - monitor_.GetAndClearRecords(&results); + syncer::ExtensionsActivity::Records results; + monitor_.GetExtensionsActivity()->GetAndClearRecords(&results); EXPECT_EQ(2U, results.size()); EXPECT_EQ(5U, results[id1_].bookmark_write_count); @@ -136,9 +137,9 @@ TEST_F(SyncChromeExtensionsActivityMonitorTest, DISABLED_Put) { // Simulate a commit failure, which augments the active record set with the // refugee records. - monitor_.PutRecords(results); - syncer::ExtensionsActivityMonitor::Records new_records; - monitor_.GetAndClearRecords(&new_records); + monitor_.GetExtensionsActivity()->PutRecords(results); + syncer::ExtensionsActivity::Records new_records; + monitor_.GetExtensionsActivity()->GetAndClearRecords(&new_records); EXPECT_EQ(2U, results.size()); EXPECT_EQ(id1_, new_records[id1_].extension_id); @@ -153,17 +154,17 @@ TEST_F(SyncChromeExtensionsActivityMonitorTest, DISABLED_Put) { TEST_F(SyncChromeExtensionsActivityMonitorTest, DISABLED_MultiGet) { FireBookmarksApiEvent<extensions::BookmarksCreateFunction>(extension1_, 5); - syncer::ExtensionsActivityMonitor::Records results; - monitor_.GetAndClearRecords(&results); + syncer::ExtensionsActivity::Records results; + monitor_.GetExtensionsActivity()->GetAndClearRecords(&results); EXPECT_EQ(1U, results.size()); EXPECT_EQ(5U, results[id1_].bookmark_write_count); - monitor_.GetAndClearRecords(&results); + monitor_.GetExtensionsActivity()->GetAndClearRecords(&results); EXPECT_TRUE(results.empty()); FireBookmarksApiEvent<extensions::BookmarksCreateFunction>(extension1_, 3); - monitor_.GetAndClearRecords(&results); + monitor_.GetExtensionsActivity()->GetAndClearRecords(&results); EXPECT_EQ(1U, results.size()); EXPECT_EQ(3U, results[id1_].bookmark_write_count); diff --git a/chrome/browser/sync/glue/history_model_worker.cc b/chrome/browser/sync/glue/history_model_worker.cc index da5c3705..dc63d5e 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(HistoryModelWorker* history_worker) - : history_worker_(history_worker) {} + explicit AddDBThreadObserverTask(base::Closure register_callback) + : register_callback_(register_callback) {} virtual bool RunOnDBThread(history::HistoryBackend* backend, history::HistoryDatabase* db) OVERRIDE { - base::MessageLoop::current()->AddDestructionObserver(history_worker_.get()); + register_callback_.Run(); return true; } @@ -57,7 +57,7 @@ class AddDBThreadObserverTask : public history::HistoryDBTask { private: virtual ~AddDBThreadObserverTask() {} - scoped_refptr<HistoryModelWorker> history_worker_; + base::Closure register_callback_; }; namespace { @@ -91,8 +91,15 @@ HistoryModelWorker::HistoryModelWorker( void HistoryModelWorker::RegisterForLoopDestruction() { CHECK(history_service_.get()); - history_service_->ScheduleDBTask(new AddDBThreadObserverTask(this), - &cancelable_consumer_); + history_service_->ScheduleDBTask( + new AddDBThreadObserverTask( + base::Bind(&HistoryModelWorker::RegisterOnDBThread, this)), + &cancelable_consumer_); +} + +void HistoryModelWorker::RegisterOnDBThread() { + base::MessageLoop::current()->AddDestructionObserver(this); + SetWorkingLoopToCurrent(); } 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 93112ab..0f55569 100644 --- a/chrome/browser/sync/glue/history_model_worker.h +++ b/chrome/browser/sync/glue/history_model_worker.h @@ -32,6 +32,10 @@ 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 6d80bef..88fee95 100644 --- a/chrome/browser/sync/glue/non_frontend_data_type_controller.cc +++ b/chrome/browser/sync/glue/non_frontend_data_type_controller.cc @@ -7,7 +7,6 @@ #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" @@ -17,12 +16,146 @@ #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, @@ -31,10 +164,9 @@ NonFrontendDataTypeController::NonFrontendDataTypeController( profile_sync_factory_(profile_sync_factory), profile_(profile), profile_sync_service_(sync_service), - abort_association_(false), - abort_association_complete_(false, false), - start_association_called_(true, false), - start_models_failed_(false) { + model_associator_(NULL), + change_processor_(NULL), + weak_ptr_factory_(this) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(profile_sync_factory_); DCHECK(profile_); @@ -45,8 +177,6 @@ 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, @@ -58,7 +188,6 @@ 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 @@ -85,131 +214,69 @@ 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; - if (!StartAssociationAsync()) { + + components_container_.reset(new BackendComponentsContainer(this)); + + if (!PostTaskOnBackendThread( + FROM_HERE, + base::Bind(&BackendComponentsContainer::Run, + base::Unretained(components_container_.get())))) { 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); - StartDoneImpl(ASSOCIATION_FAILED, - DISABLED, - local_merge_result, - syncer::SyncMergeResult(type())); - } -} - -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(); - } - + StartDone(ASSOCIATION_FAILED, + local_merge_result, + syncer::SyncMergeResult(type())); } - - 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(); +void DestroyComponentsInBackend( + NonFrontendDataTypeController::BackendComponentsContainer *containter) { + delete containter; } -} // namespace void NonFrontendDataTypeController::Stop() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK_NE(state_, NOT_RUNNING); - // 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; + // 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; } - DCHECK(start_callback_.is_null()); - // 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()); + // Call start callback if waiting for association. + if (state_ == ASSOCIATING) { + StartDone(ABORTED, + syncer::SyncMergeResult(type()), + syncer::SyncMergeResult(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; } @@ -239,13 +306,15 @@ NonFrontendDataTypeController::NonFrontendDataTypeController() profile_sync_factory_(NULL), profile_(NULL), profile_sync_service_(NULL), - abort_association_(false), - abort_association_complete_(false, false), - start_association_called_(true, false) { + model_associator_(NULL), + change_processor_(NULL), + weak_ptr_factory_(this) { } NonFrontendDataTypeController::~NonFrontendDataTypeController() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(!change_processor_); + DCHECK(!model_associator_); } bool NonFrontendDataTypeController::StartModels() { @@ -260,7 +329,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)) { @@ -269,20 +338,10 @@ void NonFrontendDataTypeController::StartDone( new_state = (start_result == ASSOCIATION_FAILED ? DISABLED : NOT_RUNNING); if (IsUnrecoverableResult(start_result)) RecordUnrecoverableError(FROM_HERE, "StartFailed"); - StopAssociation(); } - 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)); - } + StartDoneImpl(start_result, new_state, local_merge_result, + syncer_merge_result); } void NonFrontendDataTypeController::StartDoneImpl( @@ -291,21 +350,14 @@ 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_. @@ -314,13 +366,6 @@ 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) { @@ -330,7 +375,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()); @@ -349,14 +394,6 @@ 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_; @@ -381,117 +418,49 @@ void NonFrontendDataTypeController::set_state(State state) { } AssociatorInterface* NonFrontendDataTypeController::associator() const { - return model_associator_.get(); -} - -void NonFrontendDataTypeController::set_model_associator( - AssociatorInterface* associator) { - model_associator_.reset(associator); + return model_associator_; } ChangeProcessor* NonFrontendDataTypeController::change_processor() const { - return change_processor_.get(); -} - -void NonFrontendDataTypeController::set_change_processor( - ChangeProcessor* change_processor) { - change_processor_.reset(change_processor); + return 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(); - } - - DCHECK_EQ(state_, ASSOCIATING); +void NonFrontendDataTypeController::AssociationCallback( + AssociationResult result) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - if (!model_associator_->CryptoReadyIfNecessary()) { + if (result.needs_crypto) { StartDone(NEEDS_CRYPTO, - local_merge_result, - syncer_merge_result); + result.local_merge_result, + result.syncer_merge_result); return; } - 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); + if (result.unrecoverable_error) { StartDone(UNRECOVERABLE_ERROR, - local_merge_result, - syncer_merge_result); + result.local_merge_result, + result.syncer_merge_result); return; } - // 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); + RecordAssociationTime(result.association_time); + if (result.error.IsSet()) { StartDone(ASSOCIATION_FAILED, - local_merge_result, - syncer_merge_result); + result.local_merge_result, + 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(!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(); + StartDone(!result.sync_has_nodes ? OK_FIRST_RUN : OK, + result.local_merge_result, + result.syncer_merge_result); } } // 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 8a67b12c..35cacb0 100644 --- a/chrome/browser/sync/glue/non_frontend_data_type_controller.h +++ b/chrome/browser/sync/glue/non_frontend_data_type_controller.h @@ -15,6 +15,7 @@ #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; @@ -36,13 +37,16 @@ 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 -// funtionality is implemented by default. Derived classes must implement: +// functionality 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, @@ -64,6 +68,22 @@ 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(); @@ -94,7 +114,12 @@ class NonFrontendDataTypeController : public DataTypeController { // Datatype specific creation of sync components. // Note: this is performed on the datatype's thread. - virtual void CreateSyncComponents() = 0; + virtual ProfileSyncComponentsFactory::SyncComponents + CreateSyncComponents() = 0; + + // Called on UI thread during shutdown to effectively disable processing + // any changes. + virtual void DisconnectProcessor(ChangeProcessor* processor) = 0; // Start up complete, update the state and invoke the callback. // Note: this is performed on the datatype's thread. @@ -110,11 +135,6 @@ 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, @@ -133,60 +153,26 @@ 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: - // 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(); - + friend class BackendComponentsContainer; ProfileSyncComponentsFactory* const profile_sync_factory_; Profile* const profile_; ProfileSyncService* const profile_sync_service_; - 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_; + // Created on UI thread and passed to backend to create processor/associator + // and associate model. Released on backend. + scoped_ptr<BackendComponentsContainer> components_container_; - // This is added for debugging purpose. - // TODO(lipalani): Remove this after debugging. - base::WaitableEvent start_association_called_; + AssociatorInterface* model_associator_; + ChangeProcessor* change_processor_; - // This is added for debugging purpose. - // TODO(lipalani): Remove after debugging. - bool start_models_failed_; + base::WeakPtrFactory<NonFrontendDataTypeController> weak_ptr_factory_; 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 a9cbfaf..6d3adc4 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,7 +36,8 @@ class NonFrontendDataTypeControllerMock : public NonFrontendDataTypeController { bool(const tracked_objects::Location&, const base::Closure&)); MOCK_METHOD0(StartAssociation, void()); - MOCK_METHOD0(CreateSyncComponents, void()); + MOCK_METHOD0(CreateSyncComponents, + ProfileSyncComponentsFactory::SyncComponents()); MOCK_METHOD3(StartDone, void(DataTypeController::StartResult result, const syncer::SyncMergeResult& local_merge_result, @@ -46,8 +47,7 @@ class NonFrontendDataTypeControllerMock : public NonFrontendDataTypeController { DataTypeController::State new_state, const syncer::SyncMergeResult& local_merge_result, const syncer::SyncMergeResult& syncer_merge_result)); - MOCK_METHOD0(StopModels, void()); - MOCK_METHOD0(StopAssociation, void()); + MOCK_METHOD1(DisconnectProcessor, void(ChangeProcessor*)); 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 3a0f577..77f5e9e 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,12 +69,10 @@ class NonFrontendDataTypeControllerFake : public NonFrontendDataTypeController { private: virtual ~NonFrontendDataTypeControllerFake() {} - virtual void CreateSyncComponents() OVERRIDE { - ProfileSyncComponentsFactory::SyncComponents sync_components = - profile_sync_factory()-> + virtual ProfileSyncComponentsFactory::SyncComponents + CreateSyncComponents() OVERRIDE { + return 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( @@ -88,9 +86,6 @@ 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 { @@ -103,6 +98,11 @@ 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,6 +130,10 @@ class SyncNonFrontendDataTypeControllerTest : public testing::Test { } virtual void TearDown() { + if (non_frontend_dtc_->state() != + NonFrontendDataTypeController::NOT_RUNNING) { + non_frontend_dtc_->Stop(); + } db_thread_.Stop(); } @@ -160,14 +164,13 @@ class SyncNonFrontendDataTypeControllerTest : public testing::Test { } void SetStopExpectations() { - EXPECT_CALL(*dtc_mock_.get(), StopModels()); + EXPECT_CALL(*dtc_mock_.get(), DisconnectProcessor(_)); 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_) { @@ -220,6 +223,7 @@ TEST_F(SyncNonFrontendDataTypeControllerTest, StartOk) { SetStartExpectations(); SetAssociateExpectations(); SetActivateExpectations(DataTypeController::OK); + SetStopExpectations(); EXPECT_EQ(DataTypeController::NOT_RUNNING, non_frontend_dtc_->state()); Start(); WaitForDTC(); @@ -236,6 +240,7 @@ 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(); @@ -309,8 +314,6 @@ 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)); @@ -325,8 +328,8 @@ TEST_F(SyncNonFrontendDataTypeControllerTest, AbortDuringAssociationInactive) { // Same as above but abort during the Activate call. TEST_F(SyncNonFrontendDataTypeControllerTest, AbortDuringAssociationActivated) { - WaitableEvent wait_for_db_thread_pause(false, false); - WaitableEvent pause_db_thread(false, false); + WaitableEvent wait_for_association_starts(false, false); + WaitableEvent wait_for_dtc_stop(false, false); SetStartExpectations(); EXPECT_CALL(*model_associator_, CryptoReadyIfNecessary()). @@ -335,21 +338,21 @@ TEST_F(SyncNonFrontendDataTypeControllerTest, AbortDuringAssociationActivated) { WillOnce(DoAll( SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AbortAssociation()).WillOnce( - SignalEvent(&pause_db_thread)); + EXPECT_CALL(*model_associator_, AbortAssociation()); EXPECT_CALL(*model_associator_, AssociateModels(_, _)). - 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))); + WillOnce(DoAll( + SignalEvent(&wait_for_association_starts), + WaitOnEvent(&wait_for_dtc_stop), + Return(syncer::SyncError()))); 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_db_thread_pause.Wait(); + wait_for_association_starts.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 83b8cb5..e6969b7 100644 --- a/chrome/browser/sync/glue/password_change_processor.cc +++ b/chrome/browser/sync/glue/password_change_processor.cc @@ -35,7 +35,8 @@ PasswordChangeProcessor::PasswordChangeProcessor( : ChangeProcessor(error_handler), model_associator_(model_associator), password_store_(password_store), - expected_loop_(base::MessageLoop::current()) { + expected_loop_(base::MessageLoop::current()), + disconnected_(false) { DCHECK(model_associator); DCHECK(error_handler); #if defined(OS_MACOSX) @@ -56,6 +57,10 @@ 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); @@ -162,6 +167,9 @@ 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) != @@ -220,6 +228,10 @@ 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( @@ -237,9 +249,17 @@ 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(expected_loop_ == base::MessageLoop::current()); - StartObserving(); + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + password_store_->ScheduleTask( + base::Bind(&PasswordChangeProcessor::StartObserving, + base::Unretained(this))); } void PasswordChangeProcessor::StartObserving() { diff --git a/chrome/browser/sync/glue/password_change_processor.h b/chrome/browser/sync/glue/password_change_processor.h index ed4b54c..11e4372 100644 --- a/chrome/browser/sync/glue/password_change_processor.h +++ b/chrome/browser/sync/glue/password_change_processor.h @@ -56,6 +56,9 @@ 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; @@ -82,6 +85,11 @@ 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 db8df21..688c2a6 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/profile_sync_components_factory.h" +#include "chrome/browser/sync/glue/password_change_processor.h" #include "chrome/browser/sync/profile_sync_service.h" #include "content/public/browser/browser_thread.h" #include "sync/api/sync_error.h" @@ -42,7 +42,8 @@ bool PasswordDataTypeController::PostTaskOnBackendThread( const tracked_objects::Location& from_here, const base::Closure& task) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - DCHECK(password_store_.get()); + if (!password_store_) + return false; return password_store_->ScheduleTask(task); } @@ -54,16 +55,19 @@ bool PasswordDataTypeController::StartModels() { return password_store_.get() != NULL; } -void PasswordDataTypeController::CreateSyncComponents() { +ProfileSyncComponentsFactory::SyncComponents +PasswordDataTypeController::CreateSyncComponents() { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK_EQ(state(), ASSOCIATING); - 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); + return profile_sync_factory()->CreatePasswordSyncComponents( + profile_sync_service(), + password_store_.get(), + this); +} + +void PasswordDataTypeController::DisconnectProcessor( + ChangeProcessor* processor) { + static_cast<PasswordChangeProcessor*>(processor)->Disconnect(); } } // 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 75bbf4b..d9bcdf0 100644 --- a/chrome/browser/sync/glue/password_data_type_controller.h +++ b/chrome/browser/sync/glue/password_data_type_controller.h @@ -35,7 +35,9 @@ class PasswordDataTypeController : public NonFrontendDataTypeController { const tracked_objects::Location& from_here, const base::Closure& task) OVERRIDE; virtual bool StartModels() OVERRIDE; - virtual void CreateSyncComponents() OVERRIDE; + virtual ProfileSyncComponentsFactory::SyncComponents CreateSyncComponents() + OVERRIDE; + virtual void DisconnectProcessor(ChangeProcessor* processor) 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 f034c47..96416783 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_pending_(false), + abort_association_requested_(false), expected_loop_(base::MessageLoop::current()), error_handler_(error_handler) { DCHECK(sync_service_); @@ -45,17 +45,14 @@ PasswordModelAssociator::PasswordModelAssociator( #endif } -PasswordModelAssociator::~PasswordModelAssociator() {} +PasswordModelAssociator::~PasswordModelAssociator() { + DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); +} 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 @@ -76,10 +73,14 @@ 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) != @@ -94,9 +95,6 @@ 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); @@ -176,14 +174,9 @@ 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) - error = WriteToPasswordStore(&new_passwords, - &updated_passwords, - NULL); - if (error.IsSet()) { - return error; - } - - return error; + return WriteToPasswordStore(&new_passwords, + &updated_passwords, + NULL); } bool PasswordModelAssociator::DeleteAllNodes( @@ -238,8 +231,8 @@ bool PasswordModelAssociator::SyncModelHasUserCreatedNodes(bool* has_nodes) { void PasswordModelAssociator::AbortAssociation() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - base::AutoLock lock(abort_association_pending_lock_); - abort_association_pending_ = true; + base::AutoLock lock(association_lock_); + abort_association_requested_ = true; } bool PasswordModelAssociator::CryptoReadyIfNecessary() { @@ -260,11 +253,6 @@ 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 c1e2cf3..38effd6 100644 --- a/chrome/browser/sync/glue/password_model_associator.h +++ b/chrome/browser/sync/glue/password_model_associator.h @@ -118,10 +118,6 @@ 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; @@ -130,11 +126,9 @@ class PasswordModelAssociator PasswordStore* password_store_; int64 password_node_id_; - // 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_; + // Set true by AbortAssociation. + bool abort_association_requested_; + base::Lock association_lock_; 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 a3a3037..beaf5b15 100644 --- a/chrome/browser/sync/glue/password_model_worker.cc +++ b/chrome/browser/sync/glue/password_model_worker.cc @@ -56,6 +56,7 @@ 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 3ab713f..94da4d5 100644 --- a/chrome/browser/sync/glue/sync_backend_host.cc +++ b/chrome/browser/sync/glue/sync_backend_host.cc @@ -158,7 +158,7 @@ class SyncBackendHost::Core // // Called to perform initialization of the syncapi on behalf of // SyncBackendHost::Initialize. - void DoInitialize(const DoInitializeOptions& options); + void DoInitialize(scoped_ptr<DoInitializeOptions> options); // Called to perform credential update on behalf of // SyncBackendHost::UpdateCredentials. @@ -202,17 +202,12 @@ class SyncBackendHost::Core void DoFinishInitialProcessControlTypes(); // The shutdown order is a bit complicated: - // 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); + // 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(); + void DoShutdown(bool sync_disabled); void DoDestroySyncManager(); // Configuration methods that must execute on sync loop. @@ -254,15 +249,14 @@ 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 |sync_thread_|. + // be run on; the host's |registrar_->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 the - // SyncBackendHost |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 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. @@ -275,7 +269,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 DoShutdown(). + // Non-NULL only between calls to DoInitialize() and ~Core(). base::MessageLoop* sync_loop_; // Our parent's registrar (not owned). Non-NULL only between @@ -294,6 +288,8 @@ 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); }; @@ -302,11 +298,10 @@ 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), @@ -319,7 +314,6 @@ SyncBackendHost::SyncBackendHost( SyncBackendHost::SyncBackendHost(Profile* profile) : weak_ptr_factory_(this), - sync_thread_("Chrome_SyncThread"), frontend_loop_(base::MessageLoop::current()), profile_(profile), name_("Unknown"), @@ -351,23 +345,23 @@ 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, bool delete_sync_data_folder, - syncer::SyncManagerFactory* sync_manager_factory, - syncer::UnrecoverableErrorHandler* unrecoverable_error_handler, + scoped_ptr<syncer::SyncManagerFactory> sync_manager_factory, + scoped_ptr<syncer::UnrecoverableErrorHandler> unrecoverable_error_handler, syncer::ReportUnrecoverableErrorFunction report_unrecoverable_error_function) { - if (!sync_thread_.Start()) - return; + registrar_.reset(new SyncBackendRegistrar(name_, + profile_, + sync_thread.Pass())); + CHECK(registrar_->sync_thread()); 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); @@ -389,12 +383,13 @@ void SyncBackendHost::Initialize( } initialization_state_ = CREATING_SYNC_MANAGER; - InitCore(DoInitializeOptions( - sync_thread_.message_loop(), + + scoped_ptr<DoInitializeOptions> init_opts(new DoInitializeOptions( + registrar_->sync_thread()->message_loop(), registrar_.get(), routing_info, workers, - &extensions_activity_monitor_, + extensions_activity_monitor_.GetExtensionsActivity(), event_handler, sync_service_url, base::Bind(&MakeHttpBridgeFactory, @@ -402,20 +397,23 @@ void SyncBackendHost::Initialize( NetworkTimeTracker::BuildNotifierUpdateCallback()), credentials, invalidator_->GetInvalidatorClientId(), - sync_manager_factory, + sync_manager_factory.Pass(), delete_sync_data_folder, sync_prefs_->GetEncryptionBootstrapToken(), sync_prefs_->GetKeystoreEncryptionBootstrapToken(), - new InternalComponentsFactoryImpl(factory_switches), - unrecoverable_error_handler, + scoped_ptr<InternalComponentsFactory>( + new InternalComponentsFactoryImpl(factory_switches)).Pass(), + unrecoverable_error_handler.Pass(), report_unrecoverable_error_function, !cl->HasSwitch(switches::kSyncDisableOAuth2Token))); + InitCore(init_opts.Pass()); } void SyncBackendHost::UpdateCredentials(const SyncCredentials& credentials) { - DCHECK(sync_thread_.IsRunning()); - sync_thread_.message_loop()->PostTask(FROM_HERE, - base::Bind(&SyncBackendHost::Core::DoUpdateCredentials, core_.get(), + DCHECK(registrar_->sync_thread()->IsRunning()); + registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, + base::Bind(&SyncBackendHost::Core::DoUpdateCredentials, + core_.get(), credentials)); } @@ -425,14 +423,14 @@ void SyncBackendHost::StartSyncingWithServer() { syncer::ModelSafeRoutingInfo routing_info; registrar_->GetModelSafeRoutingInfo(&routing_info); - sync_thread_.message_loop()->PostTask(FROM_HERE, + registrar_->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(sync_thread_.IsRunning()); + DCHECK(registrar_->sync_thread()->IsRunning()); if (!IsNigoriEnabled()) { NOTREACHED() << "SetEncryptionPassphrase must never be called when nigori" " is disabled."; @@ -451,8 +449,9 @@ void SyncBackendHost::SetEncryptionPassphrase(const std::string& passphrase, cached_passphrase_type_ == syncer::IMPLICIT_PASSPHRASE); // Post an encryption task on the syncer thread. - sync_thread_.message_loop()->PostTask(FROM_HERE, - base::Bind(&SyncBackendHost::Core::DoSetEncryptionPassphrase, core_.get(), + registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, + base::Bind(&SyncBackendHost::Core::DoSetEncryptionPassphrase, + core_.get(), passphrase, is_explicit)); } @@ -479,8 +478,9 @@ bool SyncBackendHost::SetDecryptionPassphrase(const std::string& passphrase) { return false; // Post a decryption task on the syncer thread. - sync_thread_.message_loop()->PostTask(FROM_HERE, - base::Bind(&SyncBackendHost::Core::DoSetDecryptionPassphrase, core_.get(), + registrar_->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 @@ -496,22 +496,19 @@ bool SyncBackendHost::SetDecryptionPassphrase(const std::string& passphrase) { return true; } -void SyncBackendHost::StopSyncManagerForShutdown( - const base::Closure& closure) { +void SyncBackendHost::StopSyncManagerForShutdown() { 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(sync_thread_.IsRunning()); - sync_thread_.message_loop()->PostTask(FROM_HERE, - base::Bind( - &SyncBackendHost::Core::DoStopSyncManagerForShutdown, - core_.get(), - closure)); + DCHECK(registrar_->sync_thread()->IsRunning()); + registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, + base::Bind(&SyncBackendHost::Core::DoStopSyncManagerForShutdown, + core_.get())); } else { - core_->DoStopSyncManagerForShutdown(closure); + core_->DoStopSyncManagerForShutdown(); } } @@ -524,44 +521,22 @@ void SyncBackendHost::StopSyncingForShutdown() { // Stop listening for and forwarding locally-triggered sync refresh requests. notification_registrar_.RemoveAll(); - // 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()); - } + DCHECK(registrar_->sync_thread()->IsRunning()); + + registrar_->RequestWorkerStopOnUIThread(); + + StopSyncManagerForShutdown(); } -void SyncBackendHost::Shutdown(bool sync_disabled) { +scoped_ptr<base::Thread> SyncBackendHost::Shutdown(ShutdownOption option) { // StopSyncingForShutdown() (which nulls out |frontend_|) should be // called first. DCHECK(!frontend_); + DCHECK(registrar_->sync_thread()->IsRunning()); + + bool sync_disabled = (option == DISABLE_AND_CLAIM_THREAD); + bool sync_thread_claimed = + (option == DISABLE_AND_CLAIM_THREAD || option == STOP_AND_CLAIM_THREAD); if (invalidation_handler_registered_) { if (sync_disabled) { @@ -572,37 +547,25 @@ void SyncBackendHost::Shutdown(bool sync_disabled) { } invalidation_handler_registered_ = false; - // 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)); - } + // 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; - // 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); + // Worker cleanup. + SyncBackendRegistrar* detached_registrar = registrar_.release(); + detached_registrar->sync_thread()->message_loop()->PostTask( + FROM_HERE, + base::Bind(&SyncBackendRegistrar::Shutdown, + base::Unretained(detached_registrar))); - registrar_.reset(); js_backend_.Reset(); - core_ = NULL; // Releases reference to core_. + if (sync_thread_claimed) + return detached_registrar->ReleaseSyncThread(); + else + return scoped_ptr<base::Thread>(); } void SyncBackendHost::UnregisterInvalidationIds() { @@ -727,7 +690,7 @@ void SyncBackendHost::ConfigureDataTypes( } void SyncBackendHost::EnableEncryptEverything() { - sync_thread_.message_loop()->PostTask(FROM_HERE, + registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, base::Bind(&SyncBackendHost::Core::DoEnableEncryptEverything, core_.get())); } @@ -793,9 +756,10 @@ SyncedDeviceTracker* SyncBackendHost::GetSyncedDeviceTracker() const { return core_->synced_device_tracker(); } -void SyncBackendHost::InitCore(const DoInitializeOptions& options) { - sync_thread_.message_loop()->PostTask(FROM_HERE, - base::Bind(&SyncBackendHost::Core::DoInitialize, core_.get(), options)); +void SyncBackendHost::InitCore(scoped_ptr<DoInitializeOptions> options) { + registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, + base::Bind(&SyncBackendHost::Core::DoInitialize, + core_.get(), base::Passed(&options))); } void SyncBackendHost::RequestConfigureSyncer( @@ -814,7 +778,7 @@ void SyncBackendHost::RequestConfigureSyncer( config_types.to_purge = to_purge; config_types.to_journal = to_journal; config_types.to_unapply = to_unapply; - sync_thread_.message_loop()->PostTask(FROM_HERE, + registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, base::Bind(&SyncBackendHost::Core::DoConfigureSyncer, core_.get(), reason, @@ -876,7 +840,7 @@ void SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop( // Kick off the next step in SyncBackendHost initialization by downloading // any necessary control types. - sync_thread_.message_loop()->PostTask( + registrar_->sync_thread()->message_loop()->PostTask( FROM_HERE, base::Bind(&SyncBackendHost::Core::DoDownloadControlTypes, core_.get(), @@ -892,7 +856,7 @@ void SyncBackendHost::Observe( content::Details<const syncer::ModelTypeSet> state_details(details); const syncer::ModelTypeSet& types = *(state_details.ptr()); - sync_thread_.message_loop()->PostTask(FROM_HERE, + registrar_->sync_thread()->message_loop()->PostTask(FROM_HERE, base::Bind(&SyncBackendHost::Core::DoRefreshTypes, core_.get(), types)); } @@ -901,18 +865,18 @@ SyncBackendHost::DoInitializeOptions::DoInitializeOptions( SyncBackendRegistrar* registrar, const syncer::ModelSafeRoutingInfo& routing_info, const std::vector<syncer::ModelSafeWorker*>& workers, - syncer::ExtensionsActivityMonitor* extensions_activity_monitor, + const scoped_refptr<syncer::ExtensionsActivity>& extensions_activity, const syncer::WeakHandle<syncer::JsEventHandler>& event_handler, const GURL& service_url, MakeHttpBridgeFactoryFn make_http_bridge_factory_fn, const syncer::SyncCredentials& credentials, const std::string& invalidator_client_id, - syncer::SyncManagerFactory* sync_manager_factory, + scoped_ptr<syncer::SyncManagerFactory> sync_manager_factory, bool delete_sync_data_folder, const std::string& restored_key_for_bootstrapping, const std::string& restored_keystore_key_for_bootstrapping, - InternalComponentsFactory* internal_components_factory, - syncer::UnrecoverableErrorHandler* unrecoverable_error_handler, + scoped_ptr<InternalComponentsFactory> internal_components_factory, + scoped_ptr<syncer::UnrecoverableErrorHandler> unrecoverable_error_handler, syncer::ReportUnrecoverableErrorFunction report_unrecoverable_error_function, bool use_oauth2_token) @@ -920,19 +884,19 @@ SyncBackendHost::DoInitializeOptions::DoInitializeOptions( registrar(registrar), routing_info(routing_info), workers(workers), - extensions_activity_monitor(extensions_activity_monitor), + extensions_activity(extensions_activity), event_handler(event_handler), service_url(service_url), make_http_bridge_factory_fn(make_http_bridge_factory_fn), credentials(credentials), invalidator_client_id(invalidator_client_id), - sync_manager_factory(sync_manager_factory), + sync_manager_factory(sync_manager_factory.Pass()), delete_sync_data_folder(delete_sync_data_folder), restored_key_for_bootstrapping(restored_key_for_bootstrapping), restored_keystore_key_for_bootstrapping( restored_keystore_key_for_bootstrapping), - internal_components_factory(internal_components_factory), - unrecoverable_error_handler(unrecoverable_error_handler), + internal_components_factory(internal_components_factory.Pass()), + unrecoverable_error_handler(unrecoverable_error_handler.Pass()), report_unrecoverable_error_function( report_unrecoverable_error_function), use_oauth2_token(use_oauth2_token) { @@ -947,13 +911,13 @@ SyncBackendHost::Core::Core(const std::string& name, sync_data_folder_path_(sync_data_folder_path), host_(backend), sync_loop_(NULL), - registrar_(NULL) { + registrar_(NULL), + weak_ptr_factory_(this) { DCHECK(backend.get()); } SyncBackendHost::Core::~Core() { DCHECK(!sync_manager_.get()); - DCHECK(!sync_loop_); } void SyncBackendHost::Core::OnSyncCycleCompleted( @@ -990,9 +954,9 @@ void SyncBackendHost::Core::DoDownloadControlTypes( syncer::ModelTypeSet(), routing_info, base::Bind(&SyncBackendHost::Core::DoInitialProcessControlTypes, - this), + weak_ptr_factory_.GetWeakPtr()), base::Bind(&SyncBackendHost::Core::OnControlTypesDownloadRetry, - this)); + weak_ptr_factory_.GetWeakPtr())); } void SyncBackendHost::Core::DoRefreshTypes(syncer::ModelTypeSet types) { @@ -1030,7 +994,8 @@ void SyncBackendHost::Core::OnInitializationComplete( // Sync manager initialization is complete, so we can schedule recurring // SaveChanges. sync_loop_->PostTask(FROM_HERE, - base::Bind(&Core::StartSavingChanges, this)); + base::Bind(&Core::StartSavingChanges, + weak_ptr_factory_.GetWeakPtr())); host_.Call(FROM_HERE, &SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop, @@ -1158,14 +1123,15 @@ void SyncBackendHost::Core::DoOnIncomingInvalidation( sync_manager_->OnIncomingInvalidation(invalidation_map); } -void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) { +void SyncBackendHost::Core::DoInitialize( + scoped_ptr<DoInitializeOptions> options) { DCHECK(!sync_loop_); - sync_loop_ = options.sync_loop; + sync_loop_ = options->sync_loop; DCHECK(sync_loop_); // Blow away the partial or corrupt sync data folder before doing any more // initialization, if necessary. - if (options.delete_sync_data_folder) { + if (options->delete_sync_data_folder) { DeleteSyncDataFolder(); } @@ -1176,30 +1142,29 @@ void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) { } DCHECK(!registrar_); - registrar_ = options.registrar; + registrar_ = options->registrar; DCHECK(registrar_); - sync_manager_ = options.sync_manager_factory->CreateSyncManager(name_); + sync_manager_ = options->sync_manager_factory->CreateSyncManager(name_); sync_manager_->AddObserver(this); sync_manager_->Init(sync_data_folder_path_, - options.event_handler, - options.service_url.host() + options.service_url.path(), - options.service_url.EffectiveIntPort(), - options.service_url.SchemeIsSecure(), - options.make_http_bridge_factory_fn.Run().Pass(), - options.workers, - options.extensions_activity_monitor, - options.registrar /* as SyncManager::ChangeDelegate */, - options.credentials, - options.invalidator_client_id, - options.restored_key_for_bootstrapping, - options.restored_keystore_key_for_bootstrapping, - scoped_ptr<InternalComponentsFactory>( - options.internal_components_factory), + options->event_handler, + options->service_url.host() + options->service_url.path(), + options->service_url.EffectiveIntPort(), + options->service_url.SchemeIsSecure(), + options->make_http_bridge_factory_fn.Run().Pass(), + options->workers, + options->extensions_activity, + options->registrar /* as SyncManager::ChangeDelegate */, + options->credentials, + options->invalidator_client_id, + options->restored_key_for_bootstrapping, + options->restored_keystore_key_for_bootstrapping, + options->internal_components_factory.get(), &encryptor_, - options.unrecoverable_error_handler, - options.report_unrecoverable_error_function, - options.use_oauth2_token); + options->unrecoverable_error_handler.Pass(), + options->report_unrecoverable_error_function, + options->use_oauth2_token); // |sync_manager_| may end up being NULL here in tests (in // synchronous initialization mode). @@ -1282,7 +1247,7 @@ void SyncBackendHost::Core::DoInitialProcessControlTypes() { sync_manager_->cache_guid())); synced_device_tracker_->InitLocalDeviceInfo( base::Bind(&SyncBackendHost::Core::DoFinishInitialProcessControlTypes, - this)); + weak_ptr_factory_.GetWeakPtr())); } void SyncBackendHost::Core::DoFinishInitialProcessControlTypes() { @@ -1309,13 +1274,9 @@ void SyncBackendHost::Core::DoEnableEncryptEverything() { sync_manager_->GetEncryptionHandler()->EnableEncryptEverything(); } -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::DoStopSyncManagerForShutdown() { + if (sync_manager_) + sync_manager_->StopSyncingForShutdown(); } void SyncBackendHost::Core::DoShutdown(bool sync_disabled) { @@ -1331,9 +1292,8 @@ void SyncBackendHost::Core::DoShutdown(bool sync_disabled) { if (sync_disabled) DeleteSyncDataFolder(); - sync_loop_ = NULL; - host_.Reset(); + weak_ptr_factory_.InvalidateWeakPtrs(); } void SyncBackendHost::Core::DoDestroySyncManager() { @@ -1362,11 +1322,11 @@ void SyncBackendHost::Core::DoConfigureSyncer( config_types.to_unapply, routing_info, base::Bind(&SyncBackendHost::Core::DoFinishConfigureDataTypes, - this, + weak_ptr_factory_.GetWeakPtr(), config_types.to_download, ready_task), base::Bind(&SyncBackendHost::Core::DoRetryConfiguration, - this, + weak_ptr_factory_.GetWeakPtr(), retry_callback)); } @@ -1521,7 +1481,7 @@ void SyncBackendHost::HandleActionableErrorEventOnFrontendLoop( } void SyncBackendHost::OnInvalidatorStateChange(syncer::InvalidatorState state) { - sync_thread_.message_loop()->PostTask( + registrar_->sync_thread()->message_loop()->PostTask( FROM_HERE, base::Bind(&SyncBackendHost::Core::DoOnInvalidatorStateChange, core_.get(), @@ -1539,7 +1499,7 @@ void SyncBackendHost::OnIncomingInvalidation( invalidator_->AcknowledgeInvalidation(it->first, it->second.ack_handle); } - sync_thread_.message_loop()->PostTask( + registrar_->sync_thread()->message_loop()->PostTask( FROM_HERE, base::Bind(&SyncBackendHost::Core::DoOnIncomingInvalidation, core_.get(), @@ -1639,6 +1599,10 @@ 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 a539ec6..113ad42 100644 --- a/chrome/browser/sync/glue/sync_backend_host.h +++ b/chrome/browser/sync/glue/sync_backend_host.h @@ -16,7 +16,7 @@ #include "base/threading/thread.h" #include "chrome/browser/invalidation/invalidation_service.h" #include "chrome/browser/sync/glue/backend_data_type_configurer.h" -#include "chrome/browser/sync/glue/chrome_extensions_activity_monitor.h" +#include "chrome/browser/sync/glue/extensions_activity_monitor.h" #include "content/public/browser/notification_observer.h" #include "content/public/browser/notification_registrar.h" #include "google_apis/gaia/google_service_auth_error.h" @@ -32,6 +32,7 @@ #include "sync/notifier/invalidation_handler.h" #include "sync/protocol/encryption.pb.h" #include "sync/protocol/sync_protocol_error.h" +#include "sync/util/extensions_activity.h" #include "url/gurl.h" class Profile; @@ -176,12 +177,13 @@ 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, bool delete_sync_data_folder, - syncer::SyncManagerFactory* sync_manager_factory, - syncer::UnrecoverableErrorHandler* unrecoverable_error_handler, + scoped_ptr<syncer::SyncManagerFactory> sync_manager_factory, + scoped_ptr<syncer::UnrecoverableErrorHandler> unrecoverable_error_handler, syncer::ReportUnrecoverableErrorFunction report_unrecoverable_error_function); @@ -223,10 +225,19 @@ class SyncBackendHost virtual void StopSyncingForShutdown(); // Called on |frontend_loop_| to kick off shutdown. - // |sync_disabled| indicates if syncing is being disabled or not. // See the implementation and Core::DoShutdown for details. - // Must be called *after* StopSyncingForShutdown. - void Shutdown(bool sync_disabled); + // Must be called *after* StopSyncingForShutdown. Caller should claim sync + // thread using STOP_AND_CLAIM_THREAD or DISABLE_AND_CLAIM_THREAD if sync + // backend might be recreated later because otherwise: + // * sync loop may be stopped on main loop and cause it to be blocked. + // * new/old backend may interfere with each other if new backend is created + // before old one finishes cleanup. + enum ShutdownOption { + STOP, // Stop syncing and let backend stop sync thread. + STOP_AND_CLAIM_THREAD, // Stop syncing and return sync thread. + DISABLE_AND_CLAIM_THREAD, // Disable sync and return sync thread. + }; + scoped_ptr<base::Thread> Shutdown(ShutdownOption option); // Removes all current registrations from the backend on the // InvalidationService. @@ -293,6 +304,8 @@ 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. @@ -310,18 +323,20 @@ class SyncBackendHost SyncBackendRegistrar* registrar, const syncer::ModelSafeRoutingInfo& routing_info, const std::vector<syncer::ModelSafeWorker*>& workers, - syncer::ExtensionsActivityMonitor* extensions_activity_monitor, + const scoped_refptr<syncer::ExtensionsActivity>& extensions_activity, const syncer::WeakHandle<syncer::JsEventHandler>& event_handler, const GURL& service_url, MakeHttpBridgeFactoryFn make_http_bridge_factory_fn, const syncer::SyncCredentials& credentials, const std::string& invalidator_client_id, - syncer::SyncManagerFactory* sync_manager_factory, + scoped_ptr<syncer::SyncManagerFactory> sync_manager_factory, bool delete_sync_data_folder, const std::string& restored_key_for_bootstrapping, const std::string& restored_keystore_key_for_bootstrapping, - syncer::InternalComponentsFactory* internal_components_factory, - syncer::UnrecoverableErrorHandler* unrecoverable_error_handler, + scoped_ptr<syncer::InternalComponentsFactory> + internal_components_factory, + scoped_ptr<syncer::UnrecoverableErrorHandler> + unrecoverable_error_handler, syncer::ReportUnrecoverableErrorFunction report_unrecoverable_error_function, bool use_oauth2_token); @@ -331,27 +346,27 @@ class SyncBackendHost SyncBackendRegistrar* registrar; syncer::ModelSafeRoutingInfo routing_info; std::vector<syncer::ModelSafeWorker*> workers; - syncer::ExtensionsActivityMonitor* extensions_activity_monitor; + scoped_refptr<syncer::ExtensionsActivity> extensions_activity; syncer::WeakHandle<syncer::JsEventHandler> event_handler; GURL service_url; // Overridden by tests. MakeHttpBridgeFactoryFn make_http_bridge_factory_fn; syncer::SyncCredentials credentials; const std::string invalidator_client_id; - syncer::SyncManagerFactory* const sync_manager_factory; + scoped_ptr<syncer::SyncManagerFactory> sync_manager_factory; std::string lsid; bool delete_sync_data_folder; std::string restored_key_for_bootstrapping; std::string restored_keystore_key_for_bootstrapping; - syncer::InternalComponentsFactory* internal_components_factory; - syncer::UnrecoverableErrorHandler* unrecoverable_error_handler; + scoped_ptr<syncer::InternalComponentsFactory> internal_components_factory; + scoped_ptr<syncer::UnrecoverableErrorHandler> unrecoverable_error_handler; syncer::ReportUnrecoverableErrorFunction report_unrecoverable_error_function; bool use_oauth2_token; }; // Allows tests to perform alternate core initialization work. - virtual void InitCore(const DoInitializeOptions& options); + virtual void InitCore(scoped_ptr<DoInitializeOptions> options); // Request the syncer to reconfigure with the specfied params. // Virtual for testing. @@ -510,15 +525,12 @@ class SyncBackendHost // Handles stopping the core's SyncManager, accounting for whether // initialization is done yet. - void StopSyncManagerForShutdown(const base::Closure& closure); + void StopSyncManagerForShutdown(); 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_; @@ -528,14 +540,16 @@ class SyncBackendHost // Name used for debugging (set from profile_->GetDebugName()). const std::string name_; - // Our core, which communicates directly to the syncapi. + // 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. scoped_refptr<Core> core_; InitializationState initialization_state_; const base::WeakPtr<SyncPrefs> sync_prefs_; - ChromeExtensionsActivityMonitor extensions_activity_monitor_; + ExtensionsActivityMonitor extensions_activity_monitor_; scoped_ptr<SyncBackendRegistrar> registrar_; diff --git a/chrome/browser/sync/glue/sync_backend_host_unittest.cc b/chrome/browser/sync/glue/sync_backend_host_unittest.cc index 90eadee..2fd3ccf 100644 --- a/chrome/browser/sync/glue/sync_backend_host_unittest.cc +++ b/chrome/browser/sync/glue/sync_backend_host_unittest.cc @@ -93,25 +93,19 @@ class MockSyncFrontend : public SyncFrontend { class FakeSyncManagerFactory : public syncer::SyncManagerFactory { public: - FakeSyncManagerFactory() : fake_manager_(NULL) {} + explicit FakeSyncManagerFactory(FakeSyncManager** fake_manager) + : fake_manager_(fake_manager) { + *fake_manager_ = NULL; + } virtual ~FakeSyncManagerFactory() {} // 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_); - return scoped_ptr<SyncManager>(fake_manager_); - } - - // Returns NULL until CreateSyncManager() is called on the sync - // thread. Called on the main thread, but only after - // OnBackendInitialized() is called (which is strictly after - // CreateSyncManager is called on the sync thread). - FakeSyncManager* fake_manager() { - return fake_manager_; + *fake_manager_ = new FakeSyncManager(initial_sync_ended_types_, + progress_marker_types_, + configure_fail_types_); + return scoped_ptr<SyncManager>(*fake_manager_); } void set_initial_sync_ended_types(syncer::ModelTypeSet types) { @@ -130,7 +124,7 @@ class FakeSyncManagerFactory : public syncer::SyncManagerFactory { syncer::ModelTypeSet initial_sync_ended_types_; syncer::ModelTypeSet progress_marker_types_; syncer::ModelTypeSet configure_fail_types_; - FakeSyncManager* fake_manager_; + FakeSyncManager** fake_manager_; }; class SyncBackendHostTest : public testing::Test { @@ -151,6 +145,8 @@ class SyncBackendHostTest : public testing::Test { credentials_.email = "user@example.com"; credentials_.sync_token = "sync_token"; + fake_manager_factory_.reset(new FakeSyncManagerFactory(&fake_manager_)); + // These types are always implicitly enabled. enabled_types_.PutAll(syncer::ControlTypes()); @@ -169,7 +165,7 @@ class SyncBackendHostTest : public testing::Test { virtual void TearDown() OVERRIDE { if (backend_) { backend_->StopSyncingForShutdown(); - backend_->Shutdown(false); + backend_->Shutdown(SyncBackendHost::STOP); } backend_.reset(); sync_prefs_.reset(); @@ -186,14 +182,17 @@ class SyncBackendHostTest : public testing::Test { void InitializeBackend(bool expect_success) { EXPECT_CALL(mock_frontend_, OnBackendInitialized(_, _, expect_success)). WillOnce(InvokeWithoutArgs(QuitMessageLoop)); - backend_->Initialize(&mock_frontend_, - syncer::WeakHandle<syncer::JsEventHandler>(), - GURL(std::string()), - credentials_, - true, - &fake_manager_factory_, - &handler_, - NULL); + backend_->Initialize( + &mock_frontend_, + scoped_ptr<base::Thread>(), + syncer::WeakHandle<syncer::JsEventHandler>(), + GURL(std::string()), + credentials_, + true, + fake_manager_factory_.PassAs<syncer::SyncManagerFactory>(), + scoped_ptr<syncer::UnrecoverableErrorHandler>( + new syncer::TestUnrecoverableErrorHandler).Pass(), + NULL); base::RunLoop run_loop; BrowserThread::PostDelayedTask(BrowserThread::UI, FROM_HERE, run_loop.QuitClosure(), @@ -202,7 +201,6 @@ class SyncBackendHostTest : public testing::Test { // |fake_manager_factory_|'s fake_manager() is set on the sync // thread, but we can rely on the message loop barriers to // guarantee that we see the updated value. - fake_manager_ = fake_manager_factory_.fake_manager(); DCHECK(fake_manager_); } @@ -255,12 +253,11 @@ class SyncBackendHostTest : public testing::Test { content::TestBrowserThreadBundle thread_bundle_; StrictMock<MockSyncFrontend> mock_frontend_; syncer::SyncCredentials credentials_; - syncer::TestUnrecoverableErrorHandler handler_; scoped_ptr<TestingProfile> profile_; scoped_ptr<SyncPrefs> sync_prefs_; scoped_ptr<SyncBackendHost> backend_; + scoped_ptr<FakeSyncManagerFactory> fake_manager_factory_; FakeSyncManager* fake_manager_; - FakeSyncManagerFactory fake_manager_factory_; syncer::ModelTypeSet enabled_types_; }; @@ -302,8 +299,8 @@ TEST_F(SyncBackendHostTest, FirstTimeSync) { TEST_F(SyncBackendHostTest, Restart) { sync_prefs_->SetSyncSetupCompleted(); syncer::ModelTypeSet all_but_nigori = enabled_types_; - fake_manager_factory_.set_progress_marker_types(enabled_types_); - fake_manager_factory_.set_initial_sync_ended_types(enabled_types_); + fake_manager_factory_->set_progress_marker_types(enabled_types_); + fake_manager_factory_->set_initial_sync_ended_types(enabled_types_); InitializeBackend(true); EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Empty()); EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(), @@ -333,8 +330,8 @@ TEST_F(SyncBackendHostTest, PartialTypes) { syncer::ModelTypeSet partial_types(syncer::NIGORI, syncer::BOOKMARKS); syncer::ModelTypeSet full_types = Difference(enabled_types_, partial_types); - fake_manager_factory_.set_progress_marker_types(enabled_types_); - fake_manager_factory_.set_initial_sync_ended_types(full_types); + fake_manager_factory_->set_progress_marker_types(enabled_types_); + fake_manager_factory_->set_initial_sync_ended_types(full_types); // Bringing up the backend should purge all partial types, then proceed to // download the Nigori. @@ -512,8 +509,8 @@ TEST_F(SyncBackendHostTest, NewlySupportedTypes) { // Set sync manager behavior before passing it down. All types have progress // markers and initial sync ended except the new types. syncer::ModelTypeSet old_types = enabled_types_; - fake_manager_factory_.set_progress_marker_types(old_types); - fake_manager_factory_.set_initial_sync_ended_types(old_types); + fake_manager_factory_->set_progress_marker_types(old_types); + fake_manager_factory_->set_initial_sync_ended_types(old_types); syncer::ModelTypeSet new_types(syncer::APP_SETTINGS, syncer::EXTENSION_SETTINGS); enabled_types_.PutAll(new_types); @@ -552,8 +549,8 @@ TEST_F(SyncBackendHostTest, NewlySupportedTypesWithPartialTypes) { syncer::ModelTypeSet partial_types(syncer::NIGORI, syncer::BOOKMARKS); syncer::ModelTypeSet full_types = Difference(enabled_types_, partial_types); - fake_manager_factory_.set_progress_marker_types(old_types); - fake_manager_factory_.set_initial_sync_ended_types(full_types); + fake_manager_factory_->set_progress_marker_types(old_types); + fake_manager_factory_->set_initial_sync_ended_types(full_types); syncer::ModelTypeSet new_types(syncer::APP_SETTINGS, syncer::EXTENSION_SETTINGS); enabled_types_.PutAll(new_types); @@ -605,8 +602,8 @@ TEST_F(SyncBackendHostTest, DownloadControlTypes) { syncer::ModelTypeSet new_types(syncer::EXPERIMENTS, syncer::DEVICE_INFO); syncer::ModelTypeSet old_types = Difference(enabled_types_, new_types); - fake_manager_factory_.set_progress_marker_types(old_types); - fake_manager_factory_.set_initial_sync_ended_types(old_types); + fake_manager_factory_->set_progress_marker_types(old_types); + fake_manager_factory_->set_initial_sync_ended_types(old_types); // Bringing up the backend should download the new types without downloading // any old types. @@ -628,7 +625,7 @@ TEST_F(SyncBackendHostTest, DownloadControlTypes) { // be successful, but it returned no results. This means that the usual // download retry logic will not be invoked. TEST_F(SyncBackendHostTest, SilentlyFailToDownloadControlTypes) { - fake_manager_factory_.set_configure_fail_types(syncer::ModelTypeSet::All()); + fake_manager_factory_->set_configure_fail_types(syncer::ModelTypeSet::All()); InitializeBackend(false); } @@ -669,7 +666,7 @@ TEST_F(SyncBackendHostTest, AttemptForwardLocalRefreshRequestLate) { fake_manager_->WaitForSyncThread(); EXPECT_FALSE(types.Equals(fake_manager_->GetLastRefreshRequestTypes())); - backend_->Shutdown(false); + backend_->Shutdown(SyncBackendHost::STOP); backend_.reset(); } diff --git a/chrome/browser/sync/glue/sync_backend_registrar.cc b/chrome/browser/sync/glue/sync_backend_registrar.cc index ab24e14..7150b0d 100644 --- a/chrome/browser/sync/glue/sync_backend_registrar.cc +++ b/chrome/browser/sync/glue/sync_backend_registrar.cc @@ -20,6 +20,7 @@ #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; @@ -54,27 +55,43 @@ bool IsOnThreadForGroup(syncer::ModelType type, syncer::ModelSafeGroup group) { } // namespace SyncBackendRegistrar::SyncBackendRegistrar( - const std::string& name, Profile* profile, - base::MessageLoop* sync_loop) : + const std::string& name, + Profile* profile, + scoped_ptr<base::Thread> sync_thread) : name_(name), - profile_(profile), - sync_loop_(sync_loop), - ui_worker_(new UIModelWorker(this)), - stopped_on_ui_thread_(false) { + profile_(profile) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); CHECK(profile_); - DCHECK(sync_loop_); + + sync_thread_ = sync_thread.Pass(); + if (!sync_thread_) { + sync_thread_.reset(new base::Thread("Chrome_SyncThread")); + CHECK(sync_thread_->Start()); + } + workers_[syncer::GROUP_DB] = new DatabaseModelWorker(this); + workers_[syncer::GROUP_DB]->RegisterForLoopDestruction(); + workers_[syncer::GROUP_FILE] = new FileModelWorker(this); - workers_[syncer::GROUP_UI] = ui_worker_; - workers_[syncer::GROUP_PASSIVE] = new syncer::PassiveModelWorker(sync_loop_, - 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(); 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 = @@ -82,20 +99,17 @@ 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 emtpy. + + // This function should be called only once, shortly after construction. The + // routing info at that point is expected to be empty. DCHECK(routing_info_.empty()); // Set our initial state to reflect the current status of the sync directory. @@ -176,16 +190,13 @@ syncer::ModelTypeSet SyncBackendRegistrar::GetLastConfiguredTypes() const { return last_configured_types_; } -void SyncBackendRegistrar::StopOnUIThread() { +void SyncBackendRegistrar::RequestWorkerStopOnUIThread() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - 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(); + base::AutoLock lock(lock_); + for (WorkerMap::const_iterator it = workers_.begin(); + it != workers_.end(); ++it) { + it->second->RequestStop(); + } } void SyncBackendRegistrar::ActivateDataType( @@ -193,6 +204,8 @@ 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. @@ -213,6 +226,8 @@ 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_); @@ -282,8 +297,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 @@ -304,8 +319,58 @@ bool SyncBackendRegistrar::IsCurrentThreadSafeForModel( GetGroupForModelType(model_type, routing_info_)); } +SyncBackendRegistrar::~SyncBackendRegistrar() { + DCHECK(workers_.empty()); +} + void SyncBackendRegistrar::OnWorkerLoopDestroyed(syncer::ModelSafeGroup group) { - // Do nothing for now. + 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(); } } // namespace browser_sync diff --git a/chrome/browser/sync/glue/sync_backend_registrar.h b/chrome/browser/sync/glue/sync_backend_registrar.h index f7ea1fb..efe07e2 100644 --- a/chrome/browser/sync/glue/sync_backend_registrar.h +++ b/chrome/browser/sync/glue/sync_backend_registrar.h @@ -12,6 +12,7 @@ #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" @@ -41,19 +42,19 @@ class SyncBackendRegistrar : public syncer::SyncManager::ChangeDelegate, // |sync_loop|. Must be created on the UI thread. SyncBackendRegistrar(const std::string& name, Profile* profile, - base::MessageLoop* sync_loop); + scoped_ptr<base::Thread> sync_thread); // SyncBackendRegistrar must be destroyed as follows: // - // 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). + // 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. virtual ~SyncBackendRegistrar(); // Informs the SyncBackendRegistrar of the currently enabled set of types. @@ -80,10 +81,7 @@ class SyncBackendRegistrar : public syncer::SyncManager::ChangeDelegate, syncer::ModelTypeSet GetLastConfiguredTypes() const; // Must be called from the UI thread. (See destructor comment.) - void StopOnUIThread(); - - // Must be called from the sync thread. (See destructor comment.) - void OnSyncerShutdownComplete(); + void RequestWorkerStopOnUIThread(); // Activates the given data type (which should belong to the given // group) and starts the given change processor. Must be called @@ -117,9 +115,25 @@ 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; + 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); // Returns the change processor for the given model, or NULL if none // exists. Must be called from |group|'s native thread. @@ -140,12 +154,6 @@ 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_; @@ -163,12 +171,21 @@ class SyncBackendRegistrar : public syncer::SyncManager::ChangeDelegate, syncer::ModelSafeRoutingInfo routing_info_; // The change processors that handle the different data types. - std::map<syncer::ModelType, ChangeProcessor*> processors_; + ProcessorMap 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_; + // Parks stopped workers because they may still be referenced by syncer. + std::vector<scoped_refptr<syncer::ModelSafeWorker> > stopped_workers_; + + // Declare |sync_thread_| at the end so that it will be destroyed before + // objects above because tasks on sync thread depend on those objects, + // e.g. Shutdown() depends on |lock_|, SyncManager::Init() depends on + // workers, etc. + scoped_ptr<base::Thread> sync_thread_; + 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 979f9a5..eed132c 100644 --- a/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc +++ b/chrome/browser/sync/glue/sync_backend_registrar_unittest.cc @@ -34,18 +34,59 @@ 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() : ui_thread_(BrowserThread::UI, &loop_) {} + SyncBackendRegistrarTest() : + sync_thread_(NULL), + ui_thread_(BrowserThread::UI, &ui_loop_), + db_thread_(BrowserThread::DB), + file_thread_(BrowserThread::FILE), + io_thread_(BrowserThread::IO) {} 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( @@ -61,42 +102,41 @@ 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 loop_; + base::MessageLoop ui_loop_; syncer::TestUserShare test_user_share_; + TestingProfile profile_; + scoped_ptr<SyncBackendRegistrar> registrar_; - private: + base::Thread* sync_thread_; content::TestBrowserThread ui_thread_; + content::TestBrowserThread db_thread_; + content::TestBrowserThread file_thread_; + content::TestBrowserThread io_thread_; }; TEST_F(SyncBackendRegistrarTest, ConstructorEmpty) { - TestingProfile profile; - SyncBackendRegistrar registrar("test", &profile, &loop_); - registrar.SetInitialTypes(ModelTypeSet()); - EXPECT_FALSE(registrar.IsNigoriEnabled()); + registrar_->SetInitialTypes(ModelTypeSet()); + EXPECT_FALSE(registrar_->IsNigoriEnabled()); { std::vector<syncer::ModelSafeWorker*> workers; - registrar.GetWorkers(&workers); + registrar_->GetWorkers(&workers); EXPECT_EQ(4u, workers.size()); } - ExpectRoutingInfo(®istrar, syncer::ModelSafeRoutingInfo()); - ExpectHasProcessorsForTypes(registrar, ModelTypeSet()); - registrar.OnSyncerShutdownComplete(); - registrar.StopOnUIThread(); + ExpectRoutingInfo(registrar_.get(), syncer::ModelSafeRoutingInfo()); + ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet()); } TEST_F(SyncBackendRegistrarTest, ConstructorNonEmpty) { - TestingProfile profile; const ModelTypeSet initial_types(BOOKMARKS, NIGORI, PASSWORDS); - SyncBackendRegistrar registrar("test", &profile, &loop_); - registrar.SetInitialTypes(initial_types); - EXPECT_TRUE(registrar.IsNigoriEnabled()); + registrar_->SetInitialTypes(initial_types); + EXPECT_TRUE(registrar_->IsNigoriEnabled()); { std::vector<syncer::ModelSafeWorker*> workers; - registrar.GetWorkers(&workers); + registrar_->GetWorkers(&workers); EXPECT_EQ(4u, workers.size()); } { @@ -104,71 +144,56 @@ 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(®istrar, expected_routing_info); + ExpectRoutingInfo(registrar_.get(), expected_routing_info); } - ExpectHasProcessorsForTypes(registrar, ModelTypeSet()); - registrar.OnSyncerShutdownComplete(); - registrar.StopOnUIThread(); + ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet()); } TEST_F(SyncBackendRegistrarTest, ConfigureDataTypes) { - TestingProfile profile; - SyncBackendRegistrar registrar("test", &profile, &loop_); - registrar.SetInitialTypes(ModelTypeSet()); + 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(®istrar, expected_routing_info); + ExpectRoutingInfo(registrar_.get(), 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(®istrar, expected_routing_info); + ExpectRoutingInfo(registrar_.get(), 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(®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); + EXPECT_TRUE(registrar_->ConfigureDataTypes(ModelTypeSet(), types2).Empty()); + ExpectRoutingInfo(registrar_.get(), syncer::ModelSafeRoutingInfo()); + ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet()); + EXPECT_TRUE(ModelTypeSet().Equals(registrar_->GetLastConfiguredTypes())); } TEST_F(SyncBackendRegistrarTest, ActivateDeactivateUIDataType) { InSequence in_sequence; - TestingProfile profile; - SyncBackendRegistrar registrar("test", &profile, &loop_); - registrar.SetInitialTypes(ModelTypeSet()); + registrar_->SetInitialTypes(ModelTypeSet()); // Should do nothing. - TriggerChanges(®istrar, BOOKMARKS); + TriggerChanges(registrar_.get(), 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, _, _)); @@ -180,42 +205,40 @@ 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(®istrar, expected_routing_info); + ExpectRoutingInfo(registrar_.get(), expected_routing_info); } - ExpectHasProcessorsForTypes(registrar, types); + ExpectHasProcessorsForTypes(*registrar_, types); - TriggerChanges(®istrar, BOOKMARKS); + TriggerChanges(registrar_.get(), BOOKMARKS); - registrar.DeactivateDataType(BOOKMARKS); - ExpectRoutingInfo(®istrar, syncer::ModelSafeRoutingInfo()); - ExpectHasProcessorsForTypes(registrar, ModelTypeSet()); + registrar_->DeactivateDataType(BOOKMARKS); + ExpectRoutingInfo(registrar_.get(), syncer::ModelSafeRoutingInfo()); + ExpectHasProcessorsForTypes(*registrar_, ModelTypeSet()); // Should do nothing. - TriggerChanges(®istrar, BOOKMARKS); + TriggerChanges(registrar_.get(), BOOKMARKS); +} - registrar.OnSyncerShutdownComplete(); - registrar.StopOnUIThread(); +void ActiviateDoneOnDb(base::WaitableEvent* done) { + done->Signal(); } TEST_F(SyncBackendRegistrarTest, ActivateDeactivateNonUIDataType) { - content::TestBrowserThread db_thread(BrowserThread::DB, &loop_); InSequence in_sequence; - TestingProfile profile; - SyncBackendRegistrar registrar("test", &profile, &loop_); - registrar.SetInitialTypes(ModelTypeSet()); + registrar_->SetInitialTypes(ModelTypeSet()); // Should do nothing. - TriggerChanges(®istrar, AUTOFILL); + TriggerChanges(registrar_.get(), 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, _, _)); @@ -227,28 +250,24 @@ TEST_F(SyncBackendRegistrarTest, ActivateDeactivateNonUIDataType) { const ModelTypeSet types(AUTOFILL); EXPECT_TRUE( - 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()); + 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()); // Should do nothing. - TriggerChanges(®istrar, AUTOFILL); - - registrar.OnSyncerShutdownComplete(); - registrar.StopOnUIThread(); + TriggerChanges(registrar_.get(), AUTOFILL); } } // namespace diff --git a/chrome/browser/sync/glue/typed_url_change_processor.cc b/chrome/browser/sync/glue/typed_url_change_processor.cc index 1a86800..ca97e32 100644 --- a/chrome/browser/sync/glue/typed_url_change_processor.cc +++ b/chrome/browser/sync/glue/typed_url_change_processor.cc @@ -43,7 +43,8 @@ TypedUrlChangeProcessor::TypedUrlChangeProcessor( profile_(profile), model_associator_(model_associator), history_backend_(history_backend), - expected_loop_(base::MessageLoop::current()) { + backend_loop_(base::MessageLoop::current()), + disconnected_(false) { DCHECK(model_associator); DCHECK(history_backend); DCHECK(error_handler); @@ -55,14 +56,18 @@ TypedUrlChangeProcessor::TypedUrlChangeProcessor( } TypedUrlChangeProcessor::~TypedUrlChangeProcessor() { - DCHECK(expected_loop_ == base::MessageLoop::current()); + DCHECK(backend_loop_ == base::MessageLoop::current()); } void TypedUrlChangeProcessor::Observe( int type, const content::NotificationSource& source, const content::NotificationDetails& details) { - DCHECK(expected_loop_ == base::MessageLoop::current()); + DCHECK(backend_loop_ == base::MessageLoop::current()); + + base::AutoLock al(disconnect_lock_); + if (disconnected_) + return; DVLOG(1) << "Observed typed_url change."; if (type == chrome::NOTIFICATION_HISTORY_URLS_MODIFIED) { @@ -239,7 +244,11 @@ void TypedUrlChangeProcessor::ApplyChangesFromSyncModel( const syncer::BaseTransaction* trans, int64 model_version, const syncer::ImmutableChangeRecordList& changes) { - DCHECK(expected_loop_ == base::MessageLoop::current()); + DCHECK(backend_loop_ == base::MessageLoop::current()); + + base::AutoLock al(disconnect_lock_); + if (disconnected_) + return; syncer::ReadNode typed_url_root(trans); if (typed_url_root.InitByTagLookup(kTypedUrlTag) != @@ -295,7 +304,11 @@ void TypedUrlChangeProcessor::ApplyChangesFromSyncModel( } void TypedUrlChangeProcessor::CommitChangesFromSyncModel() { - DCHECK(expected_loop_ == base::MessageLoop::current()); + DCHECK(backend_loop_ == base::MessageLoop::current()); + + base::AutoLock al(disconnect_lock_); + if (disconnected_) + return; // 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. @@ -317,14 +330,23 @@ void TypedUrlChangeProcessor::CommitChangesFromSyncModel() { model_associator_->GetErrorPercentage()); } +void TypedUrlChangeProcessor::Disconnect() { + base::AutoLock al(disconnect_lock_); + disconnected_ = true; +} + void TypedUrlChangeProcessor::StartImpl(Profile* profile) { - DCHECK(expected_loop_ == base::MessageLoop::current()); + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK_EQ(profile, profile_); - StartObserving(); + DCHECK(history_backend_); + DCHECK(backend_loop_); + backend_loop_->PostTask(FROM_HERE, + base::Bind(&TypedUrlChangeProcessor::StartObserving, + base::Unretained(this))); } void TypedUrlChangeProcessor::StartObserving() { - DCHECK(expected_loop_ == base::MessageLoop::current()); + DCHECK(backend_loop_ == base::MessageLoop::current()); DCHECK(profile_); notification_registrar_.Add( this, chrome::NOTIFICATION_HISTORY_URLS_MODIFIED, @@ -338,7 +360,7 @@ void TypedUrlChangeProcessor::StartObserving() { } void TypedUrlChangeProcessor::StopObserving() { - DCHECK(expected_loop_ == base::MessageLoop::current()); + DCHECK(backend_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 823c45c..a5e447e 100644 --- a/chrome/browser/sync/glue/typed_url_change_processor.h +++ b/chrome/browser/sync/glue/typed_url_change_processor.h @@ -67,6 +67,9 @@ 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; @@ -101,11 +104,10 @@ 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 @@ -116,6 +118,9 @@ 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 6a5d4a7..fe7d137 100644 --- a/chrome/browser/sync/glue/typed_url_data_type_controller.cc +++ b/chrome/browser/sync/glue/typed_url_data_type_controller.cc @@ -13,6 +13,7 @@ #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" @@ -125,23 +126,20 @@ bool TypedUrlDataTypeController::PostTaskOnBackendThread( } } -void TypedUrlDataTypeController::CreateSyncComponents() { +ProfileSyncComponentsFactory::SyncComponents +TypedUrlDataTypeController::CreateSyncComponents() { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK_EQ(state(), ASSOCIATING); DCHECK(backend_); - 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); + return profile_sync_factory()->CreateTypedUrlSyncComponents( + profile_sync_service(), + backend_, + this); } -void TypedUrlDataTypeController::StopModels() { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - DCHECK(state() == STOPPING || state() == NOT_RUNNING || state() == DISABLED); - DVLOG(1) << "TypedUrlDataTypeController::StopModels(): State = " << state(); +void TypedUrlDataTypeController::DisconnectProcessor( + ChangeProcessor* processor) { + static_cast<TypedUrlChangeProcessor*>(processor)->Disconnect(); } 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 9afd3e0..95b087b 100644 --- a/chrome/browser/sync/glue/typed_url_data_type_controller.h +++ b/chrome/browser/sync/glue/typed_url_data_type_controller.h @@ -44,8 +44,9 @@ class TypedUrlDataTypeController : public NonFrontendDataTypeController { virtual bool PostTaskOnBackendThread( const tracked_objects::Location& from_here, const base::Closure& task) OVERRIDE; - virtual void CreateSyncComponents() OVERRIDE; - virtual void StopModels() OVERRIDE; + virtual ProfileSyncComponentsFactory::SyncComponents CreateSyncComponents() + OVERRIDE; + virtual void DisconnectProcessor(ChangeProcessor* processor) 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 89f0e33..b4cabc0 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()), - pending_abort_(false), + abort_requested_(false), error_handler_(error_handler), num_db_accesses_(0), num_db_errors_(0) { @@ -173,43 +173,50 @@ 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_; - 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; - } - } + bool query_succeeded = + history_backend_ && history_backend_->GetAllTypedURLs(&typed_urls); 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) != @@ -224,8 +231,6 @@ 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()); @@ -311,8 +316,6 @@ 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) { @@ -372,8 +375,6 @@ 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( @@ -392,7 +393,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 error; + return syncer::SyncError(); } void TypedUrlModelAssociator::UpdateFromSyncDB( @@ -480,13 +481,8 @@ syncer::SyncError TypedUrlModelAssociator::DisassociateModels() { } void TypedUrlModelAssociator::AbortAssociation() { - base::AutoLock lock(pending_abort_lock_); - pending_abort_ = true; -} - -bool TypedUrlModelAssociator::IsAbortPending() { - base::AutoLock lock(pending_abort_lock_); - return pending_abort_; + base::AutoLock lock(abort_lock_); + abort_requested_ = true; } 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 269ec03..5f9cd38 100644 --- a/chrome/browser/sync/glue/typed_url_model_associator.h +++ b/chrome/browser/sync/glue/typed_url_model_associator.h @@ -170,9 +170,6 @@ 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. @@ -192,11 +189,8 @@ class TypedUrlModelAssociator : public AssociatorInterface { base::MessageLoop* expected_loop_; - // 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_; + bool abort_requested_; + base::Lock abort_lock_; 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 39d3de2..c777163 100644 --- a/chrome/browser/sync/glue/typed_url_model_associator_unittest.cc +++ b/chrome/browser/sync/glue/typed_url_model_associator_unittest.cc @@ -67,36 +67,27 @@ class SyncTypedUrlModelAssociatorTest : public testing::Test { class TestTypedUrlModelAssociator : public TypedUrlModelAssociator { public: - 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(); - } + TestTypedUrlModelAssociator() + : TypedUrlModelAssociator(&mock_, NULL, NULL) {} private: ProfileSyncServiceMock mock_; - base::WaitableEvent* startup_; - base::WaitableEvent* aborted_; }; -static void CreateModelAssociator(base::WaitableEvent* startup, - base::WaitableEvent* aborted, - base::WaitableEvent* done, - TypedUrlModelAssociator** associator) { +static void CreateModelAssociatorAsync(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(startup, aborted); - // AssociateModels should be aborted and should return false. - syncer::SyncError error = (*associator)->AssociateModels(NULL, NULL); + *associator = new TestTypedUrlModelAssociator(); - // TODO(lipalani): crbug.com/122690 fix this when fixing abort. - // EXPECT_TRUE(error.IsSet()); + // Signal frontend to call AbortAssociation and proceed after it's called. + startup->Signal(); + aborted->Wait(); + syncer::SyncError error = (*associator)->AssociateModels(NULL, NULL); + EXPECT_TRUE(error.IsSet()); + EXPECT_EQ("datatype error was encountered: Association was aborted.", + error.message()); delete *associator; done->Signal(); } @@ -433,7 +424,7 @@ TEST_F(SyncTypedUrlModelAssociatorTest, TestAbort) { // model association. db_thread.Start(); base::Closure callback = base::Bind( - &CreateModelAssociator, &startup, &aborted, &done, &associator); + &CreateModelAssociatorAsync, &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 c58d358..2d8bc7d 100644 --- a/chrome/browser/sync/glue/ui_model_worker.cc +++ b/chrome/browser/sync/glue/ui_model_worker.cc @@ -21,7 +21,6 @@ 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 @@ -37,60 +36,23 @@ 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), - 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; + : syncer::ModelSafeWorker(observer) { } 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 " @@ -98,25 +60,16 @@ syncer::SyncerError UIModelWorker::DoWorkAndWaitUntilDoneImpl( return work.Run(); } - { - // 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; - } + 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; } - syncapi_event_.Signal(); // Notify that the syncapi produced work for us. work_done_or_stopped()->Wait(); + return error_info; } @@ -124,21 +77,7 @@ 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 8f71433..02b8cd3 100644 --- a/chrome/browser/sync/glue/ui_model_worker.h +++ b/chrome/browser/sync/glue/ui_model_worker.h @@ -17,86 +17,21 @@ 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 dba85e0..4d4ce74 100644 --- a/chrome/browser/sync/glue/ui_model_worker_unittest.cc +++ b/chrome/browser/sync/glue/ui_model_worker_unittest.cc @@ -18,8 +18,6 @@ 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, @@ -59,27 +57,6 @@ 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"), @@ -117,96 +94,5 @@ 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 c045d86..e74ac15 100644 --- a/chrome/browser/sync/profile_sync_service.cc +++ b/chrome/browser/sync/profile_sync_service.cc @@ -472,18 +472,21 @@ void ProfileSyncService::InitializeBackend(bool delete_stale_data) { if (delete_stale_data) ClearStaleErrors(); - backend_unrecoverable_error_handler_.reset( - new browser_sync::BackendUnrecoverableErrorHandler( - MakeWeakHandle(weak_factory_.GetWeakPtr()))); + scoped_ptr<syncer::UnrecoverableErrorHandler> + backend_unrecoverable_error_handler( + new browser_sync::BackendUnrecoverableErrorHandler( + MakeWeakHandle(weak_factory_.GetWeakPtr()))); backend_->Initialize( this, + sync_thread_.Pass(), MakeWeakHandle(sync_js_controller_.AsWeakPtr()), sync_service_url_, credentials, delete_stale_data, - &sync_manager_factory_, - backend_unrecoverable_error_handler_.get(), + scoped_ptr<syncer::SyncManagerFactory>( + new syncer::SyncManagerFactory).Pass(), + backend_unrecoverable_error_handler.Pass(), &browser_sync::ChromeReportUnrecoverableError); } @@ -721,18 +724,21 @@ void ProfileSyncService::Shutdown() { if (profile_) SigninGlobalError::GetForProfile(profile_)->RemoveProvider(this); - ShutdownImpl(false); + ShutdownImpl(browser_sync::SyncBackendHost::STOP); + + if (sync_thread_) + sync_thread_->Stop(); } -void ProfileSyncService::ShutdownImpl(bool sync_disabled) { - // 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. +void ProfileSyncService::ShutdownImpl( + browser_sync::SyncBackendHost::ShutdownOption option) { + if (!backend_) + return; + + // First, we spin down the backend to stop change processing as soon as + // possible. base::Time shutdown_start_time = base::Time::Now(); - if (backend_) { - backend_->StopSyncingForShutdown(); - } + 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 @@ -758,8 +764,7 @@ void ProfileSyncService::ShutdownImpl(bool sync_disabled) { // shutting it down. scoped_ptr<SyncBackendHost> doomed_backend(backend_.release()); if (doomed_backend) { - doomed_backend->Shutdown(sync_disabled); - + sync_thread_ = doomed_backend->Shutdown(option); doomed_backend.reset(); } base::TimeDelta shutdown_time = base::Time::Now() - shutdown_start_time; @@ -797,7 +802,7 @@ void ProfileSyncService::DisableForUser() { // PSS clients don't think we're set up while we're shutting down. sync_prefs_.ClearPreferences(); ClearUnrecoverableError(); - ShutdownImpl(true); + ShutdownImpl(browser_sync::SyncBackendHost::DISABLE_AND_CLAIM_THREAD); } bool ProfileSyncService::HasSyncSetupCompleted() const { @@ -878,8 +883,11 @@ void ProfileSyncService::OnUnrecoverableErrorImpl( // Shut all data types down. base::MessageLoop::current()->PostTask(FROM_HERE, - base::Bind(&ProfileSyncService::ShutdownImpl, weak_factory_.GetWeakPtr(), - delete_sync_database)); + base::Bind(&ProfileSyncService::ShutdownImpl, + weak_factory_.GetWeakPtr(), + delete_sync_database ? + browser_sync::SyncBackendHost::DISABLE_AND_CLAIM_THREAD : + browser_sync::SyncBackendHost::STOP_AND_CLAIM_THREAD)); } // TODO(zea): Move this logic into the DataTypeController/DataTypeManager. @@ -1242,7 +1250,7 @@ void ProfileSyncService::OnActionableError(const SyncProtocolError& error) { // Sync disabled by domain admin. we should stop syncing until next // restart. sync_disabled_by_admin_ = true; - ShutdownImpl(true); + ShutdownImpl(browser_sync::SyncBackendHost::DISABLE_AND_CLAIM_THREAD); break; default: NOTREACHED(); @@ -2021,7 +2029,7 @@ void ProfileSyncService::StopAndSuppress() { if (backend_) { backend_->UnregisterInvalidationIds(); } - ShutdownImpl(false); + ShutdownImpl(browser_sync::SyncBackendHost::STOP_AND_CLAIM_THREAD); } bool ProfileSyncService::IsStartSuppressed() const { diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h index 89edb3b..4921a78 100644 --- a/chrome/browser/sync/profile_sync_service.h +++ b/chrome/browser/sync/profile_sync_service.h @@ -632,8 +632,9 @@ class ProfileSyncService : public ProfileSyncServiceBase, void ConfigureDataTypeManager(); // Shuts down the backend sync components. - // |sync_disabled| indicates if syncing is being disabled or not. - void ShutdownImpl(bool sync_disabled); + // |option| indicates if syncing is being disabled or not, and whether + // to claim ownership of sync thread from backend. + void ShutdownImpl(browser_sync::SyncBackendHost::ShutdownOption option); // Return SyncCredentials from the TokenService. syncer::SyncCredentials GetCredentials(); @@ -900,9 +901,6 @@ class ProfileSyncService : public ProfileSyncServiceBase, // or must delay loading for some reason). browser_sync::FailedDataTypesHandler failed_data_types_handler_; - scoped_ptr<browser_sync::BackendUnrecoverableErrorHandler> - backend_unrecoverable_error_handler_; - browser_sync::DataTypeManager::ConfigureStatus configure_status_; // If |true|, there is setup UI visible so we should not start downloading @@ -912,13 +910,17 @@ class ProfileSyncService : public ProfileSyncServiceBase, // The set of currently enabled sync experiments. syncer::Experiments current_experiments_; - // Factory the backend will use to build the SyncManager. - syncer::SyncManagerFactory sync_manager_factory_; - // Sync's internal debug info listener. Used to record datatype configuration // 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_typed_url_unittest.cc b/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc index 2738c10..23c6edd 100644 --- a/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc @@ -97,6 +97,11 @@ 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() {} @@ -135,6 +140,11 @@ ACTION_P2(RunTaskOnDBThread, thread, backend) { task)); } +ACTION_P2(ShutdownHistoryService, thread, service) { + service->ShutdownBaseService(); + delete thread; +} + ACTION_P6(MakeTypedUrlSyncComponents, profile, service, @@ -168,8 +178,8 @@ class ProfileSyncServiceTypedUrlTest : public AbstractProfileSyncServiceTest { } protected: - ProfileSyncServiceTypedUrlTest() - : history_thread_("history") { + ProfileSyncServiceTypedUrlTest() { + history_thread_.reset(new Thread("history")); } virtual void SetUp() { @@ -182,17 +192,15 @@ class ProfileSyncServiceTypedUrlTest : public AbstractProfileSyncServiceTest { HistoryServiceFactory::GetInstance()->SetTestingFactoryAndUse( profile_.get(), BuildHistoryService)); EXPECT_CALL((*history_service_), ScheduleDBTask(_, _)) - .WillRepeatedly(RunTaskOnDBThread(&history_thread_, + .WillRepeatedly(RunTaskOnDBThread(history_thread_.get(), history_backend_.get())); - history_thread_.Start(); + history_thread_->Start(); } virtual void TearDown() { - history_backend_ = NULL; - history_service_ = NULL; - ProfileSyncServiceFactory::GetInstance()->SetTestingFactory( - profile_.get(), NULL); - history_thread_.Stop(); + EXPECT_CALL((*history_service_), Shutdown()) + .WillOnce(ShutdownHistoryService(history_thread_.release(), + history_service_)); profile_.reset(); AbstractProfileSyncServiceTest::TearDown(); } @@ -312,7 +320,7 @@ class ProfileSyncServiceTypedUrlTest : public AbstractProfileSyncServiceTest { return history_url; } - Thread history_thread_; + scoped_ptr<Thread> history_thread_; scoped_ptr<ProfileMock> profile_; scoped_refptr<HistoryBackendMock> history_backend_; @@ -562,7 +570,8 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeAdd) { history::URLsModifiedDetails details; details.changed_urls.push_back(added_entry); - scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); + scoped_refptr<ThreadNotifier> notifier( + new ThreadNotifier(history_thread_.get())); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED, content::Source<Profile>(profile_.get()), content::Details<history::URLsModifiedDetails>(&details)); @@ -592,7 +601,8 @@ 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_)); + scoped_refptr<ThreadNotifier> notifier( + new ThreadNotifier(history_thread_.get())); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED, content::Source<Profile>(profile_.get()), content::Details<history::URLsModifiedDetails>(&details)); @@ -629,7 +639,8 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeUpdate) { history::URLsModifiedDetails details; details.changed_urls.push_back(updated_entry); - scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); + scoped_refptr<ThreadNotifier> notifier( + new ThreadNotifier(history_thread_.get())); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED, content::Source<Profile>(profile_.get()), content::Details<history::URLsModifiedDetails>(&details)); @@ -657,7 +668,8 @@ 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_)); + scoped_refptr<ThreadNotifier> notifier( + new ThreadNotifier(history_thread_.get())); notifier->Notify(chrome::NOTIFICATION_HISTORY_URL_VISITED, content::Source<Profile>(profile_.get()), content::Details<history::URLVisitedDetails>(&details)); @@ -695,7 +707,8 @@ 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_)); + scoped_refptr<ThreadNotifier> notifier( + new ThreadNotifier(history_thread_.get())); notifier->Notify(chrome::NOTIFICATION_HISTORY_URL_VISITED, content::Source<Profile>(profile_.get()), content::Details<history::URLVisitedDetails>(&details)); @@ -735,7 +748,8 @@ 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_)); + scoped_refptr<ThreadNotifier> notifier( + new ThreadNotifier(history_thread_.get())); notifier->Notify(chrome::NOTIFICATION_HISTORY_URL_VISITED, content::Source<Profile>(profile_.get()), content::Details<history::URLVisitedDetails>(&details)); @@ -801,7 +815,8 @@ 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_)); + scoped_refptr<ThreadNotifier> notifier( + new ThreadNotifier(history_thread_.get())); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_DELETED, content::Source<Profile>(profile_.get()), content::Details<history::URLsDeletedDetails>(&changes)); @@ -839,7 +854,8 @@ 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_)); + scoped_refptr<ThreadNotifier> notifier( + new ThreadNotifier(history_thread_.get())); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_DELETED, content::Source<Profile>(profile_.get()), content::Details<history::URLsDeletedDetails>(&changes)); @@ -878,7 +894,8 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeRemoveAll) { history::URLsDeletedDetails changes; changes.all_history = true; - scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_)); + scoped_refptr<ThreadNotifier> notifier( + new ThreadNotifier(history_thread_.get())); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_DELETED, content::Source<Profile>(profile_.get()), content::Details<history::URLsDeletedDetails>(&changes)); @@ -957,6 +974,7 @@ 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. @@ -995,7 +1013,8 @@ 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_)); + scoped_refptr<ThreadNotifier> notifier( + new ThreadNotifier(history_thread_.get())); notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED, content::Source<Profile>(profile_.get()), content::Details<history::URLsModifiedDetails>(&details)); @@ -1048,7 +1067,8 @@ 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_)); + scoped_refptr<ThreadNotifier> notifier( + new ThreadNotifier(history_thread_.get())); 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 9b22da9..ff213c2 100644 --- a/chrome/browser/sync/profile_sync_service_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_unittest.cc @@ -46,6 +46,10 @@ using testing::Mock; using testing::Return; using testing::StrictMock; +void SignalDone(base::WaitableEvent* done) { + done->Signal(); +} + class ProfileSyncServiceTestHarness { public: ProfileSyncServiceTestHarness() @@ -120,6 +124,21 @@ 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(); + base::RunLoop().RunUntilIdle(); + if (service->sync_initialized()) { + return; + } + } + LOG(ERROR) << "Backend not initialized."; + } + void IssueTestTokens() { TokenService* token_service = TokenServiceFactory::GetForProfile(profile.get()); @@ -338,6 +357,7 @@ TEST_F(ProfileSyncServiceTest, TEST_F(ProfileSyncServiceTest, JsControllerProcessJsMessageBasic) { harness_.StartSyncService(); + harness_.WaitForBackendInitDone(); StrictMock<syncer::MockJsReplyHandler> reply_handler; @@ -355,6 +375,11 @@ 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(); } @@ -379,9 +404,14 @@ TEST_F(ProfileSyncServiceTest, } harness_.IssueTestTokens(); + harness_.WaitForBackendInitDone(); // This forces the sync thread to process the message and reply. - harness_.TearDown(); + base::WaitableEvent done(false, false); + harness_.service->GetBackendForTest()->GetSyncLoopForTesting() + ->PostTask(FROM_HERE, + base::Bind(&SignalDone, &done)); + done.Wait(); harness_.TearDown(); } // Make sure that things still work if sync is not enabled, but some old sync diff --git a/chrome/browser/sync/test/integration/sync_test.cc b/chrome/browser/sync/test/integration/sync_test.cc index 061fcad..ca447bc 100644 --- a/chrome/browser/sync/test/integration/sync_test.cc +++ b/chrome/browser/sync/test/integration/sync_test.cc @@ -363,6 +363,10 @@ bool SyncTest::SetupSync() { } void SyncTest::CleanUpOnMainThread() { + for (size_t i = 0; i < clients_.size(); ++i) { + clients_[i]->service()->DisableForUser(); + } + // Some of the pending messages might rely on browser windows still being // around, so run messages both before and after closing all browsers. content::RunAllPendingInMessageLoop(); diff --git a/chrome/browser/sync/test_profile_sync_service.cc b/chrome/browser/sync/test_profile_sync_service.cc index 499d2be..f69252f 100644 --- a/chrome/browser/sync/test_profile_sync_service.cc +++ b/chrome/browser/sync/test_profile_sync_service.cc @@ -64,26 +64,23 @@ scoped_ptr<syncer::HttpPostProviderFactory> MakeTestHttpBridgeFactory() { } // namespace void SyncBackendHostForProfileSyncTest::InitCore( - const DoInitializeOptions& options) { - DoInitializeOptions test_options = options; - test_options.make_http_bridge_factory_fn = + scoped_ptr<DoInitializeOptions> options) { + options->make_http_bridge_factory_fn = base::Bind(&MakeTestHttpBridgeFactory); - test_options.credentials.email = "testuser@gmail.com"; - test_options.credentials.sync_token = "token"; - test_options.restored_key_for_bootstrapping = ""; + options->credentials.email = "testuser@gmail.com"; + options->credentials.sync_token = "token"; + options->restored_key_for_bootstrapping = ""; syncer::StorageOption storage = storage_option_; // It'd be nice if we avoided creating the InternalComponentsFactory in the // first place, but SyncBackendHost will have created one by now so we must // free it. Grab the switches to pass on first. InternalComponentsFactory::Switches factory_switches = - test_options.internal_components_factory->GetSwitches(); - delete test_options.internal_components_factory; + options->internal_components_factory->GetSwitches(); + options->internal_components_factory.reset( + new TestInternalComponentsFactory(factory_switches, storage)); - test_options.internal_components_factory = - new TestInternalComponentsFactory(factory_switches, storage); - - SyncBackendHost::InitCore(test_options); + SyncBackendHost::InitCore(options.Pass()); if (synchronous_init_ && !base::MessageLoop::current()->is_running()) { // The SyncBackend posts a task to the current loop when // initialization completes. diff --git a/chrome/browser/sync/test_profile_sync_service.h b/chrome/browser/sync/test_profile_sync_service.h index 3322258..9121fd5 100644 --- a/chrome/browser/sync/test_profile_sync_service.h +++ b/chrome/browser/sync/test_profile_sync_service.h @@ -80,7 +80,7 @@ class SyncBackendHostForProfileSyncTest : public SyncBackendHost { static void SetHistoryServiceExpectations(ProfileMock* profile); protected: - virtual void InitCore(const DoInitializeOptions& options) OVERRIDE; + virtual void InitCore(scoped_ptr<DoInitializeOptions> options) OVERRIDE; private: void ContinueInitialization( diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 194f834..777d9e5 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -1989,8 +1989,6 @@ 'browser/sync/glue/change_processor.h', 'browser/sync/glue/chrome_encryptor.cc', 'browser/sync/glue/chrome_encryptor.h', - 'browser/sync/glue/chrome_extensions_activity_monitor.cc', - 'browser/sync/glue/chrome_extensions_activity_monitor.h', 'browser/sync/glue/chrome_report_unrecoverable_error.cc', 'browser/sync/glue/chrome_report_unrecoverable_error.h', 'browser/sync/glue/data_type_controller.cc', @@ -2009,6 +2007,8 @@ 'browser/sync/glue/extension_data_type_controller.h', 'browser/sync/glue/extension_setting_data_type_controller.cc', 'browser/sync/glue/extension_setting_data_type_controller.h', + 'browser/sync/glue/extensions_activity_monitor.cc', + 'browser/sync/glue/extensions_activity_monitor.h', 'browser/sync/glue/failed_data_types_handler.cc', 'browser/sync/glue/failed_data_types_handler.h', 'browser/sync/glue/favicon_cache.cc', diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 7ce6ef9..4fe0a0e 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -1189,7 +1189,6 @@ 'browser/sync/glue/change_processor_mock.cc', 'browser/sync/glue/change_processor_mock.h', 'browser/sync/glue/chrome_encryptor_unittest.cc', - 'browser/sync/glue/chrome_extensions_activity_monitor_unittest.cc', 'browser/sync/glue/data_type_controller_mock.cc', 'browser/sync/glue/data_type_controller_mock.h', 'browser/sync/glue/data_type_error_handler_mock.cc', @@ -1197,6 +1196,7 @@ 'browser/sync/glue/data_type_manager_impl_unittest.cc', 'browser/sync/glue/data_type_manager_mock.cc', 'browser/sync/glue/data_type_manager_mock.h', + 'browser/sync/glue/extensions_activity_monitor_unittest.cc', 'browser/sync/glue/fake_data_type_controller.cc', 'browser/sync/glue/fake_data_type_controller.h', 'browser/sync/glue/fake_generic_change_processor.cc', @@ -1980,7 +1980,7 @@ ['exclude', '^../extensions/'], ['exclude', '^browser/extensions/activity_log/'], ['exclude', '^browser/extensions/api/'], - ['exclude', '^browser/sync/glue/chrome_extensions_activity_monitor_unittest.cc'], + ['exclude', '^browser/sync/glue/extensions_activity_monitor_unittest.cc'], ['exclude', '^common/extensions/api/'], ['exclude', '^common/extensions/manifest_handlers/'], ['exclude', '^common/extensions/manifest_tests/'], diff --git a/sync/engine/build_commit_command.cc b/sync/engine/build_commit_command.cc index a846cfd..1bfac0f 100644 --- a/sync/engine/build_commit_command.cc +++ b/sync/engine/build_commit_command.cc @@ -42,7 +42,7 @@ BuildCommitCommand::BuildCommitCommand( syncable::BaseTransaction* trans, const sessions::OrderedCommitSet& batch_commit_set, sync_pb::ClientToServerMessage* commit_message, - ExtensionsActivityMonitor::Records* extensions_activity_buffer) + ExtensionsActivity::Records* extensions_activity_buffer) : trans_(trans), batch_commit_set_(batch_commit_set), commit_message_(commit_message), @@ -55,7 +55,7 @@ void BuildCommitCommand::AddExtensionsActivityToMessage( SyncSession* session, sync_pb::CommitMessage* message) { // We only send ExtensionsActivity to the server if bookmarks are being // committed. - ExtensionsActivityMonitor* monitor = session->context()->extensions_monitor(); + ExtensionsActivity* activity = session->context()->extensions_activity(); if (batch_commit_set_.HasBookmarkCommitId()) { // This isn't perfect, since the set of extensions activity may not // correlate exactly with the items being committed. That's OK as @@ -66,11 +66,11 @@ void BuildCommitCommand::AddExtensionsActivityToMessage( // We will push this list of extensions activity back into the // ExtensionsActivityMonitor if this commit fails. That's why we must keep // a copy of these records in the session. - monitor->GetAndClearRecords(extensions_activity_buffer_); + activity->GetAndClearRecords(extensions_activity_buffer_); - const ExtensionsActivityMonitor::Records& records = + const ExtensionsActivity::Records& records = *extensions_activity_buffer_; - for (ExtensionsActivityMonitor::Records::const_iterator it = + for (ExtensionsActivity::Records::const_iterator it = records.begin(); it != records.end(); ++it) { sync_pb::ChromiumExtensionsActivity* activity_message = diff --git a/sync/engine/build_commit_command.h b/sync/engine/build_commit_command.h index a55a0b4..405cdff 100644 --- a/sync/engine/build_commit_command.h +++ b/sync/engine/build_commit_command.h @@ -11,7 +11,7 @@ #include "sync/base/sync_export.h" #include "sync/engine/syncer_command.h" #include "sync/syncable/entry_kernel.h" -#include "sync/util/extensions_activity_monitor.h" +#include "sync/util/extensions_activity.h" namespace syncer { @@ -42,7 +42,7 @@ class SYNC_EXPORT_PRIVATE BuildCommitCommand : public SyncerCommand { syncable::BaseTransaction* trans, const sessions::OrderedCommitSet& batch_commit_set, sync_pb::ClientToServerMessage* commit_message, - ExtensionsActivityMonitor::Records* extensions_activity_buffer); + ExtensionsActivity::Records* extensions_activity_buffer); virtual ~BuildCommitCommand(); // SyncerCommand implementation. @@ -69,7 +69,7 @@ class SYNC_EXPORT_PRIVATE BuildCommitCommand : public SyncerCommand { // Output parameter; see constructor comment. sync_pb::ClientToServerMessage* commit_message_; - ExtensionsActivityMonitor::Records* extensions_activity_buffer_; + ExtensionsActivity::Records* extensions_activity_buffer_; }; } // namespace syncer diff --git a/sync/engine/commit.cc b/sync/engine/commit.cc index 5081ab3..7596d4b 100644 --- a/sync/engine/commit.cc +++ b/sync/engine/commit.cc @@ -66,7 +66,7 @@ bool PrepareCommitMessage( ModelTypeSet requested_types, sessions::OrderedCommitSet* commit_set, sync_pb::ClientToServerMessage* commit_message, - ExtensionsActivityMonitor::Records* extensions_activity_buffer) { + ExtensionsActivity::Records* extensions_activity_buffer) { TRACE_EVENT0("sync", "PrepareCommitMessage"); commit_set->Clear(); @@ -104,7 +104,7 @@ SyncerError BuildAndPostCommitsImpl(ModelTypeSet requested_types, sessions::OrderedCommitSet* commit_set) { while (!syncer->ExitRequested()) { sync_pb::ClientToServerMessage commit_message; - ExtensionsActivityMonitor::Records extensions_activity_buffer; + ExtensionsActivity::Records extensions_activity_buffer; if (!PrepareCommitMessage(session, requested_types, @@ -154,9 +154,9 @@ SyncerError BuildAndPostCommitsImpl(ModelTypeSet requested_types, // If the commit failed, return the data to the ExtensionsActivityMonitor. if (session->status_controller(). model_neutral_state().num_successful_bookmark_commits == 0) { - ExtensionsActivityMonitor* extensions_activity_monitor = - session->context()->extensions_monitor(); - extensions_activity_monitor->PutRecords(extensions_activity_buffer); + ExtensionsActivity* extensions_activity = + session->context()->extensions_activity(); + extensions_activity->PutRecords(extensions_activity_buffer); } if (processing_result != SYNCER_OK) { diff --git a/sync/engine/sync_scheduler.h b/sync/engine/sync_scheduler.h index 8eb361e..1dd1018 100644 --- a/sync/engine/sync_scheduler.h +++ b/sync/engine/sync_scheduler.h @@ -79,9 +79,7 @@ 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. - // Invokes |callback| from the sync loop once syncer is idle and all tasks - // are cancelled. - virtual void RequestStop(const base::Closure& callback) = 0; + virtual void RequestStop() = 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 3a904d1..52e1d49 100644 --- a/sync/engine/sync_scheduler_impl.cc +++ b/sync/engine/sync_scheduler_impl.cc @@ -174,7 +174,7 @@ SyncSchedulerImpl::SyncSchedulerImpl(const std::string& name, SyncSchedulerImpl::~SyncSchedulerImpl() { DCHECK(CalledOnValidThread()); - StopImpl(base::Closure()); + StopImpl(); } void SyncSchedulerImpl::OnCredentialsUpdated() { @@ -656,16 +656,15 @@ void SyncSchedulerImpl::RestartWaiting() { } } -void SyncSchedulerImpl::RequestStop(const base::Closure& callback) { +void SyncSchedulerImpl::RequestStop() { 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, - callback); + &SyncSchedulerImpl::StopImpl); } -void SyncSchedulerImpl::StopImpl(const base::Closure& callback) { +void SyncSchedulerImpl::StopImpl() { DCHECK(CalledOnValidThread()); SDVLOG(2) << "StopImpl called"; @@ -678,8 +677,6 @@ void SyncSchedulerImpl::StopImpl(const base::Closure& callback) { 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 59468e7..f0245a9 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(const base::Closure& callback) OVERRIDE; + virtual void RequestStop() 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(const base::Closure& callback); + void StopImpl(); // 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 3fb2042..a09079a 100644 --- a/sync/engine/sync_scheduler_unittest.cc +++ b/sync/engine/sync_scheduler_unittest.cc @@ -17,7 +17,7 @@ #include "sync/test/engine/fake_model_worker.h" #include "sync/test/engine/mock_connection_manager.h" #include "sync/test/engine/test_directory_setter_upper.h" -#include "sync/test/fake_extensions_activity_monitor.h" +#include "sync/util/extensions_activity.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -126,6 +126,7 @@ class SyncSchedulerTest : public testing::Test { dir_maker_.SetUp(); syncer_ = new MockSyncer(); delay_ = NULL; + extensions_activity_ = new ExtensionsActivity(); routing_info_[BOOKMARKS] = GROUP_UI; routing_info_[AUTOFILL] = GROUP_DB; @@ -147,7 +148,7 @@ class SyncSchedulerTest : public testing::Test { connection_->SetServerReachable(); context_.reset(new SyncSessionContext( connection_.get(), directory(), workers, - &extensions_activity_monitor_, + extensions_activity_.get(), std::vector<SyncEngineEventListener*>(), NULL, NULL, true, // enable keystore encryption false, // force enable pre-commit GU avoidance @@ -202,8 +203,10 @@ class SyncSchedulerTest : public testing::Test { // This stops the scheduler synchronously. void StopSyncScheduler() { - scheduler()->RequestStop(base::Bind(&SyncSchedulerTest::DoQuitLoopNow, - weak_ptr_factory_.GetWeakPtr())); + base::MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(&SyncSchedulerTest::DoQuitLoopNow, + weak_ptr_factory_.GetWeakPtr())); RunLoop(); } @@ -233,8 +236,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_; @@ -242,7 +245,7 @@ class SyncSchedulerTest : public testing::Test { MockSyncer* syncer_; MockDelayProvider* delay_; std::vector<scoped_refptr<FakeModelWorker> > workers_; - FakeExtensionsActivityMonitor extensions_activity_monitor_; + scoped_refptr<ExtensionsActivity> extensions_activity_; ModelSafeRoutingInfo routing_info_; }; diff --git a/sync/engine/syncer.h b/sync/engine/syncer.h index f2e3c73..eae6d87 100644 --- a/sync/engine/syncer.h +++ b/sync/engine/syncer.h @@ -17,7 +17,7 @@ #include "sync/engine/syncer_types.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/sessions/sync_session.h" -#include "sync/util/extensions_activity_monitor.h" +#include "sync/util/extensions_activity.h" namespace syncer { diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc index a2ccccf..db7d1a1 100644 --- a/sync/engine/syncer_unittest.cc +++ b/sync/engine/syncer_unittest.cc @@ -49,9 +49,9 @@ #include "sync/test/engine/test_id_factory.h" #include "sync/test/engine/test_syncable_utils.h" #include "sync/test/fake_encryptor.h" -#include "sync/test/fake_extensions_activity_monitor.h" #include "sync/test/fake_sync_encryption_handler.h" #include "sync/util/cryptographer.h" +#include "sync/util/extensions_activity.h" #include "sync/util/time.h" #include "testing/gtest/include/gtest/gtest.h" @@ -113,7 +113,8 @@ class SyncerTest : public testing::Test, public SyncEngineEventListener { protected: SyncerTest() - : syncer_(NULL), + : extensions_activity_(new ExtensionsActivity), + syncer_(NULL), saw_syncer_event_(false), last_client_invalidation_hint_buffer_size_(10), traffic_recorder_(0, 0) { @@ -236,7 +237,7 @@ class SyncerTest : public testing::Test, context_.reset( new SyncSessionContext( mock_server_.get(), directory(), workers, - &extensions_activity_monitor_, + extensions_activity_, listeners, NULL, &traffic_recorder_, true, // enable keystore encryption false, // force enable pre-commit GU avoidance experiment @@ -563,7 +564,7 @@ class SyncerTest : public testing::Test, TestDirectorySetterUpper dir_maker_; FakeEncryptor encryptor_; - FakeExtensionsActivityMonitor extensions_activity_monitor_; + scoped_refptr<ExtensionsActivity> extensions_activity_; scoped_ptr<MockConnectionManager> mock_server_; Syncer* syncer_; @@ -4800,18 +4801,18 @@ TEST_P(MixedResult, ExtensionsActivity) { // Put some extenions activity records into the monitor. { - ExtensionsActivityMonitor::Records records; + ExtensionsActivity::Records records; records["ABC"].extension_id = "ABC"; records["ABC"].bookmark_write_count = 2049U; records["xyz"].extension_id = "xyz"; records["xyz"].bookmark_write_count = 4U; - context_->extensions_monitor()->PutRecords(records); + context_->extensions_activity()->PutRecords(records); } SyncShareNudge(); - ExtensionsActivityMonitor::Records final_monitor_records; - context_->extensions_monitor()->GetAndClearRecords(&final_monitor_records); + ExtensionsActivity::Records final_monitor_records; + context_->extensions_activity()->GetAndClearRecords(&final_monitor_records); if (ShouldFailBookmarkCommit()) { ASSERT_EQ(2U, final_monitor_records.size()) << "Should restore records after unsuccessful bookmark commit."; diff --git a/sync/internal_api/internal_components_factory_impl.cc b/sync/internal_api/internal_components_factory_impl.cc index 1cc3b11..d5fc102 100644 --- a/sync/internal_api/internal_components_factory_impl.cc +++ b/sync/internal_api/internal_components_factory_impl.cc @@ -37,14 +37,14 @@ InternalComponentsFactoryImpl::BuildContext( ServerConnectionManager* connection_manager, syncable::Directory* directory, const std::vector<ModelSafeWorker*>& workers, - ExtensionsActivityMonitor* monitor, + ExtensionsActivity* extensions_activity, const std::vector<SyncEngineEventListener*>& listeners, sessions::DebugInfoGetter* debug_info_getter, TrafficRecorder* traffic_recorder, const std::string& invalidation_client_id) { return scoped_ptr<sessions::SyncSessionContext>( new sessions::SyncSessionContext( - connection_manager, directory, workers, monitor, + connection_manager, directory, workers, extensions_activity, listeners, debug_info_getter, traffic_recorder, switches_.encryption_method == ENCRYPTION_KEYSTORE, diff --git a/sync/internal_api/public/engine/model_safe_worker.cc b/sync/internal_api/public/engine/model_safe_worker.cc index 44e0a24..7179ed5 100644 --- a/sync/internal_api/public/engine/model_safe_worker.cc +++ b/sync/internal_api/public/engine/model_safe_worker.cc @@ -4,6 +4,7 @@ #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" @@ -84,7 +85,9 @@ std::string ModelSafeGroupToString(ModelSafeGroup group) { ModelSafeWorker::ModelSafeWorker(WorkerLoopDestructionObserver* observer) : stopped_(false), work_done_or_stopped_(false, false), - observer_(observer) {} + observer_(observer), + working_loop_(NULL), + working_loop_set_wait_(true, false) {} ModelSafeWorker::~ModelSafeWorker() {} @@ -135,4 +138,33 @@ 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 680c493..f6b7ea6 100644 --- a/sync/internal_api/public/engine/model_safe_worker.h +++ b/sync/internal_api/public/engine/model_safe_worker.h @@ -69,9 +69,16 @@ class SYNC_EXPORT ModelSafeWorker public base::MessageLoop::DestructionObserver { public: // Subclass should implement to observe destruction of the loop where - // it actually does work. + // it actually does work. Called on UI thread immediately after worker is + // created. 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); @@ -103,7 +110,14 @@ 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_; @@ -115,6 +129,11 @@ 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 9bd5a35..02036d5 100644 --- a/sync/internal_api/public/engine/passive_model_worker.cc +++ b/sync/internal_api/public/engine/passive_model_worker.cc @@ -18,7 +18,8 @@ PassiveModelWorker::~PassiveModelWorker() { } void PassiveModelWorker::RegisterForLoopDestruction() { - NOTREACHED(); + base::MessageLoop::current()->AddDestructionObserver(this); + SetWorkingLoopToCurrent(); } 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 f834c83..783731b 100644 --- a/sync/internal_api/public/engine/passive_model_worker.h +++ b/sync/internal_api/public/engine/passive_model_worker.h @@ -11,10 +11,6 @@ #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/internal_components_factory.h b/sync/internal_api/public/internal_components_factory.h index 96fd640..86509ec 100644 --- a/sync/internal_api/public/internal_components_factory.h +++ b/sync/internal_api/public/internal_components_factory.h @@ -18,7 +18,7 @@ namespace syncer { -class ExtensionsActivityMonitor; +class ExtensionsActivity; class ServerConnectionManager; class SyncEngineEventListener; class SyncScheduler; @@ -81,7 +81,7 @@ class SYNC_EXPORT InternalComponentsFactory { ServerConnectionManager* connection_manager, syncable::Directory* directory, const std::vector<ModelSafeWorker*>& workers, - ExtensionsActivityMonitor* monitor, + ExtensionsActivity* extensions_activity, const std::vector<SyncEngineEventListener*>& listeners, sessions::DebugInfoGetter* debug_info_getter, TrafficRecorder* traffic_recorder, diff --git a/sync/internal_api/public/internal_components_factory_impl.h b/sync/internal_api/public/internal_components_factory_impl.h index 6fec27f..148dd07 100644 --- a/sync/internal_api/public/internal_components_factory_impl.h +++ b/sync/internal_api/public/internal_components_factory_impl.h @@ -27,7 +27,7 @@ class SYNC_EXPORT InternalComponentsFactoryImpl ServerConnectionManager* connection_manager, syncable::Directory* directory, const std::vector<ModelSafeWorker*>& workers, - ExtensionsActivityMonitor* monitor, + ExtensionsActivity* extensions_activity, const std::vector<SyncEngineEventListener*>& listeners, sessions::DebugInfoGetter* debug_info_getter, TrafficRecorder* traffic_recorder, diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h index bc6a137..3f5316b 100644 --- a/sync/internal_api/public/sync_manager.h +++ b/sync/internal_api/public/sync_manager.h @@ -22,6 +22,7 @@ #include "sync/internal_api/public/engine/sync_status.h" #include "sync/internal_api/public/sync_encryption_handler.h" #include "sync/internal_api/public/util/report_unrecoverable_error_function.h" +#include "sync/internal_api/public/util/unrecoverable_error_handler.h" #include "sync/internal_api/public/util/weak_handle.h" #include "sync/notifier/invalidation_handler.h" #include "sync/protocol/sync_protocol_error.h" @@ -36,14 +37,13 @@ class BaseTransaction; class DataTypeDebugInfoListener; class Encryptor; struct Experiments; -class ExtensionsActivityMonitor; +class ExtensionsActivity; class HttpPostProviderFactory; class InternalComponentsFactory; class JsBackend; class JsEventHandler; class SyncEncryptionHandler; class SyncScheduler; -class UnrecoverableErrorHandler; struct UserShare; namespace sessions { @@ -310,15 +310,15 @@ class SYNC_EXPORT SyncManager : public syncer::InvalidationHandler { bool use_ssl, scoped_ptr<HttpPostProviderFactory> post_factory, const std::vector<ModelSafeWorker*>& workers, - ExtensionsActivityMonitor* extensions_activity_monitor, + ExtensionsActivity* extensions_activity, ChangeDelegate* change_delegate, const SyncCredentials& credentials, const std::string& invalidator_client_id, const std::string& restored_key_for_bootstrapping, const std::string& restored_keystore_key_for_bootstrapping, - scoped_ptr<InternalComponentsFactory> internal_components_factory, + InternalComponentsFactory* internal_components_factory, Encryptor* encryptor, - UnrecoverableErrorHandler* unrecoverable_error_handler, + scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler, ReportUnrecoverableErrorFunction report_unrecoverable_error_function, bool use_oauth2_token) = 0; @@ -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(const base::Closure& callback) = 0; + virtual void StopSyncingForShutdown() = 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 ed69a52..be06ea5 100644 --- a/sync/internal_api/public/test/fake_sync_manager.h +++ b/sync/internal_api/public/test/fake_sync_manager.h @@ -82,17 +82,16 @@ class FakeSyncManager : public SyncManager { bool use_ssl, scoped_ptr<HttpPostProviderFactory> post_factory, const std::vector<ModelSafeWorker*>& workers, - ExtensionsActivityMonitor* extensions_activity_monitor, + ExtensionsActivity* extensions_activity, ChangeDelegate* change_delegate, const SyncCredentials& credentials, const std::string& invalidator_client_id, const std::string& restored_key_for_bootstrapping, const std::string& restored_keystore_key_for_bootstrapping, - scoped_ptr<InternalComponentsFactory> internal_components_factory, + InternalComponentsFactory* internal_components_factory, Encryptor* encryptor, - UnrecoverableErrorHandler* unrecoverable_error_handler, - ReportUnrecoverableErrorFunction - report_unrecoverable_error_function, + scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler, + ReportUnrecoverableErrorFunction report_unrecoverable_error_function, bool use_oauth2_token) OVERRIDE; virtual void ThrowUnrecoverableError() OVERRIDE; virtual ModelTypeSet InitialSyncEndedTypes() OVERRIDE; @@ -115,7 +114,7 @@ class FakeSyncManager : public SyncManager { virtual void RemoveObserver(Observer* observer) OVERRIDE; virtual SyncStatus GetDetailedStatus() const OVERRIDE; virtual void SaveChanges() OVERRIDE; - virtual void StopSyncingForShutdown(const base::Closure& callback) OVERRIDE; + virtual void StopSyncingForShutdown() OVERRIDE; virtual void ShutdownOnSyncThread() OVERRIDE; virtual UserShare* GetUserShare() OVERRIDE; virtual const std::string cache_guid() OVERRIDE; diff --git a/sync/internal_api/public/test/test_internal_components_factory.h b/sync/internal_api/public/test/test_internal_components_factory.h index b608154..d846094 100644 --- a/sync/internal_api/public/test/test_internal_components_factory.h +++ b/sync/internal_api/public/test/test_internal_components_factory.h @@ -33,7 +33,7 @@ class TestInternalComponentsFactory : public InternalComponentsFactory { ServerConnectionManager* connection_manager, syncable::Directory* directory, const std::vector<ModelSafeWorker*>& workers, - ExtensionsActivityMonitor* monitor, + ExtensionsActivity* monitor, const std::vector<SyncEngineEventListener*>& listeners, sessions::DebugInfoGetter* debug_info_getter, TrafficRecorder* traffic_recorder, diff --git a/sync/internal_api/public/util/unrecoverable_error_handler.h b/sync/internal_api/public/util/unrecoverable_error_handler.h index db2f2c0..2bd2475 100644 --- a/sync/internal_api/public/util/unrecoverable_error_handler.h +++ b/sync/internal_api/public/util/unrecoverable_error_handler.h @@ -19,8 +19,7 @@ class UnrecoverableErrorHandler { // further, and will report an error status if queried. virtual void OnUnrecoverableError(const tracked_objects::Location& from_here, const std::string& message) = 0; - protected: - virtual ~UnrecoverableErrorHandler() { } + virtual ~UnrecoverableErrorHandler() {} }; } // namespace syncer diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc index 3d6158d..a541032 100644 --- a/sync/internal_api/sync_manager_impl.cc +++ b/sync/internal_api/sync_manager_impl.cc @@ -175,7 +175,6 @@ SyncManagerImpl::SyncManagerImpl(const std::string& name) invalidator_state_(DEFAULT_INVALIDATION_ERROR), traffic_recorder_(kMaxMessagesToRecord, kMaxMessageSizeToRecord), encryptor_(NULL), - unrecoverable_error_handler_(NULL), report_unrecoverable_error_function_(NULL) { // Pre-fill |notification_info_map_|. for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) { @@ -346,15 +345,15 @@ void SyncManagerImpl::Init( bool use_ssl, scoped_ptr<HttpPostProviderFactory> post_factory, const std::vector<ModelSafeWorker*>& workers, - ExtensionsActivityMonitor* extensions_activity_monitor, + ExtensionsActivity* extensions_activity, SyncManager::ChangeDelegate* change_delegate, const SyncCredentials& credentials, const std::string& invalidator_client_id, const std::string& restored_key_for_bootstrapping, const std::string& restored_keystore_key_for_bootstrapping, - scoped_ptr<InternalComponentsFactory> internal_components_factory, + InternalComponentsFactory* internal_components_factory, Encryptor* encryptor, - UnrecoverableErrorHandler* unrecoverable_error_handler, + scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler, ReportUnrecoverableErrorFunction report_unrecoverable_error_function, bool use_oauth2_token) { CHECK(!initialized_); @@ -376,7 +375,7 @@ void SyncManagerImpl::Init( database_path_ = database_location.Append( syncable::Directory::kSyncDatabaseFilename); encryptor_ = encryptor; - unrecoverable_error_handler_ = unrecoverable_error_handler; + unrecoverable_error_handler_ = unrecoverable_error_handler.Pass(); report_unrecoverable_error_function_ = report_unrecoverable_error_function; allstatus_.SetHasKeystoreKey( @@ -402,7 +401,7 @@ void SyncManagerImpl::Init( share_.directory.reset( new syncable::Directory( backing_store.release(), - unrecoverable_error_handler_, + unrecoverable_error_handler_.get(), report_unrecoverable_error_function_, sync_encryption_handler_.get(), sync_encryption_handler_->GetCryptographerUnsafe())); @@ -442,7 +441,7 @@ void SyncManagerImpl::Init( connection_manager_.get(), directory(), workers, - extensions_activity_monitor, + extensions_activity, listeners, &debug_info_event_listener_, &traffic_recorder_, @@ -620,9 +619,9 @@ void SyncManagerImpl::RemoveObserver(SyncManager::Observer* observer) { observers_.RemoveObserver(observer); } -void SyncManagerImpl::StopSyncingForShutdown(const base::Closure& callback) { +void SyncManagerImpl::StopSyncingForShutdown() { DVLOG(2) << "StopSyncingForShutdown"; - scheduler_->RequestStop(callback); + scheduler_->RequestStop(); 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 153df81..712c268 100644 --- a/sync/internal_api/sync_manager_impl.h +++ b/sync/internal_api/sync_manager_impl.h @@ -70,15 +70,15 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl : bool use_ssl, scoped_ptr<HttpPostProviderFactory> post_factory, const std::vector<ModelSafeWorker*>& workers, - ExtensionsActivityMonitor* extensions_activity_monitor, + ExtensionsActivity* extensions_activity, SyncManager::ChangeDelegate* change_delegate, const SyncCredentials& credentials, const std::string& invalidator_client_id, const std::string& restored_key_for_bootstrapping, const std::string& restored_keystore_key_for_bootstrapping, - scoped_ptr<InternalComponentsFactory> internal_components_factory, + InternalComponentsFactory* internal_components_factory, Encryptor* encryptor, - UnrecoverableErrorHandler* unrecoverable_error_handler, + scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler, ReportUnrecoverableErrorFunction report_unrecoverable_error_function, bool use_oauth2_token) OVERRIDE; @@ -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(const base::Closure& callback) OVERRIDE; + virtual void StopSyncingForShutdown() OVERRIDE; virtual void ShutdownOnSyncThread() OVERRIDE; virtual UserShare* GetUserShare() OVERRIDE; virtual const std::string cache_guid() OVERRIDE; @@ -360,7 +360,7 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl : TrafficRecorder traffic_recorder_; Encryptor* encryptor_; - UnrecoverableErrorHandler* unrecoverable_error_handler_; + scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler_; ReportUnrecoverableErrorFunction report_unrecoverable_error_function_; // Sync's encryption handler. It tracks the set of encrypted types, manages diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc index 51144b6..06bc783 100644 --- a/sync/internal_api/sync_manager_impl_unittest.cc +++ b/sync/internal_api/sync_manager_impl_unittest.cc @@ -68,9 +68,8 @@ #include "sync/test/engine/fake_sync_scheduler.h" #include "sync/test/engine/test_id_factory.h" #include "sync/test/fake_encryptor.h" -#include "sync/test/fake_extensions_activity_monitor.h" #include "sync/util/cryptographer.h" -#include "sync/util/extensions_activity_monitor.h" +#include "sync/util/extensions_activity.h" #include "sync/util/test_unrecoverable_error_handler.h" #include "sync/util/time.h" #include "testing/gmock/include/gmock/gmock.h" @@ -800,6 +799,8 @@ class SyncManagerTest : public testing::Test, void SetUp() { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); + extensions_activity_ = new ExtensionsActivity(); + SyncCredentials credentials; credentials.email = "foo@bar.com"; credentials.sync_token = "sometoken"; @@ -823,15 +824,16 @@ class SyncManagerTest : public testing::Test, false, scoped_ptr<HttpPostProviderFactory>(new TestHttpPostProviderFactory()), workers, - &extensions_activity_monitor_, + extensions_activity_.get(), this, credentials, "fake_invalidator_client_id", std::string(), std::string(), // bootstrap tokens - scoped_ptr<InternalComponentsFactory>(GetFactory()), + scoped_ptr<InternalComponentsFactory>(GetFactory()).get(), &encryptor_, - &handler_, + scoped_ptr<UnrecoverableErrorHandler>( + new TestUnrecoverableErrorHandler).Pass(), NULL, false); @@ -1010,11 +1012,10 @@ class SyncManagerTest : public testing::Test, base::ScopedTempDir temp_dir_; // Sync Id's for the roots of the enabled datatypes. std::map<ModelType, int64> type_roots_; - FakeExtensionsActivityMonitor extensions_activity_monitor_; + scoped_refptr<ExtensionsActivity> extensions_activity_; protected: FakeEncryptor encryptor_; - TestUnrecoverableErrorHandler handler_; SyncManagerImpl sync_manager_; WeakHandle<JsBackend> js_backend_; StrictMock<SyncManagerObserverMock> manager_observer_; diff --git a/sync/internal_api/test/fake_sync_manager.cc b/sync/internal_api/test/fake_sync_manager.cc index 7a6d6a4..cac310b 100644 --- a/sync/internal_api/test/fake_sync_manager.cc +++ b/sync/internal_api/test/fake_sync_manager.cc @@ -81,17 +81,16 @@ void FakeSyncManager::Init( bool use_ssl, scoped_ptr<HttpPostProviderFactory> post_factory, const std::vector<ModelSafeWorker*>& workers, - ExtensionsActivityMonitor* extensions_activity_monitor, + ExtensionsActivity* extensions_activity, ChangeDelegate* change_delegate, const SyncCredentials& credentials, const std::string& invalidator_client_id, const std::string& restored_key_for_bootstrapping, const std::string& restored_keystore_key_for_bootstrapping, - scoped_ptr<InternalComponentsFactory> internal_components_factory, + InternalComponentsFactory* internal_components_factory, Encryptor* encryptor, - UnrecoverableErrorHandler* unrecoverable_error_handler, - ReportUnrecoverableErrorFunction - report_unrecoverable_error_function, + scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler, + ReportUnrecoverableErrorFunction report_unrecoverable_error_function, bool use_oauth2_token) { sync_task_runner_ = base::ThreadTaskRunnerHandle::Get(); PurgePartiallySyncedTypes(); @@ -210,10 +209,7 @@ void FakeSyncManager::SaveChanges() { // Do nothing. } -void FakeSyncManager::StopSyncingForShutdown(const base::Closure& callback) { - if (!sync_task_runner_->PostTask(FROM_HERE, callback)) { - NOTREACHED(); - } +void FakeSyncManager::StopSyncingForShutdown() { } void FakeSyncManager::ShutdownOnSyncThread() { diff --git a/sync/internal_api/test/test_internal_components_factory.cc b/sync/internal_api/test/test_internal_components_factory.cc index 154c58cc..03d2d82 100644 --- a/sync/internal_api/test/test_internal_components_factory.cc +++ b/sync/internal_api/test/test_internal_components_factory.cc @@ -31,7 +31,7 @@ TestInternalComponentsFactory::BuildContext( ServerConnectionManager* connection_manager, syncable::Directory* directory, const std::vector<ModelSafeWorker*>& workers, - ExtensionsActivityMonitor* monitor, + ExtensionsActivity* monitor, const std::vector<SyncEngineEventListener*>& listeners, sessions::DebugInfoGetter* debug_info_getter, TrafficRecorder* traffic_recorder, diff --git a/sync/notifier/non_blocking_invalidator.cc b/sync/notifier/non_blocking_invalidator.cc index 2834f28..d4c602b 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()))) { - NOTREACHED(); + DVLOG(1) << "Network thread stopped before invalidator is destroyed."; } } diff --git a/sync/sessions/sync_session_context.cc b/sync/sessions/sync_session_context.cc index c48c32b..98ab5f0 100644 --- a/sync/sessions/sync_session_context.cc +++ b/sync/sessions/sync_session_context.cc @@ -5,7 +5,7 @@ #include "sync/sessions/sync_session_context.h" #include "sync/sessions/debug_info_getter.h" -#include "sync/util/extensions_activity_monitor.h" +#include "sync/util/extensions_activity.h" namespace syncer { namespace sessions { @@ -17,7 +17,7 @@ SyncSessionContext::SyncSessionContext( ServerConnectionManager* connection_manager, syncable::Directory* directory, const std::vector<ModelSafeWorker*>& workers, - ExtensionsActivityMonitor* extensions_activity_monitor, + ExtensionsActivity* extensions_activity, const std::vector<SyncEngineEventListener*>& listeners, DebugInfoGetter* debug_info_getter, TrafficRecorder* traffic_recorder, @@ -26,7 +26,7 @@ SyncSessionContext::SyncSessionContext( const std::string& invalidator_client_id) : connection_manager_(connection_manager), directory_(directory), - extensions_activity_monitor_(extensions_activity_monitor), + extensions_activity_(extensions_activity), notifications_enabled_(false), max_commit_batch_size_(kDefaultMaxCommitBatchSize), debug_info_getter_(debug_info_getter), diff --git a/sync/sessions/sync_session_context.h b/sync/sessions/sync_session_context.h index 7617095..718cc6c 100644 --- a/sync/sessions/sync_session_context.h +++ b/sync/sessions/sync_session_context.h @@ -32,7 +32,7 @@ namespace syncer { -class ExtensionsActivityMonitor; +class ExtensionsActivity; class ServerConnectionManager; namespace syncable { @@ -50,7 +50,7 @@ class SYNC_EXPORT_PRIVATE SyncSessionContext { SyncSessionContext(ServerConnectionManager* connection_manager, syncable::Directory* directory, const std::vector<ModelSafeWorker*>& workers, - ExtensionsActivityMonitor* extensions_activity_monitor, + ExtensionsActivity* extensions_activity, const std::vector<SyncEngineEventListener*>& listeners, DebugInfoGetter* debug_info_getter, TrafficRecorder* traffic_recorder, @@ -79,8 +79,8 @@ class SYNC_EXPORT_PRIVATE SyncSessionContext { return workers_; } - ExtensionsActivityMonitor* extensions_monitor() { - return extensions_activity_monitor_; + ExtensionsActivity* extensions_activity() { + return extensions_activity_.get(); } DebugInfoGetter* debug_info_getter() { @@ -159,7 +159,7 @@ class SYNC_EXPORT_PRIVATE SyncSessionContext { // We use this to stuff extensions activity into CommitMessages so the server // can correlate commit traffic with extension-related bookmark mutations. - ExtensionsActivityMonitor* extensions_activity_monitor_; + scoped_refptr<ExtensionsActivity> extensions_activity_; // Kept up to date with talk events to determine whether notifications are // enabled. True only if the notification channel is authorized and open. diff --git a/sync/sessions/sync_session_unittest.cc b/sync/sessions/sync_session_unittest.cc index 4acee56..979ddbd 100644 --- a/sync/sessions/sync_session_unittest.cc +++ b/sync/sessions/sync_session_unittest.cc @@ -16,7 +16,7 @@ #include "sync/syncable/syncable_write_transaction.h" #include "sync/test/engine/fake_model_worker.h" #include "sync/test/engine/test_directory_setter_upper.h" -#include "sync/test/fake_extensions_activity_monitor.h" +#include "sync/util/extensions_activity.h" #include "testing/gtest/include/gtest/gtest.h" namespace syncer { @@ -38,6 +38,8 @@ class SyncSessionTest : public testing::Test, } virtual void SetUp() { + extensions_activity_ = new ExtensionsActivity(); + routes_.clear(); routes_[BOOKMARKS] = GROUP_UI; routes_[AUTOFILL] = GROUP_DB; @@ -60,7 +62,7 @@ class SyncSessionTest : public testing::Test, NULL, NULL, workers, - &extensions_activity_monitor_, + extensions_activity_.get(), std::vector<SyncEngineEventListener*>(), NULL, NULL, @@ -146,7 +148,7 @@ class SyncSessionTest : public testing::Test, scoped_ptr<SyncSessionContext> context_; std::vector<scoped_refptr<ModelSafeWorker> > workers_; ModelSafeRoutingInfo routes_; - FakeExtensionsActivityMonitor extensions_activity_monitor_; + scoped_refptr<ExtensionsActivity> extensions_activity_; }; TEST_F(SyncSessionTest, MoreToDownloadIfDownloadFailed) { diff --git a/sync/sync_core.gypi b/sync/sync_core.gypi index 542040c..2b97fe7 100644 --- a/sync/sync_core.gypi +++ b/sync/sync_core.gypi @@ -169,8 +169,8 @@ 'util/cryptographer.h', 'util/data_type_histogram.h', 'util/encryptor.h', - 'util/extensions_activity_monitor.cc', - 'util/extensions_activity_monitor.h', + 'util/extensions_activity.cc', + 'util/extensions_activity.h', 'util/get_session_name.cc', 'util/get_session_name.h', 'util/get_session_name_ios.mm', diff --git a/sync/sync_tests.gypi b/sync/sync_tests.gypi index 32867f1..847d862 100644 --- a/sync/sync_tests.gypi +++ b/sync/sync_tests.gypi @@ -49,8 +49,6 @@ 'test/fake_encryptor.h', 'test/fake_sync_encryption_handler.h', 'test/fake_sync_encryption_handler.cc', - 'test/fake_extensions_activity_monitor.cc', - 'test/fake_extensions_activity_monitor.h', 'test/test_transaction_observer.cc', 'test/test_transaction_observer.h', 'test/null_directory_change_delegate.cc', diff --git a/sync/test/engine/fake_sync_scheduler.cc b/sync/test/engine/fake_sync_scheduler.cc index 200edb0..585248e 100644 --- a/sync/test/engine/fake_sync_scheduler.cc +++ b/sync/test/engine/fake_sync_scheduler.cc @@ -6,16 +6,14 @@ namespace syncer { -FakeSyncScheduler::FakeSyncScheduler() - : created_on_loop_(base::MessageLoop::current()) {} +FakeSyncScheduler::FakeSyncScheduler() {} FakeSyncScheduler::~FakeSyncScheduler() {} void FakeSyncScheduler::Start(Mode mode) { } -void FakeSyncScheduler::RequestStop(const base::Closure& callback) { - created_on_loop_->PostTask(FROM_HERE, callback); +void FakeSyncScheduler::RequestStop() { } void FakeSyncScheduler::ScheduleLocalNudge( diff --git a/sync/test/engine/fake_sync_scheduler.h b/sync/test/engine/fake_sync_scheduler.h index 97deb91..29757e2 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(const base::Closure& callback) OVERRIDE; + virtual void RequestStop() OVERRIDE; virtual void ScheduleLocalNudge( const base::TimeDelta& desired_delay, ModelTypeSet types, @@ -58,9 +58,6 @@ 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 diff --git a/sync/test/engine/syncer_command_test.cc b/sync/test/engine/syncer_command_test.cc index 833ed9a..68edbbd 100644 --- a/sync/test/engine/syncer_command_test.cc +++ b/sync/test/engine/syncer_command_test.cc @@ -17,6 +17,8 @@ SyncerCommandTestBase::~SyncerCommandTestBase() { } void SyncerCommandTestBase::SetUp() { + extensions_activity_ = new ExtensionsActivity(); + // The session always expects there to be a passive worker. workers()->push_back( make_scoped_refptr(new FakeModelWorker(GROUP_PASSIVE))); diff --git a/sync/test/engine/syncer_command_test.h b/sync/test/engine/syncer_command_test.h index 93106b9..89c2aba 100644 --- a/sync/test/engine/syncer_command_test.h +++ b/sync/test/engine/syncer_command_test.h @@ -22,7 +22,7 @@ #include "sync/test/engine/fake_model_worker.h" #include "sync/test/engine/mock_connection_manager.h" #include "sync/test/engine/test_directory_setter_upper.h" -#include "sync/test/fake_extensions_activity_monitor.h" +#include "sync/util/extensions_activity.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -131,7 +131,7 @@ class SyncerCommandTestBase : public testing::Test, void ResetContext() { context_.reset(new sessions::SyncSessionContext( mock_server_.get(), directory(), - GetWorkers(), &extensions_activity_monitor_, + GetWorkers(), extensions_activity_.get(), std::vector<SyncEngineEventListener*>(), &mock_debug_info_getter_, &traffic_recorder_, @@ -210,7 +210,7 @@ class SyncerCommandTestBase : public testing::Test, std::vector<scoped_refptr<ModelSafeWorker> > workers_; ModelSafeRoutingInfo routing_info_; NiceMock<MockDebugInfoGetter> mock_debug_info_getter_; - FakeExtensionsActivityMonitor extensions_activity_monitor_; + scoped_refptr<ExtensionsActivity> extensions_activity_; TrafficRecorder traffic_recorder_; DISALLOW_COPY_AND_ASSIGN(SyncerCommandTestBase); }; diff --git a/sync/test/fake_extensions_activity_monitor.cc b/sync/test/fake_extensions_activity_monitor.cc deleted file mode 100644 index 9fb20a6..0000000 --- a/sync/test/fake_extensions_activity_monitor.cc +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "sync/test/fake_extensions_activity_monitor.h" - -#include "base/logging.h" - -namespace syncer { - -FakeExtensionsActivityMonitor::FakeExtensionsActivityMonitor() {} - -FakeExtensionsActivityMonitor::~FakeExtensionsActivityMonitor() { - DCHECK(CalledOnValidThread()); -} - -void FakeExtensionsActivityMonitor::GetAndClearRecords(Records* buffer) { - DCHECK(CalledOnValidThread()); - buffer->clear(); - buffer->swap(records_); -} - -void FakeExtensionsActivityMonitor::PutRecords(const Records& records) { - DCHECK(CalledOnValidThread()); - for (Records::const_iterator i = records.begin(); i != records.end(); ++i) { - records_[i->first].extension_id = i->second.extension_id; - records_[i->first].bookmark_write_count += i->second.bookmark_write_count; - } -} - -} // namespace syncer diff --git a/sync/test/fake_extensions_activity_monitor.h b/sync/test/fake_extensions_activity_monitor.h deleted file mode 100644 index a0702e1..0000000 --- a/sync/test/fake_extensions_activity_monitor.h +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef SYNC_TEST_FAKE_EXTENSIONS_ACTIVITY_MONITOR_H_ -#define SYNC_TEST_FAKE_EXTENSIONS_ACTIVITY_MONITOR_H_ - -#include "base/compiler_specific.h" -#include "base/threading/non_thread_safe.h" -#include "sync/util/extensions_activity_monitor.h" - -namespace syncer { - -// Fake non-thread-safe implementation of ExtensionsActivityMonitor -// suitable to be used in single-threaded sync tests. -class FakeExtensionsActivityMonitor - : public ExtensionsActivityMonitor, - public base::NonThreadSafe { - public: - FakeExtensionsActivityMonitor(); - virtual ~FakeExtensionsActivityMonitor(); - - // ExtensionsActivityMonitor implementation. - virtual void GetAndClearRecords(Records* buffer) OVERRIDE; - virtual void PutRecords(const Records& records) OVERRIDE; - - private: - Records records_; -}; - -} // namespace syncer - -#endif // SYNC_TEST_FAKE_EXTENSIONS_ACTIVITY_MONITOR_H_ diff --git a/sync/tools/sync_client.cc b/sync/tools/sync_client.cc index 2db5a04..e382546 100644 --- a/sync/tools/sync_client.cc +++ b/sync/tools/sync_client.cc @@ -341,12 +341,11 @@ int SyncClientMain(int argc, char* argv[]) { base::Bind(&StubNetworkTimeUpdateCallback))); // Used only when committing bookmarks, so it's okay to leave this // as NULL. - ExtensionsActivityMonitor* extensions_activity_monitor = NULL; + ExtensionsActivity* extensions_activity = NULL; LoggingChangeDelegate change_delegate; const char kRestoredKeyForBootstrapping[] = ""; const char kRestoredKeystoreKeyForBootstrapping[] = ""; NullEncryptor null_encryptor; - LoggingUnrecoverableErrorHandler unrecoverable_error_handler; InternalComponentsFactoryImpl::Switches factory_switches = { InternalComponentsFactory::ENCRYPTION_KEYSTORE, InternalComponentsFactory::BACKOFF_NORMAL @@ -360,16 +359,16 @@ int SyncClientMain(int argc, char* argv[]) { kUseSsl, post_factory.Pass(), workers, - extensions_activity_monitor, + extensions_activity, &change_delegate, credentials, invalidator_id, kRestoredKeyForBootstrapping, kRestoredKeystoreKeyForBootstrapping, - scoped_ptr<InternalComponentsFactory>( - new InternalComponentsFactoryImpl(factory_switches)), + new InternalComponentsFactoryImpl(factory_switches), &null_encryptor, - &unrecoverable_error_handler, + scoped_ptr<UnrecoverableErrorHandler>( + new LoggingUnrecoverableErrorHandler).Pass(), &LogUnrecoverableErrorContext, false); // TODO(akalin): Avoid passing in model parameters multiple times by // organizing handling of model types. diff --git a/sync/util/extensions_activity.cc b/sync/util/extensions_activity.cc new file mode 100644 index 0000000..dcccb26 --- /dev/null +++ b/sync/util/extensions_activity.cc @@ -0,0 +1,39 @@ +// Copyright 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "sync/util/extensions_activity.h" + +namespace syncer { + +ExtensionsActivity::Record::Record() + : bookmark_write_count(0U) {} + +ExtensionsActivity::Record::~Record() {} + +ExtensionsActivity::ExtensionsActivity() {} + +ExtensionsActivity::~ExtensionsActivity() {} + +void ExtensionsActivity::GetAndClearRecords(Records* buffer) { + base::AutoLock lock(records_lock_); + buffer->clear(); + buffer->swap(records_); +} + +void ExtensionsActivity::PutRecords(const Records& records) { + base::AutoLock lock(records_lock_); + for (Records::const_iterator i = records.begin(); i != records.end(); ++i) { + records_[i->first].extension_id = i->second.extension_id; + records_[i->first].bookmark_write_count += i->second.bookmark_write_count; + } +} + +void ExtensionsActivity::UpdateRecord(const std::string& extension_id) { + base::AutoLock lock(records_lock_); + Record& record = records_[extension_id]; + record.extension_id = extension_id; + record.bookmark_write_count++; +} + +} // namespace syncer diff --git a/sync/util/extensions_activity.h b/sync/util/extensions_activity.h new file mode 100644 index 0000000..8178760 --- /dev/null +++ b/sync/util/extensions_activity.h @@ -0,0 +1,64 @@ +// Copyright 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef SYNC_UTIL_EXTENSIONS_ACTIVITY_H_ +#define SYNC_UTIL_EXTENSIONS_ACTIVITY_H_ + +#include <map> +#include <string> + +#include "base/basictypes.h" +#include "base/memory/ref_counted.h" +#include "base/synchronization/lock.h" +#include "sync/base/sync_export.h" + +namespace syncer { + +// A storage to record usage of extensions APIs to send to sync +// servers, with the ability to purge data once sync servers have +// acknowledged it (successful commit response). +class SYNC_EXPORT ExtensionsActivity + : public base::RefCountedThreadSafe<ExtensionsActivity> { + public: + // A data record of activity performed by extension |extension_id|. + struct SYNC_EXPORT Record { + Record(); + ~Record(); + + // The human-readable ID identifying the extension responsible + // for the activity reported in this Record. + std::string extension_id; + + // How many times the extension successfully invoked a write + // operation through the bookmarks API since the last CommitMessage. + uint32 bookmark_write_count; + }; + + typedef std::map<std::string, Record> Records; + + ExtensionsActivity(); + + // Fill |buffer| with all current records and then clear the + // internal records. Called on sync thread to append records to sync commit + // message. + void GetAndClearRecords(Records* buffer); + + // Merge |records| with the current set of records. Called on sync thread to + // put back records if sync commit failed. + void PutRecords(const Records& records); + + // Increment write count of the specified extension. + void UpdateRecord(const std::string& extension_id); + + private: + friend class base::RefCountedThreadSafe<ExtensionsActivity>; + ~ExtensionsActivity(); + + Records records_; + mutable base::Lock records_lock_; +}; + +} // namespace syncer + +#endif // SYNC_UTIL_EXTENSIONS_ACTIVITY_H_ diff --git a/sync/util/extensions_activity_monitor.cc b/sync/util/extensions_activity_monitor.cc deleted file mode 100644 index fabe50b..0000000 --- a/sync/util/extensions_activity_monitor.cc +++ /dev/null @@ -1,16 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "sync/util/extensions_activity_monitor.h" - -namespace syncer { - -ExtensionsActivityMonitor::Record::Record() - : bookmark_write_count(0U) {} - -ExtensionsActivityMonitor::Record::~Record() {} - -ExtensionsActivityMonitor::~ExtensionsActivityMonitor() {} - -} // namespace syncer diff --git a/sync/util/extensions_activity_monitor.h b/sync/util/extensions_activity_monitor.h deleted file mode 100644 index 699f7b5..0000000 --- a/sync/util/extensions_activity_monitor.h +++ /dev/null @@ -1,53 +0,0 @@ -// Copyright 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef SYNC_UTIL_EXTENSIONS_ACTIVITY_MONITOR_H_ -#define SYNC_UTIL_EXTENSIONS_ACTIVITY_MONITOR_H_ - -#include <map> -#include <string> - -#include "base/basictypes.h" -#include "sync/base/sync_export.h" - -namespace syncer { - -// An interface to monitor usage of extensions APIs to send to sync -// servers, with the ability to purge data once sync servers have -// acknowledged it (successful commit response). -// -// All abstract methods are called from the sync thread. -class SYNC_EXPORT ExtensionsActivityMonitor { - public: - // A data record of activity performed by extension |extension_id|. - struct SYNC_EXPORT Record { - Record(); - ~Record(); - - // The human-readable ID identifying the extension responsible - // for the activity reported in this Record. - std::string extension_id; - - // How many times the extension successfully invoked a write - // operation through the bookmarks API since the last CommitMessage. - uint32 bookmark_write_count; - }; - - typedef std::map<std::string, Record> Records; - - // Fill |buffer| with all current records and then clear the - // internal records. - virtual void GetAndClearRecords(Records* buffer) = 0; - - // Merge |records| with the current set of records, adding the - // bookmark write counts for common Records. - virtual void PutRecords(const Records& records) = 0; - - protected: - virtual ~ExtensionsActivityMonitor(); -}; - -} // namespace syncer - -#endif // SYNC_UTIL_EXTENSIONS_ACTIVITY_MONITOR_H_ |