sync: hook up ServerConnectionManager <> SyncerThread2 and tie up more loose ends

chrome/browser/sync/glue/sync_backend_host.cc

 // Copyright (c) 2011 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 <algorithm>
 
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
@@ skipping 327 matching lines @@
     // If our state is not determined then we should not have the autofill
     // profile node.
     DCHECK(false);
 
     // just set it as not migrated.
     SetAutofillMigrationState(syncable::NOT_MIGRATED);
     return;
   }
 }
 
+SyncBackendHost::PendingConfigureDataTypesState::
+PendingConfigureDataTypesState() : deleted_type(false) {}
+
 void SyncBackendHost::ConfigureDataTypes(
     const DataTypeController::TypeMap& data_type_controllers,
     const syncable::ModelTypeSet& types,
     CancelableTask* ready_task) {
   // Only one configure is allowed at a time.
-  DCHECK(!configure_ready_task_.get());
+  DCHECK(!pending_config_mode_state_.get());
+  DCHECK(!pending_download_state_.get());
   DCHECK(syncapi_initialized_);
 
   if (types.count(syncable::AUTOFILL_PROFILE) != 0) {
     ConfigureAutofillMigration();
   }
 
-  bool deleted_type = false;
-  syncable::ModelTypeBitSet added_types;
+  scoped_ptr<PendingConfigureDataTypesState> state(new
+      PendingConfigureDataTypesState());
 
   {
     base::AutoLock lock(registrar_lock_);
     for (DataTypeController::TypeMap::const_iterator it =
              data_type_controllers.begin();
          it != data_type_controllers.end(); ++it) {
       syncable::ModelType type = (*it).first;
 
       // If a type is not specified, remove it from the routing_info.
       if (types.count(type) == 0) {
         registrar_.routing_info.erase(type);
-        deleted_type = true;
+        state->deleted_type = true;
       } else {
         // Add a newly specified data type as GROUP_PASSIVE into the
         // routing_info, if it does not already exist.
         if (registrar_.routing_info.count(type) == 0) {
           registrar_.routing_info[type] = GROUP_PASSIVE;
-          added_types.set(type);
+          state->added_types.set(type);
         }
       }
     }
   }
 
-  // If no new data types were added to the passive group, no need to
-  // wait for the syncer.
-  if (core_->syncapi()->InitialSyncEndedForAllEnabledTypes()) {
-    ready_task->Run();
-    delete ready_task;
+  state->ready_task.reset(ready_task);
+  state->initial_types = types;
+  pending_config_mode_state_.reset(state.release());
+
+  // If we're doing the first configure (at startup) this is redundant as the
+  // syncer thread always must start in config mode.
+  if (using_new_syncer_thread_) {
+    core_->syncapi()->StartConfigurationMode(NewCallback(core_.get(),
+        &SyncBackendHost::Core::FinishConfigureDataTypes));
   } else {
-    // Save the task here so we can run it when the syncer finishes
-    // initializing the new data types.  It will be run only when the
-    // set of initially synced data types matches the types requested in
-    // this configure.
-    configure_ready_task_.reset(ready_task);
-    configure_initial_sync_types_ = types;
+    FinishConfigureDataTypesOnFrontendLoop();
   }
+}
 
+void SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop() {
+  DCHECK_EQ(MessageLoop::current(), frontend_loop_);
   // Nudge the syncer. This is necessary for both datatype addition/deletion.
   //
   // Deletions need a nudge in order to ensure the deletion occurs in a timely
   // manner (see issue 56416).
   //
   // In the case of additions, on the next sync cycle, the syncer should
   // notice that the routing info has changed and start the process of
   // downloading updates for newly added data types.  Once this is
-  // complete, the configure_ready_task_ is run via an
+  // complete, the configure_state_.ready_task_ is run via an
   // OnInitializationComplete notification.
-  ScheduleSyncEventForConfigChange(deleted_type, added_types);
+  bool need_download = false;
+  if (pending_config_mode_state_->deleted_type) {

[Nicolas Zea, 2011/03/25 18:53:03]

Should we check pending_config_mode_state_.get() first? Since below we transfer
the pending_config_mode to pending_download_state, but I'm not seeing where we
transfer it back.

[tim (not reviewing), 2011/03/28 20:02:25]

Discussed offline, think this is fine.

+    if (using_new_syncer_thread_) {
+      core_thread_.message_loop()->PostTask(FROM_HERE,
+          NewRunnableMethod(core_.get(),
+          &SyncBackendHost::Core::DeferNudgeForCleanup));
+    } else {
+      need_download = true;
+    }
+  }
+
+  if (core_->syncapi()->InitialSyncEndedForAllEnabledTypes()) {
+    pending_config_mode_state_->ready_task->Run();
+  } else {
+    if (!pending_config_mode_state_->added_types.any()) {
+      LOG(WARNING) << "No new types, but initial sync not finished."
+                   << "Possible sync db corruption / removal.";
+      // TODO(tim): Log / UMA / count this somehow?
+      // TODO(tim): If no added types, we could (should?) config only for
+      // types that are needed... but this is a rare corruption edge case or
+      // implies the user mucked around with their syncdb, so for now do all.
+      pending_config_mode_state_->added_types =
+          syncable::ModelTypeBitSetFromSet(
+              pending_config_mode_state_->initial_types);
+    }
+    need_download = true;
+  }
+
+  if (need_download) {

[Nicolas Zea, 2011/03/25 18:53:03]

Presumably with the new syncer implementation we can reach here if we had an old
sync cycle finish before we entered configuration mode. A comment explaining why
this is handled correctly would be useful.

[tim (not reviewing), 2011/03/28 20:02:25]

Discussed offline, we don't wind up here but HandleSyncCycleCompleted... which
only looks at pending_download_state.

+    pending_download_state_.reset(pending_config_mode_state_.release());
+    if (using_new_syncer_thread_) {
+      RequestConfig(pending_download_state_->added_types);
+    } else {
+      RequestNudge();
+    }
+  }
+
+  pending_config_mode_state_.reset();
 
   // Notify the SyncManager about the new types.
   core_thread_.message_loop()->PostTask(FROM_HERE,
       NewRunnableMethod(core_.get(),
                         &SyncBackendHost::Core::DoUpdateEnabledTypes));
 }
 
-void SyncBackendHost::ScheduleSyncEventForConfigChange(bool deleted_type,
-    const syncable::ModelTypeBitSet& added_types) {
-  // We can only nudge when we've either deleted a dataype or added one, else
-  // we can cause unnecessary syncs. Unit tests cover this.
-  if (using_new_syncer_thread_) {
-    if (added_types.size() > 0)
-      RequestConfig(added_types);
-
-    // TODO(tim): Bug 76233.  Fix this once only one impl exists.
-    if (deleted_type) {
-      core_thread_.message_loop()->PostTask(FROM_HERE,
-          NewRunnableMethod(core_.get(),
-                            &SyncBackendHost::Core::DeferNudgeForCleanup));
-    }
-  } else if (deleted_type ||
-             !core_->syncapi()->InitialSyncEndedForAllEnabledTypes()) {
-    RequestNudge();
-  }
-}
-
 void SyncBackendHost::EncryptDataTypes(
     const syncable::ModelTypeSet& encrypted_types) {
   core_thread_.message_loop()->PostTask(FROM_HERE,
      NewRunnableMethod(core_.get(),
                        &SyncBackendHost::Core::DoEncryptDataTypes,
                        encrypted_types));
 }
 
 void SyncBackendHost::RequestNudge() {
   core_thread_.message_loop()->PostTask(FROM_HERE,
       NewRunnableMethod(core_.get(), &SyncBackendHost::Core::DoRequestNudge));
 }
 
 void SyncBackendHost::RequestConfig(
     const syncable::ModelTypeBitSet& added_types) {
   DCHECK(core_->syncapi());
-  core_->syncapi()->RequestConfig(added_types);
+
+  syncable::ModelTypeBitSet types_copy(added_types);
+  if (IsNigoriEnabled())
+    types_copy.set(syncable::NIGORI);
+
+  core_->syncapi()->RequestConfig(types_copy);
 }
 
 void SyncBackendHost::ActivateDataType(
     DataTypeController* data_type_controller,
     ChangeProcessor* change_processor) {
   base::AutoLock lock(registrar_lock_);
 
   // Ensure that the given data type is in the PASSIVE group.
   browser_sync::ModelSafeRoutingInfo::iterator i =
       registrar_.routing_info.find(data_type_controller->type());
@@ skipping 108 matching lines @@
 }
 
 void SyncBackendHost::Core::NotifyEncryptionComplete(
     const syncable::ModelTypeSet& encrypted_types) {
   if (!host_)
     return;
   DCHECK_EQ(MessageLoop::current(), host_->frontend_loop_);
   host_->frontend_->OnEncryptionComplete(encrypted_types);
 }
 
+void SyncBackendHost::Core::FinishConfigureDataTypes() {
+  host_->frontend_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
+      &SyncBackendHost::Core::FinishConfigureDataTypesOnFrontendLoop));
+}
+
+void SyncBackendHost::Core::FinishConfigureDataTypesOnFrontendLoop() {
+  host_->FinishConfigureDataTypesOnFrontendLoop();
+}
+
 SyncBackendHost::Core::DoInitializeOptions::DoInitializeOptions(
     const GURL& service_url,
     sync_api::HttpPostProviderFactory* http_bridge_factory,
     const sync_api::SyncCredentials& credentials,
     bool delete_sync_data_folder,
     const std::string& restored_key_for_bootstrapping,
     bool setup_for_test_mode)
     : service_url(service_url),
       http_bridge_factory(http_bridge_factory),
       credentials(credentials),
