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..811323b78518951e52a399f3b06864ac85a27d4b 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())

[rlarocque, 2012/07/04 06:39:35]

Since the function is only a few lines long, non-virtual and only used once, I'd
prefer to see it inlined.  

[Nicolas Zea, 2012/07/09 18:40:45]

Chromium style guide generally discourages inlining unless you actually know
it'll be useful (see
http://dev.chromium.org/developers/coding-style#TOC-Inline-functions).

As you mentioned, this method is rarely used, so there's not going to be much
benefit to explicitly inlining it (and it's likely the compiler will inline
anyways).

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

By inline I meant "copy and paste the function body here".  I should have been
more clear about that.

I don't care about the performance or code size effects.  I just find it easier
to read the code when I don't have to look for functions all the time.

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

Oh, I see. I pulled it into its own method for use by tests (so they can start
the thread before initialize is called). This is also why the method is
protected, not private.

     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_,
@@ -1165,20 +1176,57 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop(
   // Run initialization state machine.
   switch (initialization_state_) {
     case NOT_INITIALIZED:
-      initialization_state_ = DOWNLOADING_NIGORI;
+    case CLEANING_NIGORI: {

[rlarocque, 2012/07/04 06:39:35]

I'm not sure it makes sense to share code between the two states in this way.

For example, the UMA_HISTOGRAM_COUNTS code will be executed twice, but only if
the CLEANING_NIGORI step is executed.  We would be unable to differentiate
between a count due to the partially synced type situation this code is intended
to handle, and one triggered by the failure of this code.  

I'd suggest breaking this up into two states.  If we need to share code (ie. the
bits about calculating the partially synced types) we can move that out into a
function.  

[Nicolas Zea, 2012/07/09 18:40:45]

Done.

+      syncer::ModelTypeSet partially_synced_types =
+          syncer::ModelTypeSet::All();
+      partially_synced_types.RemoveAll(
+          core_->sync_manager()->InitialSyncEndedTypes());
+      partially_synced_types.RemoveAll(
+          core_->sync_manager()->GetTypesWithEmptyProgressMarkerToken(
+              syncer::ModelTypeSet::All()));
+      NigoriState nigori_state;
+      // 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)) {
+        if (initialization_state_ == CLEANING_NIGORI) {
+          // We apparently failed to clean the nigori somehow. Attempting

[rlarocque, 2012/07/04 06:39:35]

Do you really expect this could happen?

If so, I think we need better handling than this.  We ought to delete the
database file, which is something that backend initialize failure no longer
does.  

Come to think of it, following the un-revert, we could start deleting the
directory on backend initialize failure once again.  Let's just do that in a
separate patch.  

[Nicolas Zea, 2012/07/09 18:40:45]

Yeah, I was planning on having that land separately.

+          // 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;
+        }
+
+        // We need to blow away the partially downloaded nigori data.
+        // Just do a configuration with the nigori disabled.
+        LOG(ERROR) << "Found partial nigori, attempting cleanup.";
+        nigori_state = WITHOUT_NIGORI;

[rlarocque, 2012/07/04 06:39:35]

I spent a long time thinking about it, and I'm mostly convinced this will work.

It's a bit safer to do this here than elsewhere, since we've barely started init
and the PSS hasn't set the backend_initialized flag yet.  The scheduler is still
in configure mode, so I don't expect any callbacks to the cryptographer from the
syncer side of things, either.

+        initialization_state_ = CLEANING_NIGORI;
+      } else {
+        nigori_state = WITH_NIGORI;
+        initialization_state_ = DOWNLOADING_NIGORI;
+      }
       ConfigureDataTypes(
           syncer::CONFIGURE_REASON_NEW_CLIENT,
           syncer::ModelTypeSet(),
           syncer::ModelTypeSet(),
-          WITH_NIGORI,
+          nigori_state,
           // Calls back into this function.
           base::Bind(
               &SyncBackendHost::
-                  HandleNigoriConfigurationCompletedOnFrontendLoop,
+              HandleNigoriConfigurationCompletedOnFrontendLoop,
               weak_ptr_factory_.GetWeakPtr(), js_backend),
           base::Bind(&SyncBackendHost::OnNigoriDownloadRetry,
                      weak_ptr_factory_.GetWeakPtr()));
       break;
+    }
     case DOWNLOADING_NIGORI:
       initialization_state_ = REFRESHING_NIGORI;
       // Triggers OnEncryptedTypesChanged() and OnEncryptionComplete()