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
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 532f18108d64cb136119e51f3f0206295d7dc8c3..66f72a09284226447de0757390c642fc9ee22ef6 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -352,9 +352,10 @@ std::string MakeUserAgentForSyncApi() {
   return user_agent;
 }
 
-syncer::HttpPostProviderFactory* MakeHttpBridgeFactory(
+scoped_ptr<syncer::HttpPostProviderFactory> MakeHttpBridgeFactory(
     const scoped_refptr<net::URLRequestContextGetter>& getter) {
-  return new syncer::HttpBridgeFactory(getter, MakeUserAgentForSyncApi());
+  return scoped_ptr<syncer::HttpPostProviderFactory>(
+      new syncer::HttpBridgeFactory(getter, MakeUserAgentForSyncApi()));
 }
 
 }  // namespace
@@ -366,10 +367,11 @@ void SyncBackendHost::Initialize(
     syncer::ModelTypeSet initial_types,
     const SyncCredentials& credentials,
     bool delete_sync_data_folder,
+    syncer::SyncManagerFactory* sync_manager_factory,
     syncer::UnrecoverableErrorHandler* unrecoverable_error_handler,
     syncer::ReportUnrecoverableErrorFunction
         report_unrecoverable_error_function) {
-  if (!sync_thread_.Start())
+  if (!StartSyncThread())
     return;
 
   frontend_ = frontend;
@@ -404,6 +406,7 @@ void SyncBackendHost::Initialize(
       credentials,
       &chrome_sync_notification_bridge_,
       &sync_notifier_factory_,
+      sync_manager_factory,
       delete_sync_data_folder,
       sync_prefs_->GetEncryptionBootstrapToken(),
       syncer::SyncManager::NON_TEST,
@@ -598,7 +601,7 @@ void SyncBackendHost::ConfigureDataTypes(
     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
+  // changes that happen while the syncer is reconfiguring, and will only
   // trigger another call to ConfigureDataTypes once the current reconfiguration
   // completes).
   DCHECK_GT(initialization_state_, NOT_INITIALIZED);
@@ -712,6 +715,12 @@ void SyncBackendHost::GetModelSafeRoutingInfo(
   }
 }
 
+bool SyncBackendHost::StartSyncThread() {
+  if (!sync_thread_.IsRunning())
+    return sync_thread_.Start();
+  return true;
+}
+
 void SyncBackendHost::InitCore(const DoInitializeOptions& options) {
   sync_thread_.message_loop()->PostTask(FROM_HERE,
       base::Bind(&SyncBackendHost::Core::DoInitialize, core_.get(), options));
@@ -772,6 +781,7 @@ SyncBackendHost::DoInitializeOptions::DoInitializeOptions(
     const syncer::SyncCredentials& credentials,
     ChromeSyncNotificationBridge* chrome_sync_notification_bridge,
     syncer::SyncNotifierFactory* sync_notifier_factory,
+    syncer::SyncManagerFactory* sync_manager_factory,
     bool delete_sync_data_folder,
     const std::string& restored_key_for_bootstrapping,
     syncer::SyncManager::TestingMode testing_mode,
@@ -789,6 +799,7 @@ SyncBackendHost::DoInitializeOptions::DoInitializeOptions(
       credentials(credentials),
       chrome_sync_notification_bridge(chrome_sync_notification_bridge),
       sync_notifier_factory(sync_notifier_factory),
+      sync_manager_factory(sync_manager_factory),
       delete_sync_data_folder(delete_sync_data_folder),
       restored_key_for_bootstrapping(restored_key_for_bootstrapping),
       testing_mode(testing_mode),
@@ -955,7 +966,7 @@ void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) {
   registrar_ = options.registrar;
   DCHECK(registrar_);
 
-  sync_manager_.reset(new syncer::SyncManager(name_));
+  sync_manager_ = options.sync_manager_factory->CreateSyncManager(name_);
   sync_manager_->AddObserver(this);
   success = sync_manager_->Init(
       sync_data_folder_path_,
@@ -964,15 +975,15 @@ void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) {
       options.service_url.EffectiveIntPort(),
       options.service_url.SchemeIsSecure(),
       BrowserThread::GetBlockingPool(),
-      options.make_http_bridge_factory_fn.Run(),
+      options.make_http_bridge_factory_fn.Run().Pass(),
       options.routing_info,
       options.workers,
       options.extensions_activity_monitor,
       options.registrar /* as SyncManager::ChangeDelegate */,
       options.credentials,
-      new BridgedSyncNotifier(
+      scoped_ptr<syncer::SyncNotifier>(new BridgedSyncNotifier(
           options.chrome_sync_notification_bridge,
-          options.sync_notifier_factory->CreateSyncNotifier()),
+          options.sync_notifier_factory->CreateSyncNotifier())),
       options.restored_key_for_bootstrapping,
       options.testing_mode,
       &encryptor_,
@@ -1143,6 +1154,21 @@ void SyncBackendHost::OnNigoriDownloadRetry() {
   frontend_->OnSyncConfigureRetry();
 }
 
+namespace {
+
+syncer::ModelTypeSet GetPartiallySyncedTypes(
+      syncer::SyncManager* sync_manager) {
+    syncer::ModelTypeSet partially_synced_types =

[rlarocque, 2012/07/09 19:22:58]

This indentation doesn't look right.

[Nicolas Zea, 2012/07/09 19:42:50]

Done.

+        syncer::ModelTypeSet::All();
+    partially_synced_types.RemoveAll(sync_manager->InitialSyncEndedTypes());
+    partially_synced_types.RemoveAll(
+        sync_manager->GetTypesWithEmptyProgressMarkerToken(
+            syncer::ModelTypeSet::All()));
+    return partially_synced_types;
+}
+
+}  // namespace
+
 void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop(
     const syncer::WeakHandle<syncer::JsBackend>& js_backend, bool success) {
   DCHECK_NE(NOT_ATTEMPTED, initialization_state_);
@@ -1164,7 +1190,59 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop(
 
   // Run initialization state machine.
   switch (initialization_state_) {
-    case NOT_INITIALIZED:
+    case NOT_INITIALIZED: {
+      NigoriState nigori_state;
+      syncer::ModelTypeSet partially_synced_types = GetPartiallySyncedTypes(
+          core_->sync_manager());
+
+      // Although it's possible for any type to be in a partially downloaded
+      // state, we only care about nigori as it's the only one we download
+      // before the backend is fully initialized. Other types will be
+      // re-downloaded at configuration time, at which point we can handle
+      // migrations and other events.
+      UMA_HISTOGRAM_COUNTS("Sync.PartiallySyncedTypes",
+                           partially_synced_types.Size());
+
+      if (partially_synced_types.Has(syncer::NIGORI)) {
+        // Configure without nigori to force it to be cleaned (
+        // SyncBackendRegistrar will remove nigori from routing info as part
+        // of the configuration).
+        nigori_state = WITHOUT_NIGORI;
+        initialization_state_ = CLEANING_NIGORI;
+      } else {
+        // Normal configuration. Will do nothing if the nigori is already
+        // downloaded and applied.
+        nigori_state = WITH_NIGORI;
+        initialization_state_ = DOWNLOADING_NIGORI;
+      }
+      ConfigureDataTypes(

[rlarocque, 2012/07/09 19:22:58]

I don't like the nigori_state variable, or the fact that there are two ways to
call ConfigureDataTypes(WITH_NIGORI).  We should talk about refactoring this.

[Nicolas Zea, 2012/07/09 19:42:50]

Can you be more specific about what you dislike? There are two ways because
there are two states, which I think makes sense, although I suppose this way is
a bit more "simple state machine"-y.

[rlarocque, 2012/07/09 20:30:22]

Resolved offline.

The summary is that I wanted to see Configure(WITH_NIGORI) and
Configure(WITHOUT_NIGORI) treated as if they were two separate functions.  In
other words, no using a boolean value to switch between them.

Making that change alone would make things look a bit odd, triggering more
refactoring.

We decided that this version is good enough as is.

+          syncer::CONFIGURE_REASON_NEW_CLIENT,
+          syncer::ModelTypeSet(),
+          syncer::ModelTypeSet(),
+          nigori_state,
+          // Calls back into this function.
+          base::Bind(
+              &SyncBackendHost::
+                  HandleNigoriConfigurationCompletedOnFrontendLoop,
+              weak_ptr_factory_.GetWeakPtr(), js_backend),
+          base::Bind(&SyncBackendHost::OnNigoriDownloadRetry,
+                     weak_ptr_factory_.GetWeakPtr()));
+      break;
+    }
+    case CLEANING_NIGORI: {
+      syncer::ModelTypeSet partially_synced_types = GetPartiallySyncedTypes(
+          core_->sync_manager());
+      if (partially_synced_types.Has(syncer::NIGORI)) {
+        // We apparently failed to clean the nigori somehow. Attempting
+        // to continue could lead to crashes (see bug 133219), so we fail
+        // gracefully instead by triggering an unrecoverable error.
+        LOG(ERROR) << "Failed to cleanup partial nigori.";
+        initialization_state_ = NOT_INITIALIZED;
+        frontend_->OnBackendInitialized(
+            syncer::WeakHandle<syncer::JsBackend>(), false);
+        return;
+      }
+
       initialization_state_ = DOWNLOADING_NIGORI;
       ConfigureDataTypes(
           syncer::CONFIGURE_REASON_NEW_CLIENT,
@@ -1179,6 +1257,7 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop(
           base::Bind(&SyncBackendHost::OnNigoriDownloadRetry,
                      weak_ptr_factory_.GetWeakPtr()));
       break;
+    }
     case DOWNLOADING_NIGORI:
       initialization_state_ = REFRESHING_NIGORI;
       // Triggers OnEncryptedTypesChanged() and OnEncryptionComplete()