diff options
| -rw-r--r-- | chrome/browser/sync/glue/backend_data_type_configurer.h | 8 | ||||
| -rw-r--r-- | chrome/browser/sync/glue/data_type_manager_impl_unittest.cc | 8 | ||||
| -rw-r--r-- | chrome/browser/sync/glue/sync_backend_host.cc | 409 | ||||
| -rw-r--r-- | chrome/browser/sync/glue/sync_backend_host.h | 78 | ||||
| -rw-r--r-- | chrome/browser/sync/test_profile_sync_service.cc | 38 | ||||
| -rw-r--r-- | chrome/browser/sync/test_profile_sync_service.h | 11 | ||||
| -rw-r--r-- | sync/engine/sync_scheduler.cc | 255 | ||||
| -rw-r--r-- | sync/engine/sync_scheduler.h | 49 | ||||
| -rw-r--r-- | sync/engine/sync_scheduler_unittest.cc | 192 | ||||
| -rw-r--r-- | sync/engine/sync_scheduler_whitebox_unittest.cc | 2 | ||||
| -rw-r--r-- | sync/internal_api/sync_manager.cc | 77 | ||||
| -rw-r--r-- | sync/internal_api/sync_manager.h | 38 | ||||
| -rw-r--r-- | sync/internal_api/syncapi_unittest.cc | 89 | ||||
| -rw-r--r-- | sync/sync.gyp | 1 | ||||
| -rw-r--r-- | sync/test/callback_counter.h | 29 |
15 files changed, 732 insertions, 552 deletions
diff --git a/chrome/browser/sync/glue/backend_data_type_configurer.h b/chrome/browser/sync/glue/backend_data_type_configurer.h index 161bc14..f15986b 100644 --- a/chrome/browser/sync/glue/backend_data_type_configurer.h +++ b/chrome/browser/sync/glue/backend_data_type_configurer.h @@ -34,11 +34,11 @@ class BackendDataTypeConfigurer { // Nigori. virtual void ConfigureDataTypes( sync_api::ConfigureReason reason, - syncable::ModelTypeSet types_to_add, - syncable::ModelTypeSet types_to_remove, + const syncable::ModelTypeSet& types_to_add, + const syncable::ModelTypeSet& types_to_remove, NigoriState nigori_state, - base::Callback<void(syncable::ModelTypeSet)> ready_task, - base::Callback<void()> retry_callback) = 0; + const base::Callback<void(syncable::ModelTypeSet)>& ready_task, + const base::Callback<void()>& retry_callback) = 0; protected: virtual ~BackendDataTypeConfigurer() {} 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 c344d48..cc0db2b 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc @@ -42,11 +42,11 @@ class FakeBackendDataTypeConfigurer : public BackendDataTypeConfigurer { virtual void ConfigureDataTypes( sync_api::ConfigureReason reason, - ModelTypeSet types_to_add, - ModelTypeSet types_to_remove, + const ModelTypeSet& types_to_add, + const ModelTypeSet& types_to_remove, NigoriState nigori_state, - base::Callback<void(ModelTypeSet)> ready_task, - base::Callback<void()> retry_callback) OVERRIDE { + const base::Callback<void(ModelTypeSet)>& ready_task, + const base::Callback<void()>& retry_callback) OVERRIDE { last_nigori_state_ = nigori_state; last_ready_task_ = ready_task; } diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc index 66f6307..a4f09e7 100644 --- a/chrome/browser/sync/glue/sync_backend_host.cc +++ b/chrome/browser/sync/glue/sync_backend_host.cc @@ -131,10 +131,6 @@ class SyncBackendHost::Core // Called to clear server data. void DoRequestClearServerData(); - // Called to cleanup disabled types. - void DoRequestCleanupDisabledTypes( - const browser_sync::ModelSafeRoutingInfo& routing_info); - // Called to set the passphrase for encryption. void DoSetEncryptionPassphrase(const std::string& passphrase, bool is_explicit); @@ -164,14 +160,18 @@ class SyncBackendHost::Core void DoStopSyncManagerForShutdown(const base::Closure& closure); void DoShutdown(bool stopping_sync); - virtual void DoRequestConfig( - const browser_sync::ModelSafeRoutingInfo& routing_info, + // Configuration methods that must execute on sync loop. + void DoConfigureSyncer( + sync_api::ConfigureReason reason, syncable::ModelTypeSet types_to_config, - sync_api::ConfigureReason reason); - - // Start the configuration mode. |callback| is called on the sync - // thread. - virtual void DoStartConfiguration(const base::Closure& callback); + const browser_sync::ModelSafeRoutingInfo routing_info, + const base::Callback<void(syncable::ModelTypeSet)>& ready_task, + const base::Closure& retry_callback); + void DoFinishConfigureDataTypes( + syncable::ModelTypeSet types_to_config, + const base::Callback<void(syncable::ModelTypeSet)>& ready_task); + void DoRetryConfiguration( + const base::Closure& retry_callback); // Set the base request context to use when making HTTP calls. // This method will add a reference to the context to persist it @@ -184,9 +184,6 @@ class SyncBackendHost::Core // sync databases), as well as shutdown when you're no longer syncing. void DeleteSyncDataFolder(); - // A callback from the SyncerThread when it is safe to continue config. - void FinishConfigureDataTypes(); - private: friend class base::RefCountedThreadSafe<SyncBackendHost::Core>; friend class SyncBackendHostForProfileSyncTest; @@ -556,60 +553,79 @@ void SyncBackendHost::Shutdown(bool sync_disabled) { core_ = NULL; // Releases reference to core_. } +// TODO(sync): remove |disabled_types| completely, as it's just the set of +// registered types - the set of desired types. void SyncBackendHost::ConfigureDataTypes( sync_api::ConfigureReason reason, - syncable::ModelTypeSet types_to_add, - syncable::ModelTypeSet types_to_remove, + const syncable::ModelTypeSet& desired_types, + const syncable::ModelTypeSet& disabled_types, NigoriState nigori_state, - base::Callback<void(syncable::ModelTypeSet)> ready_task, - base::Callback<void()> retry_callback) { - syncable::ModelTypeSet types_to_add_with_nigori = types_to_add; - syncable::ModelTypeSet types_to_remove_with_nigori = types_to_remove; + const base::Callback<void(syncable::ModelTypeSet)>& ready_task, + const base::Callback<void()>& retry_callback) { + syncable::ModelTypeSet desired_types_with_nigori = desired_types; + syncable::ModelTypeSet disabled_types_with_nigori = disabled_types; if (nigori_state == WITH_NIGORI) { - types_to_add_with_nigori.Put(syncable::NIGORI); - types_to_remove_with_nigori.Remove(syncable::NIGORI); + desired_types_with_nigori.Put(syncable::NIGORI); + disabled_types_with_nigori.Remove(syncable::NIGORI); } else { - types_to_add_with_nigori.Remove(syncable::NIGORI); - types_to_remove_with_nigori.Put(syncable::NIGORI); + desired_types_with_nigori.Remove(syncable::NIGORI); + disabled_types_with_nigori.Put(syncable::NIGORI); } - // Only one configure is allowed at a time. - DCHECK(!pending_config_mode_state_.get()); - DCHECK(!pending_download_state_.get()); + // Only one configure is allowed at a time (DataTypeManager handles user + // changes that happen while the syncer is reconfiguraing, and will only + // trigger another call to ConfigureDataTypes once the current reconfiguration + // completes). DCHECK_GT(initialization_state_, NOT_INITIALIZED); - pending_config_mode_state_.reset(new PendingConfigureDataTypesState()); - pending_config_mode_state_->ready_task = ready_task; - pending_config_mode_state_->types_to_add = types_to_add_with_nigori; - pending_config_mode_state_->added_types = - registrar_->ConfigureDataTypes(types_to_add_with_nigori, - types_to_remove_with_nigori); - pending_config_mode_state_->reason = reason; - pending_config_mode_state_->retry_callback = retry_callback; - - // Cleanup disabled types before starting configuration so that - // callers can assume that the data types are cleaned up once - // configuration is done. - if (!types_to_remove_with_nigori.Empty()) { - ModelSafeRoutingInfo routing_info; - registrar_->GetModelSafeRoutingInfo(&routing_info); - sync_thread_.message_loop()->PostTask( - FROM_HERE, - base::Bind(&SyncBackendHost::Core::DoRequestCleanupDisabledTypes, - core_.get(), - routing_info)); - } + // enabled_types \setunion disabled_types gives the set of all registered + // types (i.e. types with datatype controllers registered). + ModelSafeRoutingInfo routing_info; + registrar_->ConfigureDataTypes(desired_types_with_nigori, + disabled_types_with_nigori); + registrar_->GetModelSafeRoutingInfo(&routing_info); - StartConfiguration( - base::Bind(&SyncBackendHost::Core::FinishConfigureDataTypes, - core_.get())); -} + // |enabled_types| is desired_types_with_nigori \setintersect types with + // model safe workers (e.g. the password store may fail to start up, in which + // case we have no model safe worker for it and should not enable it). + // TODO(sync): This shouldn't be necessary. We should do whatever filtering + // we need beforehand as part of ProfileSyncComponentsFactory's + // RegisterDataTypes. + const syncable::ModelTypeSet enabled_types = + GetRoutingInfoTypes(routing_info); -void SyncBackendHost::StartConfiguration(const base::Closure& callback) { - // Put syncer in the config mode. DTM will put us in normal mode once it is - // done. This is to ensure we dont do a normal sync when we are doing model - // association. - sync_thread_.message_loop()->PostTask(FROM_HERE, base::Bind( - &SyncBackendHost::Core::DoStartConfiguration, core_.get(), callback)); + // Figure out which types need to actually be downloaded. We pass those on + // to the syncer while it's in configuration mode so that they can be + // downloaded before we perform association. Once we switch to normal mode + // downloads will get applied normally and hit the datatype's change + // processor. + // A datatype is in need of downloading if any of the following are true: + // 1. it's enabled and initial_sync_ended is false (initial_sync_ended is + // set after applying updates, and hence is a more conservative measure + // than having a non-empty progress marker, which is set during + // StoreTimestamps). + // 2. the type is NIGORI, and any other datatype is being downloaded (nigori + // is always included if we download a datatype). + // TODO(sync): consider moving this logic onto the sync thread (perhaps + // as part of SyncManager::ConfigureSyncer). + syncable::ModelTypeSet initial_sync_ended_types = + core_->sync_manager()->InitialSyncEndedTypes(); + initial_sync_ended_types.RetainAll(enabled_types); + syncable::ModelTypeSet types_to_config = + Difference(enabled_types, initial_sync_ended_types); + if (!types_to_config.Empty() && enabled_types.Has(syncable::NIGORI)) + types_to_config.Put(syncable::NIGORI); + + SDVLOG(1) << "Types " + << syncable::ModelTypeSetToString(types_to_config) + << " 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. + RequestConfigureSyncer(reason, + types_to_config, + routing_info, + ready_task, + retry_callback); } void SyncBackendHost::EnableEncryptEverything() { @@ -687,157 +703,47 @@ void SyncBackendHost::InitCore(const DoInitializeOptions& options) { base::Bind(&SyncBackendHost::Core::DoInitialize, core_.get(), options)); } -void SyncBackendHost::HandleSyncCycleCompletedOnFrontendLoop( - const SyncSessionSnapshot& snapshot) { - if (!frontend_) - return; - DCHECK_EQ(MessageLoop::current(), frontend_loop_); - - last_snapshot_ = snapshot; - - SDVLOG(1) << "Got snapshot " << snapshot.ToString(); - - const syncable::ModelTypeSet to_migrate = - snapshot.syncer_status().types_needing_local_migration; - if (!to_migrate.Empty()) - frontend_->OnMigrationNeededForTypes(to_migrate); - - // Process any changes to the datatypes we're syncing. - // TODO(sync): add support for removing types. - if (initialized()) - AddExperimentalTypes(); - - // If we are waiting for a configuration change, check here to see - // if this sync cycle has initialized all of the types we've been - // waiting for. - if (pending_download_state_.get()) { - const syncable::ModelTypeSet types_to_add = - pending_download_state_->types_to_add; - const syncable::ModelTypeSet added_types = - pending_download_state_->added_types; - DCHECK(types_to_add.HasAll(added_types)); - const syncable::ModelTypeSet initial_sync_ended = - snapshot.initial_sync_ended(); - const syncable::ModelTypeSet failed_configuration_types = - Difference(added_types, initial_sync_ended); - SDVLOG(1) - << "Added types: " - << syncable::ModelTypeSetToString(added_types) - << ", configured types: " - << syncable::ModelTypeSetToString(initial_sync_ended) - << ", failed configuration types: " - << syncable::ModelTypeSetToString(failed_configuration_types); - - if (!failed_configuration_types.Empty() && - snapshot.retry_scheduled()) { - // Inform the caller that download failed but we are retrying. - if (!pending_download_state_->retry_in_progress) { - pending_download_state_->retry_callback.Run(); - pending_download_state_->retry_in_progress = true; - } - // Nothing more to do. - return; - } - - scoped_ptr<PendingConfigureDataTypesState> state( - pending_download_state_.release()); - state->ready_task.Run(failed_configuration_types); - - // Syncer did not report an error but did not download everything - // we requested either. So abort. The caller of the config will cleanup. - if (!failed_configuration_types.Empty()) - return; - } - - if (initialized()) - frontend_->OnSyncCycleCompleted(); -} - -void SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop() { - DCHECK_EQ(MessageLoop::current(), frontend_loop_); - // Nudge the syncer. This is necessary for both datatype addition/deletion. - // - // Deletions need a nudge in order to ensure the deletion occurs in a timely - // manner (see issue 56416). - // - // In the case of additions, on the next sync cycle, the syncer should - // notice that the routing info has changed and start the process of - // downloading updates for newly added data types. Once this is - // complete, the configure_state_.ready_task_ is run via an - // OnInitializationComplete notification. - - SDVLOG(1) << "Syncer in config mode. SBH executing " - << "FinishConfigureDataTypesOnFrontendLoop"; - - - ModelSafeRoutingInfo routing_info; - registrar_->GetModelSafeRoutingInfo(&routing_info); - const syncable::ModelTypeSet enabled_types = - GetRoutingInfoTypes(routing_info); +void SyncBackendHost::RequestConfigureSyncer( + sync_api::ConfigureReason reason, + syncable::ModelTypeSet types_to_config, + const browser_sync::ModelSafeRoutingInfo& routing_info, + const base::Callback<void(syncable::ModelTypeSet)>& ready_task, + const base::Closure& retry_callback) { + sync_thread_.message_loop()->PostTask(FROM_HERE, + base::Bind(&SyncBackendHost::Core::DoConfigureSyncer, + core_.get(), + reason, + types_to_config, + routing_info, + ready_task, + retry_callback)); +} + +void SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop( + const syncable::ModelTypeSet& types_to_configure, + const syncable::ModelTypeSet& configured_types, + const base::Callback<void(syncable::ModelTypeSet)>& ready_task) { + const syncable::ModelTypeSet failed_configuration_types = + Difference(types_to_configure, configured_types); + SDVLOG(1) + << "Added types: " + << syncable::ModelTypeSetToString(types_to_configure) + << ", configured types: " + << syncable::ModelTypeSetToString(configured_types) + << ", failed configuration types: " + << syncable::ModelTypeSetToString(failed_configuration_types); // Update |chrome_sync_notification_bridge_|'s enabled types here as it has // to happen on the UI thread. - chrome_sync_notification_bridge_.UpdateEnabledTypes(enabled_types); - - if (pending_config_mode_state_->added_types.Empty() && - !core_->sync_manager()->InitialSyncEndedTypes().HasAll(enabled_types)) { - - // TODO(tim): Log / UMA / count this somehow? - // Add only the types with empty progress markers. Note: it is possible - // that some types have their initial_sync_ended be false but with non - // empty progress marker. Which is ok as the rest of the changes would - // be downloaded on a regular nudge and initial_sync_ended should be set - // to true. However this is a very corner case. So it is not explicitly - // handled. - pending_config_mode_state_->added_types = - sync_api::GetTypesWithEmptyProgressMarkerToken(enabled_types, - GetUserShare()); - } - - // If we've added types, we always want to request a nudge/config (even if - // the initial sync is ended), in case we could not decrypt the data. - if (pending_config_mode_state_->added_types.Empty()) { - SDVLOG(1) << "No new types added; calling ready_task directly"; - // No new types - just notify the caller that the types are available. - const syncable::ModelTypeSet failed_configuration_types; - pending_config_mode_state_->ready_task.Run(failed_configuration_types); - } else { - pending_download_state_.reset(pending_config_mode_state_.release()); - - // Always configure nigori if it's enabled. - syncable::ModelTypeSet types_to_config = - pending_download_state_->added_types; - if (IsNigoriEnabled()) { - // Note: Nigori is the only type that gets added with a nonempty - // progress marker during config. If the server returns a migration - // error then we will go into unrecoverable error. We dont handle it - // explicitly because server might help us out here by not sending a - // migraiton error for nigori during config. - types_to_config.Put(syncable::NIGORI); - } - SDVLOG(1) << "Types " - << syncable::ModelTypeSetToString(types_to_config) - << " added; calling DoRequestConfig"; - ModelSafeRoutingInfo routing_info; - registrar_->GetModelSafeRoutingInfo(&routing_info); - sync_thread_.message_loop()->PostTask(FROM_HERE, - base::Bind(&SyncBackendHost::Core::DoRequestConfig, - core_.get(), - routing_info, - types_to_config, - pending_download_state_->reason)); - } - - pending_config_mode_state_.reset(); + chrome_sync_notification_bridge_.UpdateEnabledTypes(configured_types); // Notify SyncManager (especially the notification listener) about new types. sync_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(&SyncBackendHost::Core::DoUpdateEnabledTypes, core_.get(), - enabled_types)); -} + configured_types)); -bool SyncBackendHost::IsDownloadingNigoriForTest() const { - return initialization_state_ == DOWNLOADING_NIGORI; + if (!ready_task.is_null()) + ready_task.Run(failed_configuration_types); } SyncBackendHost::DoInitializeOptions::DoInitializeOptions( @@ -894,14 +800,6 @@ SyncBackendHost::Core::~Core() { DCHECK(!sync_loop_); } -SyncBackendHost::PendingConfigureDataTypesState:: -PendingConfigureDataTypesState() - : reason(sync_api::CONFIGURE_REASON_UNKNOWN), - retry_in_progress(false) {} - -SyncBackendHost::PendingConfigureDataTypesState:: -~PendingConfigureDataTypesState() {} - void SyncBackendHost::Core::OnSyncCycleCompleted( const SyncSessionSnapshot& snapshot) { if (!sync_loop_) @@ -1153,12 +1051,6 @@ void SyncBackendHost::Core::DoRequestClearServerData() { sync_manager_->RequestClearServerData(); } -void SyncBackendHost::Core::DoRequestCleanupDisabledTypes( - const browser_sync::ModelSafeRoutingInfo& routing_info) { - DCHECK_EQ(MessageLoop::current(), sync_loop_); - sync_manager_->RequestCleanupDisabledTypes(routing_info); -} - void SyncBackendHost::Core::DoSetEncryptionPassphrase( const std::string& passphrase, bool is_explicit) { @@ -1210,18 +1102,46 @@ void SyncBackendHost::Core::DoShutdown(bool sync_disabled) { host_.Reset(); } -void SyncBackendHost::Core::DoRequestConfig( - const browser_sync::ModelSafeRoutingInfo& routing_info, +void SyncBackendHost::Core::DoConfigureSyncer( + sync_api::ConfigureReason reason, syncable::ModelTypeSet types_to_config, - sync_api::ConfigureReason reason) { + const browser_sync::ModelSafeRoutingInfo routing_info, + const base::Callback<void(syncable::ModelTypeSet)>& ready_task, + const base::Closure& retry_callback) { DCHECK_EQ(MessageLoop::current(), sync_loop_); - sync_manager_->RequestConfig(routing_info, types_to_config, reason); + sync_manager_->ConfigureSyncer( + reason, + types_to_config, + routing_info, + base::Bind(&SyncBackendHost::Core::DoFinishConfigureDataTypes, + this, + types_to_config, + ready_task), + base::Bind(&SyncBackendHost::Core::DoRetryConfiguration, + this, + retry_callback)); } -void SyncBackendHost::Core::DoStartConfiguration( - const base::Closure& callback) { +void SyncBackendHost::Core::DoFinishConfigureDataTypes( + syncable::ModelTypeSet types_to_config, + const base::Callback<void(syncable::ModelTypeSet)>& ready_task) { + DCHECK_EQ(MessageLoop::current(), sync_loop_); + syncable::ModelTypeSet configured_types = + sync_manager_->InitialSyncEndedTypes(); + configured_types.RetainAll(types_to_config); + host_.Call(FROM_HERE, + &SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop, + types_to_config, + configured_types, + ready_task); +} + +void SyncBackendHost::Core::DoRetryConfiguration( + const base::Closure& retry_callback) { DCHECK_EQ(MessageLoop::current(), sync_loop_); - sync_manager_->StartConfigurationMode(callback); + host_.Call(FROM_HERE, + &SyncBackendHost::RetryConfigurationOnFrontendLoop, + retry_callback); } void SyncBackendHost::Core::DeleteSyncDataFolder() { @@ -1232,13 +1152,6 @@ void SyncBackendHost::Core::DeleteSyncDataFolder() { } } -void SyncBackendHost::Core::FinishConfigureDataTypes() { - DCHECK_EQ(MessageLoop::current(), sync_loop_); - host_.Call( - FROM_HERE, - &SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop); -} - void SyncBackendHost::Core::StartSavingChanges() { // We may already be shut down. if (!sync_loop_) @@ -1289,14 +1202,6 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop( return; } - // If setup has completed, start off in DOWNLOADING_NIGORI so that - // we start off by refreshing nigori. - CHECK(sync_prefs_.get()); - if (sync_prefs_->HasSyncSetupCompleted() && - initialization_state_ < DOWNLOADING_NIGORI) { - initialization_state_ = DOWNLOADING_NIGORI; - } - // Run initialization state machine. switch (initialization_state_) { case NOT_INITIALIZED: @@ -1337,6 +1242,36 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop( } } +void SyncBackendHost::HandleSyncCycleCompletedOnFrontendLoop( + const SyncSessionSnapshot& snapshot) { + if (!frontend_) + return; + DCHECK_EQ(MessageLoop::current(), frontend_loop_); + + last_snapshot_ = snapshot; + + SDVLOG(1) << "Got snapshot " << snapshot.ToString(); + + const syncable::ModelTypeSet to_migrate = + snapshot.syncer_status().types_needing_local_migration; + if (!to_migrate.Empty()) + frontend_->OnMigrationNeededForTypes(to_migrate); + + // Process any changes to the datatypes we're syncing. + // TODO(sync): add support for removing types. + if (initialized()) + AddExperimentalTypes(); + + if (initialized()) + frontend_->OnSyncCycleCompleted(); +} + +void SyncBackendHost::RetryConfigurationOnFrontendLoop( + const base::Closure& retry_callback) { + SDVLOG(1) << "Failed to complete configuration, informing of retry."; + retry_callback.Run(); +} + void SyncBackendHost::PersistEncryptionBootstrapToken( const std::string& token) { CHECK(sync_prefs_.get()); diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h index 600e516..00aa77e 100644 --- a/chrome/browser/sync/glue/sync_backend_host.h +++ b/chrome/browser/sync/glue/sync_backend_host.h @@ -226,15 +226,11 @@ class SyncBackendHost : public BackendDataTypeConfigurer { // is non-empty, then an error was encountered). virtual void ConfigureDataTypes( sync_api::ConfigureReason reason, - syncable::ModelTypeSet types_to_add, - syncable::ModelTypeSet types_to_remove, + const syncable::ModelTypeSet& desired_types, + const syncable::ModelTypeSet& disabled_types, NigoriState nigori_state, - base::Callback<void(syncable::ModelTypeSet)> ready_task, - base::Callback<void()> retry_callback) OVERRIDE; - - // Makes an asynchronous call to syncer to switch to config mode. When done - // syncer will call us back on FinishConfigureDataTypes. - virtual void StartConfiguration(const base::Closure& callback); + const base::Callback<void(syncable::ModelTypeSet)>& ready_task, + const base::Callback<void()>& retry_callback) OVERRIDE; // Turns on encryption of all present and future sync data. virtual void EnableEncryptEverything(); @@ -333,16 +329,20 @@ class SyncBackendHost : public BackendDataTypeConfigurer { // Allows tests to perform alternate core initialization work. virtual void InitCore(const DoInitializeOptions& options); - // Called from Core::OnSyncCycleCompleted to handle updating frontend - // thread components. - void HandleSyncCycleCompletedOnFrontendLoop( - const sessions::SyncSessionSnapshot& snapshot); - - // Called to finish the job of ConfigureDataTypes once the syncer is in - // configuration mode. - void FinishConfigureDataTypesOnFrontendLoop(); + // Request the syncer to reconfigure with the specfied params. + // Virtual for testing. + virtual void RequestConfigureSyncer( + sync_api::ConfigureReason reason, + syncable::ModelTypeSet types_to_config, + const browser_sync::ModelSafeRoutingInfo& routing_info, + const base::Callback<void(syncable::ModelTypeSet)>& ready_task, + const base::Closure& retry_callback); - bool IsDownloadingNigoriForTest() const; + // Called when the syncer has finished performing a configuration. + void FinishConfigureDataTypesOnFrontendLoop( + const syncable::ModelTypeSet& types_to_configure, + const syncable::ModelTypeSet& configured_types, + const base::Callback<void(syncable::ModelTypeSet)>& ready_task); private: // The real guts of SyncBackendHost, to keep the public client API clean. @@ -365,47 +365,32 @@ class SyncBackendHost : public BackendDataTypeConfigurer { INITIALIZED, // Initialization is complete. }; - struct PendingConfigureDataTypesState { - PendingConfigureDataTypesState(); - ~PendingConfigureDataTypesState(); - - // The ready_task will be run when configuration is done with the - // set of all types that failed configuration (i.e., if its - // argument is non-empty, then an error was encountered). - base::Callback<void(syncable::ModelTypeSet)> ready_task; - - // The retry callback will be run when the download failed due to a - // transient error. This is to notify DTM so it can apropriately inform - // the UI. Note: The retry_callback will be run only once and after - // that we will not notify DTM until the sync is successful or in a - // permanent error state. - base::Callback<void()> retry_callback; - - // The set of types that we are waiting to be initially synced in a - // configuration cycle. - syncable::ModelTypeSet types_to_add; - - // Additional details about which types were added. - syncable::ModelTypeSet added_types; - sync_api::ConfigureReason reason; - bool retry_in_progress; - }; - // Checks if we have received a notice to turn on experimental datatypes // (via the nigori node) and informs the frontend if that is the case. // Note: it is illegal to call this before the backend is initialized. void AddExperimentalTypes(); // Downloading of nigori failed and will be retried. - virtual void OnNigoriDownloadRetry(); + void OnNigoriDownloadRetry(); // InitializationComplete passes through the SyncBackendHost to forward // on to |frontend_|, and so that tests can intercept here if they need to // set up initial conditions. - virtual void HandleInitializationCompletedOnFrontendLoop( + void HandleInitializationCompletedOnFrontendLoop( const WeakHandle<JsBackend>& js_backend, bool success); + // Called from Core::OnSyncCycleCompleted to handle updating frontend + // thread components. + void HandleSyncCycleCompletedOnFrontendLoop( + const sessions::SyncSessionSnapshot& snapshot); + + // Called when the syncer failed to perform a configuration and will + // eventually retry. FinishingConfigurationOnFrontendLoop(..) will be called + // on successful completion. + void RetryConfigurationOnFrontendLoop( + const base::Closure& retry_callback); + // Helpers to persist a token that can be used to bootstrap sync encryption // across browser restart to avoid requiring the user to re-enter their // passphrase. |token| must be valid UTF-8 as we use the PrefService for @@ -509,9 +494,6 @@ class SyncBackendHost : public BackendDataTypeConfigurer { // The frontend which we serve (and are owned by). SyncFrontend* frontend_; - scoped_ptr<PendingConfigureDataTypesState> pending_download_state_; - scoped_ptr<PendingConfigureDataTypesState> pending_config_mode_state_; - // We cache the cryptographer's pending keys whenever NotifyPassphraseRequired // is called. This way, before the UI calls SetDecryptionPassphrase on the // syncer, it can avoid the overhead of an asynchronous decryption call and diff --git a/chrome/browser/sync/test_profile_sync_service.cc b/chrome/browser/sync/test_profile_sync_service.cc index 6d16734..4a89b8c 100644 --- a/chrome/browser/sync/test_profile_sync_service.cc +++ b/chrome/browser/sync/test_profile_sync_service.cc @@ -44,19 +44,6 @@ SyncBackendHostForProfileSyncTest::SyncBackendHostForProfileSyncTest( SyncBackendHostForProfileSyncTest::~SyncBackendHostForProfileSyncTest() {} -void SyncBackendHostForProfileSyncTest:: - SimulateSyncCycleCompletedInitialSyncEnded( - const tracked_objects::Location& location) { - syncable::ModelTypeSet sync_ended; - if (!fail_initial_download_) - sync_ended = syncable::ModelTypeSet::All(); - syncable::ModelTypePayloadMap download_progress_markers; - HandleSyncCycleCompletedOnFrontendLoop(SyncSessionSnapshot( - SyncerStatus(), ErrorCounters(), 0, false, - sync_ended, download_progress_markers, false, false, 0, 0, 0, 0, - SyncSourceInfo(), false, 0, base::Time::Now(), false)); -} - namespace { sync_api::HttpPostProviderFactory* MakeTestHttpBridgeFactory() { @@ -85,20 +72,19 @@ void SyncBackendHostForProfileSyncTest::InitCore( } } -void SyncBackendHostForProfileSyncTest::StartConfiguration( - const base::Closure& callback) { - SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop(); - if (IsDownloadingNigoriForTest()) { - syncable::ModelTypeSet sync_ended; +void SyncBackendHostForProfileSyncTest::RequestConfigureSyncer( + sync_api::ConfigureReason reason, + syncable::ModelTypeSet types_to_config, + const browser_sync::ModelSafeRoutingInfo& routing_info, + const base::Callback<void(syncable::ModelTypeSet)>& ready_task, + const base::Closure& retry_callback) { + syncable::ModelTypeSet sync_ended; + if (!fail_initial_download_) + sync_ended.PutAll(types_to_config); - if (!fail_initial_download_) - sync_ended.Put(syncable::NIGORI); - syncable::ModelTypePayloadMap download_progress_markers; - HandleSyncCycleCompletedOnFrontendLoop(SyncSessionSnapshot( - SyncerStatus(), ErrorCounters(), 0, false, - sync_ended, download_progress_markers, false, false, 0, 0, 0, 0, - SyncSourceInfo(), false, 0, base::Time::Now(), false)); - } + FinishConfigureDataTypesOnFrontendLoop(types_to_config, + sync_ended, + ready_task); } void SyncBackendHostForProfileSyncTest::SetHistoryServiceExpectations( diff --git a/chrome/browser/sync/test_profile_sync_service.h b/chrome/browser/sync/test_profile_sync_service.h index 1227e5f..efec408 100644 --- a/chrome/browser/sync/test_profile_sync_service.h +++ b/chrome/browser/sync/test_profile_sync_service.h @@ -45,11 +45,12 @@ class SyncBackendHostForProfileSyncTest : public SyncBackendHost { MOCK_METHOD1(RequestNudge, void(const tracked_objects::Location&)); - // Called when a nudge comes in. - void SimulateSyncCycleCompletedInitialSyncEnded( - const tracked_objects::Location&); - - virtual void StartConfiguration(const base::Closure& callback) OVERRIDE; + virtual void RequestConfigureSyncer( + sync_api::ConfigureReason reason, + syncable::ModelTypeSet types_to_config, + const browser_sync::ModelSafeRoutingInfo& routing_info, + const base::Callback<void(syncable::ModelTypeSet)>& ready_task, + const base::Closure& retry_callback) OVERRIDE; static void SetHistoryServiceExpectations(ProfileMock* profile); diff --git a/sync/engine/sync_scheduler.cc b/sync/engine/sync_scheduler.cc index f798d67..e745d19 100644 --- a/sync/engine/sync_scheduler.cc +++ b/sync/engine/sync_scheduler.cc @@ -68,6 +68,24 @@ bool IsActionableError( } } // namespace +ConfigurationParams::ConfigurationParams() + : source(GetUpdatesCallerInfo::UNKNOWN), + keystore_key_status(KEYSTORE_KEY_UNNECESSARY) {} +ConfigurationParams::ConfigurationParams( + const sync_pb::GetUpdatesCallerInfo::GetUpdatesSource& source, + const syncable::ModelTypeSet& types_to_download, + const browser_sync::ModelSafeRoutingInfo& routing_info, + KeystoreKeyStatus keystore_key_status, + const base::Closure& ready_task) + : source(source), + types_to_download(types_to_download), + routing_info(routing_info), + keystore_key_status(keystore_key_status), + ready_task(ready_task) { + DCHECK(!ready_task.is_null()); +} +ConfigurationParams::~ConfigurationParams() {} + SyncScheduler::DelayProvider::DelayProvider() {} SyncScheduler::DelayProvider::~DelayProvider() {} @@ -99,12 +117,16 @@ SyncScheduler::SyncSessionJob::~SyncSessionJob() {} SyncScheduler::SyncSessionJob::SyncSessionJob(SyncSessionJobPurpose purpose, base::TimeTicks start, - linked_ptr<sessions::SyncSession> session, bool is_canary_job, - const tracked_objects::Location& from_here) : purpose(purpose), - scheduled_start(start), - session(session), - is_canary_job(is_canary_job), - from_here(from_here) { + linked_ptr<sessions::SyncSession> session, + bool is_canary_job, + ConfigurationParams config_params, + const tracked_objects::Location& from_here) + : purpose(purpose), + scheduled_start(start), + session(session), + is_canary_job(is_canary_job), + config_params(config_params), + from_here(from_here) { } const char* SyncScheduler::SyncSessionJob::GetPurposeString( @@ -247,7 +269,7 @@ void SyncScheduler::UpdateServerConnectionManagerStatus( connection_code_ = code; } -void SyncScheduler::Start(Mode mode, const base::Closure& callback) { +void SyncScheduler::Start(Mode mode) { DCHECK_EQ(MessageLoop::current(), sync_loop_); std::string thread_name = MessageLoop::current()->thread_name(); if (thread_name.empty()) @@ -264,8 +286,6 @@ void SyncScheduler::Start(Mode mode, const base::Closure& callback) { Mode old_mode = mode_; mode_ = mode; AdjustPolling(NULL); // Will kick start poll timer if needed. - if (!callback.is_null()) - callback.Run(); if (old_mode != mode_) { // We just changed our mode. See if there are any pending jobs that we could @@ -284,6 +304,112 @@ void SyncScheduler::SendInitialSnapshot() { session_context_->NotifyListeners(event); } +namespace { + +// Helper to extract the routing info and workers corresponding to types in +// |types| from |current_routes| and |current_workers|. +void BuildModelSafeParams( + const ModelTypeSet& types_to_download, + const ModelSafeRoutingInfo& current_routes, + const std::vector<ModelSafeWorker*>& current_workers, + ModelSafeRoutingInfo* result_routes, + std::vector<ModelSafeWorker*>* result_workers) { + std::set<ModelSafeGroup> active_groups; + active_groups.insert(GROUP_PASSIVE); + for (ModelTypeSet::Iterator iter = types_to_download.First(); iter.Good(); + iter.Inc()) { + syncable::ModelType type = iter.Get(); + ModelSafeRoutingInfo::const_iterator route = current_routes.find(type); + DCHECK(route != current_routes.end()); + ModelSafeGroup group = route->second; + (*result_routes)[type] = group; + active_groups.insert(group); + } + + for(std::vector<ModelSafeWorker*>::const_iterator iter = + current_workers.begin(); iter != current_workers.end(); ++iter) { + if (active_groups.count((*iter)->GetModelSafeGroup()) > 0) + result_workers->push_back(*iter); + } +} + +} // namespace. + +bool SyncScheduler::ScheduleConfiguration(const ConfigurationParams& params) { + DCHECK_EQ(MessageLoop::current(), sync_loop_); + DCHECK(IsConfigRelatedUpdateSourceValue(params.source)); + DCHECK_EQ(CONFIGURATION_MODE, mode_); + DCHECK(!params.ready_task.is_null()); + SDVLOG(2) << "Reconfiguring syncer."; + + // Only one configuration is allowed at a time. Verify we're not waiting + // for a pending configure job. + DCHECK(!wait_interval_.get() || !wait_interval_->pending_configure_job.get()); + + // TODO(sync): now that ModelChanging commands only use those workers within + // the routing info, we don't really need |restricted_workers|. Remove it. + // crbug.com/133030 + browser_sync::ModelSafeRoutingInfo restricted_routes; + std::vector<ModelSafeWorker*> restricted_workers; + BuildModelSafeParams(params.types_to_download, + params.routing_info, + session_context_->workers(), + &restricted_routes, + &restricted_workers); + session_context_->set_routing_info(params.routing_info); + + // We rely on this not failing, so don't need to worry about checking for + // success. In addition, this will be removed as part of crbug.com/131433. + SyncSessionJob cleanup_job( + SyncSessionJob::CLEANUP_DISABLED_TYPES, + TimeTicks::Now(), + make_linked_ptr(CreateSyncSession(SyncSourceInfo())), + false, + ConfigurationParams(), + FROM_HERE); + DoSyncSessionJob(cleanup_job); + + if (params.keystore_key_status == ConfigurationParams::KEYSTORE_KEY_NEEDED) { + // TODO(zea): implement in such a way that we can handle failures and the + // subsequent retrys the scheduler might perform. See crbug.com/129665. + NOTIMPLEMENTED(); + } + + // Only reconfigure if we have types to download. + if (!params.types_to_download.Empty()) { + DCHECK(!restricted_routes.empty()); + linked_ptr<SyncSession> session(new SyncSession( + session_context_, + this, + SyncSourceInfo(params.source, + syncable::ModelTypePayloadMapFromRoutingInfo( + restricted_routes, + std::string())), + restricted_routes, + restricted_workers)); + SyncSessionJob job(SyncSessionJob::CONFIGURATION, + TimeTicks::Now(), + session, + false, + params, + FROM_HERE); + DoSyncSessionJob(job); + + // If we failed, the job would have been saved as the pending configure + // job and a wait interval would have been set. + if (!session->Succeeded()) { + DCHECK(wait_interval_.get() && + wait_interval_->pending_configure_job.get()); + return false; + } + } else { + SDVLOG(2) << "No change in routing info, calling ready task directly."; + params.ready_task.Run(); + } + + return true; +} + SyncScheduler::JobProcessDecision SyncScheduler::DecideWhileInWaitInterval( const SyncSessionJob& job) { DCHECK_EQ(MessageLoop::current(), sync_loop_); @@ -382,7 +508,8 @@ void SyncScheduler::InitOrCoalescePendingJob(const SyncSessionJob& job) { s->delegate(), s->source(), s->routing_info(), s->workers())); SyncSessionJob new_job(SyncSessionJob::NUDGE, job.scheduled_start, - make_linked_ptr(session.release()), false, job.from_here); + make_linked_ptr(session.release()), false, + ConfigurationParams(), job.from_here); pending_nudge_.reset(new SyncSessionJob(new_job)); return; @@ -428,11 +555,17 @@ void SyncScheduler::SaveJob(const SyncSessionJob& job) { DCHECK(wait_interval_.get()); DCHECK(mode_ == CONFIGURATION_MODE); + // Config params should always get set. + DCHECK(!job.config_params.ready_task.is_null()); SyncSession* old = job.session.get(); SyncSession* s(new SyncSession(session_context_, this, old->source(), old->routing_info(), old->workers())); - SyncSessionJob new_job(job.purpose, TimeTicks::Now(), - make_linked_ptr(s), false, job.from_here); + SyncSessionJob new_job(job.purpose, + TimeTicks::Now(), + make_linked_ptr(s), + false, + job.config_params, + job.from_here); wait_interval_->pending_configure_job.reset(new SyncSessionJob(new_job)); } // drop the rest. // TODO(sync): Is it okay to drop the rest? It's weird that @@ -451,23 +584,16 @@ struct ModelSafeWorkerGroupIs { void SyncScheduler::ClearUserData() { DCHECK_EQ(MessageLoop::current(), sync_loop_); - SyncSessionJob job(SyncSessionJob::CLEAR_USER_DATA, TimeTicks::Now(), + SyncSessionJob job(SyncSessionJob::CLEAR_USER_DATA, + TimeTicks::Now(), make_linked_ptr(CreateSyncSession(SyncSourceInfo())), false, + ConfigurationParams(), FROM_HERE); DoSyncSessionJob(job); } -void SyncScheduler::CleanupDisabledTypes() { - DCHECK_EQ(MessageLoop::current(), sync_loop_); - SyncSessionJob job(SyncSessionJob::CLEANUP_DISABLED_TYPES, TimeTicks::Now(), - make_linked_ptr(CreateSyncSession(SyncSourceInfo())), - false, - FROM_HERE); - DoSyncSessionJob(job); -} - void SyncScheduler::ScheduleNudgeAsync( const TimeDelta& delay, NudgeSource source, ModelTypeSet types, @@ -525,7 +651,7 @@ void SyncScheduler::ScheduleNudgeImpl( SyncSession* session(CreateSyncSession(info)); SyncSessionJob job(SyncSessionJob::NUDGE, TimeTicks::Now() + delay, make_linked_ptr(session), is_canary_job, - nudge_location); + ConfigurationParams(), nudge_location); session = NULL; if (!ShouldRunJob(job)) @@ -556,77 +682,6 @@ void SyncScheduler::ScheduleNudgeImpl( ScheduleSyncSessionJob(job); } -// Helper to extract the routing info and workers corresponding to types in -// |types| from |current_routes| and |current_workers|. -void GetModelSafeParamsForTypes(ModelTypeSet types, - const ModelSafeRoutingInfo& current_routes, - const std::vector<ModelSafeWorker*>& current_workers, - ModelSafeRoutingInfo* result_routes, - std::vector<ModelSafeWorker*>* result_workers) { - bool passive_group_added = false; - - typedef std::vector<ModelSafeWorker*>::const_iterator iter; - for (ModelTypeSet::Iterator it = types.First(); - it.Good(); it.Inc()) { - const syncable::ModelType t = it.Get(); - ModelSafeRoutingInfo::const_iterator route = current_routes.find(t); - DCHECK(route != current_routes.end()); - ModelSafeGroup group = route->second; - - (*result_routes)[t] = group; - iter w_tmp_it = std::find_if(current_workers.begin(), current_workers.end(), - ModelSafeWorkerGroupIs(group)); - if (w_tmp_it != current_workers.end()) { - iter result_workers_it = std::find_if( - result_workers->begin(), result_workers->end(), - ModelSafeWorkerGroupIs(group)); - if (result_workers_it == result_workers->end()) - result_workers->push_back(*w_tmp_it); - - if (group == GROUP_PASSIVE) - passive_group_added = true; - } else { - NOTREACHED(); - } - } - - // Always add group passive. - if (passive_group_added == false) { - iter it = std::find_if(current_workers.begin(), current_workers.end(), - ModelSafeWorkerGroupIs(GROUP_PASSIVE)); - if (it != current_workers.end()) - result_workers->push_back(*it); - else - NOTREACHED(); - } -} - -void SyncScheduler::ScheduleConfiguration( - ModelTypeSet types, - GetUpdatesCallerInfo::GetUpdatesSource source) { - DCHECK_EQ(MessageLoop::current(), sync_loop_); - DCHECK(IsConfigRelatedUpdateSourceValue(source)); - SDVLOG(2) << "Scheduling a config"; - - ModelSafeRoutingInfo routes; - std::vector<ModelSafeWorker*> workers; - GetModelSafeParamsForTypes(types, - session_context_->routing_info(), - session_context_->workers(), - &routes, &workers); - - SyncSession* session = new SyncSession(session_context_, this, - SyncSourceInfo(source, - syncable::ModelTypePayloadMapFromRoutingInfo( - routes, std::string())), - routes, workers); - SyncSessionJob job(SyncSessionJob::CONFIGURATION, TimeTicks::Now(), - make_linked_ptr(session), - false, - FROM_HERE); - DoSyncSessionJob(job); -} - const char* SyncScheduler::GetModeString(SyncScheduler::Mode mode) { switch (mode) { ENUM_CASE(CONFIGURATION_MODE); @@ -829,6 +884,11 @@ void SyncScheduler::FinishSyncSessionJob(const SyncSessionJob& job) { // here; see DCHECKs in SaveJob(). (See http://crbug.com/90868.) SaveJob(job); return; // Nothing to do. + } else if (job.session->Succeeded() && + !job.config_params.ready_task.is_null()) { + // If this was a configuration job with a ready task, invoke it now that + // we finished successfully. + job.config_params.ready_task.Run(); } SDVLOG(2) << "Updating the next polling time after SyncMain"; @@ -931,11 +991,15 @@ void SyncScheduler::HandleContinuationError( wait_interval_.reset(new WaitInterval(WaitInterval::EXPONENTIAL_BACKOFF, length)); if (old_job.purpose == SyncSessionJob::CONFIGURATION) { + SDVLOG(2) << "Configuration did not succeed, scheduling retry."; + // Config params should always get set. + DCHECK(!old_job.config_params.ready_task.is_null()); SyncSession* old = old_job.session.get(); SyncSession* s(new SyncSession(session_context_, this, old->source(), old->routing_info(), old->workers())); SyncSessionJob job(old_job.purpose, TimeTicks::Now() + length, - make_linked_ptr(s), false, FROM_HERE); + make_linked_ptr(s), false, old_job.config_params, + FROM_HERE); wait_interval_->pending_configure_job.reset(new SyncSessionJob(job)); } else { // We are not in configuration mode. So wait_interval's pending job @@ -1057,6 +1121,7 @@ void SyncScheduler::PollTimerCallback() { SyncSessionJob job(SyncSessionJob::POLL, TimeTicks::Now(), make_linked_ptr(s), false, + ConfigurationParams(), FROM_HERE); ScheduleSyncSessionJob(job); diff --git a/sync/engine/sync_scheduler.h b/sync/engine/sync_scheduler.h index d32ee25..8ebd2aa 100644 --- a/sync/engine/sync_scheduler.h +++ b/sync/engine/sync_scheduler.h @@ -37,6 +37,32 @@ namespace browser_sync { struct ServerConnectionEvent; +struct ConfigurationParams { + enum KeystoreKeyStatus { + KEYSTORE_KEY_UNNECESSARY, + KEYSTORE_KEY_NEEDED + }; + ConfigurationParams(); + ConfigurationParams( + const sync_pb::GetUpdatesCallerInfo::GetUpdatesSource& source, + const syncable::ModelTypeSet& types_to_download, + const browser_sync::ModelSafeRoutingInfo& routing_info, + KeystoreKeyStatus keystore_key_status, + const base::Closure& ready_task); + ~ConfigurationParams(); + + // Source for the configuration. + sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source; + // The types that should be downloaded. + syncable::ModelTypeSet types_to_download; + // The new routing info (superset of types to be downloaded). + ModelSafeRoutingInfo routing_info; + // Whether we need to perform a GetKey command. + KeystoreKeyStatus keystore_key_status; + // Callback to invoke on configuration completion. + base::Closure ready_task; +}; + class SyncScheduler : public sessions::SyncSession::Delegate { public: enum Mode { @@ -63,10 +89,15 @@ class SyncScheduler : public sessions::SyncSession::Delegate { // Start the scheduler with the given mode. If the scheduler is // already started, switch to the given mode, although some - // scheduled tasks from the old mode may still run. If non-NULL, - // |callback| will be invoked when the mode has been changed to - // |mode|. Takes ownership of |callback|. - void Start(Mode mode, const base::Closure& callback); + // scheduled tasks from the old mode may still run. + virtual void Start(Mode mode); + + // Schedules the configuration task specified by |params|. Returns true if + // the configuration task executed immediately, false if it had to be + // scheduled for a later attempt. |params.ready_task| is invoked whenever the + // configuration task executes. + // Note: must already be in CONFIGURATION mode. + virtual bool ScheduleConfiguration(const ConfigurationParams& params); // Request that any running syncer task stop as soon as possible and // cancel all scheduled tasks. This function can be called from any thread, @@ -86,14 +117,6 @@ class SyncScheduler : public sessions::SyncSession::Delegate { const syncable::ModelTypePayloadMap& types_with_payloads, const tracked_objects::Location& nudge_location); - // Schedule a configuration cycle. May execute immediately or at a later time - // (depending on backoff/throttle state). - // Note: The source argument of this function must come from the subset of - // GetUpdatesCallerInfo values related to configurations. - void ScheduleConfiguration( - syncable::ModelTypeSet types, - sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source); - // TODO(tim): remove this. crbug.com/131336 void ClearUserData(); @@ -164,6 +187,7 @@ class SyncScheduler : public sessions::SyncSession::Delegate { SyncSessionJob(); SyncSessionJob(SyncSessionJobPurpose purpose, base::TimeTicks start, linked_ptr<sessions::SyncSession> session, bool is_canary_job, + ConfigurationParams config_params, const tracked_objects::Location& nudge_location); ~SyncSessionJob(); static const char* GetPurposeString(SyncSessionJobPurpose purpose); @@ -172,6 +196,7 @@ class SyncScheduler : public sessions::SyncSession::Delegate { base::TimeTicks scheduled_start; linked_ptr<sessions::SyncSession> session; bool is_canary_job; + ConfigurationParams config_params; // This is the location the job came from. Used for debugging. // In case of multiple nudges getting coalesced this stores the diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc index a6d8059..7aa30ab 100644 --- a/sync/engine/sync_scheduler_unittest.cc +++ b/sync/engine/sync_scheduler_unittest.cc @@ -12,6 +12,7 @@ #include "sync/engine/syncer.h" #include "sync/engine/throttled_data_type_tracker.h" #include "sync/sessions/test_util.h" +#include "sync/test/callback_counter.h" #include "sync/test/engine/fake_model_worker.h" #include "sync/test/engine/mock_connection_manager.h" #include "sync/test/engine/test_directory_setter_upper.h" @@ -27,6 +28,7 @@ using testing::DoAll; using testing::Eq; using testing::Invoke; using testing::Mock; +using testing::Not; using testing::Return; using testing::WithArg; @@ -69,6 +71,14 @@ void PumpLoop() { RunLoop(); } +ModelSafeRoutingInfo TypesToRoutingInfo(const ModelTypeSet& types) { + ModelSafeRoutingInfo routes; + for (ModelTypeSet::Iterator iter = types.First(); iter.Good(); iter.Inc()) { + routes[iter.Get()] = GROUP_PASSIVE; + } + return routes; +} + // Convenient to use in tests wishing to analyze SyncShare calls over time. static const size_t kMinNumSamples = 5; class SyncSchedulerTest : public testing::Test { @@ -152,7 +162,7 @@ class SyncSchedulerTest : public testing::Test { } void StartSyncScheduler(SyncScheduler::Mode mode) { - scheduler()->Start(mode, base::Closure()); + scheduler()->Start(mode); } // This stops the scheduler synchronously. @@ -291,14 +301,23 @@ TEST_F(SyncSchedulerTest, Config) { SyncShareRecords records; const ModelTypeSet model_types(syncable::BOOKMARKS); - EXPECT_CALL(*syncer(), SyncShare(_,_,_)) + EXPECT_CALL(*syncer(), + SyncShare(_,_,_)) + .WillOnce(Invoke(sessions::test_util::SimulateSuccess)) .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess), WithArg<0>(RecordSyncShare(&records)))); StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); - scheduler()->ScheduleConfiguration( - model_types, GetUpdatesCallerInfo::RECONFIGURATION); + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + model_types, + TypesToRoutingInfo(model_types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_TRUE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(1, counter.times_called()); ASSERT_EQ(1U, records.snapshots.size()); EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types, @@ -315,7 +334,9 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) { SyncShareRecords records; const ModelTypeSet model_types(syncable::BOOKMARKS); - EXPECT_CALL(*syncer(), SyncShare(_,_,_)) + EXPECT_CALL(*syncer(), + SyncShare(_,_,_)) + .WillOnce(Invoke(sessions::test_util::SimulateSuccess)) .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed), WithArg<0>(RecordSyncShare(&records)))) .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess), @@ -324,61 +345,27 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) { StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); ASSERT_EQ(0U, records.snapshots.size()); - scheduler()->ScheduleConfiguration( - model_types, GetUpdatesCallerInfo::RECONFIGURATION); + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + model_types, + TypesToRoutingInfo(model_types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_FALSE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(0, counter.times_called()); ASSERT_EQ(1U, records.snapshots.size()); RunLoop(); ASSERT_EQ(2U, records.snapshots.size()); + ASSERT_EQ(1, counter.times_called()); EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types, records.snapshots[1].source().types)); EXPECT_EQ(GetUpdatesCallerInfo::RECONFIGURATION, records.snapshots[1].source().updates_source); } -// Issue 2 config commands. Second one right after the first has failed -// and make sure LATEST is executed. -TEST_F(SyncSchedulerTest, MultipleConfigWithBackingOff) { - const ModelTypeSet - model_types1(syncable::BOOKMARKS), - model_types2(syncable::AUTOFILL); - UseMockDelayProvider(); - EXPECT_CALL(*delay(), GetDelay(_)) - .WillRepeatedly(Return(TimeDelta::FromMilliseconds(30))); - SyncShareRecords records; - - EXPECT_CALL(*syncer(), SyncShare(_,_,_)) - .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed), - WithArg<0>(RecordSyncShare(&records)))) - .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed), - WithArg<0>(RecordSyncShare(&records)))) - .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess), - WithArg<0>(RecordSyncShare(&records)))); - - StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); - - ASSERT_EQ(0U, records.snapshots.size()); - scheduler()->ScheduleConfiguration( - model_types1, GetUpdatesCallerInfo::RECONFIGURATION); - - ASSERT_EQ(1U, records.snapshots.size()); - scheduler()->ScheduleConfiguration( - model_types2, GetUpdatesCallerInfo::RECONFIGURATION); - - // A canary job gets posted when we go into exponential backoff. - RunLoop(); - - ASSERT_EQ(2U, records.snapshots.size()); - RunLoop(); - - ASSERT_EQ(3U, records.snapshots.size()); - EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types2, - records.snapshots[2].source().types)); - EXPECT_EQ(GetUpdatesCallerInfo::RECONFIGURATION, - records.snapshots[2].source().updates_source); -} - // Issue a nudge when the config has failed. Make sure both the config and // nudge are executed. TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) { @@ -401,19 +388,28 @@ TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) { StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); ASSERT_EQ(0U, records.snapshots.size()); - scheduler()->ScheduleConfiguration( - model_types, GetUpdatesCallerInfo::RECONFIGURATION); - + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + model_types, + TypesToRoutingInfo(model_types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_FALSE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(0, counter.times_called()); ASSERT_EQ(1U, records.snapshots.size()); + scheduler()->ScheduleNudgeAsync( zero(), NUDGE_SOURCE_LOCAL, model_types, FROM_HERE); RunLoop(); - ASSERT_EQ(2U, records.snapshots.size()); + ASSERT_EQ(0, counter.times_called()); + RunLoop(); + ASSERT_EQ(3U, records.snapshots.size()); + ASSERT_EQ(1, counter.times_called()); // Now change the mode so nudge can execute. - ASSERT_EQ(3U, records.snapshots.size()); StartSyncScheduler(SyncScheduler::NORMAL_MODE); ASSERT_EQ(4U, records.snapshots.size()); @@ -718,13 +714,19 @@ TEST_F(SyncSchedulerTest, HasMoreToSyncThenFails) { EXPECT_TRUE(RunAndGetBackoff()); } -// Test that no syncing occurs when throttled. +// Test that no syncing occurs when throttled (although CleanupDisabledTypes +// is allowed). TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) { const ModelTypeSet types(syncable::BOOKMARKS); TimeDelta poll(TimeDelta::FromMilliseconds(5)); TimeDelta throttle(TimeDelta::FromMinutes(10)); scheduler()->OnReceivedLongPollIntervalUpdate(poll); - EXPECT_CALL(*syncer(), SyncShare(_,_,_)) + + EXPECT_CALL(*syncer(), + SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)) + .WillOnce(Invoke(sessions::test_util::SimulateSuccess)); + EXPECT_CALL(*syncer(), SyncShare(_,Not(CLEANUP_DISABLED_TYPES), + Not(CLEANUP_DISABLED_TYPES))) .WillOnce(WithArg<0>(sessions::test_util::SimulateThrottled(throttle))) .WillRepeatedly(AddFailureAndQuitLoopNow()); @@ -736,8 +738,15 @@ TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) { StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); - scheduler()->ScheduleConfiguration( - types, GetUpdatesCallerInfo::RECONFIGURATION); + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + types, + TypesToRoutingInfo(types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_FALSE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(0, counter.times_called()); } TEST_F(SyncSchedulerTest, ThrottlingExpires) { @@ -770,6 +779,7 @@ TEST_F(SyncSchedulerTest, ConfigurationMode) { SyncShareRecords records; scheduler()->OnReceivedLongPollIntervalUpdate(poll); EXPECT_CALL(*syncer(), SyncShare(_,_,_)) + .WillOnce(Invoke(sessions::test_util::SimulateSuccess)) .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess), WithArg<0>(RecordSyncShare(&records)))); @@ -783,8 +793,15 @@ TEST_F(SyncSchedulerTest, ConfigurationMode) { const ModelTypeSet config_types(syncable::BOOKMARKS); - scheduler()->ScheduleConfiguration( - config_types, GetUpdatesCallerInfo::RECONFIGURATION); + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + config_types, + TypesToRoutingInfo(config_types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_TRUE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(1, counter.times_called()); ASSERT_EQ(1U, records.snapshots.size()); EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(config_types, @@ -853,7 +870,7 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) { scheduler()->OnReceivedLongPollIntervalUpdate(poll); UseMockDelayProvider(); - EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(1) + EXPECT_CALL(*syncer(), SyncShare(_,_,_)) .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulateCommitFailed), RecordSyncShareMultiple(&r, 1U))); EXPECT_CALL(*delay(), GetDelay(_)). @@ -895,9 +912,15 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) { StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); - scheduler()->CleanupDisabledTypes(); - scheduler()->ScheduleConfiguration( - types, GetUpdatesCallerInfo::RECONFIGURATION); + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + types, + TypesToRoutingInfo(types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_FALSE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(0, counter.times_called()); StartSyncScheduler(SyncScheduler::NORMAL_MODE); @@ -1066,28 +1089,49 @@ TEST_F(SyncSchedulerTest, SyncerSteps) { StopSyncScheduler(); Mock::VerifyAndClearExpectations(syncer()); - // Configuration. + // Configuration (always includes a cleanup disabled types). + EXPECT_CALL(*syncer(), + SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)) + .WillOnce(Invoke(sessions::test_util::SimulateSuccess)); EXPECT_CALL(*syncer(), SyncShare(_, DOWNLOAD_UPDATES, APPLY_UPDATES)) .WillOnce(Invoke(sessions::test_util::SimulateSuccess)); StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); - scheduler()->ScheduleConfiguration( - ModelTypeSet(), GetUpdatesCallerInfo::RECONFIGURATION); - + syncable::ModelTypeSet model_types(syncable::BOOKMARKS); + CallbackCounter counter; + ConfigurationParams params( + GetUpdatesCallerInfo::RECONFIGURATION, + model_types, + TypesToRoutingInfo(model_types), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter))); + ASSERT_TRUE(scheduler()->ScheduleConfiguration(params)); + ASSERT_EQ(1, counter.times_called()); + // Runs directly so no need to pump the loop. StopSyncScheduler(); Mock::VerifyAndClearExpectations(syncer()); - // Cleanup disabled types. + // Cleanup disabled types. Because no types are being configured, we just + // perform the cleanup. EXPECT_CALL(*syncer(), - SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)) - .WillOnce(Invoke(sessions::test_util::SimulateSuccess)); - StartSyncScheduler(SyncScheduler::NORMAL_MODE); - - scheduler()->CleanupDisabledTypes(); + SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)). + WillOnce(Invoke(sessions::test_util::SimulateSuccess)); + StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE); + CallbackCounter counter2; + ConfigurationParams params2( + GetUpdatesCallerInfo::RECONFIGURATION, + ModelTypeSet(), + ModelSafeRoutingInfo(), + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY, + base::Bind(&CallbackCounter::Callback, base::Unretained(&counter2))); + ASSERT_TRUE(scheduler()->ScheduleConfiguration(params2)); + ASSERT_EQ(1, counter2.times_called()); StopSyncScheduler(); Mock::VerifyAndClearExpectations(syncer()); + StartSyncScheduler(SyncScheduler::NORMAL_MODE); + // Poll. EXPECT_CALL(*syncer(), SyncShare(_, SYNCER_BEGIN, SYNCER_END)) .Times(AtLeast(1)) diff --git a/sync/engine/sync_scheduler_whitebox_unittest.cc b/sync/engine/sync_scheduler_whitebox_unittest.cc index 34ff7d9..390a6cd 100644 --- a/sync/engine/sync_scheduler_whitebox_unittest.cc +++ b/sync/engine/sync_scheduler_whitebox_unittest.cc @@ -107,6 +107,7 @@ class SyncSchedulerWhiteboxTest : public testing::Test { SyncScheduler::SyncSessionJob job(purpose, TimeTicks::Now(), make_linked_ptr(s), false, + ConfigurationParams(), FROM_HERE); return DecideOnJob(job); } @@ -160,6 +161,7 @@ TEST_F(SyncSchedulerWhiteboxTest, SaveNudgeWhileTypeThrottled) { TimeTicks::Now(), make_linked_ptr(s), false, + ConfigurationParams(), FROM_HERE); SyncScheduler::JobProcessDecision decision = DecideOnJob(job); diff --git a/sync/internal_api/sync_manager.cc b/sync/internal_api/sync_manager.cc index 8a2ff70..4cb5127 100644 --- a/sync/internal_api/sync_manager.cc +++ b/sync/internal_api/sync_manager.cc @@ -59,6 +59,7 @@ using base::TimeDelta; using browser_sync::AllStatus; +using browser_sync::ConfigurationParams; using browser_sync::Cryptographer; using browser_sync::Encryptor; using browser_sync::JsArgList; @@ -397,6 +398,8 @@ class SyncManager::SyncInternal const std::string& name, const JsArgList& args, const WeakHandle<JsReplyHandler>& reply_handler) OVERRIDE; + void SetSyncSchedulerForTest(scoped_ptr<SyncScheduler> scheduler); + private: struct NotificationInfo { int total_count; @@ -816,46 +819,44 @@ bool SyncManager::IsUsingExplicitPassphrase() { return data_ && data_->IsUsingExplicitPassphrase(); } -void SyncManager::RequestCleanupDisabledTypes( - const browser_sync::ModelSafeRoutingInfo& routing_info) { - DCHECK(thread_checker_.CalledOnValidThread()); - if (data_->scheduler()) { - data_->session_context()->set_routing_info(routing_info); - data_->scheduler()->CleanupDisabledTypes(); - } -} - void SyncManager::RequestClearServerData() { DCHECK(thread_checker_.CalledOnValidThread()); if (data_->scheduler()) data_->scheduler()->ClearUserData(); } -void SyncManager::RequestConfig( - const browser_sync::ModelSafeRoutingInfo& routing_info, - const ModelTypeSet& types, ConfigureReason reason) { +void SyncManager::ConfigureSyncer( + ConfigureReason reason, + const syncable::ModelTypeSet& types_to_config, + const browser_sync::ModelSafeRoutingInfo& new_routing_info, + const base::Closure& ready_task, + const base::Closure& retry_task) { DCHECK(thread_checker_.CalledOnValidThread()); - if (!data_->scheduler()) { - LOG(INFO) - << "SyncManager::RequestConfig: bailing out because scheduler is " - << "null"; - return; - } - StartConfigurationMode(base::Closure()); - data_->session_context()->set_routing_info(routing_info); - data_->scheduler()->ScheduleConfiguration(types, GetSourceFromReason(reason)); -} + DCHECK(!ready_task.is_null()); + DCHECK(!retry_task.is_null()); + + // TODO(zea): set this based on whether cryptographer has keystore + // encryption key or not (requires opening a transaction). crbug.com/129665. + ConfigurationParams::KeystoreKeyStatus keystore_key_status = + ConfigurationParams::KEYSTORE_KEY_UNNECESSARY; + + ConfigurationParams params(GetSourceFromReason(reason), + types_to_config, + new_routing_info, + keystore_key_status, + ready_task); -void SyncManager::StartConfigurationMode(const base::Closure& callback) { - DCHECK(thread_checker_.CalledOnValidThread()); if (!data_->scheduler()) { LOG(INFO) - << "SyncManager::StartConfigurationMode: could not start " - << "configuration mode because because scheduler is null"; + << "SyncManager::ConfigureSyncer: could not configure because " + << "scheduler is null"; + params.ready_task.Run(); return; } - data_->scheduler()->Start( - browser_sync::SyncScheduler::CONFIGURATION_MODE, callback); + + data_->scheduler()->Start(SyncScheduler::CONFIGURATION_MODE); + if (!data_->scheduler()->ScheduleConfiguration(params)) + retry_task.Run(); } bool SyncManager::SyncInternal::Init( @@ -941,8 +942,7 @@ bool SyncManager::SyncInternal::Init( if (signed_in) { if (scheduler()) { - scheduler()->Start( - browser_sync::SyncScheduler::CONFIGURATION_MODE, base::Closure()); + scheduler()->Start(SyncScheduler::CONFIGURATION_MODE); } initialized_ = true; @@ -1116,9 +1116,13 @@ void SyncManager::SyncInternal::NotifyCryptographerState( void SyncManager::SyncInternal::StartSyncingNormally( const browser_sync::ModelSafeRoutingInfo& routing_info) { // Start the sync scheduler. - if (scheduler()) { // NULL during certain unittests. + if (scheduler()) { // NULL during certain unittests. + // TODO(sync): We always want the newest set of routes when we switch back + // to normal mode. Figure out how to enforce set_routing_info is always + // appropriately set and that it's only modified when switching to normal + // mode. session_context()->set_routing_info(routing_info); - scheduler()->Start(SyncScheduler::NORMAL_MODE, base::Closure()); + scheduler()->Start(SyncScheduler::NORMAL_MODE); } } @@ -2353,6 +2357,11 @@ void SyncManager::SyncInternal::RemoveObserver( observers_.RemoveObserver(observer); } +void SyncManager::SyncInternal::SetSyncSchedulerForTest( + scoped_ptr<SyncScheduler> sync_scheduler) { + scheduler_ = sync_scheduler.Pass(); +} + SyncStatus SyncManager::GetDetailedStatus() const { return data_->GetStatus(); } @@ -2383,6 +2392,10 @@ TimeDelta SyncManager::GetNudgeDelayTimeDelta( return data_->GetNudgeDelayTimeDelta(model_type); } +void SyncManager::SetSyncSchedulerForTest(scoped_ptr<SyncScheduler> scheduler) { + data_->SetSyncSchedulerForTest(scheduler.Pass()); +} + syncable::ModelTypeSet SyncManager::GetEncryptedDataTypesForTest() const { ReadTransaction trans(FROM_HERE, GetUserShare()); return GetEncryptedTypes(&trans); diff --git a/sync/internal_api/sync_manager.h b/sync/internal_api/sync_manager.h index 04594fd..04181fe 100644 --- a/sync/internal_api/sync_manager.h +++ b/sync/internal_api/sync_manager.h @@ -26,11 +26,13 @@ #include "sync/util/weak_handle.h" namespace browser_sync { +struct ConfigurationParams; class Encryptor; struct Experiments; class ExtensionsActivityMonitor; class JsBackend; class JsEventHandler; +class SyncScheduler; namespace sessions { class SyncSessionSnapshot; @@ -431,24 +433,24 @@ class SyncManager { // error to call this when we don't have pending keys. void SetDecryptionPassphrase(const std::string& passphrase); - // Puts the SyncScheduler into a mode where no normal nudge or poll traffic - // will occur, but calls to RequestConfig will be supported. If |callback| - // is provided, it will be invoked (from the internal SyncScheduler) when - // the thread has changed to configuration mode. - void StartConfigurationMode(const base::Closure& callback); - - // Switches the mode of operation to CONFIGURATION_MODE and - // schedules a config task to fetch updates for |types|. - void RequestConfig(const browser_sync::ModelSafeRoutingInfo& routing_info, - const syncable::ModelTypeSet& types, - sync_api::ConfigureReason reason); - - void RequestCleanupDisabledTypes( - const browser_sync::ModelSafeRoutingInfo& routing_info); - // Request a clearing of all data on the server void RequestClearServerData(); + // Switches the mode of operation to CONFIGURATION_MODE and performs + // any configuration tasks needed as determined by the params. Once complete, + // syncer will remain in CONFIGURATION_MODE until StartSyncingNormally is + // called. + // |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. + void ConfigureSyncer( + ConfigureReason reason, + const syncable::ModelTypeSet& types_to_config, + const browser_sync::ModelSafeRoutingInfo& new_routing_info, + const base::Closure& ready_task, + const base::Closure& retry_task); + // Adds a listener to be notified of sync events. // NOTE: It is OK (in fact, it's probably a good idea) to call this before // having received OnInitializationCompleted. @@ -543,11 +545,17 @@ class SyncManager { static const FilePath::CharType kSyncDatabaseFilename[]; private: + friend class SyncManagerTest; FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, NudgeDelayTest); // For unit tests. base::TimeDelta GetNudgeDelayTimeDelta(const syncable::ModelType& model_type); + // Set the internal scheduler for testing purposes. + // TODO(sync): Use dependency injection instead. crbug.com/133061 + void SetSyncSchedulerForTest( + scoped_ptr<browser_sync::SyncScheduler> scheduler); + base::ThreadChecker thread_checker_; // An opaque pointer to the nested private class. diff --git a/sync/internal_api/syncapi_unittest.cc b/sync/internal_api/syncapi_unittest.cc index 991aed6..6380db3 100644 --- a/sync/internal_api/syncapi_unittest.cc +++ b/sync/internal_api/syncapi_unittest.cc @@ -24,6 +24,7 @@ #include "base/utf_string_conversions.h" #include "base/values.h" #include "sync/engine/nigori_util.h" +#include "sync/engine/sync_scheduler.h" #include "sync/internal_api/change_record.h" #include "sync/internal_api/http_post_provider_factory.h" #include "sync/internal_api/http_post_provider_interface.h" @@ -54,6 +55,7 @@ #include "sync/sessions/sync_session.h" #include "sync/syncable/syncable.h" #include "sync/syncable/syncable_id.h" +#include "sync/test/callback_counter.h" #include "sync/test/fake_encryptor.h" #include "sync/test/fake_extensions_activity_monitor.h" #include "sync/util/cryptographer.h" @@ -64,6 +66,8 @@ #include "testing/gtest/include/gtest/gtest.h" using base::ExpectDictStringValue; +using browser_sync::CallbackCounter; +using browser_sync::ConfigurationParams; using browser_sync::Cryptographer; using browser_sync::FakeEncryptor; using browser_sync::FakeExtensionsActivityMonitor; @@ -80,6 +84,7 @@ using browser_sync::MockJsReplyHandler; using browser_sync::ModelSafeRoutingInfo; using browser_sync::ModelSafeWorker; using browser_sync::sessions::SyncSessionSnapshot; +using browser_sync::SyncScheduler; using browser_sync::TestUnrecoverableErrorHandler; using browser_sync::WeakHandle; using syncable::IS_DEL; @@ -92,8 +97,10 @@ using syncable::SPECIFICS; using testing::_; using testing::AnyNumber; using testing::AtLeast; +using testing::DoAll; using testing::InSequence; using testing::Invoke; +using testing::Return; using testing::SaveArg; using testing::StrictMock; @@ -926,6 +933,10 @@ class SyncManagerTest : public testing::Test, return true; } + void SetScheduler(scoped_ptr<SyncScheduler> scheduler) { + sync_manager_.SetSyncSchedulerForTest(scheduler.Pass()); + } + private: // Needed by |sync_manager_|. MessageLoop message_loop_; @@ -2507,4 +2518,82 @@ TEST_F(SyncManagerTest, SetPreviouslyEncryptedSpecifics) { } } +namespace { + +class MockSyncScheduler : public SyncScheduler { + public: + MockSyncScheduler() : SyncScheduler("name", NULL, NULL) {} + virtual ~MockSyncScheduler() {} + + MOCK_METHOD1(Start, void(SyncScheduler::Mode)); + MOCK_METHOD1(ScheduleConfiguration, bool(const ConfigurationParams&)); +}; + +} // namespace + +// Test that the configuration params are properly created and sent to +// ScheduleConfigure. No callback should be invoked. +TEST_F(SyncManagerTest, BasicConfiguration) { + ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION; + syncable::ModelTypeSet types_to_download(syncable::BOOKMARKS, + syncable::PREFERENCES); + browser_sync::ModelSafeRoutingInfo new_routing_info; + GetModelSafeRoutingInfo(&new_routing_info); + + scoped_ptr<MockSyncScheduler> scheduler(new MockSyncScheduler()); + ConfigurationParams params; + EXPECT_CALL(*scheduler, Start(SyncScheduler::CONFIGURATION_MODE)); + EXPECT_CALL(*scheduler, ScheduleConfiguration(_)). + WillOnce(DoAll(SaveArg<0>(¶ms), Return(true))); + SetScheduler(scheduler.PassAs<SyncScheduler>()); + + CallbackCounter ready_task_counter, retry_task_counter; + sync_manager_.ConfigureSyncer( + reason, + types_to_download, + new_routing_info, + base::Bind(&CallbackCounter::Callback, + base::Unretained(&ready_task_counter)), + base::Bind(&CallbackCounter::Callback, + base::Unretained(&retry_task_counter))); + EXPECT_EQ(0, ready_task_counter.times_called()); + EXPECT_EQ(0, retry_task_counter.times_called()); + EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION, + params.source); + EXPECT_TRUE(types_to_download.Equals(params.types_to_download)); + EXPECT_EQ(new_routing_info, params.routing_info); +} + +// Test that the retry callback is invoked on configuration failure. +TEST_F(SyncManagerTest, ConfigurationRetry) { + ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION; + syncable::ModelTypeSet types_to_download(syncable::BOOKMARKS, + syncable::PREFERENCES); + browser_sync::ModelSafeRoutingInfo new_routing_info; + GetModelSafeRoutingInfo(&new_routing_info); + + scoped_ptr<MockSyncScheduler> scheduler(new MockSyncScheduler()); + ConfigurationParams params; + EXPECT_CALL(*scheduler, Start(SyncScheduler::CONFIGURATION_MODE)); + EXPECT_CALL(*scheduler, ScheduleConfiguration(_)). + WillOnce(DoAll(SaveArg<0>(¶ms), Return(false))); + SetScheduler(scheduler.PassAs<SyncScheduler>()); + + CallbackCounter ready_task_counter, retry_task_counter; + sync_manager_.ConfigureSyncer( + reason, + types_to_download, + new_routing_info, + base::Bind(&CallbackCounter::Callback, + base::Unretained(&ready_task_counter)), + base::Bind(&CallbackCounter::Callback, + base::Unretained(&retry_task_counter))); + EXPECT_EQ(0, ready_task_counter.times_called()); + EXPECT_EQ(1, retry_task_counter.times_called()); + EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION, + params.source); + EXPECT_TRUE(types_to_download.Equals(params.types_to_download)); + EXPECT_EQ(new_routing_info, params.routing_info); +} + } // namespace browser_sync diff --git a/sync/sync.gyp b/sync/sync.gyp index 73e103c..5372b9d 100644 --- a/sync/sync.gyp +++ b/sync/sync.gyp @@ -375,6 +375,7 @@ 'sessions/test_util.h', 'syncable/syncable_mock.cc', 'syncable/syncable_mock.h', + 'test/callback_counter.h', 'test/engine/fake_model_worker.cc', 'test/engine/fake_model_worker.h', 'test/engine/mock_connection_manager.cc', diff --git a/sync/test/callback_counter.h b/sync/test/callback_counter.h new file mode 100644 index 0000000..71586ec --- /dev/null +++ b/sync/test/callback_counter.h @@ -0,0 +1,29 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef SYNC_TEST_CALLBACK_COUNTER_H_ +#define SYNC_TEST_CALLBACK_COUNTER_H_ +#pragma once + +namespace browser_sync { + +// Helper class to track how many times a callback is triggered. +class CallbackCounter { + public: + CallbackCounter() { Reset(); } + ~CallbackCounter() {} + + void Reset() { times_called_ = 0; } + void Callback() { ++times_called_; } + int times_called() const { return times_called_; } + + private: + int times_called_; + + DISALLOW_COPY_AND_ASSIGN(CallbackCounter); +}; + +} // namespace browser_sync + +#endif // SYNC_TEST_CALLBACK_COUNTER_H_ |