Make SyncBackendRegistrar aware of loaded data

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 d86bdaed7ca1d96b01b496c1643640f4461e1fed..529f420c0d9e97506cb01c405436cacbf6d72483 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -91,7 +91,8 @@ class SyncBackendHost::Core
       const syncer::sessions::SyncSessionSnapshot& snapshot) OVERRIDE;
   virtual void OnInitializationComplete(
       const syncer::WeakHandle<syncer::JsBackend>& js_backend,
-      bool success) OVERRIDE;
+      bool success,
+      syncer::ModelTypeSet restored_types) OVERRIDE;
   virtual void OnConnectionStatusChange(
       syncer::ConnectionStatus status) OVERRIDE;
   virtual void OnPassphraseRequired(
@@ -367,7 +368,6 @@ void SyncBackendHost::Initialize(
     SyncFrontend* frontend,
     const syncer::WeakHandle<syncer::JsEventHandler>& event_handler,
     const GURL& sync_service_url,
-    syncer::ModelTypeSet initial_types,
     const SyncCredentials& credentials,
     bool delete_sync_data_folder,
     syncer::SyncManagerFactory* sync_manager_factory,
@@ -384,14 +384,7 @@ void SyncBackendHost::Initialize(
   frontend_ = frontend;
   DCHECK(frontend);
 
-  syncer::ModelTypeSet initial_types_with_nigori(initial_types);
-  CHECK(sync_prefs_.get());
-  if (sync_prefs_->HasSyncSetupCompleted()) {
-    initial_types_with_nigori.Put(syncer::NIGORI);
-  }
-
-  registrar_.reset(new SyncBackendRegistrar(initial_types_with_nigori,
-                                            name_,
+  registrar_.reset(new SyncBackendRegistrar(name_,
                                             profile_,
                                             sync_thread_.message_loop()));
   syncer::ModelSafeRoutingInfo routing_info;
@@ -599,6 +592,14 @@ void SyncBackendHost::ConfigureDataTypes(
     NigoriState nigori_state,
     const base::Callback<void(syncer::ModelTypeSet)>& ready_task,
     const base::Callback<void()>& retry_callback) {
+  // Only one configure is allowed at a time.  This is guaranteed by our
+  // callers.  The SyncBackendHost requests one configure as the backend is
+  // initializing and waits for it to complete.  After initialization, all
+  // configurations will pass through the DataTypeManager, which is careful to
+  // never send a new configure request until the current request succeeds.
+
+  DCHECK_GT(initialization_state_, NOT_INITIALIZED);
+
   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) {
@@ -608,53 +609,61 @@ void SyncBackendHost::ConfigureDataTypes(
     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 reconfiguring, and will only
-  // trigger another call to ConfigureDataTypes once the current reconfiguration
-  // completes).
-  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.
+  // The SyncBackendRegistrar's routing info will be updated by removing the
+  // types_to_remove_with_nigori from the list then adding

[Nicolas Zea, 2012/07/23 20:47:15]

add then remove.

[rlarocque, 2012/07/23 23:16:10]

You mentioned this in your first set of comments, so I fixed it.  Then, when I
was re-reading your comments, decided to swap them again.

Fixed.  Again.

+  // types_to_add_with_nigori.  Any types which are not in either of those sets
+  // will remain untouched.
+  //
+  // Types which were not in the list previously are not cached locally, so we

[Nicolas Zea, 2012/07/23 20:47:15]

Cached seems kind of ambiguous. Perhaps just say are not fully downloaded?

[rlarocque, 2012/07/23 23:16:10]

Done.

+  // must ask the syncer to download them.  Any newly supported datatypes will
+  // not have been in that routing info list, so they will be among the types
+  // downloaded if they are enabled.
+  //
+  // The SyncBackendRegistrar's state was initially derrived from the types

[Nicolas Zea, 2012/07/23 20:47:15]

derrived ->derived

[rlarocque, 2012/07/23 23:16:10]

Done.

+  // marked initial_sync_ended when the sync database was loaded.  Afterwards it
+  // is modified only by this function.  We expect it to remain in sync with the
+  // backend because configuration requests are never aborted; they are retried
+  // until they succeed or the browser is closed.
+
+  syncer::ModelTypeSet types_to_download = registrar_->ConfigureDataTypes(
+      types_to_add_with_nigori, types_to_remove_with_nigori);
+  if (!types_to_download.Empty())
+    types_to_download.Put(syncer::NIGORI);
+
+  // TODO(sync): crbug.com/137550.
+  // It's dangerous to configure types that have progress markers.  Types with
+  // progress markers can trigger a MIGRATION_DONE response.  We are not
+  // prepared to handle a migration during a configure, so we must ensure that
+  // all our types_to_download actually contain no data before we sync them.
+  //
+  // The most common way to end up in this situation used to be types which had
+  // !initial_sync_ended, but did have some progress markers.  We avoid problems
+  // with those types by purging the data of any such partially synced types
+  // soon after we load the directory.
+  //
+  // Another possible scenario is that we have newly supported or newly enabled
+  // data types being downloaded here but the nigori type, which is always
+  // included in any GetUpdates request, requires migration.  The server has
+  // code to detect this scenario based on the configure reason, the fact that
+  // the nigori type is the only requested type which requires migration, and
+  // that the requested types list includes at least one non-nigori type.  It
+  // will not send a MIGRATION_DONE response in that case.  We still need to be
+  // careful to not send progress markers for non-nigori types, though.  If a
+  // non-nigori type in the request requires migration, a MIGRATION_DONE
+  // response will be sent.
+
   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);
-
-  // 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);
 
   SDVLOG(1) << "Types "
-            << syncer::ModelTypeSetToString(types_to_config)
+            << syncer::ModelTypeSetToString(types_to_download)
             << " 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,
+                         types_to_download,
                          routing_info,
                          ready_task,
                          retry_callback);
@@ -681,6 +690,11 @@ syncer::UserShare* SyncBackendHost::GetUserShare() const {
   return core_->sync_manager()->GetUserShare();
 }
 
+// Bypasses the initialized() check.
+syncer::UserShare* SyncBackendHost::GetUserShareForTest() const {
+  return core_->sync_manager()->GetUserShare();
+}
+
 SyncBackendHost::Status SyncBackendHost::GetDetailedStatus() {
   DCHECK(initialized());
   return core_->sync_manager()->GetDetailedStatus();
@@ -771,6 +785,13 @@ void SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop(
     ready_task.Run(failed_configuration_types);
 }
 
+void SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop(
+    const syncer::WeakHandle<syncer::JsBackend>& js_backend, bool success,
+    syncer::ModelTypeSet restored_types) {
+  registrar_->SetInitialTypes(restored_types);
+  HandleInitializationCompletedOnFrontendLoop(js_backend, success);
+}
+
 SyncBackendHost::DoInitializeOptions::DoInitializeOptions(
     MessageLoop* sync_loop,
     SyncBackendRegistrar* registrar,
@@ -842,12 +863,13 @@ void SyncBackendHost::Core::OnSyncCycleCompleted(
 
 void SyncBackendHost::Core::OnInitializationComplete(
     const syncer::WeakHandle<syncer::JsBackend>& js_backend,
-    bool success) {
+    bool success,
+    const syncer::ModelTypeSet restored_types) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   host_.Call(
       FROM_HERE,
-      &SyncBackendHost::HandleInitializationCompletedOnFrontendLoop,
-      js_backend, success);
+      &SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop,
+      js_backend, success, restored_types);
 
   if (success) {
     // Initialization is complete, so we can schedule recurring SaveChanges.
@@ -978,7 +1000,6 @@ void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) {
       options.service_url.SchemeIsSecure(),
       BrowserThread::GetBlockingPool(),
       options.make_http_bridge_factory_fn.Run().Pass(),
-      options.routing_info,
       options.workers,
       options.extensions_activity_monitor,
       options.registrar /* as SyncManager::ChangeDelegate */,
@@ -1176,17 +1197,15 @@ 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:
+      // This configuration should result in a download request if the nigori
+      // type's initial_sync_ended bit is unset.  If the download request
+      // contains progress markers, there is a risk that the server will try to
+      // trigger migration.  That would be disastrous, so we must rely on the
+      // backend to ensure that this type never has both progress markers and

[Nicolas Zea, 2012/07/23 20:47:15]

did you mean rely on the sync manager purging? Since the backend isn't really
involved in that aspect.

[rlarocque, 2012/07/23 23:16:10]

I'm using "backend" as a generic term to refer to the code on the sync thread. 
I figured that would make the comment less likely to go stale if we changed the
specifics of our purging implementation.  

However, the comment's not very useful if people don't understand it.  I'll
change it to sync manager.

+      // !initial_sync_ended.
       initialization_state_ = DOWNLOADING_NIGORI;
       ConfigureDataTypes(
           syncer::CONFIGURE_REASON_NEW_CLIENT,