diff --git a/chrome/browser/sync/backend_migrator_unittest.cc b/chrome/browser/sync/backend_migrator_unittest.cc index 4fa099cc359f..b2a6ac72b02c 100644 --- a/chrome/browser/sync/backend_migrator_unittest.cc +++ b/chrome/browser/sync/backend_migrator_unittest.cc @@ -82,6 +82,7 @@ class SyncBackendMigratorTest : public testing::Test { status, requested_types, errors, + syncer::ModelTypeSet(), syncer::ModelTypeSet()); migrator_->OnConfigureDone(result); } diff --git a/chrome/browser/sync/glue/backend_data_type_configurer.h b/chrome/browser/sync/glue/backend_data_type_configurer.h index 38bdf9a317fe..308dee6f6876 100644 --- a/chrome/browser/sync/glue/backend_data_type_configurer.h +++ b/chrome/browser/sync/glue/backend_data_type_configurer.h @@ -25,7 +25,8 @@ class BackendDataTypeConfigurer { // Data of such types is left as it is, no // downloading or purging. DISABLED, // Not syncing. Disabled by user. - FAILED, // Not syncing due to unrecoverable error. + FATAL, // Not syncing due to unrecoverable error. + CRYPTO, // Not syncing due to a cryptographer error. }; typedef std::map DataTypeConfigStateMap; diff --git a/chrome/browser/sync/glue/data_type_manager.cc b/chrome/browser/sync/glue/data_type_manager.cc index 3f42c9163b65..b1cc03986f06 100644 --- a/chrome/browser/sync/glue/data_type_manager.cc +++ b/chrome/browser/sync/glue/data_type_manager.cc @@ -22,12 +22,14 @@ DataTypeManager::ConfigureResult::ConfigureResult( ConfigureStatus status, syncer::ModelTypeSet requested_types, std::map failed_data_types, - syncer::ModelTypeSet waiting_to_start) + syncer::ModelTypeSet waiting_to_start, + syncer::ModelTypeSet needs_crypto) : status(status), requested_types(requested_types), failed_data_types(failed_data_types), - waiting_to_start(waiting_to_start) { - if (!failed_data_types.empty()) { + waiting_to_start(waiting_to_start), + needs_crypto(needs_crypto) { + if (!failed_data_types.empty() || !needs_crypto.Empty()) { DCHECK_NE(OK, status); } } diff --git a/chrome/browser/sync/glue/data_type_manager.h b/chrome/browser/sync/glue/data_type_manager.h index 6e0daafa96a3..8221c7b32fdc 100644 --- a/chrome/browser/sync/glue/data_type_manager.h +++ b/chrome/browser/sync/glue/data_type_manager.h @@ -27,8 +27,7 @@ class DataTypeManager { // types. CONFIGURING, // Data types are being started. - BLOCKED, // We can't move forward with configuration because some - // external action must take place (i.e. passphrase). + RETRYING, // Retrying a pending reconfiguration. CONFIGURED, // All enabled data types are running. STOPPING // Data types are being stopped. @@ -42,8 +41,6 @@ class DataTypeManager { PARTIAL_SUCCESS, // Some data types had an error while starting up. ABORTED, // Start was aborted by calling Stop() before // all types were started. - CONFIGURE_BLOCKED, // Configuration was blocked due to missing - // passphrase. UNRECOVERABLE_ERROR // We got an unrecoverable error during startup. }; @@ -56,7 +53,8 @@ class DataTypeManager { syncer::ModelTypeSet requested_types, std::map failed_data_types, - syncer::ModelTypeSet waiting_to_start); + syncer::ModelTypeSet waiting_to_start, + syncer::ModelTypeSet needs_crypto); ~ConfigureResult(); ConfigureStatus status; syncer::ModelTypeSet requested_types; @@ -70,6 +68,10 @@ class DataTypeManager { // background. When these types are loaded DataTypeManager will // be informed and another configured cycle will be started. syncer::ModelTypeSet waiting_to_start; + + // Those types that are unable to start due to the cryptographer not being + // ready. + syncer::ModelTypeSet needs_crypto; }; virtual ~DataTypeManager() {} diff --git a/chrome/browser/sync/glue/data_type_manager_impl.cc b/chrome/browser/sync/glue/data_type_manager_impl.cc index fc34a8a0a604..d0cb846b9717 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl.cc @@ -28,6 +28,22 @@ using content::BrowserThread; namespace browser_sync { +namespace { + +FailedDataTypesHandler::TypeErrorMap +GenerateCryptoErrorsForTypes(syncer::ModelTypeSet encrypted_types) { + FailedDataTypesHandler::TypeErrorMap crypto_errors; + for (syncer::ModelTypeSet::Iterator iter = encrypted_types.First(); + iter.Good(); iter.Inc()) { + crypto_errors[iter.Get()] = syncer::SyncError(FROM_HERE, + "Cryptographer not ready.", + iter.Get()); + } + return crypto_errors; +} + +} // namespace + DataTypeManagerImpl::AssociationTypesInfo::AssociationTypesInfo() {} DataTypeManagerImpl::AssociationTypesInfo::~AssociationTypesInfo() {} @@ -48,7 +64,9 @@ DataTypeManagerImpl::DataTypeManagerImpl( debug_info_listener_(debug_info_listener), model_association_manager_(controllers, this), observer_(observer), - failed_data_types_handler_(failed_data_types_handler) { + failed_data_types_handler_(failed_data_types_handler), + encryption_handler_(encryption_handler) { + DCHECK(failed_data_types_handler_); DCHECK(configurer_); DCHECK(observer_); } @@ -74,6 +92,8 @@ void DataTypeManagerImpl::ConfigureImpl( syncer::ConfigureReason reason) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK_NE(reason, syncer::CONFIGURE_REASON_UNKNOWN); + DVLOG(1) << "Configuring for " << syncer::ModelTypeSetToString(desired_types) + << " with reason " << reason; if (state_ == STOPPING) { // You can not set a configuration while stopping. LOG(ERROR) << "Configuration set while stopping."; @@ -82,9 +102,12 @@ void DataTypeManagerImpl::ConfigureImpl( if (state_ == CONFIGURED && last_requested_types_.Equals(desired_types) && - reason == syncer::CONFIGURE_REASON_RECONFIGURATION) { - // If we're already configured and the types haven't changed, we can exit - // out early. + reason == syncer::CONFIGURE_REASON_RECONFIGURATION && + syncer::Intersection(failed_data_types_handler_->GetFailedTypes(), + last_requested_types_).Empty()) { + // If the set of enabled types hasn't changed and there are no failing + // types, we can exit out early. + DVLOG(1) << "Reconfigure with same types, bypassing confguration."; NotifyStart(); ConfigureResult result(OK, last_requested_types_); NotifyDone(result); @@ -93,8 +116,8 @@ void DataTypeManagerImpl::ConfigureImpl( last_requested_types_ = desired_types; last_configure_reason_ = reason; - // Only proceed if we're in a steady state or blocked. - if (state_ != STOPPED && state_ != CONFIGURED && state_ != BLOCKED) { + // Only proceed if we're in a steady state or retrying. + if (state_ != STOPPED && state_ != CONFIGURED && state_ != RETRYING) { DVLOG(1) << "Received configure request while configuration in flight. " << "Postponing until current configuration complete."; needs_reconfigure_ = true; @@ -107,43 +130,76 @@ void DataTypeManagerImpl::ConfigureImpl( BackendDataTypeConfigurer::DataTypeConfigStateMap DataTypeManagerImpl::BuildDataTypeConfigStateMap( const syncer::ModelTypeSet& types_being_configured) const { - // In four steps: - // 1. Add last_requested_types_ as CONFIGURE_INACTIVE. - // 2. Flip |types_being_configured| to CONFIGURE_ACTIVE. - // 3. Set other types as DISABLED. - // 4. Overwrite state of failed types according to failed_data_types_handler_. + // 1. Get the failed types (both due to fatal and crypto errors). + // 2. Add the difference between last_requested_types_ and the failed types + // as CONFIGURE_INACTIVE. + // 3. Flip |types_being_configured| to CONFIGURE_ACTIVE. + // 4. Set non-enabled user types as DISABLED. + // 5. Set the fatal and crypto types to their respective states. + syncer::ModelTypeSet fatal_types; + syncer::ModelTypeSet crypto_types; + fatal_types = failed_data_types_handler_->GetFatalErrorTypes(); + crypto_types = failed_data_types_handler_->GetCryptoErrorTypes(); + syncer::ModelTypeSet enabled_types = last_requested_types_; + enabled_types.RemoveAll(fatal_types); + enabled_types.RemoveAll(crypto_types); + syncer::ModelTypeSet disabled_types = + syncer::Difference( + syncer::Union(syncer::UserTypes(), syncer::ControlTypes()), + enabled_types); + syncer::ModelTypeSet to_configure = syncer::Intersection( + enabled_types, types_being_configured); + DVLOG(1) << "Enabling: " << syncer::ModelTypeSetToString(enabled_types); + DVLOG(1) << "Configuring: " << syncer::ModelTypeSetToString(to_configure); + DVLOG(1) << "Disabling: " << syncer::ModelTypeSetToString(disabled_types); + BackendDataTypeConfigurer::DataTypeConfigStateMap config_state_map; BackendDataTypeConfigurer::SetDataTypesState( - BackendDataTypeConfigurer::CONFIGURE_INACTIVE, last_requested_types_, + BackendDataTypeConfigurer::CONFIGURE_INACTIVE, enabled_types, &config_state_map); BackendDataTypeConfigurer::SetDataTypesState( - BackendDataTypeConfigurer::CONFIGURE_ACTIVE, - types_being_configured, &config_state_map); + BackendDataTypeConfigurer::CONFIGURE_ACTIVE, to_configure, + &config_state_map); BackendDataTypeConfigurer::SetDataTypesState( - BackendDataTypeConfigurer::DISABLED, - syncer::Difference(syncer::UserTypes(), last_requested_types_), + BackendDataTypeConfigurer::DISABLED, disabled_types, &config_state_map); BackendDataTypeConfigurer::SetDataTypesState( - BackendDataTypeConfigurer::DISABLED, - syncer::Difference(syncer::ControlTypes(), last_requested_types_), + BackendDataTypeConfigurer::FATAL, fatal_types, &config_state_map); - if (failed_data_types_handler_) { - BackendDataTypeConfigurer::SetDataTypesState( - BackendDataTypeConfigurer::FAILED, - failed_data_types_handler_->GetFailedTypes(), + BackendDataTypeConfigurer::SetDataTypesState( + BackendDataTypeConfigurer::CRYPTO, crypto_types, &config_state_map); - } return config_state_map; } void DataTypeManagerImpl::Restart(syncer::ConfigureReason reason) { DVLOG(1) << "Restarting..."; - model_association_manager_.Initialize(last_requested_types_); + + // Check for new or resolved data type crypto errors. + if (encryption_handler_->IsPassphraseRequired()) { + syncer::ModelTypeSet encrypted_types = + encryption_handler_->GetEncryptedDataTypes(); + encrypted_types.RetainAll(last_requested_types_); + encrypted_types.RemoveAll( + failed_data_types_handler_->GetCryptoErrorTypes()); + FailedDataTypesHandler::TypeErrorMap crypto_errors = + GenerateCryptoErrorsForTypes(encrypted_types); + failed_data_types_handler_->UpdateFailedDataTypes( + crypto_errors, + FailedDataTypesHandler::CRYPTO); + } else { + failed_data_types_handler_->ResetCryptoErrors(); + } + + syncer::ModelTypeSet failed_types = + failed_data_types_handler_->GetFailedTypes(); + syncer::ModelTypeSet enabled_types = + syncer::Difference(last_requested_types_, failed_types); + + model_association_manager_.Initialize(enabled_types); last_restart_time_ = base::Time::Now(); - configure_result_ = ConfigureResult(); - configure_result_.status = OK; - DCHECK(state_ == STOPPED || state_ == CONFIGURED || state_ == BLOCKED); + DCHECK(state_ == STOPPED || state_ == CONFIGURED || state_ == RETRYING); // Starting from a "steady state" (stopped or configured) state // should send a start notification. @@ -152,7 +208,7 @@ void DataTypeManagerImpl::Restart(syncer::ConfigureReason reason) { model_association_manager_.StopDisabledTypes(); - download_types_queue_ = PrioritizeTypes(last_requested_types_); + download_types_queue_ = PrioritizeTypes(enabled_types); association_types_queue_ = std::queue(); // Tell the backend about the new set of data types we wish to sync. @@ -201,20 +257,17 @@ void DataTypeManagerImpl::ProcessReconfigure() { // the most recent set of desired types, so we just call configure. // Note: we do this whether or not GetControllersNeedingStart is true, // because we may need to stop datatypes. - SetBlockedAndNotify(); DVLOG(1) << "Reconfiguring due to previous configure attempt occuring while" << " busy."; - // Unwind the stack before executing configure. The method configure and its - // callees are not re-entrant. - base::MessageLoop::current()->PostTask( - FROM_HERE, - base::Bind(&DataTypeManagerImpl::ConfigureImpl, - weak_ptr_factory_.GetWeakPtr(), - last_requested_types_, - last_configure_reason_)); - + // Note: ConfigureImpl is called directly, rather than posted, in order to + // ensure that any purging/unapplying/journaling happens while the set of + // failed types is still up to date. If stack unwinding were to be done + // via PostTask, the failed data types may be reset before the purging was + // performed. + state_ = RETRYING; needs_reconfigure_ = false; + ConfigureImpl(last_requested_types_, last_configure_reason_); } void DataTypeManagerImpl::OnDownloadRetry() { @@ -328,6 +381,29 @@ void DataTypeManagerImpl::OnModelAssociationDone( if (state_ == STOPPING) return; + // Don't reconfigure due to failed data types if we have an unrecoverable + // error or have already aborted. + if (result.status == PARTIAL_SUCCESS) { + if (!result.needs_crypto.Empty()) { + needs_reconfigure_ = true; + syncer::ModelTypeSet encrypted_types = result.needs_crypto; + encrypted_types.RemoveAll( + failed_data_types_handler_->GetCryptoErrorTypes()); + FailedDataTypesHandler::TypeErrorMap crypto_errors = + GenerateCryptoErrorsForTypes(encrypted_types); + failed_data_types_handler_->UpdateFailedDataTypes( + crypto_errors, + FailedDataTypesHandler::CRYPTO); + } + if (!result.failed_data_types.empty()) { + needs_reconfigure_ = true; + failed_data_types_handler_->UpdateFailedDataTypes( + result.failed_data_types, + FailedDataTypesHandler::STARTUP); + } + } + + // Ignore abort/unrecoverable error if we need to reconfigure anyways. if (needs_reconfigure_) { association_types_queue_ = std::queue(); ProcessReconfigure(); @@ -341,27 +417,33 @@ void DataTypeManagerImpl::OnModelAssociationDone( return; } - if (result.status == CONFIGURE_BLOCKED) { - SetBlockedAndNotify(); - return; - } - DCHECK(result.status == PARTIAL_SUCCESS || result.status == OK); - - if (result.status == PARTIAL_SUCCESS) - configure_result_.status = PARTIAL_SUCCESS; - configure_result_.requested_types.PutAll(result.requested_types); - configure_result_.failed_data_types.insert( - result.failed_data_types.begin(), - result.failed_data_types.end()); - configure_result_.waiting_to_start.PutAll(result.waiting_to_start); + DCHECK(!result.status == OK || + (result.needs_crypto.Empty() && + result.failed_data_types.empty())); + + // It's possible this is a retry to disable failed types, in which case + // the association would be SUCCESS, but the overall configuration should + // still be PARTIAL_SUCCESS. + syncer::ModelTypeSet failed_data_types = + failed_data_types_handler_->GetFailedTypes(); + ConfigureStatus status = result.status; + if (!syncer::Intersection(last_requested_types_, + failed_data_types).Empty() && result.status == OK) { + status = PARTIAL_SUCCESS; + } association_types_queue_.pop(); if (!association_types_queue_.empty()) { StartNextAssociation(); } else if (download_types_queue_.empty()) { state_ = CONFIGURED; - NotifyDone(configure_result_); + ConfigureResult configure_result(status, + result.requested_types, + failed_data_types_handler_->GetAllErrors(), + result.waiting_to_start, + result.needs_crypto); + NotifyDone(configure_result); } } @@ -381,13 +463,15 @@ void DataTypeManagerImpl::Stop() { if (state_ == STOPPED) return; - bool need_to_notify = state_ == DOWNLOAD_PENDING || state_ == CONFIGURING; + bool need_to_notify = + state_ == DOWNLOAD_PENDING || state_ == CONFIGURING; StopImpl(); if (need_to_notify) { ConfigureResult result(ABORTED, last_requested_types_, std::map(), + syncer::ModelTypeSet(), syncer::ModelTypeSet()); NotifyDone(result); } @@ -406,6 +490,7 @@ void DataTypeManagerImpl::Abort(ConfigureStatus status, ConfigureResult result(status, last_requested_types_, errors, + syncer::ModelTypeSet(), syncer::ModelTypeSet()); NotifyDone(result); } @@ -464,17 +549,6 @@ DataTypeManager::State DataTypeManagerImpl::state() const { return state_; } -void DataTypeManagerImpl::SetBlockedAndNotify() { - // Drop download callbacks. - weak_ptr_factory_.InvalidateWeakPtrs(); - - state_ = BLOCKED; - AddToConfigureTime(); - DVLOG(1) << "Accumulated spent configuring: " - << configure_time_delta_.InSecondsF() << "s"; - observer_->OnConfigureBlocked(); -} - void DataTypeManagerImpl::AddToConfigureTime() { DCHECK(!last_restart_time_.is_null()); configure_time_delta_ += (base::Time::Now() - last_restart_time_); diff --git a/chrome/browser/sync/glue/data_type_manager_impl.h b/chrome/browser/sync/glue/data_type_manager_impl.h index 7af69fd4875e..c2fb91bc25df 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl.h +++ b/chrome/browser/sync/glue/data_type_manager_impl.h @@ -98,7 +98,6 @@ class DataTypeManagerImpl : public DataTypeManager, void OnDownloadRetry(); void NotifyStart(); void NotifyDone(const ConfigureResult& result); - void SetBlockedAndNotify(); // Add to |configure_time_delta_| the time since we last called // Restart(). @@ -139,7 +138,7 @@ class DataTypeManagerImpl : public DataTypeManager, base::Time last_restart_time_; // The accumulated time spent between calls to Restart() and going - // to the DONE/BLOCKED state. + // to the DONE state. base::TimeDelta configure_time_delta_; // Sync's datatype debug info listener, which we pass model association @@ -175,9 +174,9 @@ class DataTypeManagerImpl : public DataTypeManager, }; std::queue association_types_queue_; - // Configuration result. This would eventually be sent to the listeners after - // all the types have been given a chance to start. - ConfigureResult configure_result_; + // The encryption handler lets the DataTypeManager know the state of sync + // datatype encryption. + const browser_sync::DataTypeEncryptionHandler* encryption_handler_; DISALLOW_COPY_AND_ASSIGN(DataTypeManagerImpl); }; diff --git a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc index 9b2100f478c5..a341d61f1993 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc @@ -8,6 +8,7 @@ #include "base/message_loop.h" #include "chrome/browser/sync/glue/backend_data_type_configurer.h" #include "chrome/browser/sync/glue/data_type_controller.h" +#include "chrome/browser/sync/glue/data_type_encryption_handler.h" #include "chrome/browser/sync/glue/data_type_manager_observer.h" #include "chrome/browser/sync/glue/failed_data_types_handler.h" #include "chrome/browser/sync/glue/fake_data_type_controller.h" @@ -32,6 +33,14 @@ using testing::_; using testing::Mock; using testing::ResultOf; +namespace { + +// Used by SetConfigureDoneExpectation. +DataTypeManager::ConfigureStatus GetStatus( + const DataTypeManager::ConfigureResult& result) { + return result.status; +} + // Fake BackendDataTypeConfigurer implementation that simply stores away the // callback passed into ConfigureDataTypes. class FakeBackendDataTypeConfigurer : public BackendDataTypeConfigurer { @@ -77,17 +86,42 @@ class DataTypeManagerObserverMock : public DataTypeManagerObserver { DataTypeManagerObserverMock() {} virtual ~DataTypeManagerObserverMock() {} - MOCK_METHOD0(OnConfigureBlocked, void()); MOCK_METHOD1(OnConfigureDone, void(const browser_sync::DataTypeManager::ConfigureResult&)); MOCK_METHOD0(OnConfigureRetry, void()); MOCK_METHOD0(OnConfigureStart, void()); }; -// Used by SetConfigureDoneExpectation. -DataTypeManager::ConfigureStatus GetStatus( - const DataTypeManager::ConfigureResult& result) { - return result.status; +class FakeDataTypeEncryptionHandler : public DataTypeEncryptionHandler { + public: + FakeDataTypeEncryptionHandler(); + virtual ~FakeDataTypeEncryptionHandler(); + + virtual bool IsPassphraseRequired() const OVERRIDE; + virtual syncer::ModelTypeSet GetEncryptedDataTypes() const OVERRIDE; + + void set_passphrase_required(bool passphrase_required) { + passphrase_required_ = passphrase_required; + } + void set_encrypted_types(syncer::ModelTypeSet encrypted_types) { + encrypted_types_ = encrypted_types; + } + private: + bool passphrase_required_; + syncer::ModelTypeSet encrypted_types_; +}; + +FakeDataTypeEncryptionHandler::FakeDataTypeEncryptionHandler() + : passphrase_required_(false) {} +FakeDataTypeEncryptionHandler::~FakeDataTypeEncryptionHandler() {} + +bool FakeDataTypeEncryptionHandler::IsPassphraseRequired() const { + return passphrase_required_; +} + +syncer::ModelTypeSet +FakeDataTypeEncryptionHandler::GetEncryptedDataTypes() const { + return encrypted_types_; } class TestDataTypeManager : public DataTypeManagerImpl { @@ -97,11 +131,12 @@ class TestDataTypeManager : public DataTypeManagerImpl { debug_info_listener, BackendDataTypeConfigurer* configurer, const DataTypeController::TypeMap* controllers, + const DataTypeEncryptionHandler* encryption_handler, DataTypeManagerObserver* observer, FailedDataTypesHandler* failed_data_types_handler) : DataTypeManagerImpl(debug_info_listener, controllers, - NULL, + encryption_handler, configurer, observer, failed_data_types_handler) {} @@ -143,6 +178,7 @@ class SyncDataTypeManagerImplTest : public testing::Test { syncer::WeakHandle(), &configurer_, &controllers_, + &encryption_handler_, &observer_, &failed_data_types_handler_)); } @@ -151,10 +187,6 @@ class SyncDataTypeManagerImplTest : public testing::Test { EXPECT_CALL(observer_, OnConfigureStart()); } - void SetConfigureBlockedExpectation() { - EXPECT_CALL(observer_, OnConfigureBlocked()); - } - void SetConfigureDoneExpectation(DataTypeManager::ConfigureStatus status) { EXPECT_CALL(observer_, OnConfigureDone(ResultOf(&GetStatus, status))); } @@ -197,6 +229,11 @@ class SyncDataTypeManagerImplTest : public testing::Test { static_cast(it->second.get())); } + void FailEncryptionFor(syncer::ModelTypeSet encrypted_types) { + encryption_handler_.set_passphrase_required(true); + encryption_handler_.set_encrypted_types(encrypted_types); + } + base::MessageLoopForUI ui_loop_; content::TestBrowserThread ui_thread_; DataTypeController::TypeMap controllers_; @@ -204,6 +241,7 @@ class SyncDataTypeManagerImplTest : public testing::Test { DataTypeManagerObserverMock observer_; scoped_ptr dtm_; FailedDataTypesHandler failed_data_types_handler_; + FakeDataTypeEncryptionHandler encryption_handler_; }; // Set up a DTM with no controllers, configure it, finish downloading, @@ -363,15 +401,13 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureOneStopWhileAssociating) { // 1) Configure. // 2) Finish the download for step 1. // 3) Finish starting the controller with the NEEDS_CRYPTO status. -// 4) Configure again. -// 5) Finish the download for step 4. -// 6) Finish starting the controller successfully. -// 7) Stop the DTM. +// 4) Complete download for the reconfiguration without the controller. +// 5) Stop the DTM. TEST_F(SyncDataTypeManagerImplTest, OneWaitingForCrypto) { AddController(PASSWORDS); SetConfigureStartExpectation(); - SetConfigureBlockedExpectation(); + SetConfigureDoneExpectation(DataTypeManager::PARTIAL_SUCCESS); const ModelTypeSet types(PASSWORDS); @@ -384,25 +420,15 @@ TEST_F(SyncDataTypeManagerImplTest, OneWaitingForCrypto) { EXPECT_EQ(DataTypeManager::CONFIGURING, dtm_->state()); // Step 3. + FailEncryptionFor(types); GetController(PASSWORDS)->FinishStart(DataTypeController::NEEDS_CRYPTO); - EXPECT_EQ(DataTypeManager::BLOCKED, dtm_->state()); - - Mock::VerifyAndClearExpectations(&observer_); - SetConfigureDoneExpectation(DataTypeManager::OK); - - // Step 4. - Configure(dtm_.get(), types); EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm_->state()); - // Step 5. - FinishDownload(*dtm_, types, ModelTypeSet()); - EXPECT_EQ(DataTypeManager::CONFIGURING, dtm_->state()); - - // Step 6. - GetController(PASSWORDS)->FinishStart(DataTypeController::OK); + // Step 4. + FinishDownload(*dtm_, ModelTypeSet(), ModelTypeSet()); EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state()); - // Step 7. + // Step 5. dtm_->Stop(); EXPECT_EQ(DataTypeManager::STOPPED, dtm_->state()); } @@ -519,7 +545,6 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureWhileOneInFlight) { AddController(PREFERENCES); SetConfigureStartExpectation(); - SetConfigureBlockedExpectation(); SetConfigureDoneExpectation(DataTypeManager::OK); // Step 1. @@ -536,11 +561,6 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureWhileOneInFlight) { // Step 4. GetController(BOOKMARKS)->FinishStart(DataTypeController::OK); - EXPECT_EQ(DataTypeManager::BLOCKED, dtm_->state()); - - // Pump the loop to run the posted DTMI::ConfigureImpl() task from - // DTMI::ProcessReconfigure() (triggered by FinishStart()). - ui_loop_.RunUntilIdle(); EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm_->state()); // Step 5. @@ -613,9 +633,10 @@ TEST_F(SyncDataTypeManagerImplTest, SecondControllerFails) { // // 1) Configure with both controllers. // 2) Finish the download for step 1. -// 3) Finish starting the first controller with an association failure. -// 4) Finish starting the second controller successfully. -// 5) Stop the DTM. +// 3) Finish starting the first controller successfully. +// 4) Finish starting the second controller with an association failure. +// 5) Finish the purge/reconfigure without the failed type. +// 6) Stop the DTM. // // The association failure from step 3 should be ignored. // @@ -643,9 +664,13 @@ TEST_F(SyncDataTypeManagerImplTest, OneControllerFailsAssociation) { // Step 4. GetController(PREFERENCES)->FinishStart( DataTypeController::ASSOCIATION_FAILED); - EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state()); + EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm_->state()); // Step 5. + FinishDownload(*dtm_, ModelTypeSet(BOOKMARKS), ModelTypeSet()); + EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state()); + + // Step 6. dtm_->Stop(); EXPECT_EQ(DataTypeManager::STOPPED, dtm_->state()); } @@ -663,7 +688,6 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureWhileDownloadPending) { AddController(PREFERENCES); SetConfigureStartExpectation(); - SetConfigureBlockedExpectation(); SetConfigureDoneExpectation(DataTypeManager::OK); // Step 1. @@ -676,11 +700,6 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureWhileDownloadPending) { // Step 3. FinishDownload(*dtm_, ModelTypeSet(BOOKMARKS), ModelTypeSet()); - EXPECT_EQ(DataTypeManager::BLOCKED, dtm_->state()); - - // Pump the loop to run the posted DTMI::ConfigureImpl() task from - // DTMI::ProcessReconfigure() (triggered by step 3). - ui_loop_.RunUntilIdle(); EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm_->state()); // Step 4. @@ -714,7 +733,6 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureWhileDownloadPendingWithFailure) { AddController(PREFERENCES); SetConfigureStartExpectation(); - SetConfigureBlockedExpectation(); SetConfigureDoneExpectation(DataTypeManager::OK); // Step 1. @@ -727,11 +745,6 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureWhileDownloadPendingWithFailure) { // Step 3. FinishDownload(*dtm_, ModelTypeSet(BOOKMARKS), ModelTypeSet(BOOKMARKS)); - EXPECT_EQ(DataTypeManager::BLOCKED, dtm_->state()); - - // Pump the loop to run the posted DTMI::ConfigureImpl() task from - // DTMI::ProcessReconfigure() (triggered by step 3). - ui_loop_.RunUntilIdle(); EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm_->state()); // Step 4. @@ -818,19 +831,14 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureDuringPurge) { // - PREFERENCES: which is new and will need to be downloaded, and // - NIGORI: (added implicitly because it is a control type) which // the DTM is part-way through purging. - SetConfigureBlockedExpectation(); Configure(dtm_.get(), ModelTypeSet(BOOKMARKS, PREFERENCES)); EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm_->state()); // Invoke the callback we've been waiting for since we asked to purge NIGORI. FinishDownload(*dtm_, ModelTypeSet(), ModelTypeSet()); - EXPECT_EQ(DataTypeManager::BLOCKED, dtm_->state()); Mock::VerifyAndClearExpectations(&observer_); SetConfigureDoneExpectation(DataTypeManager::OK); - // Pump the loop to run the posted DTMI::ConfigureImpl() task from - // DTMI::ProcessReconfigure(). - ui_loop_.RunUntilIdle(); EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm_->state()); // Now invoke the callback for the second configure request. @@ -888,7 +896,6 @@ TEST_F(SyncDataTypeManagerImplTest, PrioritizedConfigurationReconfigure) { // Initial configure. SetConfigureStartExpectation(); SetConfigureDoneExpectation(DataTypeManager::OK); - SetConfigureBlockedExpectation(); // Reconfigure while associating PREFERENCES and downloading BOOKMARKS. configurer_.set_expected_configure_types(ModelTypeSet(PREFERENCES)); @@ -907,12 +914,9 @@ TEST_F(SyncDataTypeManagerImplTest, PrioritizedConfigurationReconfigure) { // Reconfiguration starts after downloading and association of previous // types finish. + configurer_.set_expected_configure_types(ModelTypeSet(PREFERENCES)); FinishDownload(*dtm_, ModelTypeSet(BOOKMARKS), ModelTypeSet()); GetController(PREFERENCES)->FinishStart(DataTypeController::OK); - EXPECT_EQ(DataTypeManager::BLOCKED, dtm_->state()); - - configurer_.set_expected_configure_types(ModelTypeSet(PREFERENCES)); - ui_loop_.RunUntilIdle(); // Let reconfigure closure run. EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm_->state()); configurer_.set_expected_configure_types(ModelTypeSet(BOOKMARKS, APPS)); @@ -940,6 +944,7 @@ TEST_F(SyncDataTypeManagerImplTest, PrioritizedConfigurationStop) { // Initial configure. SetConfigureStartExpectation(); + SetConfigureDoneExpectation(DataTypeManager::ABORTED); // Initially only PREFERENCES is configured. configurer_.set_expected_configure_types(ModelTypeSet(PREFERENCES)); @@ -1036,4 +1041,6 @@ TEST_F(SyncDataTypeManagerImplTest, PrioritizedConfigurationAssociationError) { EXPECT_EQ(DataTypeController::NOT_RUNNING, GetController(BOOKMARKS)->state()); } +} // namespace + } // namespace browser_sync diff --git a/chrome/browser/sync/glue/data_type_manager_observer.h b/chrome/browser/sync/glue/data_type_manager_observer.h index 9a86c1c3d4de..f67efd96be5b 100644 --- a/chrome/browser/sync/glue/data_type_manager_observer.h +++ b/chrome/browser/sync/glue/data_type_manager_observer.h @@ -13,7 +13,6 @@ namespace browser_sync { // DataTypeManager through this interface. class DataTypeManagerObserver { public: - virtual void OnConfigureBlocked() = 0; virtual void OnConfigureDone( const browser_sync::DataTypeManager::ConfigureResult& result) = 0; virtual void OnConfigureRetry() = 0; diff --git a/chrome/browser/sync/glue/failed_data_types_handler.cc b/chrome/browser/sync/glue/failed_data_types_handler.cc index 8f792b702440..594911fcf4e6 100644 --- a/chrome/browser/sync/glue/failed_data_types_handler.cc +++ b/chrome/browser/sync/glue/failed_data_types_handler.cc @@ -30,12 +30,6 @@ FailedDataTypesHandler::FailedDataTypesHandler() { FailedDataTypesHandler::~FailedDataTypesHandler() { } -syncer::ModelTypeSet FailedDataTypesHandler::GetFailedTypes() const { - syncer::ModelTypeSet result = GetTypesFromErrorMap(startup_errors_); - result.PutAll(GetTypesFromErrorMap(runtime_errors_)); - return result; -} - bool FailedDataTypesHandler::UpdateFailedDataTypes( const TypeErrorMap& errors, FailureType failure_type) { @@ -43,6 +37,8 @@ bool FailedDataTypesHandler::UpdateFailedDataTypes( runtime_errors_.insert(errors.begin(), errors.end()); } else if (failure_type == STARTUP) { startup_errors_.insert(errors.begin(), errors.end()); + } else if (failure_type == CRYPTO) { + crypto_errors_.insert(errors.begin(), errors.end()); } else { NOTREACHED(); } @@ -53,21 +49,45 @@ bool FailedDataTypesHandler::UpdateFailedDataTypes( void FailedDataTypesHandler::Reset() { startup_errors_.clear(); runtime_errors_.clear(); + crypto_errors_.clear(); +} + +void FailedDataTypesHandler::ResetCryptoErrors() { + crypto_errors_.clear(); } FailedDataTypesHandler::TypeErrorMap FailedDataTypesHandler::GetAllErrors() const { TypeErrorMap result; - if (AnyFailedDatatype()) { + if (AnyFailedDataType()) { result = startup_errors_; result.insert(runtime_errors_.begin(), runtime_errors_.end()); + result.insert(crypto_errors_.begin(), crypto_errors_.end()); } return result; } -bool FailedDataTypesHandler::AnyFailedDatatype() const { - return (!startup_errors_.empty() || !runtime_errors_.empty()); +syncer::ModelTypeSet FailedDataTypesHandler::GetFailedTypes() const { + syncer::ModelTypeSet result = GetFatalErrorTypes(); + result.PutAll(GetCryptoErrorTypes()); + return result; +} + +syncer::ModelTypeSet FailedDataTypesHandler::GetFatalErrorTypes() const { + syncer::ModelTypeSet result = GetTypesFromErrorMap(startup_errors_); + result.PutAll(GetTypesFromErrorMap(runtime_errors_)); + return result; +} + +syncer::ModelTypeSet FailedDataTypesHandler::GetCryptoErrorTypes() const { + syncer::ModelTypeSet result = GetTypesFromErrorMap(crypto_errors_); + return result; +} + +bool FailedDataTypesHandler::AnyFailedDataType() const { + return (!startup_errors_.empty() || !runtime_errors_.empty() || + !crypto_errors_.empty()); } } // namespace browser_sync diff --git a/chrome/browser/sync/glue/failed_data_types_handler.h b/chrome/browser/sync/glue/failed_data_types_handler.h index bb63f0b6fb1f..1998cd808be5 100644 --- a/chrome/browser/sync/glue/failed_data_types_handler.h +++ b/chrome/browser/sync/glue/failed_data_types_handler.h @@ -19,7 +19,10 @@ class FailedDataTypesHandler { STARTUP, // The datatype encountered a runtime error. - RUNTIME + RUNTIME, + + // The datatype encountered a cryptographer related error. + CRYPTO }; typedef std::map TypeErrorMap; @@ -34,15 +37,24 @@ class FailedDataTypesHandler { // Resets the current set of data type errors. void Reset(); + // Resets the set of types with cryptographer errors. + void ResetCryptoErrors(); + // Returns a list of all the errors this class has recorded. TypeErrorMap GetAllErrors() const; - // Returns the types that are failing. + // Returns all types with errors. syncer::ModelTypeSet GetFailedTypes() const; + // Returns the types that are failing due to startup or runtime errors. + syncer::ModelTypeSet GetFatalErrorTypes() const; + + // Returns the types that are failing due to cryptographer errors. + syncer::ModelTypeSet GetCryptoErrorTypes() const; + private: - // Returns if there are any failed types. - bool AnyFailedDatatype() const; + // Returns true if there are any types with errors. + bool AnyFailedDataType() const; // List of data types that failed at startup due to association errors. TypeErrorMap startup_errors_; @@ -50,6 +62,9 @@ class FailedDataTypesHandler { // List of data types that failed at runtime. TypeErrorMap runtime_errors_; + // List of data types that failed due to the cryptographer not being ready. + TypeErrorMap crypto_errors_; + DISALLOW_COPY_AND_ASSIGN(FailedDataTypesHandler); }; diff --git a/chrome/browser/sync/glue/fake_data_type_controller.cc b/chrome/browser/sync/glue/fake_data_type_controller.cc index df36a195c2eb..d424a1a98a4d 100644 --- a/chrome/browser/sync/glue/fake_data_type_controller.cc +++ b/chrome/browser/sync/glue/fake_data_type_controller.cc @@ -70,10 +70,11 @@ void FakeDataTypeController::FinishStart(StartResult result) { local_merge_result.set_error( syncer::SyncError(FROM_HERE, "Fake error", type())); } - last_start_callback_.Run(result, - local_merge_result, - syncer_merge_result); + StartCallback start_callback = last_start_callback_; last_start_callback_.Reset(); + start_callback.Run(result, + local_merge_result, + syncer_merge_result); } // * -> NOT_RUNNING diff --git a/chrome/browser/sync/glue/model_association_manager.cc b/chrome/browser/sync/glue/model_association_manager.cc index c472c2bbb98f..4c68714f650f 100644 --- a/chrome/browser/sync/glue/model_association_manager.cc +++ b/chrome/browser/sync/glue/model_association_manager.cc @@ -143,6 +143,7 @@ void ModelAssociationManager::Initialize(syncer::ModelTypeSet desired_types) { needs_stop_.clear(); failed_data_types_info_.clear(); associating_types_.Clear(); + needs_crypto_types_.Clear(); state_ = INITIALIZED_TO_CONFIGURE; DVLOG(1) << "ModelAssociationManager: Initializing"; @@ -206,7 +207,9 @@ void ModelAssociationManager::ResetForReconfiguration() { DVLOG(1) << "ModelAssociationManager: Reseting for reconfiguration"; needs_start_.clear(); needs_stop_.clear(); + associating_types_.Clear(); failed_data_types_info_.clear(); + needs_crypto_types_.Clear(); } void ModelAssociationManager::StopDisabledTypes() { @@ -262,16 +265,17 @@ void ModelAssociationManager::Stop() { } } + DataTypeManager::ConfigureResult result(DataTypeManager::ABORTED, + associating_types_, + failed_data_types_info_, + syncer::ModelTypeSet(), + needs_crypto_types_); + failed_data_types_info_.clear(); + needs_crypto_types_.Clear(); if (need_to_call_model_association_done) { DVLOG(1) << "ModelAssociationManager: Calling OnModelAssociationDone"; - DataTypeManager::ConfigureResult result(DataTypeManager::ABORTED, - associating_types_, - failed_data_types_info_, - syncer::ModelTypeSet()); result_processor_->OnModelAssociationDone(result); } - - failed_data_types_info_.clear(); } bool ModelAssociationManager::GetControllersNeedingStart( @@ -338,6 +342,9 @@ void ModelAssociationManager::TypeStartCallback( DVLOG(1) << "ModelAssociationManager: Encountered a failed type"; AppendToFailedDatatypesAndLogError(start_result, local_merge_result.error()); + } else if (start_result == DataTypeController::NEEDS_CRYPTO) { + DVLOG(1) << "ModelAssociationManager: Encountered an undecryptable type"; + needs_crypto_types_.Put(started_dtc->type()); } // Track the merge results if we succeeded or an association failure @@ -399,7 +406,8 @@ void ModelAssociationManager::TypeStartCallback( DataTypeManager::ConfigureResult configure_result(configure_status, associating_types_, errors, - syncer::ModelTypeSet()); + syncer::ModelTypeSet(), + needs_crypto_types_); result_processor_->OnModelAssociationDone(configure_result); } @@ -516,27 +524,12 @@ void ModelAssociationManager::StartAssociatingNextType() { // We are done with this cycle of association. state_ = IDLE; - // Do a fresh calculation to see if controllers need starting to account for - // things like encryption, which may still need to be sorted out before we - // can announce we're "Done" configuration entirely. - if (GetControllersNeedingStart(NULL)) { - DVLOG(1) << "ModelAssociationManager: GetControllersNeedingStart" - << " returned true. Blocking DataTypeManager"; - - DataTypeManager::ConfigureResult configure_result( - DataTypeManager::CONFIGURE_BLOCKED, - associating_types_, - failed_data_types_info_, - syncer::ModelTypeSet()); - state_ = IDLE; - result_processor_->OnModelAssociationDone(configure_result); - return; - } DataTypeManager::ConfigureStatus configure_status = DataTypeManager::OK; if (!failed_data_types_info_.empty() || - !GetTypesWaitingToLoad().Empty()) { + !GetTypesWaitingToLoad().Empty() || + !needs_crypto_types_.Empty()) { // We have not configured all types that we have been asked to configure. // Either we have failed types or types that have not completed loading // yet. @@ -547,7 +540,8 @@ void ModelAssociationManager::StartAssociatingNextType() { DataTypeManager::ConfigureResult result(configure_status, associating_types_, failed_data_types_info_, - GetTypesWaitingToLoad()); + GetTypesWaitingToLoad(), + needs_crypto_types_); result_processor_->OnModelAssociationDone(result); return; } diff --git a/chrome/browser/sync/glue/model_association_manager.h b/chrome/browser/sync/glue/model_association_manager.h index ccb8ec7e261d..5d4ed256aa78 100644 --- a/chrome/browser/sync/glue/model_association_manager.h +++ b/chrome/browser/sync/glue/model_association_manager.h @@ -135,6 +135,10 @@ class ModelAssociationManager { // been given a chance to start. std::map failed_data_types_info_; + // The set of types that can't configure due to cryptographer errors. + syncer::ModelTypeSet needs_crypto_types_; + + // The order in which association of the datatypes should be performed. std::map start_order_; // This illustration explains the movement of one DTC through various lists. diff --git a/chrome/browser/sync/glue/model_association_manager_unittest.cc b/chrome/browser/sync/glue/model_association_manager_unittest.cc index b3e3ba5e6fc0..a36596f6375a 100644 --- a/chrome/browser/sync/glue/model_association_manager_unittest.cc +++ b/chrome/browser/sync/glue/model_association_manager_unittest.cc @@ -82,6 +82,7 @@ TEST_F(SyncModelAssociationManagerTest, SimpleModelStart) { DataTypeManager::OK, types, std::map(), + syncer::ModelTypeSet(), syncer::ModelTypeSet()); EXPECT_CALL(result_processor_, OnModelAssociationDone(_)). WillOnce(VerifyResult(expected_result)); @@ -111,6 +112,7 @@ TEST_F(SyncModelAssociationManagerTest, StopModelBeforeFinish) { DataTypeManager::ABORTED, types, std::map(), + syncer::ModelTypeSet(), syncer::ModelTypeSet()); EXPECT_CALL(result_processor_, OnModelAssociationDone(_)). @@ -140,6 +142,7 @@ TEST_F(SyncModelAssociationManagerTest, StopAfterFinish) { DataTypeManager::OK, types, std::map(), + syncer::ModelTypeSet(), syncer::ModelTypeSet()); EXPECT_CALL(result_processor_, OnModelAssociationDone(_)). WillOnce(VerifyResult(expected_result)); @@ -174,6 +177,7 @@ TEST_F(SyncModelAssociationManagerTest, TypeFailModelAssociation) { DataTypeManager::PARTIAL_SUCCESS, types, errors, + syncer::ModelTypeSet(), syncer::ModelTypeSet()); EXPECT_CALL(result_processor_, OnModelAssociationDone(_)). WillOnce(VerifyResult(expected_result)); @@ -204,6 +208,7 @@ TEST_F(SyncModelAssociationManagerTest, TypeReturnUnrecoverableError) { DataTypeManager::UNRECOVERABLE_ERROR, types, errors, + syncer::ModelTypeSet(), syncer::ModelTypeSet()); EXPECT_CALL(result_processor_, OnModelAssociationDone(_)). WillOnce(VerifyResult(expected_result)); @@ -235,7 +240,8 @@ TEST_F(SyncModelAssociationManagerTest, InitializeAbortsLoad) { DataTypeManager::PARTIAL_SUCCESS, types, std::map(), - expected_types_waiting_to_load); + expected_types_waiting_to_load, + syncer::ModelTypeSet()); model_association_manager.Initialize(types); model_association_manager.StopDisabledTypes(); @@ -275,6 +281,7 @@ TEST_F(SyncModelAssociationManagerTest, InitializeAbortsLoad) { DataTypeManager::OK, types, std::map(), + syncer::ModelTypeSet(), syncer::ModelTypeSet()); EXPECT_CALL(result_processor_, OnModelAssociationDone(_)). WillOnce(VerifyResult(expected_result_done)); @@ -308,12 +315,14 @@ TEST_F(SyncModelAssociationManagerTest, ModelStartWithSlowLoadingType) { DataTypeManager::PARTIAL_SUCCESS, types, std::map(), - expected_types_waiting_to_load); + expected_types_waiting_to_load, + syncer::ModelTypeSet()); DataTypeManager::ConfigureResult expected_result_done( DataTypeManager::OK, types, std::map(), + syncer::ModelTypeSet(), syncer::ModelTypeSet()); EXPECT_CALL(result_processor_, OnModelAssociationDone(_)). @@ -371,11 +380,13 @@ TEST_F(SyncModelAssociationManagerTest, StartMultipleTimes) { DataTypeManager::OK, syncer::ModelTypeSet(syncer::BOOKMARKS), std::map(), + syncer::ModelTypeSet(), syncer::ModelTypeSet()); DataTypeManager::ConfigureResult result_2nd( DataTypeManager::OK, syncer::ModelTypeSet(syncer::APPS), std::map(), + syncer::ModelTypeSet(), syncer::ModelTypeSet()); EXPECT_CALL(result_processor_, OnModelAssociationDone(_)). Times(2). diff --git a/chrome/browser/sync/glue/shared_change_processor.cc b/chrome/browser/sync/glue/shared_change_processor.cc index f8be7be98149..0dd0833df45a 100644 --- a/chrome/browser/sync/glue/shared_change_processor.cc +++ b/chrome/browser/sync/glue/shared_change_processor.cc @@ -65,7 +65,7 @@ base::WeakPtr SharedChangeProcessor::Connect( base::WeakPtr local_service = sync_factory->GetSyncableServiceForType(type); if (!local_service.get()) { - NOTREACHED() << "SyncableService destroyed before DTC was stopped."; + LOG(WARNING) << "SyncableService destroyed before DTC was stopped."; disconnected_ = true; return base::WeakPtr(); } diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc index 23d9524b6522..7fd2164d9b97 100644 --- a/chrome/browser/sync/glue/sync_backend_host.cc +++ b/chrome/browser/sync/glue/sync_backend_host.cc @@ -77,6 +77,21 @@ using syncer::InternalComponentsFactoryImpl; using syncer::sessions::SyncSessionSnapshot; using syncer::SyncCredentials; +namespace { + +// Helper struct to handle currying params to +// SyncBackendHost::Core::DoConfigureSyncer. +struct DoConfigureSyncerTypes { + DoConfigureSyncerTypes() {} + ~DoConfigureSyncerTypes() {} + syncer::ModelTypeSet to_download; + syncer::ModelTypeSet to_journal; + syncer::ModelTypeSet to_unapply; + syncer::ModelTypeSet to_ignore; +}; + +} // namespace + // Helper macros to log with the syncer thread name; useful when there // are multiple syncers involved. @@ -213,8 +228,7 @@ class SyncBackendHost::Core // Configuration methods that must execute on sync loop. void DoConfigureSyncer( syncer::ConfigureReason reason, - syncer::ModelTypeSet types_to_config, - syncer::ModelTypeSet failed_types, + const DoConfigureSyncerTypes& config_types, const syncer::ModelSafeRoutingInfo routing_info, const base::Callback& ready_task, @@ -703,10 +717,19 @@ void SyncBackendHost::ConfigureDataTypes( // backend because configuration requests are never aborted; they are retried // until they succeed or the backend is shut down. + syncer::ModelTypeSet disabled_types = + GetDataTypesInState(DISABLED, config_state_map); + syncer::ModelTypeSet fatal_types = + GetDataTypesInState(FATAL, config_state_map); + syncer::ModelTypeSet crypto_types = + GetDataTypesInState(CRYPTO, config_state_map); + disabled_types.PutAll(fatal_types); + disabled_types.PutAll(crypto_types); syncer::ModelTypeSet types_to_download = registrar_->ConfigureDataTypes( GetDataTypesInState(CONFIGURE_ACTIVE, config_state_map), - syncer::Union(GetDataTypesInState(DISABLED, config_state_map), - GetDataTypesInState(FAILED, config_state_map))); + disabled_types); + syncer::ModelTypeSet inactive_types = + GetDataTypesInState(CONFIGURE_INACTIVE, config_state_map); types_to_download.RemoveAll(syncer::ProxyTypes()); if (!types_to_download.Empty()) types_to_download.Put(syncer::NIGORI); @@ -739,12 +762,23 @@ void SyncBackendHost::ConfigureDataTypes( SDVLOG(1) << "Types " << syncer::ModelTypeSetToString(types_to_download) << " added; calling DoConfigureSyncer"; - // TODO(zea): figure out how to bypass this call if no types are being - // configured and GetKey is not needed. For now we rely on determining the - // need for GetKey as part of the SyncManager::ConfigureSyncer logic. + // Divide up the types into their corresponding actions (each is mutually + // exclusive): + // - Types which have just been added to the routing info (types_to_download): + // are downloaded. + // - Types which have encountered a fatal error (fatal_types) are deleted + // from the directory and journaled in the delete journal. + // - Types which have encountered a cryptographer error (crypto_types) are + // unapplied (local state is purged but sync state is not). + // - All other types not in the routing info (types just disabled) are deleted + // from the directory. + // - Everything else (enabled types and already disabled types) is not + // touched. RequestConfigureSyncer(reason, types_to_download, - GetDataTypesInState(FAILED, config_state_map), + fatal_types, + crypto_types, + inactive_types, routing_info, ready_task, retry_callback); @@ -824,18 +858,24 @@ void SyncBackendHost::InitCore(const DoInitializeOptions& options) { void SyncBackendHost::RequestConfigureSyncer( syncer::ConfigureReason reason, - syncer::ModelTypeSet types_to_config, - syncer::ModelTypeSet failed_types, + syncer::ModelTypeSet to_download, + syncer::ModelTypeSet to_journal, + syncer::ModelTypeSet to_unapply, + syncer::ModelTypeSet to_ignore, const syncer::ModelSafeRoutingInfo& routing_info, const base::Callback& ready_task, const base::Closure& retry_callback) { + DoConfigureSyncerTypes config_types; + config_types.to_download = to_download; + config_types.to_journal = to_journal; + config_types.to_unapply = to_unapply; + config_types.to_ignore = to_ignore; sync_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(&SyncBackendHost::Core::DoConfigureSyncer, core_.get(), reason, - types_to_config, - failed_types, + config_types, routing_info, ready_task, retry_callback)); @@ -986,12 +1026,14 @@ void SyncBackendHost::Core::DoDownloadControlTypes( registrar_->GetModelSafeRoutingInfo(&routing_info); SDVLOG(1) << "Control Types " << syncer::ModelTypeSetToString(new_control_types) - << " added; calling DoConfigureSyncer"; + << " added; calling ConfigureSyncer"; sync_manager_->ConfigureSyncer( reason, new_control_types, syncer::ModelTypeSet(), + syncer::ModelTypeSet(), + syncer::ModelTypeSet(), routing_info, base::Bind(&SyncBackendHost::Core::DoInitialProcessControlTypes, this), @@ -1400,8 +1442,7 @@ void SyncBackendHost::Core::DoDestroySyncManager() { void SyncBackendHost::Core::DoConfigureSyncer( syncer::ConfigureReason reason, - syncer::ModelTypeSet types_to_config, - syncer::ModelTypeSet failed_types, + const DoConfigureSyncerTypes& config_types, const syncer::ModelSafeRoutingInfo routing_info, const base::Callback& ready_task, @@ -1409,12 +1450,14 @@ void SyncBackendHost::Core::DoConfigureSyncer( DCHECK_EQ(base::MessageLoop::current(), sync_loop_); sync_manager_->ConfigureSyncer( reason, - types_to_config, - failed_types, + config_types.to_download, + config_types.to_journal, + config_types.to_unapply, + config_types.to_ignore, routing_info, base::Bind(&SyncBackendHost::Core::DoFinishConfigureDataTypes, this, - types_to_config, + config_types.to_download, ready_task), base::Bind(&SyncBackendHost::Core::DoRetryConfiguration, this, diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h index 03db00bc480e..42c2b21b8940 100644 --- a/chrome/browser/sync/glue/sync_backend_host.h +++ b/chrome/browser/sync/glue/sync_backend_host.h @@ -313,6 +313,7 @@ class SyncBackendHost typedef base::Callback(void)> MakeHttpBridgeFactoryFn; + // Utility struct for holding initialization options. struct DoInitializeOptions { DoInitializeOptions( base::MessageLoop* sync_loop, @@ -366,8 +367,10 @@ class SyncBackendHost // Virtual for testing. virtual void RequestConfigureSyncer( syncer::ConfigureReason reason, - syncer::ModelTypeSet types_to_config, - syncer::ModelTypeSet failed_types, + syncer::ModelTypeSet to_download, + syncer::ModelTypeSet to_journal, + syncer::ModelTypeSet to_unapply, + syncer::ModelTypeSet to_ignore, const syncer::ModelSafeRoutingInfo& routing_info, const base::Callback& ready_task, diff --git a/chrome/browser/sync/glue/ui_data_type_controller.cc b/chrome/browser/sync/glue/ui_data_type_controller.cc index 878e91d0a476..830999e97770 100644 --- a/chrome/browser/sync/glue/ui_data_type_controller.cc +++ b/chrome/browser/sync/glue/ui_data_type_controller.cc @@ -133,7 +133,7 @@ void UIDataTypeController::Associate() { if (!local_service_.get()) { syncer::SyncError error(FROM_HERE, "Failed to connect to syncer.", type()); local_merge_result.set_error(error); - StartDone(UNRECOVERABLE_ERROR, + StartDone(ASSOCIATION_FAILED, local_merge_result, syncer_merge_result); return; diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc index 455bcfd0a17d..a8cbde53c15a 100644 --- a/chrome/browser/sync/profile_sync_service.cc +++ b/chrome/browser/sync/profile_sync_service.cc @@ -201,20 +201,6 @@ bool ProfileSyncService::IsSyncTokenAvailable() { return false; return token_service->HasTokenForService(GaiaConstants::kSyncService); } -#if defined(OS_ANDROID) -bool ProfileSyncService::ShouldEnablePasswordSyncForAndroid() const { - if (!GetPreferredDataTypes().Has(syncer::PASSWORDS)) - return false; - // If backend has not completed initializing we cannot check if the - // cryptographer is ready. - if (!sync_initialized()) - return false; - // On Android we do not want to prompt user to enter a passphrase. If - // passwords cannot be decrypted we just disable them. - syncer::ReadTransaction trans(FROM_HERE, GetUserShare()); - return IsCryptographerReady(&trans); -} -#endif void ProfileSyncService::Initialize() { DCHECK(!invalidator_registrar_.get()); @@ -487,15 +473,6 @@ bool ProfileSyncService::IsEncryptedDatatypeEnabled() const { return !Intersection(preferred_types, encrypted_types).Empty(); } -void ProfileSyncService::OnSyncConfigureDone( - DataTypeManager::ConfigureResult result) { - if (failed_data_types_handler_.UpdateFailedDataTypes( - result.failed_data_types, - FailedDataTypesHandler::STARTUP)) { - ReconfigureDatatypeManager(); - } -} - void ProfileSyncService::OnSyncConfigureRetry() { // Note: in order to handle auth failures that arise before the backend is // initialized (e.g. from invalidation notifier, or downloading new control @@ -847,6 +824,7 @@ void ProfileSyncService::OnUnrecoverableErrorImpl( delete_sync_database)); } +// TODO(zea): Move this logic into the DataTypeController/DataTypeManager. void ProfileSyncService::DisableBrokenDatatype( syncer::ModelType type, const tracked_objects::Location& from_here, @@ -1117,6 +1095,14 @@ void ProfileSyncService::OnPassphraseRequired( << syncer::PassphraseRequiredReasonToString(reason); passphrase_required_reason_ = reason; + const syncer::ModelTypeSet types = GetPreferredDataTypes(); + if (data_type_manager_) { + // Reconfigure without the encrypted types (excluded implicitly via the + // failed datatypes handler). + data_type_manager_->Configure(types, + syncer::CONFIGURE_REASON_CRYPTO); + } + // Notify observers that the passphrase status may have changed. NotifyObservers(); } @@ -1135,22 +1121,13 @@ void ProfileSyncService::OnPassphraseAccepted() { // types are enabled, and we don't want to clobber the true passphrase error. passphrase_required_reason_ = syncer::REASON_PASSPHRASE_NOT_REQUIRED; -#if defined(OS_ANDROID) - // Re-enable passwords if we have disabled them. - if (failed_data_types_handler_.GetFailedTypes().Has(syncer::PASSWORDS) && - ShouldEnablePasswordSyncForAndroid()) { - // Clear the data type errors. - failed_data_types_handler_.Reset(); - } -#endif - // Make sure the data types that depend on the passphrase are started at // this time. - const syncer::ModelTypeSet types = GetActiveDataTypes(); + const syncer::ModelTypeSet types = GetPreferredDataTypes(); if (data_type_manager_) { - // Unblock the data type manager if necessary. + // Re-enable any encrypted types if necessary. data_type_manager_->Configure(types, - syncer::CONFIGURE_REASON_RECONFIGURATION); + syncer::CONFIGURE_REASON_CRYPTO); } NotifyObservers(); @@ -1236,10 +1213,6 @@ void ProfileSyncService::OnActionableError(const SyncProtocolError& error) { NotifyObservers(); } -void ProfileSyncService::OnConfigureBlocked() { - NotifyObservers(); -} - void ProfileSyncService::OnConfigureDone( const browser_sync::DataTypeManager::ConfigureResult& result) { // We should have cleared our cached passphrase before we get here (in @@ -1307,12 +1280,6 @@ void ProfileSyncService::OnConfigureDone( return; } - // Now handle partial success and full success. - base::MessageLoop::current()->PostTask( - FROM_HERE, - base::Bind(&ProfileSyncService::OnSyncConfigureDone, - weak_factory_.GetWeakPtr(), result)); - // We should never get in a state where we have no encrypted datatypes // enabled, and yet we still think we require a passphrase for decryption. DCHECK(!(IsPassphraseRequiredForDecryption() && @@ -1532,6 +1499,12 @@ void ProfileSyncService::OnUserChoseDatatypes( sync_prefs_.SetKeepEverythingSynced(sync_everything); failed_data_types_handler_.Reset(); + if (GetActiveDataTypes().Has(syncer::HISTORY_DELETE_DIRECTIVES) && + encrypted_types_.Has(syncer::SESSIONS)) { + DisableBrokenDatatype(syncer::HISTORY_DELETE_DIRECTIVES, + FROM_HERE, + "Delete directives not supported with encryption."); + } ChangePreferredDataTypes(chosen_types); AcknowledgeSyncedTypes(); NotifyObservers(); @@ -1642,26 +1615,7 @@ void ProfileSyncService::ConfigureDataTypeManager() { base::Unretained(this)))); } - // This is probably where we want to trigger configuration of priority - // datatypes, but we need to resolve crbug.com/226195 first. - -#if defined(OS_ANDROID) - if (GetActiveDataTypes().Has(syncer::PASSWORDS) && - !ShouldEnablePasswordSyncForAndroid()) { - DisableBrokenDatatype(syncer::PASSWORDS, FROM_HERE, "Not supported."); - } -#endif - - if (IsPassphraseRequiredForDecryption()) { - // We need a passphrase still. We don't bother to attempt to configure - // until we receive an OnPassphraseAccepted (which triggers a configure). - DVLOG(1) << "ProfileSyncService::ConfigureDataTypeManager bailing out " - << "because a passphrase required"; - NotifyObservers(); - return; - } - - const syncer::ModelTypeSet types = GetActiveDataTypes(); + const syncer::ModelTypeSet types = GetPreferredDataTypes(); syncer::ConfigureReason reason = syncer::CONFIGURE_REASON_UNKNOWN; if (!HasSyncSetupCompleted()) { reason = syncer::CONFIGURE_REASON_NEW_CLIENT; diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h index ed7bf1307c38..5aa7143b8eb4 100644 --- a/chrome/browser/sync/profile_sync_service.h +++ b/chrome/browser/sync/profile_sync_service.h @@ -318,7 +318,6 @@ class ProfileSyncService : public ProfileSyncServiceBase, const syncer::SyncProtocolError& error) OVERRIDE; // DataTypeManagerObserver implementation. - virtual void OnConfigureBlocked() OVERRIDE; virtual void OnConfigureDone( const browser_sync::DataTypeManager::ConfigureResult& result) OVERRIDE; virtual void OnConfigureRetry() OVERRIDE; @@ -561,13 +560,6 @@ class ProfileSyncService : public ProfileSyncServiceBase, // encryption_pending() must be checked. virtual bool EncryptEverythingEnabled() const; -#if defined(OS_ANDROID) - // Android does not display password prompts, passwords are only allowed to be - // synced if Cryptographer has already been initialized and does not have - // pending keys. - bool ShouldEnablePasswordSyncForAndroid() const; -#endif - // Returns true if the syncer is waiting for new datatypes to be encrypted. virtual bool encryption_pending() const; @@ -763,11 +755,6 @@ class ProfileSyncService : public ProfileSyncServiceBase, // Create and register a new datatype controller. void RegisterNewDataType(syncer::ModelType data_type); - // Helper method to process SyncConfigureDone after unwinding the stack that - // originally posted this SyncConfigureDone. - void OnSyncConfigureDone( - browser_sync::DataTypeManager::ConfigureResult result); - // Reconfigures the data type manager with the latest enabled types. // Note: Does not initialize the backend if it is not already initialized. // This function needs to be called only after sync has been initialized diff --git a/chrome/browser/sync/profile_sync_service_android.cc b/chrome/browser/sync/profile_sync_service_android.cc index 9dfe02013f33..9e570e6573ec 100644 --- a/chrome/browser/sync/profile_sync_service_android.cc +++ b/chrome/browser/sync/profile_sync_service_android.cc @@ -362,10 +362,7 @@ jboolean ProfileSyncServiceAndroid::IsPassphraseRequiredForDecryption( // DataTypeManager, after configuration password datatype shall be disabled. const syncer::ModelTypeSet encrypted_types = sync_service_->GetEncryptedDataTypes(); - const bool are_passwords_the_only_encrypted_type = - encrypted_types.Has(syncer::PASSWORDS) && encrypted_types.Size() == 1 && - !sync_service_->ShouldEnablePasswordSyncForAndroid(); - return !are_passwords_the_only_encrypted_type; + return !encrypted_types.Equals(syncer::ModelTypeSet(syncer::PASSWORDS)); } return false; } diff --git a/chrome/browser/sync/profile_sync_service_password_unittest.cc b/chrome/browser/sync/profile_sync_service_password_unittest.cc index 0d1095282a32..30fedc3c46bc 100644 --- a/chrome/browser/sync/profile_sync_service_password_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_password_unittest.cc @@ -77,10 +77,6 @@ ACTION_P(AcquireSyncTransaction, password_test_service) { DVLOG(1) << "Sync transaction acquired."; } -static void QuitMessageLoop() { - base::MessageLoop::current()->Quit(); -} - class NullPasswordStore : public MockPasswordStore { public: NullPasswordStore() {} @@ -108,16 +104,6 @@ class PasswordTestProfileSyncService : public TestProfileSyncService { virtual ~PasswordTestProfileSyncService() {} - virtual void OnPassphraseRequired( - syncer::PassphraseRequiredReason reason, - const sync_pb::EncryptedData& pending_keys) OVERRIDE { - // We purposely don't let passphrase_required_reason_ get set here, in order - // to let the datatype manager get blocked later (at which point we then - // set the encryption passphrase). - // On a normal client, we would have initialized the cryptographer with the - // login credentials. - } - virtual void OnPassphraseAccepted() OVERRIDE { if (!callback_.is_null()) callback_.Run(); @@ -125,10 +111,6 @@ class PasswordTestProfileSyncService : public TestProfileSyncService { TestProfileSyncService::OnPassphraseAccepted(); } - virtual void OnConfigureBlocked() OVERRIDE { - QuitMessageLoop(); - } - static BrowserContextKeyedService* Build(content::BrowserContext* context) { Profile* profile = static_cast(context); SigninManagerBase* signin = diff --git a/chrome/browser/sync/profile_sync_service_startup_unittest.cc b/chrome/browser/sync/profile_sync_service_startup_unittest.cc index d3d88d21bbeb..0ec55c706cfd 100644 --- a/chrome/browser/sync/profile_sync_service_startup_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_startup_unittest.cc @@ -500,6 +500,7 @@ TEST_F(ProfileSyncServiceStartupTest, StartFailure) { status, syncer::ModelTypeSet(), errors, + syncer::ModelTypeSet(), syncer::ModelTypeSet()); EXPECT_CALL(*data_type_manager, Configure(_, _)). WillRepeatedly( diff --git a/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc b/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc index f21781e13d1c..171f0f5a2f4c 100644 --- a/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc +++ b/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc @@ -1856,8 +1856,6 @@ IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, ASSERT_TRUE(IsEncrypted(0, syncer::BOOKMARKS)); ASSERT_TRUE(IsEncrypted(1, syncer::BOOKMARKS)); ASSERT_TRUE(GetClient(1)->service()->IsPassphraseRequired()); - ASSERT_GT(GetClient(1)->GetLastSessionSnapshot().num_encryption_conflicts(), - 3); // The encrypted nodes. // Client 1 adds bookmarks between the first two and between the second two. ASSERT_TRUE(AddURL(0, 1, IndexedURLTitle(3), GURL(IndexedURL(3))) != NULL); diff --git a/chrome/browser/sync/test_profile_sync_service.cc b/chrome/browser/sync/test_profile_sync_service.cc index 06d586a04003..4606b4e9b32e 100644 --- a/chrome/browser/sync/test_profile_sync_service.cc +++ b/chrome/browser/sync/test_profile_sync_service.cc @@ -103,18 +103,20 @@ void SyncBackendHostForProfileSyncTest::UpdateCredentials( void SyncBackendHostForProfileSyncTest::RequestConfigureSyncer( syncer::ConfigureReason reason, - syncer::ModelTypeSet types_to_config, - syncer::ModelTypeSet failed_types, + syncer::ModelTypeSet to_download, + syncer::ModelTypeSet to_journal, + syncer::ModelTypeSet to_unapply, + syncer::ModelTypeSet to_ignore, const syncer::ModelSafeRoutingInfo& routing_info, const base::Callback& ready_task, const base::Closure& retry_callback) { syncer::ModelTypeSet failed_configuration_types; if (fail_initial_download_) - failed_configuration_types = types_to_config; + failed_configuration_types = to_download; FinishConfigureDataTypesOnFrontendLoop( - syncer::Difference(types_to_config, failed_configuration_types), + syncer::Difference(to_download, failed_configuration_types), failed_configuration_types, ready_task); } diff --git a/chrome/browser/sync/test_profile_sync_service.h b/chrome/browser/sync/test_profile_sync_service.h index a38335717036..42c5aa0c97db 100644 --- a/chrome/browser/sync/test_profile_sync_service.h +++ b/chrome/browser/sync/test_profile_sync_service.h @@ -60,8 +60,10 @@ class SyncBackendHostForProfileSyncTest : public SyncBackendHost { virtual void RequestConfigureSyncer( syncer::ConfigureReason reason, - syncer::ModelTypeSet types_to_config, - syncer::ModelTypeSet failed_types, + syncer::ModelTypeSet to_download, + syncer::ModelTypeSet to_journal, + syncer::ModelTypeSet to_unapply, + syncer::ModelTypeSet to_ignore, const syncer::ModelSafeRoutingInfo& routing_info, const base::Callback& ready_task, diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc index adae4f2fd952..c1196ef97bac 100644 --- a/sync/engine/syncer_unittest.cc +++ b/sync/engine/syncer_unittest.cc @@ -1092,6 +1092,7 @@ TEST_F(SyncerTest, TestPurgeWhileUnsynced) { } directory()->PurgeEntriesWithTypeIn(ModelTypeSet(PREFERENCES), + ModelTypeSet(), ModelTypeSet()); SyncShareNudge(); @@ -1126,8 +1127,9 @@ TEST_F(SyncerTest, TestPurgeWhileUnapplied) { parent.Put(syncable::ID, parent_id_); } - directory()->PurgeEntriesWithTypeIn( - ModelTypeSet(BOOKMARKS), ModelTypeSet()); + directory()->PurgeEntriesWithTypeIn(ModelTypeSet(BOOKMARKS), + ModelTypeSet(), + ModelTypeSet()); SyncShareNudge(); directory()->SaveChanges(); @@ -1165,7 +1167,8 @@ TEST_F(SyncerTest, TestPurgeWithJournal) { } directory()->PurgeEntriesWithTypeIn(ModelTypeSet(PREFERENCES, BOOKMARKS), - ModelTypeSet(BOOKMARKS)); + ModelTypeSet(BOOKMARKS), + ModelTypeSet()); { // Verify bookmark nodes are saved in delete journal but not preference // node. diff --git a/sync/internal_api/public/configure_reason.h b/sync/internal_api/public/configure_reason.h index 1eefc72b3bec..28a99f61cf97 100644 --- a/sync/internal_api/public/configure_reason.h +++ b/sync/internal_api/public/configure_reason.h @@ -28,6 +28,10 @@ enum ConfigureReason { // A new datatype is enabled for syncing due to a client upgrade. CONFIGURE_REASON_NEWLY_ENABLED_DATA_TYPE, + + // A configuration due to enabling or disabling encrypted types due to + // cryptographer errors/resolutions. + CONFIGURE_REASON_CRYPTO, }; } // namespace syncer diff --git a/sync/internal_api/public/engine/model_safe_worker.cc b/sync/internal_api/public/engine/model_safe_worker.cc index 3e2096a9423d..a4d142b4b3c8 100644 --- a/sync/internal_api/public/engine/model_safe_worker.cc +++ b/sync/internal_api/public/engine/model_safe_worker.cc @@ -54,7 +54,7 @@ ModelSafeGroup GetGroupForModelType(const ModelType type, ModelSafeRoutingInfo::const_iterator it = routes.find(type); if (it == routes.end()) { if (type != UNSPECIFIED && type != TOP_LEVEL_FOLDER) - LOG(WARNING) << "Entry does not belong to active ModelSafeGroup!"; + DVLOG(1) << "Entry does not belong to active ModelSafeGroup!"; return GROUP_PASSIVE; } return it->second; diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h index d754bf01a35a..3f961fba8480 100644 --- a/sync/internal_api/public/sync_manager.h +++ b/sync/internal_api/public/sync_manager.h @@ -375,16 +375,20 @@ class SYNC_EXPORT SyncManager { // syncer will remain in CONFIGURATION_MODE until StartSyncingNormally is // called. // Data whose types are not in |new_routing_info| are purged from sync - // directory. The purged data is backed up in delete journal for recovery in - // next session if its type is in |failed_types|. + // directory, unless they're part of |to_ignore|, in which case they're left + // untouched. The purged data is backed up in delete journal for recovery in + // next session if its type is in |to_journal|. If in |to_unapply| + // only the local data is removed; the server data is preserved. // |ready_task| is invoked when the configuration completes. // |retry_task| is invoked if the configuration job could not immediately // execute. |ready_task| will still be called when it eventually // does finish. virtual void ConfigureSyncer( ConfigureReason reason, - ModelTypeSet types_to_config, - ModelTypeSet failed_types, + ModelTypeSet to_download, + ModelTypeSet to_journal, + ModelTypeSet to_unapply, + ModelTypeSet to_ignore, const ModelSafeRoutingInfo& new_routing_info, const base::Closure& ready_task, const base::Closure& retry_task) = 0; diff --git a/sync/internal_api/public/test/fake_sync_manager.h b/sync/internal_api/public/test/fake_sync_manager.h index 25b28f66ba81..e4cb09fda6be 100644 --- a/sync/internal_api/public/test/fake_sync_manager.h +++ b/sync/internal_api/public/test/fake_sync_manager.h @@ -114,8 +114,10 @@ class FakeSyncManager : public SyncManager { const ModelSafeRoutingInfo& routing_info) OVERRIDE; virtual void ConfigureSyncer( ConfigureReason reason, - ModelTypeSet types_to_config, - ModelTypeSet failed_types, + ModelTypeSet to_download, + ModelTypeSet to_journal, + ModelTypeSet to_unapply, + ModelTypeSet to_ignore, const ModelSafeRoutingInfo& new_routing_info, const base::Closure& ready_task, const base::Closure& retry_task) OVERRIDE; diff --git a/sync/internal_api/sync_encryption_handler_impl.cc b/sync/internal_api/sync_encryption_handler_impl.cc index 48ad2a8d6c8e..2e28e75a83f0 100644 --- a/sync/internal_api/sync_encryption_handler_impl.cc +++ b/sync/internal_api/sync_encryption_handler_impl.cc @@ -810,10 +810,8 @@ void SyncEncryptionHandlerImpl::ReEncryptEverything( continue; WriteNode child(trans); - if (child.InitByIdLookup(child_id) != BaseNode::INIT_OK) { - NOTREACHED(); - continue; - } + if (child.InitByIdLookup(child_id) != BaseNode::INIT_OK) + continue; // Possible for locally deleted items. if (child.GetIsFolder()) { to_visit.push(child.GetFirstChildId()); } diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc index 50c56d0dd25a..b63e54bf6e9e 100644 --- a/sync/internal_api/sync_manager_impl.cc +++ b/sync/internal_api/sync_manager_impl.cc @@ -80,6 +80,7 @@ GetUpdatesCallerInfo::GetUpdatesSource GetSourceFromReason( case CONFIGURE_REASON_NEW_CLIENT: return GetUpdatesCallerInfo::NEW_CLIENT; case CONFIGURE_REASON_NEWLY_ENABLED_DATA_TYPE: + case CONFIGURE_REASON_CRYPTO: return GetUpdatesCallerInfo::NEWLY_SUPPORTED_DATATYPE; default: NOTREACHED(); @@ -297,8 +298,10 @@ ModelTypeSet SyncManagerImpl::GetTypesWithEmptyProgressMarkerToken( void SyncManagerImpl::ConfigureSyncer( ConfigureReason reason, - ModelTypeSet types_to_config, - ModelTypeSet failed_types, + ModelTypeSet to_download, + ModelTypeSet to_journal, + ModelTypeSet to_unapply, + ModelTypeSet to_ignore, const ModelSafeRoutingInfo& new_routing_info, const base::Closure& ready_task, const base::Closure& retry_task) { @@ -310,9 +313,28 @@ void SyncManagerImpl::ConfigureSyncer( ModelTypeSet previous_types = ModelTypeSet::All(); if (!session_context_->routing_info().empty()) previous_types = GetRoutingInfoTypes(session_context_->routing_info()); + + // By removing the |to_ignore| types from the previous types, they won't be + // treated as disabled, and therefore won't be purged. + previous_types.RemoveAll(to_ignore); + + DVLOG(1) << "Configuring -" + << "\n\t" << "types to download: " + << ModelTypeSetToString(to_download) + << "\n\t" << "current types: " + << ModelTypeSetToString(GetRoutingInfoTypes(new_routing_info)) + << "\n\t" << "previous types: " + << ModelTypeSetToString(previous_types) + << "\n\t" << "to_journal: " + << ModelTypeSetToString(to_journal) + << "\n\t" << "to_unapply: " + << ModelTypeSetToString(to_unapply) + << "\n\t" << "to_ignore: " + << ModelTypeSetToString(to_ignore); if (!PurgeDisabledTypes(previous_types, GetRoutingInfoTypes(new_routing_info), - failed_types)) { + to_journal, + to_unapply)) { // We failed to cleanup the types. Invoke the ready task without actually // configuring any types. The caller should detect this as a configuration // failure and act appropriately. @@ -321,7 +343,7 @@ void SyncManagerImpl::ConfigureSyncer( } ConfigurationParams params(GetSourceFromReason(reason), - types_to_config, + to_download, new_routing_info, ready_task); @@ -574,21 +596,26 @@ bool SyncManagerImpl::PurgePartiallySyncedTypes() { if (partially_synced_types.Empty()) return true; return directory()->PurgeEntriesWithTypeIn(partially_synced_types, + ModelTypeSet(), ModelTypeSet()); } bool SyncManagerImpl::PurgeDisabledTypes( ModelTypeSet previously_enabled_types, ModelTypeSet currently_enabled_types, - ModelTypeSet failed_types) { + ModelTypeSet to_journal, + ModelTypeSet to_unapply) { ModelTypeSet disabled_types = Difference(previously_enabled_types, currently_enabled_types); if (disabled_types.Empty()) return true; - DVLOG(1) << "Purging disabled types " << ModelTypeSetToString(disabled_types); - return directory()->PurgeEntriesWithTypeIn(disabled_types, failed_types); + DCHECK(disabled_types.HasAll(to_journal)); + DCHECK(disabled_types.HasAll(to_unapply)); + return directory()->PurgeEntriesWithTypeIn(disabled_types, + to_journal, + to_unapply); } void SyncManagerImpl::UpdateCredentials(const SyncCredentials& credentials) { diff --git a/sync/internal_api/sync_manager_impl.h b/sync/internal_api/sync_manager_impl.h index acc017577237..a21b6e815159 100644 --- a/sync/internal_api/sync_manager_impl.h +++ b/sync/internal_api/sync_manager_impl.h @@ -104,8 +104,10 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl : const ModelSafeRoutingInfo& routing_info) OVERRIDE; virtual void ConfigureSyncer( ConfigureReason reason, - ModelTypeSet types_to_config, - ModelTypeSet failed_types, + ModelTypeSet to_download, + ModelTypeSet to_journal, + ModelTypeSet to_unapply, + ModelTypeSet to_ignore, const ModelSafeRoutingInfo& new_routing_info, const base::Closure& ready_task, const base::Closure& retry_task) OVERRIDE; @@ -199,6 +201,7 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl : FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, OnNotificationStateChange); FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, OnIncomingNotification); FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, PurgeDisabledTypes); + FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, PurgeUnappliedTypes); struct NotificationInfo { NotificationInfo(); @@ -238,10 +241,14 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl : bool OpenDirectory(const std::string& username); // Purge those types from |previously_enabled_types| that are no longer - // enabled in |currently_enabled_types|. + // enabled in |currently_enabled_types|. |to_journal| and |to_unapply| + // specify types that require special handling. |to_journal| types are saved + // into the delete journal, while |to_unapply| have only their local data + // deleted, while their server data is preserved. bool PurgeDisabledTypes(ModelTypeSet previously_enabled_types, ModelTypeSet currently_enabled_types, - ModelTypeSet failed_types); + ModelTypeSet to_journal, + ModelTypeSet to_unapply); void RequestNudgeForDataTypes( const tracked_objects::Location& nudge_location, diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc index 01b776e2f299..ff04a0b81264 100644 --- a/sync/internal_api/sync_manager_impl_unittest.cc +++ b/sync/internal_api/sync_manager_impl_unittest.cc @@ -2909,6 +2909,8 @@ TEST_F(SyncManagerTestWithMockScheduler, BasicConfiguration) { reason, types_to_download, ModelTypeSet(), + ModelTypeSet(), + ModelTypeSet(), new_routing_info, base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_task_counter)), @@ -2967,6 +2969,8 @@ TEST_F(SyncManagerTestWithMockScheduler, ReConfiguration) { reason, types_to_download, ModelTypeSet(), + ModelTypeSet(), + ModelTypeSet(), new_routing_info, base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_task_counter)), @@ -3001,6 +3005,8 @@ TEST_F(SyncManagerTestWithMockScheduler, ConfigurationRetry) { reason, types_to_download, ModelTypeSet(), + ModelTypeSet(), + ModelTypeSet(), new_routing_info, base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_task_counter)), @@ -3107,7 +3113,9 @@ TEST_F(SyncManagerTest, PurgeDisabledTypes) { // Verify all the enabled types remain after cleanup, and all the disabled // types were purged. - sync_manager_.PurgeDisabledTypes(ModelTypeSet::All(), enabled_types, + sync_manager_.PurgeDisabledTypes(ModelTypeSet::All(), + enabled_types, + ModelTypeSet(), ModelTypeSet()); EXPECT_TRUE(enabled_types.Equals(sync_manager_.InitialSyncEndedTypes())); EXPECT_TRUE(disabled_types.Equals( @@ -3120,13 +3128,161 @@ TEST_F(SyncManagerTest, PurgeDisabledTypes) { Difference(ModelTypeSet::All(), disabled_types); // Verify only the non-disabled types remain after cleanup. - sync_manager_.PurgeDisabledTypes(enabled_types, new_enabled_types, + sync_manager_.PurgeDisabledTypes(enabled_types, + new_enabled_types, + ModelTypeSet(), ModelTypeSet()); EXPECT_TRUE(new_enabled_types.Equals(sync_manager_.InitialSyncEndedTypes())); EXPECT_TRUE(disabled_types.Equals( sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All()))); } +// Test PurgeDisabledTypes properly unapplies types by deleting their local data +// and preserving their server data and progress marker. +TEST_F(SyncManagerTest, PurgeUnappliedTypes) { + ModelSafeRoutingInfo routing_info; + GetModelSafeRoutingInfo(&routing_info); + ModelTypeSet unapplied_types = ModelTypeSet(BOOKMARKS, PREFERENCES); + ModelTypeSet enabled_types = GetRoutingInfoTypes(routing_info); + ModelTypeSet disabled_types = Difference(ModelTypeSet::All(), enabled_types); + + // The harness should have initialized the enabled_types for us. + EXPECT_TRUE(enabled_types.Equals(sync_manager_.InitialSyncEndedTypes())); + + // Set progress markers for all types. + ModelTypeSet protocol_types = ProtocolTypes(); + for (ModelTypeSet::Iterator iter = protocol_types.First(); iter.Good(); + iter.Inc()) { + SetProgressMarkerForType(iter.Get(), true); + } + + // Add the following kinds of items: + // 1. Fully synced preference. + // 2. Locally created preference, server unknown, unsynced + // 3. Locally deleted preference, server known, unsynced + // 4. Server deleted preference, locally known. + // 5. Server created preference, locally unknown, unapplied. + // 6. A fully synced bookmark (no unique_client_tag). + UserShare* share = sync_manager_.GetUserShare(); + sync_pb::EntitySpecifics pref_specifics; + AddDefaultFieldValue(PREFERENCES, &pref_specifics); + sync_pb::EntitySpecifics bm_specifics; + AddDefaultFieldValue(BOOKMARKS, &bm_specifics); + int pref1_meta = MakeServerNode( + share, PREFERENCES, "pref1", "hash1", pref_specifics); + int64 pref2_meta = MakeNode(share, PREFERENCES, "pref2"); + int pref3_meta = MakeServerNode( + share, PREFERENCES, "pref3", "hash3", pref_specifics); + int pref4_meta = MakeServerNode( + share, PREFERENCES, "pref4", "hash4", pref_specifics); + int pref5_meta = MakeServerNode( + share, PREFERENCES, "pref5", "hash5", pref_specifics); + int bookmark_meta = MakeServerNode( + share, BOOKMARKS, "bookmark", "", bm_specifics); + + { + syncable::WriteTransaction trans(FROM_HERE, + syncable::SYNCER, + share->directory.get()); + // Pref's 1 and 2 are already set up properly. + // Locally delete pref 3. + syncable::MutableEntry pref3(&trans, GET_BY_HANDLE, pref3_meta); + pref3.Put(IS_DEL, true); + pref3.Put(IS_UNSYNCED, true); + // Delete pref 4 at the server. + syncable::MutableEntry pref4(&trans, GET_BY_HANDLE, pref4_meta); + pref4.Put(syncable::SERVER_IS_DEL, true); + pref4.Put(syncable::IS_UNAPPLIED_UPDATE, true); + pref4.Put(syncable::SERVER_VERSION, 2); + // Pref 5 is an new unapplied update. + syncable::MutableEntry pref5(&trans, GET_BY_HANDLE, pref5_meta); + pref5.Put(syncable::IS_UNAPPLIED_UPDATE, true); + pref5.Put(syncable::IS_DEL, true); + pref5.Put(syncable::BASE_VERSION, -1); + // Bookmark is already set up properly + } + + // Take a snapshot to clear all the dirty bits. + share->directory.get()->SaveChanges(); + + // Now request a purge for the unapplied types. + disabled_types.PutAll(unapplied_types); + ModelTypeSet new_enabled_types = + Difference(ModelTypeSet::All(), disabled_types); + sync_manager_.PurgeDisabledTypes(enabled_types, + new_enabled_types, + ModelTypeSet(), + unapplied_types); + + // Verify the unapplied types still have progress markers and initial sync + // ended after cleanup. + EXPECT_TRUE(sync_manager_.InitialSyncEndedTypes().HasAll(unapplied_types)); + EXPECT_TRUE( + sync_manager_.GetTypesWithEmptyProgressMarkerToken(unapplied_types). + Empty()); + + // Ensure the items were unapplied as necessary. + { + syncable::ReadTransaction trans(FROM_HERE, share->directory.get()); + syncable::Entry pref_node(&trans, GET_BY_HANDLE, pref1_meta); + ASSERT_TRUE(pref_node.good()); + EXPECT_TRUE(pref_node.GetKernelCopy().is_dirty()); + EXPECT_FALSE(pref_node.Get(syncable::IS_UNSYNCED)); + EXPECT_TRUE(pref_node.Get(syncable::IS_UNAPPLIED_UPDATE)); + EXPECT_TRUE(pref_node.Get(IS_DEL)); + EXPECT_GT(pref_node.Get(syncable::SERVER_VERSION), 0); + EXPECT_EQ(pref_node.Get(syncable::BASE_VERSION), -1); + + // Pref 2 should just be locally deleted. + syncable::Entry pref2_node(&trans, GET_BY_HANDLE, pref2_meta); + ASSERT_TRUE(pref2_node.good()); + EXPECT_TRUE(pref2_node.GetKernelCopy().is_dirty()); + EXPECT_FALSE(pref2_node.Get(syncable::IS_UNSYNCED)); + EXPECT_TRUE(pref2_node.Get(syncable::IS_DEL)); + EXPECT_FALSE(pref2_node.Get(syncable::IS_UNAPPLIED_UPDATE)); + EXPECT_TRUE(pref2_node.Get(IS_DEL)); + EXPECT_EQ(pref2_node.Get(syncable::SERVER_VERSION), 0); + EXPECT_EQ(pref2_node.Get(syncable::BASE_VERSION), -1); + + syncable::Entry pref3_node(&trans, GET_BY_HANDLE, pref3_meta); + ASSERT_TRUE(pref3_node.good()); + EXPECT_TRUE(pref3_node.GetKernelCopy().is_dirty()); + EXPECT_FALSE(pref3_node.Get(syncable::IS_UNSYNCED)); + EXPECT_TRUE(pref3_node.Get(syncable::IS_UNAPPLIED_UPDATE)); + EXPECT_TRUE(pref3_node.Get(IS_DEL)); + EXPECT_GT(pref3_node.Get(syncable::SERVER_VERSION), 0); + EXPECT_EQ(pref3_node.Get(syncable::BASE_VERSION), -1); + + syncable::Entry pref4_node(&trans, GET_BY_HANDLE, pref4_meta); + ASSERT_TRUE(pref4_node.good()); + EXPECT_TRUE(pref4_node.GetKernelCopy().is_dirty()); + EXPECT_FALSE(pref4_node.Get(syncable::IS_UNSYNCED)); + EXPECT_TRUE(pref4_node.Get(syncable::IS_UNAPPLIED_UPDATE)); + EXPECT_TRUE(pref4_node.Get(IS_DEL)); + EXPECT_GT(pref4_node.Get(syncable::SERVER_VERSION), 0); + EXPECT_EQ(pref4_node.Get(syncable::BASE_VERSION), -1); + + // Pref 5 should remain untouched. + syncable::Entry pref5_node(&trans, GET_BY_HANDLE, pref5_meta); + ASSERT_TRUE(pref5_node.good()); + EXPECT_FALSE(pref5_node.GetKernelCopy().is_dirty()); + EXPECT_FALSE(pref5_node.Get(syncable::IS_UNSYNCED)); + EXPECT_TRUE(pref5_node.Get(syncable::IS_UNAPPLIED_UPDATE)); + EXPECT_TRUE(pref5_node.Get(IS_DEL)); + EXPECT_GT(pref5_node.Get(syncable::SERVER_VERSION), 0); + EXPECT_EQ(pref5_node.Get(syncable::BASE_VERSION), -1); + + syncable::Entry bookmark_node(&trans, GET_BY_HANDLE, bookmark_meta); + ASSERT_TRUE(bookmark_node.good()); + EXPECT_TRUE(bookmark_node.GetKernelCopy().is_dirty()); + EXPECT_FALSE(bookmark_node.Get(syncable::IS_UNSYNCED)); + EXPECT_TRUE(bookmark_node.Get(syncable::IS_UNAPPLIED_UPDATE)); + EXPECT_TRUE(bookmark_node.Get(IS_DEL)); + EXPECT_GT(bookmark_node.Get(syncable::SERVER_VERSION), 0); + EXPECT_EQ(bookmark_node.Get(syncable::BASE_VERSION), -1); + } +} + // A test harness to exercise the code that processes and passes changes from // the "SYNCER"-WriteTransaction destructor, through the SyncManager, to the // ChangeProcessor. diff --git a/sync/internal_api/test/fake_sync_manager.cc b/sync/internal_api/test/fake_sync_manager.cc index 674c5be10ef4..4769585343a8 100644 --- a/sync/internal_api/test/fake_sync_manager.cc +++ b/sync/internal_api/test/fake_sync_manager.cc @@ -192,8 +192,10 @@ void FakeSyncManager::StartSyncingNormally( void FakeSyncManager::ConfigureSyncer( ConfigureReason reason, - ModelTypeSet types_to_config, - ModelTypeSet failed_types, + ModelTypeSet to_download, + ModelTypeSet to_journal, + ModelTypeSet to_unapply, + ModelTypeSet to_ignore, const ModelSafeRoutingInfo& new_routing_info, const base::Closure& ready_task, const base::Closure& retry_task) { @@ -201,7 +203,8 @@ void FakeSyncManager::ConfigureSyncer( ModelTypeSet enabled_types = GetRoutingInfoTypes(new_routing_info); ModelTypeSet disabled_types = Difference( ModelTypeSet::All(), enabled_types); - ModelTypeSet success_types = types_to_config; + disabled_types.RemoveAll(to_ignore); + ModelTypeSet success_types = to_download; success_types.RemoveAll(configure_fail_types_); DVLOG(1) << "Faking configuration. Downloading: " @@ -210,7 +213,9 @@ void FakeSyncManager::ConfigureSyncer( // Update our fake directory by clearing and fake-downloading as necessary. UserShare* share = GetUserShare(); - share->directory->PurgeEntriesWithTypeIn(disabled_types, ModelTypeSet()); + share->directory->PurgeEntriesWithTypeIn(disabled_types, + to_journal, + to_unapply); for (ModelTypeSet::Iterator it = success_types.First(); it.Good(); it.Inc()) { // We must be careful to not create the same root node twice. if (!initial_sync_ended_types_.Has(it.Get())) { diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc index c2dcc1627bc8..eb15cd6684e6 100644 --- a/sync/syncable/directory.cc +++ b/sync/syncable/directory.cc @@ -570,11 +570,101 @@ bool Directory::VacuumAfterSaveChanges(const SaveChangesSnapshot& snapshot) { return true; } -bool Directory::PurgeEntriesWithTypeIn(ModelTypeSet types, - ModelTypeSet types_to_journal) { - types.RemoveAll(ProxyTypes()); +void Directory::UnapplyEntry(EntryKernel* entry) { + int64 handle = entry->ref(META_HANDLE); + ModelType server_type = GetModelTypeFromSpecifics( + entry->ref(SERVER_SPECIFICS)); + + // Clear enough so that on the next sync cycle all local data will + // be overwritten. + // Note: do not modify the root node in order to preserve the + // initial sync ended bit for this type (else on the next restart + // this type will be treated as disabled and therefore fully purged). + if (IsRealDataType(server_type) && + ModelTypeToRootTag(server_type) == entry->ref(UNIQUE_SERVER_TAG)) { + return; + } + + // Set the unapplied bit if this item has server data. + if (IsRealDataType(server_type) && !entry->ref(IS_UNAPPLIED_UPDATE)) { + entry->put(IS_UNAPPLIED_UPDATE, true); + kernel_->unapplied_update_metahandles[server_type].insert(handle); + entry->mark_dirty(&kernel_->dirty_metahandles); + } + + // Unset the unsynced bit. + if (entry->ref(IS_UNSYNCED)) { + kernel_->unsynced_metahandles.erase(handle); + entry->put(IS_UNSYNCED, false); + entry->mark_dirty(&kernel_->dirty_metahandles); + } + + // Mark the item as locally deleted. No deleted items are allowed in the + // parent child index. + if (!entry->ref(IS_DEL)) { + kernel_->parent_child_index.Remove(entry); + entry->put(IS_DEL, true); + entry->mark_dirty(&kernel_->dirty_metahandles); + } + + // Set the version to the "newly created" version. + if (entry->ref(BASE_VERSION) != CHANGES_VERSION) { + entry->put(BASE_VERSION, CHANGES_VERSION); + entry->mark_dirty(&kernel_->dirty_metahandles); + } + + // At this point locally created items that aren't synced will become locally + // deleted items, and purged on the next snapshot. All other items will match + // the state they would have had if they were just created via a server + // update. See MutableEntry::MutableEntry(.., CreateNewUpdateItem, ..). +} + +void Directory::DeleteEntry(bool save_to_journal, + EntryKernel* entry, + EntryKernelSet* entries_to_journal) { + int64 handle = entry->ref(META_HANDLE); + ModelType server_type = GetModelTypeFromSpecifics( + entry->ref(SERVER_SPECIFICS)); + + kernel_->metahandles_to_purge.insert(handle); + + size_t num_erased = 0; + num_erased = kernel_->metahandles_map.erase(entry->ref(META_HANDLE)); + DCHECK_EQ(1u, num_erased); + num_erased = kernel_->ids_map.erase(entry->ref(ID).value()); + DCHECK_EQ(1u, num_erased); + num_erased = kernel_->unsynced_metahandles.erase(handle); + DCHECK_EQ(entry->ref(IS_UNSYNCED), num_erased > 0); + num_erased = + kernel_->unapplied_update_metahandles[server_type].erase(handle); + DCHECK_EQ(entry->ref(IS_UNAPPLIED_UPDATE), num_erased > 0); + if (kernel_->parent_child_index.Contains(entry)) + kernel_->parent_child_index.Remove(entry); + + if (!entry->ref(UNIQUE_CLIENT_TAG).empty()) { + num_erased = + kernel_->client_tags_map.erase(entry->ref(UNIQUE_CLIENT_TAG)); + DCHECK_EQ(1u, num_erased); + } + if (!entry->ref(UNIQUE_SERVER_TAG).empty()) { + num_erased = + kernel_->server_tags_map.erase(entry->ref(UNIQUE_SERVER_TAG)); + DCHECK_EQ(1u, num_erased); + } + + if (save_to_journal) { + entries_to_journal->insert(entry); + } else { + delete entry; + } +} + +bool Directory::PurgeEntriesWithTypeIn(ModelTypeSet disabled_types, + ModelTypeSet types_to_journal, + ModelTypeSet types_to_unapply) { + disabled_types.RemoveAll(ProxyTypes()); - if (types.Empty()) + if (disabled_types.Empty()) return true; { @@ -602,9 +692,8 @@ bool Directory::PurgeEntriesWithTypeIn(ModelTypeSet types, ModelType local_type = GetModelTypeFromSpecifics(local_specifics); ModelType server_type = GetModelTypeFromSpecifics(server_specifics); - // Note the dance around incrementing |it|, since we sometimes erase(). - if ((IsRealDataType(local_type) && types.Has(local_type)) || - (IsRealDataType(server_type) && types.Has(server_type))) { + if ((IsRealDataType(local_type) && disabled_types.Has(local_type)) || + (IsRealDataType(server_type) && disabled_types.Has(server_type))) { to_purge.insert(it->second); } } @@ -612,57 +701,37 @@ bool Directory::PurgeEntriesWithTypeIn(ModelTypeSet types, for (std::set::iterator it = to_purge.begin(); it != to_purge.end(); ++it) { EntryKernel* entry = *it; - int64 handle = entry->ref(META_HANDLE); - const sync_pb::EntitySpecifics& local_specifics = entry->ref(SPECIFICS); + const sync_pb::EntitySpecifics& local_specifics = + (*it)->ref(SPECIFICS); const sync_pb::EntitySpecifics& server_specifics = - entry->ref(SERVER_SPECIFICS); + (*it)->ref(SERVER_SPECIFICS); ModelType local_type = GetModelTypeFromSpecifics(local_specifics); ModelType server_type = GetModelTypeFromSpecifics(server_specifics); - kernel_->metahandles_to_purge.insert(handle); - - size_t num_erased = 0; - num_erased = kernel_->metahandles_map.erase(entry->ref(META_HANDLE)); - DCHECK_EQ(1u, num_erased); - num_erased = kernel_->ids_map.erase(entry->ref(ID).value()); - DCHECK_EQ(1u, num_erased); - num_erased = kernel_->unsynced_metahandles.erase(handle); - DCHECK_EQ(entry->ref(IS_UNSYNCED), num_erased > 0); - num_erased = - kernel_->unapplied_update_metahandles[server_type].erase(handle); - DCHECK_EQ(entry->ref(IS_UNAPPLIED_UPDATE), num_erased > 0); - if (kernel_->parent_child_index.Contains(entry)) - kernel_->parent_child_index.Remove(entry); - - if (!entry->ref(UNIQUE_CLIENT_TAG).empty()) { - num_erased = - kernel_->client_tags_map.erase(entry->ref(UNIQUE_CLIENT_TAG)); - DCHECK_EQ(1u, num_erased); - } - if (!entry->ref(UNIQUE_SERVER_TAG).empty()) { - num_erased = - kernel_->server_tags_map.erase(entry->ref(UNIQUE_SERVER_TAG)); - DCHECK_EQ(1u, num_erased); - } - - if ((types_to_journal.Has(local_type) || - types_to_journal.Has(server_type)) && - (delete_journal_->IsDeleteJournalEnabled(local_type) || - delete_journal_->IsDeleteJournalEnabled(server_type))) { - entries_to_journal.insert(entry); + if (types_to_unapply.Has(local_type) || + types_to_unapply.Has(server_type)) { + UnapplyEntry(entry); } else { - delete entry; + bool save_to_journal = + (types_to_journal.Has(local_type) || + types_to_journal.Has(server_type)) && + (delete_journal_->IsDeleteJournalEnabled(local_type) || + delete_journal_->IsDeleteJournalEnabled(server_type)); + DeleteEntry(save_to_journal, entry, &entries_to_journal); } } delete_journal_->AddJournalBatch(&trans, entries_to_journal); - // Ensure meta tracking for these data types reflects the deleted state. - for (ModelTypeSet::Iterator it = types.First(); + // Ensure meta tracking for these data types reflects the purged state. + for (ModelTypeSet::Iterator it = disabled_types.First(); it.Good(); it.Inc()) { - kernel_->persisted_info.reset_download_progress(it.Get()); kernel_->persisted_info.transaction_version[it.Get()] = 0; + + // Don't discard progress markers for unapplied types. + if (!types_to_unapply.Has(it.Get())) + kernel_->persisted_info.reset_download_progress(it.Get()); } } } diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h index b5818f67b48d..4bfd27492a99 100644 --- a/sync/syncable/directory.h +++ b/sync/syncable/directory.h @@ -367,19 +367,27 @@ class SYNC_EXPORT Directory { // should not be invoked outside of tests. bool FullyCheckTreeInvariants(BaseTransaction *trans); - // Purges all data associated with any entries whose ModelType or - // ServerModelType is found in |types|, from sync directory _both_ in memory - // and on disk. |types_to_journal| should be subset of |types| and data - // of |types_to_journal| are saved in delete journal to help prevent - // back-from-dead problem due to offline delete in next sync session. Only - // valid, "real" model types are allowed in |types| (see model_type.h for - // definitions). "Purge" is just meant to distinguish from "deleting" - // entries, which means something different in the syncable namespace. + // Purges data associated with any entries whose ModelType or ServerModelType + // is found in |disabled_types|, from sync directory _both_ in memory and on + // disk. Only valid, "real" model types are allowed in |disabled_types| (see + // model_type.h for definitions). + // 1. Data associated with |types_to_journal| is saved in the delete journal + // to help prevent back-from-dead problem due to offline delete in the next + // sync session. |types_to_journal| must be a subset of |disabled_types|. + // 2. Data associated with |types_to_unapply| is reset to an "unapplied" + // state, wherein all local data is deleted and IS_UNAPPLIED is set to true. + // This is useful when there's no benefit in discarding the currently + // downloaded state, such as when there are cryptographer errors. + // |types_to_unapply| must be a subset of |disabled_types|. + // 3. All other data is purged entirely. + // Note: "Purge" is just meant to distinguish from "deleting" entries, which + // means something different in the syncable namespace. // WARNING! This can be real slow, as it iterates over all entries. // WARNING! Performs synchronous I/O. // Returns: true on success, false if an error was encountered. - virtual bool PurgeEntriesWithTypeIn(ModelTypeSet types, - ModelTypeSet types_to_journal); + virtual bool PurgeEntriesWithTypeIn(ModelTypeSet disabled_types, + ModelTypeSet types_to_journal, + ModelTypeSet types_to_unapply); private: // A helper that implements the logic of checking tree invariants. @@ -526,6 +534,12 @@ class SYNC_EXPORT Directory { const ScopedKernelLock& lock, const Id& parent_id, Directory::ChildHandles* result); + // Helper methods used by PurgeDisabledTypes. + void UnapplyEntry(EntryKernel* entry); + void DeleteEntry(bool save_to_journal, + EntryKernel* entry, + EntryKernelSet* entries_to_journal); + Kernel* kernel_; scoped_ptr store_; diff --git a/sync/syncable/syncable_mock.h b/sync/syncable/syncable_mock.h index 55068edd20c5..8d0e88b24f5d 100644 --- a/sync/syncable/syncable_mock.h +++ b/sync/syncable/syncable_mock.h @@ -28,7 +28,9 @@ class MockDirectory : public Directory { MOCK_METHOD1(GetEntryByClientTag, syncable::EntryKernel*(const std::string&)); - MOCK_METHOD2(PurgeEntriesWithTypeIn, bool(ModelTypeSet, ModelTypeSet)); + MOCK_METHOD3(PurgeEntriesWithTypeIn, bool(ModelTypeSet, + ModelTypeSet, + ModelTypeSet)); private: syncable::NullDirectoryChangeDelegate delegate_; diff --git a/sync/syncable/syncable_unittest.cc b/sync/syncable/syncable_unittest.cc index 115bb680573e..950a6b4cac57 100644 --- a/sync/syncable/syncable_unittest.cc +++ b/sync/syncable/syncable_unittest.cc @@ -586,7 +586,7 @@ TEST_F(SyncableDirectoryTest, TakeSnapshotGetsMetahandlesToPurge) { } ModelTypeSet to_purge(BOOKMARKS); - dir_->PurgeEntriesWithTypeIn(to_purge, ModelTypeSet()); + dir_->PurgeEntriesWithTypeIn(to_purge, ModelTypeSet(), ModelTypeSet()); Directory::SaveChangesSnapshot snapshot1; base::AutoLock scoped_lock(dir_->kernel_->save_changes_mutex); @@ -595,7 +595,7 @@ TEST_F(SyncableDirectoryTest, TakeSnapshotGetsMetahandlesToPurge) { to_purge.Clear(); to_purge.Put(PREFERENCES); - dir_->PurgeEntriesWithTypeIn(to_purge, ModelTypeSet()); + dir_->PurgeEntriesWithTypeIn(to_purge, ModelTypeSet(), ModelTypeSet()); dir_->HandleSaveChangesFailure(snapshot1); @@ -1774,7 +1774,7 @@ TEST_F(OnDiskSyncableDirectoryTest, TestPurgeEntriesWithTypeIn) { ASSERT_EQ(10U, all_set.size()); } - dir_->PurgeEntriesWithTypeIn(types_to_purge, ModelTypeSet()); + dir_->PurgeEntriesWithTypeIn(types_to_purge, ModelTypeSet(), ModelTypeSet()); // We first query the in-memory data, and then reload the directory (without // saving) to verify that disk does not still have the data. @@ -2033,7 +2033,7 @@ TEST_F(OnDiskSyncableDirectoryTest, TestSaveChangesFailureWithPurge) { ASSERT_TRUE(dir_->good()); ModelTypeSet set(BOOKMARKS); - dir_->PurgeEntriesWithTypeIn(set, ModelTypeSet()); + dir_->PurgeEntriesWithTypeIn(set, ModelTypeSet(), ModelTypeSet()); EXPECT_TRUE(IsInMetahandlesToPurge(handle1)); ASSERT_FALSE(dir_->SaveChanges()); EXPECT_TRUE(IsInMetahandlesToPurge(handle1));