Make SyncBackendRegistrar aware of loaded data

chrome/browser/sync/glue/sync_backend_host.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "build/build_config.h"
 
 #include "chrome/browser/sync/glue/sync_backend_host.h"
 
 #include <algorithm>
 #include <map>
@@ skipping 73 matching lines @@
        const FilePath& sync_data_folder_path,
        const base::WeakPtr<SyncBackendHost>& backend);
 
   // SyncManager::Observer implementation.  The Core just acts like an air
   // traffic controller here, forwarding incoming messages to appropriate
   // landing threads.
   virtual void OnSyncCycleCompleted(
       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(
       syncer::PassphraseRequiredReason reason,
       const sync_pb::EncryptedData& pending_keys) OVERRIDE;
   virtual void OnPassphraseAccepted() OVERRIDE;
   virtual void OnBootstrapTokenUpdated(
       const std::string& bootstrap_token) OVERRIDE;
   virtual void OnStopSyncingPermanently() OVERRIDE;
   virtual void OnUpdatedToken(const std::string& token) OVERRIDE;
@@ skipping 255 matching lines @@
   return scoped_ptr<syncer::HttpPostProviderFactory>(
       new syncer::HttpBridgeFactory(getter, MakeUserAgentForSyncApi()));
 }
 
 }  // namespace
 
 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,
     syncer::UnrecoverableErrorHandler* unrecoverable_error_handler,
     syncer::ReportUnrecoverableErrorFunction
         report_unrecoverable_error_function) {
   if (!sync_thread_.Start())
     return;
 
   chrome_sync_notification_bridge_.reset(
       new ChromeSyncNotificationBridge(
           profile_, sync_thread_.message_loop_proxy()));
 
   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;
   std::vector<syncer::ModelSafeWorker*> workers;
   registrar_->GetModelSafeRoutingInfo(&routing_info);
   registrar_->GetWorkers(&workers);
 
   initialization_state_ = CREATING_SYNC_MANAGER;
   InitCore(DoInitializeOptions(
       sync_thread_.message_loop(),
@@ skipping 187 matching lines @@
   core_ = NULL;  // Releases reference to core_.
 }
 
 void SyncBackendHost::ConfigureDataTypes(
     syncer::ConfigureReason reason,
     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) {
+  // 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) {
     types_to_add_with_nigori.Put(syncer::NIGORI);
     types_to_remove_with_nigori.Remove(syncer::NIGORI);
   } else {
     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);
 }
 
 void SyncBackendHost::EnableEncryptEverything() {
   sync_thread_.message_loop()->PostTask(FROM_HERE,
      base::Bind(&SyncBackendHost::Core::DoEnableEncryptEverything,
                 core_.get()));
 }
 
 void SyncBackendHost::ActivateDataType(
     syncer::ModelType type, syncer::ModelSafeGroup group,
     ChangeProcessor* change_processor) {
   registrar_->ActivateDataType(type, group, change_processor, GetUserShare());
 }
 
 void SyncBackendHost::DeactivateDataType(syncer::ModelType type) {
   registrar_->DeactivateDataType(type);
 }
 
 syncer::UserShare* SyncBackendHost::GetUserShare() const {
   DCHECK(initialized());
   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();
 }
 
 SyncSessionSnapshot SyncBackendHost::GetLastSessionSnapshot() const {
   return last_snapshot_;
 }
 
 bool SyncBackendHost::HasUnsyncedItems() const {
@@ skipping 70 matching lines @@
 
   // 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));
 
   if (!ready_task.is_null())
     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,
     const syncer::ModelSafeRoutingInfo& routing_info,
     const std::vector<syncer::ModelSafeWorker*>& workers,
     syncer::ExtensionsActivityMonitor* extensions_activity_monitor,
     const syncer::WeakHandle<syncer::JsEventHandler>& event_handler,
     const GURL& service_url,
     MakeHttpBridgeFactoryFn make_http_bridge_factory_fn,
     const syncer::SyncCredentials& credentials,
@@ skipping 51 matching lines @@
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   host_.Call(
       FROM_HERE,
       &SyncBackendHost::HandleSyncCycleCompletedOnFrontendLoop,
       snapshot);
 }
 
 
 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.
     sync_loop_->PostTask(FROM_HERE,
                          base::Bind(&Core::StartSavingChanges, this));
   }
 }
 
 void SyncBackendHost::Core::OnConnectionStatusChange(
     syncer::ConnectionStatus status) {
@@ skipping 110 matching lines @@
   sync_manager_ = options.sync_manager_factory->CreateSyncManager(name_);
   sync_manager_->AddObserver(this);
   success = sync_manager_->Init(
       sync_data_folder_path_,
       options.event_handler,
       options.service_url.host() + options.service_url.path(),
       options.service_url.EffectiveIntPort(),
       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 */,
       options.credentials,
       scoped_ptr<syncer::SyncNotifier>(new BridgedSyncNotifier(
           options.chrome_sync_notification_bridge,
           options.sync_notifier_factory->CreateSyncNotifier())),
       options.restored_key_for_bootstrapping,
       scoped_ptr<InternalComponentsFactory>(
           options.internal_components_factory),
@@ skipping 177 matching lines @@
     initialization_state_ = NOT_INITIALIZED;
 
   DCHECK_EQ(MessageLoop::current(), frontend_loop_);
   if (!success) {
     initialization_state_ = NOT_INITIALIZED;
     frontend_->OnBackendInitialized(
         syncer::WeakHandle<syncer::JsBackend>(), false);
     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,
           syncer::ModelTypeSet(),
           syncer::ModelTypeSet(),
           WITH_NIGORI,
           // Calls back into this function.
           base::Bind(
               &SyncBackendHost::
                   HandleNigoriConfigurationCompletedOnFrontendLoop,
@@ skipping 175 matching lines @@
       FROM_HERE,
       base::Bind(&SyncBackendHost::Core::DoRefreshNigori,
                  core_.get(), sync_thread_done_callback));
 }
 
 #undef SDVLOG
 
 #undef SLOG
 
 }  // namespace browser_sync