Revert 146262 - Revert "Revert 142517 - [Sync] Refactor sync configuration logic."

chrome/browser/sync/glue/sync_backend_host.cc

Index: chrome/browser/sync/glue/sync_backend_host.cc
===================================================================
--- chrome/browser/sync/glue/sync_backend_host.cc	(revision 146499)
+++ chrome/browser/sync/glue/sync_backend_host.cc	(working copy)
@@ -126,6 +126,10 @@
   // initialization and authentication).
   void DoStartSyncing(const syncer::ModelSafeRoutingInfo& routing_info);
 
+  // Called to cleanup disabled types.
+  void DoRequestCleanupDisabledTypes(
+      const syncer::ModelSafeRoutingInfo& routing_info);
+
   // Called to set the passphrase for encryption.
   void DoSetEncryptionPassphrase(const std::string& passphrase,
                                  bool is_explicit);
@@ -155,19 +159,15 @@
   void DoStopSyncManagerForShutdown(const base::Closure& closure);
   void DoShutdown(bool stopping_sync);
 
-  // Configuration methods that must execute on sync loop.
-  void DoConfigureSyncer(
-      syncer::ConfigureReason reason,
+  virtual void DoRequestConfig(
+      const syncer::ModelSafeRoutingInfo& routing_info,
       syncer::ModelTypeSet types_to_config,
-      const syncer::ModelSafeRoutingInfo routing_info,
-      const base::Callback<void(syncer::ModelTypeSet)>& ready_task,
-      const base::Closure& retry_callback);
-  void DoFinishConfigureDataTypes(
-      syncer::ModelTypeSet types_to_config,
-      const base::Callback<void(syncer::ModelTypeSet)>& ready_task);
-  void DoRetryConfiguration(
-      const base::Closure& retry_callback);
+      syncer::ConfigureReason reason);
 
+  // Start the configuration mode.  |callback| is called on the sync
+  // thread.
+  virtual void DoStartConfiguration(const base::Closure& callback);
+
   // Set the base request context to use when making HTTP calls.
   // This method will add a reference to the context to persist it
   // on the IO thread. Must be removed from IO thread.
@@ -179,6 +179,9 @@
   // 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;
@@ -586,8 +589,8 @@
     syncer::ModelTypeSet types_to_add,
     syncer::ModelTypeSet types_to_remove,
     NigoriState nigori_state,
-    const base::Callback<void(syncer::ModelTypeSet)>& ready_task,
-    const base::Callback<void()>& retry_callback) {
+    base::Callback<void(syncer::ModelTypeSet)> ready_task,
+    base::Callback<void()> retry_callback) {
   syncer::ModelTypeSet types_to_add_with_nigori = types_to_add;
   syncer::ModelTypeSet types_to_remove_with_nigori = types_to_remove;
   if (nigori_state == WITH_NIGORI) {
@@ -597,58 +600,46 @@
     types_to_add_with_nigori.Remove(syncer::NIGORI);
     types_to_remove_with_nigori.Put(syncer::NIGORI);
   }
-  // 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).
+  // Only one configure is allowed at a time.
+  DCHECK(!pending_config_mode_state_.get());
+  DCHECK(!pending_download_state_.get());
   DCHECK_GT(initialization_state_, NOT_INITIALIZED);
 
-  // The new set of enabled types is types_to_add_with_nigori + the
-  // previously enabled types (on restart, the preferred types are already
-  // enabled) - types_to_remove_with_nigori. After reconfiguring the registrar,
-  // the new routing info will reflect the set of enabled types.
-  syncer::ModelSafeRoutingInfo routing_info;
-  registrar_->ConfigureDataTypes(types_to_add_with_nigori,
-                                 types_to_remove_with_nigori);
-  registrar_->GetModelSafeRoutingInfo(&routing_info);
-  const syncer::ModelTypeSet enabled_types =
-      GetRoutingInfoTypes(routing_info);
+  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;
 
-  // 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).
-  syncer::ModelTypeSet initial_sync_ended_types =
-      core_->sync_manager()->InitialSyncEndedTypes();
-  initial_sync_ended_types.RetainAll(enabled_types);
-  syncer::ModelTypeSet types_to_config =
-      Difference(enabled_types, initial_sync_ended_types);
-  if (!types_to_config.Empty() && enabled_types.Has(syncer::NIGORI))
-    types_to_config.Put(syncer::NIGORI);
+  // 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()) {
+    syncer::ModelSafeRoutingInfo routing_info;
+    registrar_->GetModelSafeRoutingInfo(&routing_info);
+    sync_thread_.message_loop()->PostTask(
+        FROM_HERE,
+        base::Bind(&SyncBackendHost::Core::DoRequestCleanupDisabledTypes,
+                   core_.get(),
+                   routing_info));
+  }
 
-  SDVLOG(1) << "Types "
-            << syncer::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);
+  StartConfiguration(
+      base::Bind(&SyncBackendHost::Core::FinishConfigureDataTypes,
+                 core_.get()));
 }
 
