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

sync/internal_api/sync_manager.cc

Index: sync/internal_api/sync_manager.cc
diff --git a/sync/internal_api/sync_manager.cc b/sync/internal_api/sync_manager.cc
index c8a293ea85f3ffc7e861d42891072d6535ca265d..e8ba9dd45dcadeb6335191a7f8cb8a61723361e8 100644
--- a/sync/internal_api/sync_manager.cc
+++ b/sync/internal_api/sync_manager.cc
@@ -197,6 +197,11 @@ class SyncManager::SyncInternal
   // went wrong.
   bool SignIn(const SyncCredentials& credentials);
 
+  // Purge from the directory those types with non-empty progress markers
+  // but without initial synced ended set.
+  // Returns false if an error occurred, true otherwise.
+  bool PurgePartiallySyncedTypes();
+
   // Update tokens that we're using in Sync. Email must stay the same.
   void UpdateCredentials(const SyncCredentials& credentials);
 
@@ -357,6 +362,21 @@ class SyncManager::SyncInternal
   // Called only by our NetworkChangeNotifier.
   virtual void OnIPAddressChanged() OVERRIDE;
 
+  ModelTypeSet GetTypesWithEmptyProgressMarkerToken(ModelTypeSet types) {
+    DCHECK(initialized_);
+    syncer::ModelTypeSet result;
+    for (syncer::ModelTypeSet::Iterator i = types.First();
+         i.Good(); i.Inc()) {
+      sync_pb::DataTypeProgressMarker marker;
+      directory()->GetDownloadProgress(i.Get(), &marker);
+
+      if (marker.token().empty())
+        result.Put(i.Get());
+
+    }
+    return result;
+  }
+
   syncer::ModelTypeSet InitialSyncEndedTypes() {
     DCHECK(initialized_);
     return directory()->initial_sync_ended_types();
@@ -376,6 +396,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;
@@ -749,6 +771,15 @@ syncer::ModelTypeSet SyncManager::InitialSyncEndedTypes() {
   return data_->InitialSyncEndedTypes();
 }
 
+syncer::ModelTypeSet SyncManager::GetTypesWithEmptyProgressMarkerToken(
+    syncer::ModelTypeSet types) {
+  return data_->GetTypesWithEmptyProgressMarkerToken(types);
+}
+
+bool SyncManager::PurgePartiallySyncedTypes() {
+  return data_->PurgePartiallySyncedTypes();
+}
+
 void SyncManager::StartSyncingNormally(
     const syncer::ModelSafeRoutingInfo& routing_info) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -793,40 +824,39 @@ bool SyncManager::IsUsingExplicitPassphrase() {
   return data_ && data_->IsUsingExplicitPassphrase();
 }
 
-void SyncManager::RequestCleanupDisabledTypes(
-    const syncer::ModelSafeRoutingInfo& routing_info) {
+void SyncManager::ConfigureSyncer(
+    ConfigureReason reason,
+    const syncer::ModelTypeSet& types_to_config,
+    const syncer::ModelSafeRoutingInfo& new_routing_info,
+    const base::Closure& ready_task,
+    const base::Closure& retry_task) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (data_->scheduler()) {
-    data_->session_context()->set_routing_info(routing_info);
-    data_->scheduler()->CleanupDisabledTypes();
-  }
-}
+  DCHECK(!ready_task.is_null());
+  DCHECK(!retry_task.is_null());
 
-void SyncManager::RequestConfig(
-    const syncer::ModelSafeRoutingInfo& routing_info,
-    const ModelTypeSet& types, ConfigureReason reason) {
-  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));
-}
+  // 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(
-      syncer::SyncScheduler::CONFIGURATION_MODE, callback);
+
+  data_->scheduler()->Start(syncer::SyncScheduler::CONFIGURATION_MODE);
+  if (!data_->scheduler()->ScheduleConfiguration(params))
+    retry_task.Run();
+
 }
 
 bool SyncManager::SyncInternal::Init(
@@ -923,16 +953,29 @@ bool SyncManager::SyncInternal::Init(
     scheduler_.reset(new SyncScheduler(name_, session_context(), new Syncer()));
   }
 
-  bool signed_in = SignIn(credentials);
+  bool success = SignIn(credentials);
 
-  if (signed_in) {
+  if (success) {
     if (scheduler()) {
-      scheduler()->Start(
-          syncer::SyncScheduler::CONFIGURATION_MODE, base::Closure());
+      scheduler()->Start(syncer::SyncScheduler::CONFIGURATION_MODE);
     }
 
     initialized_ = true;
 
+    // Unapplied datatypes (those that do not have initial sync ended set) get
+    // re-downloaded during any configuration. But, it's possible for a datatype
+    // to have a progress marker but not have initial sync ended yet, making
+    // it a candidate for migration. This is a problem, as the DataTypeManager
+    // does not support a migration while it's already in the middle of a
+    // configuration. As a result, any partially synced datatype can stall the
+    // DTM, waiting for the configuration to complete, which it never will due
+    // to the migration error. In addition, a partially synced nigori will
+    // trigger the migration logic before the backend is initialized, resulting
+    // in crashes. We therefore detect and purge any partially synced types as
+    // part of initialization.
+    if (!PurgePartiallySyncedTypes())
+      success = false;
+
     // Cryptographer should only be accessed while holding a
     // transaction.  Grabbing the user share for the transaction
     // checks the initialization state, so this must come after
@@ -950,14 +993,14 @@ bool SyncManager::SyncInternal::Init(
   FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                     OnInitializationComplete(
                         MakeWeakHandle(weak_ptr_factory_.GetWeakPtr()),
-                        signed_in));
+                        success));
 
-  if (!signed_in && testing_mode_ == NON_TEST)
+  if (!success && testing_mode_ == NON_TEST)
     return false;
 
   sync_notifier_->AddObserver(this);
 
-  return signed_in;
+  return success;
 }
 
 void SyncManager::SyncInternal::UpdateCryptographerAndNigori(
@@ -1102,9 +1145,13 @@ void SyncManager::SyncInternal::NotifyCryptographerState(
 void SyncManager::SyncInternal::StartSyncingNormally(
     const syncer::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);
   }
 }
 
