From 1f51ad94964af7f256ac1d762a04cfac1e9935c5 Mon Sep 17 00:00:00 2001 From: "zea@chromium.org" Date: Fri, 7 Jun 2013 02:30:24 +0000 Subject: [PATCH] [Sync] Allow enabling partial sets of types instead of blocking Blocking the datatype manager when datatypes fail to start is a sign of encryption issues, but is not fatal to sync. Therefore, it makes more sense to sync what we can (particularly so we can receive birthday updates), while marking those types with encryption problems as failed. To accomplish this we move all automatic reconfiguration logic into the data type manager itself, and add support for delayed association by unapplying all types with cryptographer errors. This also lays the groundwork for delayed association when a transaction version discrepancy is detected. BUG=238712 Review URL: https://chromiumcodereview.appspot.com/15013004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@204695 0039d316-1c4b-4281-b951-d872f2087c98 --- .../browser/sync/backend_migrator_unittest.cc | 1 + .../sync/glue/backend_data_type_configurer.h | 3 +- chrome/browser/sync/glue/data_type_manager.cc | 8 +- chrome/browser/sync/glue/data_type_manager.h | 12 +- .../sync/glue/data_type_manager_impl.cc | 202 ++++++++++++------ .../sync/glue/data_type_manager_impl.h | 9 +- .../glue/data_type_manager_impl_unittest.cc | 129 +++++------ .../sync/glue/data_type_manager_observer.h | 1 - .../sync/glue/failed_data_types_handler.cc | 38 +++- .../sync/glue/failed_data_types_handler.h | 23 +- .../sync/glue/fake_data_type_controller.cc | 7 +- .../sync/glue/model_association_manager.cc | 44 ++-- .../sync/glue/model_association_manager.h | 4 + .../model_association_manager_unittest.cc | 15 +- .../sync/glue/shared_change_processor.cc | 2 +- chrome/browser/sync/glue/sync_backend_host.cc | 79 +++++-- chrome/browser/sync/glue/sync_backend_host.h | 7 +- .../sync/glue/ui_data_type_controller.cc | 2 +- chrome/browser/sync/profile_sync_service.cc | 84 ++------ chrome/browser/sync/profile_sync_service.h | 13 -- .../sync/profile_sync_service_android.cc | 5 +- .../profile_sync_service_password_unittest.cc | 18 -- .../profile_sync_service_startup_unittest.cc | 1 + .../two_client_bookmarks_sync_test.cc | 2 - .../browser/sync/test_profile_sync_service.cc | 10 +- .../browser/sync/test_profile_sync_service.h | 6 +- sync/engine/syncer_unittest.cc | 9 +- sync/internal_api/public/configure_reason.h | 4 + .../public/engine/model_safe_worker.cc | 2 +- sync/internal_api/public/sync_manager.h | 12 +- .../public/test/fake_sync_manager.h | 6 +- .../sync_encryption_handler_impl.cc | 6 +- sync/internal_api/sync_manager_impl.cc | 41 +++- sync/internal_api/sync_manager_impl.h | 15 +- .../sync_manager_impl_unittest.cc | 160 +++++++++++++- sync/internal_api/test/fake_sync_manager.cc | 13 +- sync/syncable/directory.cc | 159 ++++++++++---- sync/syncable/directory.h | 34 ++- sync/syncable/syncable_mock.h | 4 +- sync/syncable/syncable_unittest.cc | 8 +- 40 files changed, 794 insertions(+), 404 deletions(-) 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));