+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));
+}
+
 void SyncBackendHost::EnableEncryptEverything() {
   sync_thread_.message_loop()->PostTask(FROM_HERE,
      base::Bind(&SyncBackendHost::Core::DoEnableEncryptEverything,
@@ -717,47 +708,154 @@
       base::Bind(&SyncBackendHost::Core::DoInitialize, core_.get(), options));
 }
 
-void SyncBackendHost::RequestConfigureSyncer(
-    syncer::ConfigureReason reason,
-    syncer::ModelTypeSet types_to_config,
-    const syncer::ModelSafeRoutingInfo& routing_info,
-    const base::Callback<void(syncer::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::HandleSyncCycleCompletedOnFrontendLoop(
+    const SyncSessionSnapshot& snapshot) {
+  if (!frontend_)
+    return;
+  DCHECK_EQ(MessageLoop::current(), frontend_loop_);
+
+  last_snapshot_ = snapshot;
+
+  SDVLOG(1) << "Got snapshot " << snapshot.ToString();
+
+  const syncer::ModelTypeSet to_migrate =
+      snapshot.model_neutral_state().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 syncer::ModelTypeSet types_to_add =
+        pending_download_state_->types_to_add;
+    const syncer::ModelTypeSet added_types =
+        pending_download_state_->added_types;
+    DCHECK(types_to_add.HasAll(added_types));
+    const syncer::ModelTypeSet initial_sync_ended =
+        snapshot.initial_sync_ended();
+    const syncer::ModelTypeSet failed_configuration_types =
+        Difference(added_types, initial_sync_ended);
+    SDVLOG(1)
+        << "Added types: "
+        << syncer::ModelTypeSetToString(added_types)
+        << ", configured types: "
+        << syncer::ModelTypeSetToString(initial_sync_ended)
+        << ", failed configuration types: "
+        << syncer::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(
-    syncer::ModelTypeSet types_to_configure,
-    syncer::ModelTypeSet configured_types,
-    const base::Callback<void(syncer::ModelTypeSet)>& ready_task) {
-  const syncer::ModelTypeSet failed_configuration_types =
-      Difference(types_to_configure, configured_types);
-  SDVLOG(1)
-      << "Added types: "
-      << syncer::ModelTypeSetToString(types_to_configure)
-      << ", configured types: "
-      << syncer::ModelTypeSetToString(configured_types)
-      << ", failed configuration types: "
-      << syncer::ModelTypeSetToString(failed_configuration_types);
+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";
+
+  syncer::ModelSafeRoutingInfo routing_info;
+  registrar_->GetModelSafeRoutingInfo(&routing_info);
+  const syncer::ModelTypeSet enabled_types = GetRoutingInfoTypes(routing_info);
+
   // Update |chrome_sync_notification_bridge_|'s enabled types here as it has
   // to happen on the UI thread.
-  chrome_sync_notification_bridge_.UpdateEnabledTypes(configured_types);
+  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 =
+        syncer::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 syncer::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.
+    syncer::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(syncer::NIGORI);
+    }
+    SDVLOG(1) << "Types "
+              << syncer::ModelTypeSetToString(types_to_config)
+              << " added; calling DoRequestConfig";
+    syncer::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();
+
   // Notify SyncManager (especially the notification listener) about new types.
   sync_thread_.message_loop()->PostTask(FROM_HERE,
       base::Bind(&SyncBackendHost::Core::DoUpdateEnabledTypes, core_.get(),
-                 configured_types));
+                 enabled_types));
+}
 
-  if (!ready_task.is_null())
-    ready_task.Run(failed_configuration_types);
+bool SyncBackendHost::IsDownloadingNigoriForTest() const {
+  return initialization_state_ == DOWNLOADING_NIGORI;
 }
 
 SyncBackendHost::DoInitializeOptions::DoInitializeOptions(
@@ -815,6 +913,14 @@
   DCHECK(!sync_loop_);
 }
 
+SyncBackendHost::PendingConfigureDataTypesState::
+PendingConfigureDataTypesState()
+    : reason(syncer::CONFIGURE_REASON_UNKNOWN),
+      retry_in_progress(false) {}
+
+SyncBackendHost::PendingConfigureDataTypesState::
+~PendingConfigureDataTypesState() {}
+
 void SyncBackendHost::Core::OnSyncCycleCompleted(
     const SyncSessionSnapshot& snapshot) {
   if (!sync_loop_)
@@ -1010,6 +1116,12 @@
   sync_manager_->StartSyncingNormally(routing_info);
 }
 
+void SyncBackendHost::Core::DoRequestCleanupDisabledTypes(
+    const syncer::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) {
@@ -1061,48 +1173,20 @@
   host_.Reset();
 }
 
-void SyncBackendHost::Core::DoConfigureSyncer(
-    syncer::ConfigureReason reason,
+void SyncBackendHost::Core::DoRequestConfig(
+    const syncer::ModelSafeRoutingInfo& routing_info,
     syncer::ModelTypeSet types_to_config,
-    const syncer::ModelSafeRoutingInfo routing_info,
-    const base::Callback<void(syncer::ModelTypeSet)>& ready_task,
-    const base::Closure& retry_callback) {
+    syncer::ConfigureReason reason) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  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));
+  sync_manager_->RequestConfig(routing_info, types_to_config, reason);
 }
 
-void SyncBackendHost::Core::DoFinishConfigureDataTypes(
-    syncer::ModelTypeSet types_to_config,
-    const base::Callback<void(syncer::ModelTypeSet)>& ready_task) {
+void SyncBackendHost::Core::DoStartConfiguration(
+    const base::Closure& callback) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  syncer::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);
+  sync_manager_->StartConfigurationMode(callback);
 }
 
-void SyncBackendHost::Core::DoRetryConfiguration(
-    const base::Closure& retry_callback) {
-  DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  host_.Call(FROM_HERE,
-             &SyncBackendHost::RetryConfigurationOnFrontendLoop,
-             retry_callback);
-}
-
 void SyncBackendHost::Core::DeleteSyncDataFolder() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   if (file_util::DirectoryExists(sync_data_folder_path_)) {
@@ -1111,6 +1195,13 @@
   }
 }
 
+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_)
@@ -1162,6 +1253,14 @@
     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:
@@ -1202,36 +1301,6 @@
   }
 }
 
-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 syncer::ModelTypeSet to_migrate =
-      snapshot.model_neutral_state().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());