diff options
23 files changed, 233 insertions, 89 deletions
diff --git a/chrome/browser/sync/glue/autofill_data_type_controller.h b/chrome/browser/sync/glue/autofill_data_type_controller.h index cc11a400..9c73c81d 100755 --- a/chrome/browser/sync/glue/autofill_data_type_controller.h +++ b/chrome/browser/sync/glue/autofill_data_type_controller.h @@ -21,7 +21,6 @@ class ChangeProcessor; // A class that manages the startup and shutdown of autofill sync. class AutofillDataTypeController : public DataTypeController, - public UnrecoverableErrorHandler, public NotificationObserver { public: AutofillDataTypeController( diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller.cc b/chrome/browser/sync/glue/bookmark_data_type_controller.cc index 744b7e6..3e8e474 100644 --- a/chrome/browser/sync/glue/bookmark_data_type_controller.cc +++ b/chrome/browser/sync/glue/bookmark_data_type_controller.cc @@ -27,7 +27,8 @@ BookmarkDataTypeController::BookmarkDataTypeController( profile_(profile), sync_service_(sync_service), state_(NOT_RUNNING), - merge_allowed_(false) { + merge_allowed_(false), + unrecoverable_error_detected_(false) { DCHECK(profile_sync_factory); DCHECK(profile); DCHECK(sync_service); @@ -39,6 +40,7 @@ BookmarkDataTypeController::~BookmarkDataTypeController() { void BookmarkDataTypeController::Start(bool merge_allowed, StartCallback* start_callback) { DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); + unrecoverable_error_detected_ = false; if (state_ != NOT_RUNNING) { start_callback->Run(BUSY); delete start_callback; @@ -71,7 +73,9 @@ void BookmarkDataTypeController::Stop() { DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); // If Stop() is called while Start() is waiting for the bookmark // model to load, abort the start. - if (state_ == MODEL_STARTING) { + if (unrecoverable_error_detected_) { + FinishStart(UNRECOVERABLE_ERROR); + } else if (state_ == MODEL_STARTING) { FinishStart(ABORTED); } @@ -89,6 +93,12 @@ void BookmarkDataTypeController::Stop() { merge_allowed_ = false; } +void BookmarkDataTypeController::OnUnrecoverableError() { + unrecoverable_error_detected_ = true; + // The ProfileSyncService will invoke our Stop() method in response to this. + sync_service_->OnUnrecoverableError(); +} + void BookmarkDataTypeController::Observe(NotificationType type, const NotificationSource& source, const NotificationDetails& details) { @@ -102,12 +112,15 @@ void BookmarkDataTypeController::Associate() { state_ = ASSOCIATING; ProfileSyncFactory::SyncComponents sync_components = - profile_sync_factory_->CreateBookmarkSyncComponents(sync_service_); + profile_sync_factory_->CreateBookmarkSyncComponents(sync_service_, this); model_associator_.reset(sync_components.model_associator); change_processor_.reset(sync_components.change_processor); - bool needs_merge = model_associator_->ChromeModelHasUserCreatedNodes() && - model_associator_->SyncModelHasUserCreatedNodes(); + bool needs_merge = model_associator_->ChromeModelHasUserCreatedNodes(); + if (unrecoverable_error_detected_) return; + needs_merge &= model_associator_->SyncModelHasUserCreatedNodes(); + if (unrecoverable_error_detected_) return; + if (needs_merge && !merge_allowed_) { model_associator_.reset(); change_processor_.reset(); @@ -118,7 +131,10 @@ void BookmarkDataTypeController::Associate() { base::TimeTicks start_time = base::TimeTicks::Now(); bool first_run = !model_associator_->SyncModelHasUserCreatedNodes(); + if (unrecoverable_error_detected_) return; bool merge_success = model_associator_->AssociateModels(); + if (unrecoverable_error_detected_) return; + UMA_HISTOGRAM_TIMES("Sync.BookmarkAssociationTime", base::TimeTicks::Now() - start_time); if (!merge_success) { diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller.h b/chrome/browser/sync/glue/bookmark_data_type_controller.h index 49a3b94..14e326e 100644 --- a/chrome/browser/sync/glue/bookmark_data_type_controller.h +++ b/chrome/browser/sync/glue/bookmark_data_type_controller.h @@ -59,6 +59,9 @@ class BookmarkDataTypeController : public DataTypeController, return state_; } + // UnrecoverableErrorHandler interface. + virtual void OnUnrecoverableError(); + // NotificationObserver interface. virtual void Observe(NotificationType type, const NotificationSource& source, @@ -77,6 +80,7 @@ class BookmarkDataTypeController : public DataTypeController, State state_; bool merge_allowed_; + bool unrecoverable_error_detected_; scoped_ptr<StartCallback> start_callback_; scoped_ptr<AssociatorInterface> model_associator_; diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc b/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc index 0fc7ed7..78728c9 100644 --- a/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc @@ -27,6 +27,8 @@ using browser_sync::ChangeProcessorMock; using browser_sync::DataTypeController; using browser_sync::ModelAssociatorMock; using testing::_; +using testing::DoAll; +using testing::Invoke; using testing::Return; class StartCallback { @@ -70,7 +72,7 @@ class BookmarkDataTypeControllerTest : public testing::Test { } void SetAssociateExpectations() { - EXPECT_CALL(*profile_sync_factory_, CreateBookmarkSyncComponents(_)); + EXPECT_CALL(*profile_sync_factory_, CreateBookmarkSyncComponents(_, _)); EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()). WillRepeatedly(Return(false)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()). @@ -150,7 +152,7 @@ TEST_F(BookmarkDataTypeControllerTest, StartBusy) { TEST_F(BookmarkDataTypeControllerTest, StartNeedsMerge) { SetStartExpectations(); - EXPECT_CALL(*profile_sync_factory_, CreateBookmarkSyncComponents(_)); + EXPECT_CALL(*profile_sync_factory_, CreateBookmarkSyncComponents(_, _)); EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()). WillRepeatedly(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()). @@ -177,7 +179,7 @@ TEST_F(BookmarkDataTypeControllerTest, StartMergeAllowed) { TEST_F(BookmarkDataTypeControllerTest, StartAssociationFailed) { SetStartExpectations(); // Set up association to fail. - EXPECT_CALL(*profile_sync_factory_, CreateBookmarkSyncComponents(_)); + EXPECT_CALL(*profile_sync_factory_, CreateBookmarkSyncComponents(_, _)); EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()). WillRepeatedly(Return(false)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()). @@ -191,6 +193,30 @@ TEST_F(BookmarkDataTypeControllerTest, StartAssociationFailed) { EXPECT_EQ(DataTypeController::NOT_RUNNING, bookmark_dtc_->state()); } +TEST_F(BookmarkDataTypeControllerTest, + StartAssociationTriggersUnrecoverableError) { + SetStartExpectations(); + // Set up association to fail with an unrecoverable error. + EXPECT_CALL(*profile_sync_factory_, CreateBookmarkSyncComponents(_, _)); + EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()). + WillOnce(Return(false)); + EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()). + WillOnce(DoAll(Invoke(bookmark_dtc_.get(), + &BookmarkDataTypeController::OnUnrecoverableError), + Return(false))); + EXPECT_CALL(service_, OnUnrecoverableError()). + WillOnce(Invoke(bookmark_dtc_.get(), + &BookmarkDataTypeController::Stop)); + EXPECT_CALL(service_, DeactivateDataType(bookmark_dtc_.get(), + change_processor_)); + EXPECT_CALL(*model_associator_, DisassociateModels()); + + EXPECT_CALL(start_callback_, Run(DataTypeController::UNRECOVERABLE_ERROR)); + bookmark_dtc_->Start(true, + NewCallback(&start_callback_, &StartCallback::Run)); + EXPECT_EQ(DataTypeController::NOT_RUNNING, bookmark_dtc_->state()); +} + TEST_F(BookmarkDataTypeControllerTest, StartAborted) { SetStartExpectations(); EXPECT_CALL(bookmark_model_, IsLoaded()).WillRepeatedly(Return(false)); @@ -215,5 +241,4 @@ TEST_F(BookmarkDataTypeControllerTest, Stop) { EXPECT_EQ(DataTypeController::RUNNING, bookmark_dtc_->state()); bookmark_dtc_->Stop(); EXPECT_EQ(DataTypeController::NOT_RUNNING, bookmark_dtc_->state()); - } diff --git a/chrome/browser/sync/glue/bookmark_model_associator.cc b/chrome/browser/sync/glue/bookmark_model_associator.cc index 975ae8e..19ddb7c 100644 --- a/chrome/browser/sync/glue/bookmark_model_associator.cc +++ b/chrome/browser/sync/glue/bookmark_model_associator.cc @@ -153,8 +153,10 @@ const BookmarkNode* BookmarkNodeIdIndex::Find(int64 id) const { } BookmarkModelAssociator::BookmarkModelAssociator( - ProfileSyncService* sync_service) + ProfileSyncService* sync_service, + UnrecoverableErrorHandler* error_handler) : sync_service_(sync_service), + error_handler_(error_handler), ALLOW_THIS_IN_INITIALIZER_LIST(persist_associations_(this)) { DCHECK(sync_service_); } @@ -221,12 +223,12 @@ bool BookmarkModelAssociator::ChromeModelHasUserCreatedNodes() { bool BookmarkModelAssociator::SyncModelHasUserCreatedNodes() { int64 bookmark_bar_sync_id; if (!GetSyncIdForTaggedNode(kBookmarkBarTag, &bookmark_bar_sync_id)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } int64 other_bookmarks_sync_id; if (!GetSyncIdForTaggedNode(kOtherBookmarksTag, &other_bookmarks_sync_id)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } @@ -235,13 +237,13 @@ bool BookmarkModelAssociator::SyncModelHasUserCreatedNodes() { sync_api::ReadNode bookmark_bar_node(&trans); if (!bookmark_bar_node.InitByIdLookup(bookmark_bar_sync_id)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } sync_api::ReadNode other_bookmarks_node(&trans); if (!other_bookmarks_node.InitByIdLookup(other_bookmarks_sync_id)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } @@ -328,14 +330,14 @@ bool BookmarkModelAssociator::BuildAssociations() { // To prime our association, we associate the top-level nodes, Bookmark Bar // and Other Bookmarks. if (!AssociateTaggedPermanentNode(model->other_node(), kOtherBookmarksTag)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); LOG(ERROR) << "Server did not create top-level nodes. Possibly we " << "are running against an out-of-date server?"; return false; } if (!AssociateTaggedPermanentNode(model->GetBookmarkBarNode(), kBookmarkBarTag)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); LOG(ERROR) << "Server did not create top-level nodes. Possibly we " << "are running against an out-of-date server?"; return false; @@ -360,7 +362,7 @@ bool BookmarkModelAssociator::BuildAssociations() { sync_api::ReadNode sync_parent(&trans); if (!sync_parent.InitByIdLookup(sync_parent_id)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } // Only folder nodes are pushed on to the stack. @@ -376,7 +378,7 @@ bool BookmarkModelAssociator::BuildAssociations() { while (sync_child_id != sync_api::kInvalidId) { sync_api::WriteNode sync_child_node(&trans); if (!sync_child_node.InitByIdLookup(sync_child_id)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } @@ -447,7 +449,7 @@ void BookmarkModelAssociator::PersistAssociations() { int64 sync_id = *iter; sync_api::WriteNode sync_node(&trans); if (!sync_node.InitByIdLookup(sync_id)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return; } const BookmarkNode* node = GetChromeNodeFromSyncId(sync_id); @@ -474,13 +476,13 @@ bool BookmarkModelAssociator::LoadAssociations() { int64 bookmark_bar_id; if (!GetSyncIdForTaggedNode(kBookmarkBarTag, &bookmark_bar_id)) { // We should always be able to find the permanent nodes. - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } int64 other_bookmarks_id; if (!GetSyncIdForTaggedNode(kOtherBookmarksTag, &other_bookmarks_id)) { // We should always be able to find the permanent nodes. - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } @@ -506,7 +508,7 @@ bool BookmarkModelAssociator::LoadAssociations() { ++sync_node_count; sync_api::ReadNode sync_parent(&trans); if (!sync_parent.InitByIdLookup(parent_id)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } @@ -533,7 +535,7 @@ bool BookmarkModelAssociator::LoadAssociations() { dfs_stack.push(child_id); sync_api::ReadNode child_node(&trans); if (!child_node.InitByIdLookup(child_id)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } child_id = child_node.GetSuccessorId(); diff --git a/chrome/browser/sync/glue/bookmark_model_associator.h b/chrome/browser/sync/glue/bookmark_model_associator.h index 9874a65..0e0f031 100644 --- a/chrome/browser/sync/glue/bookmark_model_associator.h +++ b/chrome/browser/sync/glue/bookmark_model_associator.h @@ -26,6 +26,7 @@ class ProfileSyncService; namespace browser_sync { class BookmarkChangeProcessor; +class UnrecoverableErrorHandler; // Contains all model association related logic: // * Algorithm to associate bookmark model and sync model. @@ -35,7 +36,8 @@ class BookmarkModelAssociator : public PerDataTypeAssociatorInterface<BookmarkNode, int64> { public: static syncable::ModelType model_type() { return syncable::BOOKMARKS; } - explicit BookmarkModelAssociator(ProfileSyncService* sync_service); + BookmarkModelAssociator(ProfileSyncService* sync_service, + UnrecoverableErrorHandler* error_handler); virtual ~BookmarkModelAssociator() { } // AssociatorInterface implementation. @@ -122,6 +124,7 @@ class BookmarkModelAssociator const sync_api::BaseNode* sync_node) const; ProfileSyncService* sync_service_; + UnrecoverableErrorHandler* error_handler_; BookmarkIdToSyncIdMap id_map_; SyncIdToBookmarkNodeMap id_map_inverse_; // Stores sync ids for dirty associations. diff --git a/chrome/browser/sync/glue/data_type_controller.h b/chrome/browser/sync/glue/data_type_controller.h index a074889..61114d4 100644 --- a/chrome/browser/sync/glue/data_type_controller.h +++ b/chrome/browser/sync/glue/data_type_controller.h @@ -10,6 +10,7 @@ #include "base/callback.h" #include "chrome/browser/sync/engine/model_safe_worker.h" #include "chrome/browser/sync/syncable/model_type.h" +#include "chrome/browser/sync/unrecoverable_error_handler.h" namespace browser_sync { @@ -17,7 +18,8 @@ namespace browser_sync { // need to run model associator or change processor on other threads. class DataTypeController : public base::RefCountedThreadSafe<DataTypeController, - ChromeThread::DeleteOnUIThread> { + ChromeThread::DeleteOnUIThread>, + public UnrecoverableErrorHandler { public: enum State { NOT_RUNNING, // The controller has never been started or has @@ -44,7 +46,8 @@ class DataTypeController // merge_allowed = true will allow this data // type to start. ASSOCIATION_FAILED, // An error occurred during model association. - ABORTED // Start was aborted by calling Stop(). + ABORTED, // Start was aborted by calling Stop(). + UNRECOVERABLE_ERROR // An unrecoverable error occured. }; typedef Callback1<StartResult>::Type StartCallback; diff --git a/chrome/browser/sync/glue/data_type_controller_mock.h b/chrome/browser/sync/glue/data_type_controller_mock.h index 71b3bc0..5c58887 100644 --- a/chrome/browser/sync/glue/data_type_controller_mock.h +++ b/chrome/browser/sync/glue/data_type_controller_mock.h @@ -19,6 +19,7 @@ class DataTypeControllerMock : public DataTypeController { MOCK_CONST_METHOD0(name, const char*()); MOCK_METHOD0(model_safe_group, browser_sync::ModelSafeGroup()); MOCK_METHOD0(state, State()); + MOCK_METHOD0(OnUnrecoverableError, void()); }; } // namespace browser_sync diff --git a/chrome/browser/sync/glue/preference_data_type_controller.cc b/chrome/browser/sync/glue/preference_data_type_controller.cc index 312e4f8..602d82a 100644 --- a/chrome/browser/sync/glue/preference_data_type_controller.cc +++ b/chrome/browser/sync/glue/preference_data_type_controller.cc @@ -11,6 +11,7 @@ #include "chrome/browser/sync/glue/preference_model_associator.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/browser/sync/profile_sync_factory.h" +#include "chrome/browser/sync/unrecoverable_error_handler.h" namespace browser_sync { @@ -19,7 +20,8 @@ PreferenceDataTypeController::PreferenceDataTypeController( ProfileSyncService* sync_service) : profile_sync_factory_(profile_sync_factory), sync_service_(sync_service), - state_(NOT_RUNNING) { + state_(NOT_RUNNING), + unrecoverable_error_detected_(false) { DCHECK(profile_sync_factory); DCHECK(sync_service); } @@ -30,49 +32,60 @@ PreferenceDataTypeController::~PreferenceDataTypeController() { void PreferenceDataTypeController::Start(bool merge_allowed, StartCallback* start_callback) { DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); + unrecoverable_error_detected_ = false; if (state_ != NOT_RUNNING) { start_callback->Run(BUSY); delete start_callback; return; } + start_callback_.reset(start_callback); // No additional services need to be started before we can proceed // with model association. ProfileSyncFactory::SyncComponents sync_components = - profile_sync_factory_->CreatePreferenceSyncComponents(sync_service_); + profile_sync_factory_->CreatePreferenceSyncComponents(sync_service_, + this); model_associator_.reset(sync_components.model_associator); change_processor_.reset(sync_components.change_processor); - bool needs_merge = model_associator_->ChromeModelHasUserCreatedNodes() && - model_associator_->SyncModelHasUserCreatedNodes(); + + bool needs_merge = model_associator_->ChromeModelHasUserCreatedNodes(); + if (unrecoverable_error_detected_) return; + needs_merge &= model_associator_->SyncModelHasUserCreatedNodes(); + if (unrecoverable_error_detected_) return; + if (needs_merge && !merge_allowed) { model_associator_.reset(); change_processor_.reset(); - start_callback->Run(NEEDS_MERGE); - delete start_callback; + FinishStart(NEEDS_MERGE); return; } base::TimeTicks start_time = base::TimeTicks::Now(); bool first_run = !model_associator_->SyncModelHasUserCreatedNodes(); + if (unrecoverable_error_detected_) return; bool merge_success = model_associator_->AssociateModels(); + if (unrecoverable_error_detected_) return; + UMA_HISTOGRAM_TIMES("Sync.PreferenceAssociationTime", base::TimeTicks::Now() - start_time); if (!merge_success) { model_associator_.reset(); change_processor_.reset(); - start_callback->Run(ASSOCIATION_FAILED); - delete start_callback; + FinishStart(ASSOCIATION_FAILED); return; } sync_service_->ActivateDataType(this, change_processor_.get()); state_ = RUNNING; - start_callback->Run(first_run ? OK_FIRST_RUN : OK); - delete start_callback; + FinishStart(first_run ? OK_FIRST_RUN : OK); } void PreferenceDataTypeController::Stop() { DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); + if (unrecoverable_error_detected_) { + FinishStart(UNRECOVERABLE_ERROR); + } + if (change_processor_ != NULL) sync_service_->DeactivateDataType(this, change_processor_.get()); @@ -85,4 +98,14 @@ void PreferenceDataTypeController::Stop() { state_ = NOT_RUNNING; } +void PreferenceDataTypeController::OnUnrecoverableError() { + unrecoverable_error_detected_ = true; + sync_service_->OnUnrecoverableError(); +} + +void PreferenceDataTypeController::FinishStart(StartResult result) { + start_callback_->Run(result); + start_callback_.reset(); +} + } // namespace browser_sync diff --git a/chrome/browser/sync/glue/preference_data_type_controller.h b/chrome/browser/sync/glue/preference_data_type_controller.h index 68305ca..d515b71 100644 --- a/chrome/browser/sync/glue/preference_data_type_controller.h +++ b/chrome/browser/sync/glue/preference_data_type_controller.h @@ -49,12 +49,20 @@ class PreferenceDataTypeController : public DataTypeController { return state_; } + // UnrecoverableErrorHandler interface. + virtual void OnUnrecoverableError(); + private: + // Helper method to run the stashed start callback with a given result. + void FinishStart(StartResult result); + ProfileSyncFactory* profile_sync_factory_; ProfileSyncService* sync_service_; State state_; + bool unrecoverable_error_detected_; + scoped_ptr<StartCallback> start_callback_; scoped_ptr<AssociatorInterface> model_associator_; scoped_ptr<ChangeProcessor> change_processor_; diff --git a/chrome/browser/sync/glue/preference_data_type_controller_unittest.cc b/chrome/browser/sync/glue/preference_data_type_controller_unittest.cc index 108a2e8..7082698 100644 --- a/chrome/browser/sync/glue/preference_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/preference_data_type_controller_unittest.cc @@ -20,6 +20,8 @@ using browser_sync::ChangeProcessorMock; using browser_sync::DataTypeController; using browser_sync::ModelAssociatorMock; using testing::_; +using testing::DoAll; +using testing::Invoke; using testing::Return; class StartCallback { @@ -40,12 +42,15 @@ class PreferenceDataTypeControllerTest : public testing::Test { } protected: - void SetAssociateExpectations() { + void SetStartExpectations() { model_associator_ = new ModelAssociatorMock(); change_processor_ = new ChangeProcessorMock(); - EXPECT_CALL(*profile_sync_factory_, CreatePreferenceSyncComponents(_)). + EXPECT_CALL(*profile_sync_factory_, CreatePreferenceSyncComponents(_, _)). WillOnce(Return(ProfileSyncFactory::SyncComponents(model_associator_, change_processor_))); + } + + void SetAssociateExpectations() { EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()). WillRepeatedly(Return(false)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()). @@ -74,6 +79,7 @@ class PreferenceDataTypeControllerTest : public testing::Test { }; TEST_F(PreferenceDataTypeControllerTest, Start) { + SetStartExpectations(); SetAssociateExpectations(); SetActivateExpectations(); EXPECT_EQ(DataTypeController::NOT_RUNNING, preference_dtc_->state()); @@ -84,6 +90,7 @@ TEST_F(PreferenceDataTypeControllerTest, Start) { } TEST_F(PreferenceDataTypeControllerTest, StartFirstRun) { + SetStartExpectations(); SetAssociateExpectations(); SetActivateExpectations(); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()). @@ -94,6 +101,7 @@ TEST_F(PreferenceDataTypeControllerTest, StartFirstRun) { } TEST_F(PreferenceDataTypeControllerTest, StartNeedsMerge) { + SetStartExpectations(); SetAssociateExpectations(); EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()). WillRepeatedly(Return(true)); @@ -106,6 +114,7 @@ TEST_F(PreferenceDataTypeControllerTest, StartNeedsMerge) { } TEST_F(PreferenceDataTypeControllerTest, StartMergeAllowed) { + SetStartExpectations(); SetAssociateExpectations(); SetActivateExpectations(); EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()). @@ -119,6 +128,7 @@ TEST_F(PreferenceDataTypeControllerTest, StartMergeAllowed) { } TEST_F(PreferenceDataTypeControllerTest, StartAssociationFailed) { + SetStartExpectations(); SetAssociateExpectations(); EXPECT_CALL(*model_associator_, AssociateModels()). WillRepeatedly(Return(false)); @@ -129,7 +139,32 @@ TEST_F(PreferenceDataTypeControllerTest, StartAssociationFailed) { EXPECT_EQ(DataTypeController::NOT_RUNNING, preference_dtc_->state()); } +TEST_F(PreferenceDataTypeControllerTest, + StartAssociationTriggersUnrecoverableError) { + SetStartExpectations(); + // Set up association to fail with an unrecoverable error. + EXPECT_CALL(*model_associator_, ChromeModelHasUserCreatedNodes()). + WillOnce(Return(false)); + EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes()). + WillOnce(DoAll(Invoke( + preference_dtc_.get(), + &PreferenceDataTypeController::OnUnrecoverableError), + Return(false))); + EXPECT_CALL(service_, OnUnrecoverableError()). + WillOnce(Invoke(preference_dtc_.get(), + &PreferenceDataTypeController::Stop)); + EXPECT_CALL(service_, DeactivateDataType(preference_dtc_.get(), + change_processor_)); + EXPECT_CALL(*model_associator_, DisassociateModels()); + + EXPECT_CALL(start_callback_, Run(DataTypeController::UNRECOVERABLE_ERROR)); + preference_dtc_->Start(true, + NewCallback(&start_callback_, &StartCallback::Run)); + EXPECT_EQ(DataTypeController::NOT_RUNNING, preference_dtc_->state()); +} + TEST_F(PreferenceDataTypeControllerTest, Stop) { + SetStartExpectations(); SetAssociateExpectations(); SetActivateExpectations(); SetStopExpectations(); @@ -142,5 +177,4 @@ TEST_F(PreferenceDataTypeControllerTest, Stop) { EXPECT_EQ(DataTypeController::RUNNING, preference_dtc_->state()); preference_dtc_->Stop(); EXPECT_EQ(DataTypeController::NOT_RUNNING, preference_dtc_->state()); - } diff --git a/chrome/browser/sync/glue/preference_model_associator.cc b/chrome/browser/sync/glue/preference_model_associator.cc index 934220e..761c2db 100644 --- a/chrome/browser/sync/glue/preference_model_associator.cc +++ b/chrome/browser/sync/glue/preference_model_associator.cc @@ -18,8 +18,10 @@ namespace browser_sync { PreferenceModelAssociator::PreferenceModelAssociator( - ProfileSyncService* sync_service) + ProfileSyncService* sync_service, + UnrecoverableErrorHandler* error_handler) : sync_service_(sync_service), + error_handler_(error_handler), preferences_node_id_(sync_api::kInvalidId), ALLOW_THIS_IN_INITIALIZER_LIST(persist_associations_(this)) { DCHECK(sync_service_); @@ -38,7 +40,7 @@ bool PreferenceModelAssociator::AssociateModels() { int64 root_id; if (!GetSyncIdForTaggedNode(kPreferencesTag, &root_id)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); LOG(ERROR) << "Server did not create the top-level preferences node. We " << "might be running against an out-of-date server."; return false; @@ -48,7 +50,7 @@ bool PreferenceModelAssociator::AssociateModels() { sync_service()->backend()->GetUserShareHandle()); sync_api::ReadNode root(&trans); if (!root.InitByIdLookup(root_id)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); LOG(ERROR) << "Server did not create the top-level preferences node. We " << "might be running against an out-of-date server."; return false; @@ -71,7 +73,7 @@ bool PreferenceModelAssociator::AssociateModels() { if (!value.get()) { LOG(ERROR) << "Failed to deserialize preference value: " << reader.error_message(); - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } @@ -89,7 +91,7 @@ bool PreferenceModelAssociator::AssociateModels() { sync_api::WriteNode node(&trans); if (!node.InitUniqueByCreation(syncable::PREFERENCES, root, tag)) { LOG(ERROR) << "Failed to create preference sync node."; - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } @@ -105,7 +107,7 @@ bool PreferenceModelAssociator::AssociateModels() { JSONStringValueSerializer json(&serialized); if (!json.Serialize(*(pref->GetValue()))) { LOG(ERROR) << "Failed to serialize preference value."; - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return false; } sync_pb::PreferenceSpecifics preference; @@ -221,7 +223,7 @@ void PreferenceModelAssociator::PersistAssociations() { int64 sync_id = *iter; sync_api::WriteNode sync_node(&trans); if (!sync_node.InitByIdLookup(sync_id)) { - sync_service_->OnUnrecoverableError(); + error_handler_->OnUnrecoverableError(); return; } // TODO(sync): Make ExternalId a string? diff --git a/chrome/browser/sync/glue/preference_model_associator.h b/chrome/browser/sync/glue/preference_model_associator.h index aa0669a..2fb51ff 100644 --- a/chrome/browser/sync/glue/preference_model_associator.h +++ b/chrome/browser/sync/glue/preference_model_associator.h @@ -14,6 +14,7 @@ #include "base/task.h" #include "chrome/browser/pref_service.h" #include "chrome/browser/sync/glue/model_associator.h" +#include "chrome/browser/sync/unrecoverable_error_handler.h" class ProfileSyncService; @@ -31,7 +32,8 @@ class PreferenceModelAssociator std::wstring> { public: static syncable::ModelType model_type() { return syncable::PREFERENCES; } - explicit PreferenceModelAssociator(ProfileSyncService* sync_service); + PreferenceModelAssociator(ProfileSyncService* sync_service, + UnrecoverableErrorHandler* error_handler); virtual ~PreferenceModelAssociator() { } // Returns the list of preference names that should be monitored for changes. @@ -96,6 +98,7 @@ class PreferenceModelAssociator void PersistAssociations(); ProfileSyncService* sync_service_; + UnrecoverableErrorHandler* error_handler_; std::set<std::wstring> synced_preferences_; int64 preferences_node_id_; diff --git a/chrome/browser/sync/profile_sync_factory.h b/chrome/browser/sync/profile_sync_factory.h index b11856b..6aef77f 100644 --- a/chrome/browser/sync/profile_sync_factory.h +++ b/chrome/browser/sync/profile_sync_factory.h @@ -10,6 +10,7 @@ #include "chrome/browser/sync/glue/change_processor.h" #include "chrome/browser/sync/glue/data_type_controller.h" #include "chrome/browser/sync/glue/model_associator.h" +#include "chrome/browser/sync/unrecoverable_error_handler.h" class ProfileSyncService; class WebDatabase; @@ -58,13 +59,15 @@ class ProfileSyncFactory { // bookmark data type. The pointers in the return struct are owned // by the caller. virtual SyncComponents CreateBookmarkSyncComponents( - ProfileSyncService* profile_sync_service) = 0; + ProfileSyncService* profile_sync_service, + browser_sync::UnrecoverableErrorHandler* error_handler) = 0; // Instantiates both a model associator and change processor for the // preference data type. The pointers in the return struct are // owned by the caller. virtual SyncComponents CreatePreferenceSyncComponents( - ProfileSyncService* profile_sync_service) = 0; + ProfileSyncService* profile_sync_service, + browser_sync::UnrecoverableErrorHandler* error_handler) = 0; }; #endif // CHROME_BROWSER_SYNC_PROFILE_SYNC_FACTORY_H__ diff --git a/chrome/browser/sync/profile_sync_factory_impl.cc b/chrome/browser/sync/profile_sync_factory_impl.cc index a1454c3..8f3e1c2 100644 --- a/chrome/browser/sync/profile_sync_factory_impl.cc +++ b/chrome/browser/sync/profile_sync_factory_impl.cc @@ -30,9 +30,11 @@ using browser_sync::BookmarkModelAssociator; using browser_sync::DataTypeController; using browser_sync::DataTypeManager; using browser_sync::DataTypeManagerImpl; +using browser_sync::PreferenceDataTypeController; using browser_sync::PreferenceChangeProcessor; using browser_sync::PreferenceDataTypeController; using browser_sync::PreferenceModelAssociator; +using browser_sync::UnrecoverableErrorHandler; ProfileSyncFactoryImpl::ProfileSyncFactoryImpl( Profile* profile, @@ -93,22 +95,26 @@ ProfileSyncFactoryImpl::CreateAutofillSyncComponents( ProfileSyncFactory::SyncComponents ProfileSyncFactoryImpl::CreateBookmarkSyncComponents( - ProfileSyncService* profile_sync_service) { + ProfileSyncService* profile_sync_service, + UnrecoverableErrorHandler* error_handler) { BookmarkModelAssociator* model_associator = - new BookmarkModelAssociator(profile_sync_service); + new BookmarkModelAssociator(profile_sync_service, + error_handler); BookmarkChangeProcessor* change_processor = new BookmarkChangeProcessor(model_associator, - profile_sync_service); + error_handler); return SyncComponents(model_associator, change_processor); } ProfileSyncFactory::SyncComponents ProfileSyncFactoryImpl::CreatePreferenceSyncComponents( - ProfileSyncService* profile_sync_service) { + ProfileSyncService* profile_sync_service, + UnrecoverableErrorHandler* error_handler) { PreferenceModelAssociator* model_associator = - new PreferenceModelAssociator(profile_sync_service); + new PreferenceModelAssociator(profile_sync_service, + error_handler); PreferenceChangeProcessor* change_processor = new PreferenceChangeProcessor(model_associator, - profile_sync_service); + error_handler); return SyncComponents(model_associator, change_processor); } diff --git a/chrome/browser/sync/profile_sync_factory_impl.h b/chrome/browser/sync/profile_sync_factory_impl.h index 94d6028..c47718e 100644 --- a/chrome/browser/sync/profile_sync_factory_impl.h +++ b/chrome/browser/sync/profile_sync_factory_impl.h @@ -28,10 +28,12 @@ class ProfileSyncFactoryImpl : public ProfileSyncFactory { browser_sync::UnrecoverableErrorHandler* error_handler); virtual SyncComponents CreateBookmarkSyncComponents( - ProfileSyncService* profile_sync_service); + ProfileSyncService* profile_sync_service, + browser_sync::UnrecoverableErrorHandler* error_handler); virtual SyncComponents CreatePreferenceSyncComponents( - ProfileSyncService* profile_sync_service); + ProfileSyncService* profile_sync_service, + browser_sync::UnrecoverableErrorHandler* error_handler); private: Profile* profile_; diff --git a/chrome/browser/sync/profile_sync_factory_mock.cc b/chrome/browser/sync/profile_sync_factory_mock.cc index 1a5179a..68fc0456 100644 --- a/chrome/browser/sync/profile_sync_factory_mock.cc +++ b/chrome/browser/sync/profile_sync_factory_mock.cc @@ -16,7 +16,7 @@ ProfileSyncFactoryMock::ProfileSyncFactoryMock( ChangeProcessor* bookmark_change_processor) : bookmark_model_associator_(bookmark_model_associator), bookmark_change_processor_(bookmark_change_processor) { - ON_CALL(*this, CreateBookmarkSyncComponents(_)). + ON_CALL(*this, CreateBookmarkSyncComponents(_, _)). WillByDefault( InvokeWithoutArgs( this, diff --git a/chrome/browser/sync/profile_sync_factory_mock.h b/chrome/browser/sync/profile_sync_factory_mock.h index 77ee37f..71f65fd 100644 --- a/chrome/browser/sync/profile_sync_factory_mock.h +++ b/chrome/browser/sync/profile_sync_factory_mock.h @@ -33,10 +33,12 @@ class ProfileSyncFactoryMock : public ProfileSyncFactory { ProfileSyncService* profile_sync_service, WebDatabase* web_database, browser_sync::UnrecoverableErrorHandler* error_handler)); - MOCK_METHOD1(CreateBookmarkSyncComponents, - SyncComponents(ProfileSyncService* profile_sync_service)); - MOCK_METHOD1(CreatePreferenceSyncComponents, - SyncComponents(ProfileSyncService* profile_sync_service)); + MOCK_METHOD2(CreateBookmarkSyncComponents, + SyncComponents(ProfileSyncService* profile_sync_service, + browser_sync::UnrecoverableErrorHandler* error_handler)); + MOCK_METHOD2(CreatePreferenceSyncComponents, + SyncComponents(ProfileSyncService* profile_sync_service, + browser_sync::UnrecoverableErrorHandler* error_handler)); private: SyncComponents MakeBookmarkSyncComponents(); diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h index 109e814..18eca8a 100644 --- a/chrome/browser/sync/profile_sync_service.h +++ b/chrome/browser/sync/profile_sync_service.h @@ -20,28 +20,10 @@ #include "chrome/browser/sync/notification_method.h" #include "chrome/browser/sync/sync_setup_wizard.h" #include "chrome/browser/sync/syncable/model_type.h" +#include "chrome/browser/sync/unrecoverable_error_handler.h" #include "googleurl/src/gurl.h" #include "testing/gtest/include/gtest/gtest_prod.h" -class ProfileSyncFactory; - -namespace browser_sync { - -class ChangeProcessor; - -class UnrecoverableErrorHandler { - public: - // Call this when normal operation detects that the bookmark model and the - // syncer model are inconsistent, or similar. The ProfileSyncService will - // try to avoid doing any work to avoid crashing or corrupting things - // further, and will report an error status if queried. - virtual void OnUnrecoverableError() = 0; - protected: - virtual ~UnrecoverableErrorHandler() { } -}; - -} - // Various UI components such as the New Tab page can be driven by observing // the ProfileSyncService through this interface. class ProfileSyncServiceObserver { diff --git a/chrome/browser/sync/profile_sync_service_preference_unittest.cc b/chrome/browser/sync/profile_sync_service_preference_unittest.cc index f4a5a7d..f17afa2 100644 --- a/chrome/browser/sync/profile_sync_service_preference_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_preference_unittest.cc @@ -31,7 +31,7 @@ class TestPreferenceModelAssociator : public TestModelAssociator<PreferenceModelAssociator> { public: explicit TestPreferenceModelAssociator(ProfileSyncService* service) - : TestModelAssociator<PreferenceModelAssociator>(service) { + : TestModelAssociator<PreferenceModelAssociator>(service, service) { } }; @@ -62,7 +62,7 @@ class ProfileSyncServicePreferenceTest : public testing::Test { model_associator_ = new TestPreferenceModelAssociator(service_.get()); change_processor_ = new PreferenceChangeProcessor(model_associator_, service_.get()); - EXPECT_CALL(factory_, CreatePreferenceSyncComponents(_)). + EXPECT_CALL(factory_, CreatePreferenceSyncComponents(_, _)). WillOnce(Return(ProfileSyncFactory::SyncComponents( model_associator_, change_processor_))); EXPECT_CALL(factory_, CreateDataTypeManager(_)). diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc index 8aca17e..af0d578 100644 --- a/chrome/browser/sync/profile_sync_service_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_unittest.cc @@ -46,7 +46,7 @@ class TestBookmarkModelAssociator : public TestModelAssociator<BookmarkModelAssociator> { public: explicit TestBookmarkModelAssociator(ProfileSyncService* service) - : TestModelAssociator<BookmarkModelAssociator>(service) { + : TestModelAssociator<BookmarkModelAssociator>(service, service) { } }; @@ -236,7 +236,7 @@ class ProfileSyncServiceTest : public testing::Test { model_associator_ = new TestBookmarkModelAssociator(service_.get()); change_processor_ = new BookmarkChangeProcessor(model_associator_, service_.get()); - EXPECT_CALL(factory_, CreateBookmarkSyncComponents(_)). + EXPECT_CALL(factory_, CreateBookmarkSyncComponents(_, _)). WillOnce(Return(ProfileSyncFactory::SyncComponents( model_associator_, change_processor_))); EXPECT_CALL(factory_, CreateDataTypeManager(_)). @@ -1314,7 +1314,7 @@ TEST_F(ProfileSyncServiceTestWithData, MAYBE_TestStartupWithOldSyncData) { model_associator_ = new TestBookmarkModelAssociator(service_.get()); change_processor_ = new BookmarkChangeProcessor(model_associator_, service_.get()); - EXPECT_CALL(factory_, CreateBookmarkSyncComponents(_)). + EXPECT_CALL(factory_, CreateBookmarkSyncComponents(_, _)). WillOnce(Return(ProfileSyncFactory::SyncComponents( model_associator_, change_processor_))); EXPECT_CALL(factory_, CreateDataTypeManager(_)). diff --git a/chrome/browser/sync/profile_sync_test_util.h b/chrome/browser/sync/profile_sync_test_util.h index 65b5263..e78ab09 100644 --- a/chrome/browser/sync/profile_sync_test_util.h +++ b/chrome/browser/sync/profile_sync_test_util.h @@ -13,6 +13,7 @@ #include "chrome/browser/sync/glue/data_type_manager_impl.h" #include "chrome/browser/sync/profile_sync_factory.h" #include "chrome/browser/sync/profile_sync_service.h" +#include "chrome/browser/sync/unrecoverable_error_handler.h" #include "chrome/test/sync/test_http_bridge_factory.h" #include "testing/gmock/include/gmock/gmock.h" @@ -25,8 +26,10 @@ ACTION(MakeDataTypeManager) { template <class ModelAssociatorImpl> class TestModelAssociator : public ModelAssociatorImpl { public: - explicit TestModelAssociator(ProfileSyncService* service) - : ModelAssociatorImpl(service) { + explicit TestModelAssociator( + ProfileSyncService* service, + browser_sync::UnrecoverableErrorHandler* error_handler) + : ModelAssociatorImpl(service, error_handler) { } TestModelAssociator(ProfileSyncService* service, diff --git a/chrome/browser/sync/unrecoverable_error_handler.h b/chrome/browser/sync/unrecoverable_error_handler.h new file mode 100644 index 0000000..2bad8c5 --- /dev/null +++ b/chrome/browser/sync/unrecoverable_error_handler.h @@ -0,0 +1,23 @@ +// Copyright (c) 2010 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_UNRECOVERABLE_ERROR_HANDLER_H_ +#define CHROME_BROWSER_SYNC_UNRECOVERABLE_ERROR_HANDLER_H_ + +namespace browser_sync { + +class UnrecoverableErrorHandler { + public: + // Call this when normal operation detects that the chrome model and the + // syncer model are inconsistent, or similar. The ProfileSyncService will + // try to avoid doing any work to avoid crashing or corrupting things + // further, and will report an error status if queried. + virtual void OnUnrecoverableError() = 0; + protected: + virtual ~UnrecoverableErrorHandler() { } +}; + +} + +#endif // CHROME_BROWSER_SYNC_UNRECOVERABLE_ERROR_HANDLER_H_ |