@@ -1158,6 +1205,20 @@ bool SyncManager::SyncInternal::SignIn(const SyncCredentials& credentials) {
   return true;
 }
 
+bool SyncManager::SyncInternal::PurgePartiallySyncedTypes() {
+  syncer::ModelTypeSet partially_synced_types =
+      syncer::ModelTypeSet::All();
+  partially_synced_types.RemoveAll(InitialSyncEndedTypes());
+  partially_synced_types.RemoveAll(GetTypesWithEmptyProgressMarkerToken(
+      syncer::ModelTypeSet::All()));
+
+  UMA_HISTOGRAM_COUNTS("Sync.PartiallySyncedTypes",
+                       partially_synced_types.Size());
+  if (partially_synced_types.Empty())
+    return true;
+  return directory()->PurgeEntriesWithTypeIn(partially_synced_types);
+}
+
 void SyncManager::SyncInternal::UpdateCredentials(
     const SyncCredentials& credentials) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -2340,6 +2401,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();
 }
@@ -2370,6 +2436,10 @@ TimeDelta SyncManager::GetNudgeDelayTimeDelta(
   return data_->GetNudgeDelayTimeDelta(model_type);
 }
 
+void SyncManager::SetSyncSchedulerForTest(scoped_ptr<SyncScheduler> scheduler) {
+  data_->SetSyncSchedulerForTest(scheduler.Pass());
+}
+
 syncer::ModelTypeSet SyncManager::GetEncryptedDataTypesForTest() const {
   ReadTransaction trans(FROM_HERE, GetUserShare());
   return GetEncryptedTypes(&trans);
@@ -2459,20 +2529,4 @@ bool InitialSyncEndedForTypes(syncer::ModelTypeSet types,
   return true;
 }
 
-syncer::ModelTypeSet GetTypesWithEmptyProgressMarkerToken(
-    syncer::ModelTypeSet types,
-    syncer::UserShare* share) {
-  syncer::ModelTypeSet result;
-  for (syncer::ModelTypeSet::Iterator i = types.First();
-       i.Good(); i.Inc()) {
-    sync_pb::DataTypeProgressMarker marker;
-    share->directory->GetDownloadProgress(i.Get(), &marker);
-
-    if (marker.token().empty())
-      result.Put(i.Get());
-
-  }
-  return result;
-}
-
 }  // namespace syncer