@@ skipping 266 matching lines @@
     SyncSessionSnapshot* snapshot) {
   if (!host_ || !host_->frontend_)
     return;
   DCHECK_EQ(MessageLoop::current(), host_->frontend_loop_);
 
   host_->last_snapshot_.reset(snapshot);
 
   // If we are waiting for a configuration change, check here to see
   // if this sync cycle has initialized all of the types we've been
   // waiting for.
-  if (host_->configure_ready_task_.get()) {
-    bool found_all = true;
+  if (host_->pending_download_state_.get()) {
+    bool found_all_added = true;
     for (syncable::ModelTypeSet::const_iterator it =
-             host_->configure_initial_sync_types_.begin();
-         it != host_->configure_initial_sync_types_.end(); ++it) {
-      found_all &= snapshot->initial_sync_ended.test(*it);
+             host_->pending_download_state_->initial_types.begin();
+         it != host_->pending_download_state_->initial_types.end();
+         ++it) {
+      if (host_->pending_download_state_->added_types.test(*it))
+        found_all_added &= snapshot->initial_sync_ended.test(*it);
     }
-
-    if (found_all) {
-      host_->configure_ready_task_->Run();
-      host_->configure_ready_task_.reset();
-      host_->configure_initial_sync_types_.clear();
+    if (!found_all_added) {
+      DCHECK(!host_->using_new_syncer_thread_);
+    } else {
+      host_->pending_download_state_->ready_task->Run();
+      host_->pending_download_state_.reset();
     }
   }
   host_->frontend_->OnSyncCycleCompleted();
 }
 
 void SyncBackendHost::Core::OnInitializationComplete() {
   if (!host_ || !host_->frontend_)
     return;  // We may have been told to Shutdown before initialization
              // completed.
 
@@ skipping 245 matching lines @@
   DCHECK_EQ(MessageLoop::current(), host_->core_thread_.message_loop());
   syncapi_->GetJsBackend()->ProcessMessage(name, args, sender);
 }
 
 void SyncBackendHost::Core::DeferNudgeForCleanup() {
   DCHECK_EQ(MessageLoop::current(), host_->core_thread_.message_loop());
   deferred_nudge_for_cleanup_requested_ = true;
 }
 
 }  // namespace browser_sync