[Sync] Make BackendMigrator not wait for full sync cycles

chrome/browser/sync/backend_migrator.cc

Index: chrome/browser/sync/backend_migrator.cc
diff --git a/chrome/browser/sync/backend_migrator.cc b/chrome/browser/sync/backend_migrator.cc
index 0c7c15ca039a4d3722997c0874aaccad9fd5c9e0..0f8f418f9ebaea4333edcd98dfa3e58bccb43e60 100644
--- a/chrome/browser/sync/backend_migrator.cc
+++ b/chrome/browser/sync/backend_migrator.cc
@@ -6,13 +6,16 @@
 
 #include <algorithm>
 
+#include "base/message_loop.h"
 #include "base/string_number_conversions.h"
-#include "chrome/browser/sync/glue/data_type_manager.h"
+#include "base/tracked_objects.h"
 #include "chrome/browser/sync/internal_api/configure_reason.h"
+#include "chrome/browser/sync/internal_api/read_transaction.h"
 #include "chrome/browser/sync/profile_sync_service.h"
+#include "chrome/browser/sync/protocol/sync.pb.h"
 #include "chrome/browser/sync/sessions/session_state.h"
+#include "chrome/browser/sync/syncable/directory_manager.h"
 #include "chrome/common/chrome_notification_types.h"
-#include "content/browser/browser_thread.h"
 #include "content/common/notification_details.h"
 #include "content/common/notification_source.h"
 
@@ -21,59 +24,103 @@ using syncable::ModelTypeSet;
 namespace browser_sync {
 
 using sessions::SyncSessionSnapshot;
+using syncable::ModelTypeToString;
 
-BackendMigrator::BackendMigrator(ProfileSyncService* service,
+MigrationObserver::~MigrationObserver() {}
+
+BackendMigrator::BackendMigrator(const std::string& name,
+                                 sync_api::UserShare* user_share,
+                                 ProfileSyncService* service,
                                  DataTypeManager* manager)
-    : state_(IDLE), service_(service), manager_(manager),
-      restart_migration_(false),
-      method_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+    : name_(name), user_share_(user_share), service_(service),
+      manager_(manager), state_(IDLE),
+      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
   registrar_.Add(this, chrome::NOTIFICATION_SYNC_CONFIGURE_DONE,
                  Source<DataTypeManager>(manager_));
-  service_->AddObserver(this);
 }
 
 BackendMigrator::~BackendMigrator() {
-  service_->RemoveObserver(this);
 }
 
-bool BackendMigrator::HasStartedMigrating() const {
-  return state_ >= DISABLING_TYPES;
-}
+// Helper macros to log with the syncer thread name; useful when there
+// are multiple syncer threads involved.
+
+#define SLOG(severity) LOG(severity) << name_ << ": "
+
+#define SVLOG(verbose_level) VLOG(verbose_level) << name_ << ": "
 
 void BackendMigrator::MigrateTypes(const syncable::ModelTypeSet& types) {
+  const ModelTypeSet old_to_migrate = to_migrate_;
   {
     ModelTypeSet temp;
     std::set_union(to_migrate_.begin(), to_migrate_.end(),
                    types.begin(), types.end(),
                    std::inserter(temp, temp.end()));
-    to_migrate_ = temp;
+    std::swap(temp, to_migrate_);
   }
-
-  if (HasStartedMigrating()) {
-    VLOG(1) << "BackendMigrator::MigrateTypes: STARTED_MIGRATING early-out.";
-    restart_migration_ = true;
+  SVLOG(1) << "MigrateTypes called with " << ModelTypeSetToString(types)
+           << ", old_to_migrate = " << ModelTypeSetToString(old_to_migrate)
+          << ", to_migrate_ = " << ModelTypeSetToString(to_migrate_);
+  if (old_to_migrate == to_migrate_) {
+    SVLOG(1) << "MigrateTypes called with no new types; ignoring";
     return;
   }
 
-  if (manager_->state() != DataTypeManager::CONFIGURED) {
-    VLOG(1) << "BackendMigrator::MigrateTypes: manager CONFIGURED early-out.";
-    state_ = WAITING_TO_START;
+  if (state_ == IDLE)
+    ChangeState(WAITING_TO_START);
+
+  if (state_ == WAITING_TO_START) {
+    if (!TryStart())
+      SVLOG(1) << "Manager not configured; waiting";
     return;
   }
 
+  DCHECK_GT(state_, WAITING_TO_START);
+  // If we're already migrating, interrupt the current migration.
+  RestartMigration();
+}
+
+void BackendMigrator::AddMigrationObserver(MigrationObserver* observer) {
+  migration_observers_.AddObserver(observer);
+}
+
+void BackendMigrator::RemoveMigrationObserver(MigrationObserver* observer) {
+  migration_observers_.RemoveObserver(observer);
+}
+
+void BackendMigrator::ChangeState(State new_state) {
+  state_ = new_state;
+  FOR_EACH_OBSERVER(MigrationObserver, migration_observers_,
+                    OnMigrationStateChange());
+}
+
+bool BackendMigrator::TryStart() {
+  DCHECK_EQ(state_, WAITING_TO_START);
+  if (manager_->state() == DataTypeManager::CONFIGURED) {
+    RestartMigration();
+    return true;
+  }
+  return false;
+}
+
+void BackendMigrator::RestartMigration() {
   // We'll now disable any running types that need to be migrated.
-  state_ = DISABLING_TYPES;
+  ChangeState(DISABLING_TYPES);
   ModelTypeSet full_set;
   service_->GetPreferredDataTypes(&full_set);
   ModelTypeSet difference;
   std::set_difference(full_set.begin(), full_set.end(),
                       to_migrate_.begin(), to_migrate_.end(),
                       std::inserter(difference, difference.end()));
-  VLOG(1) << "BackendMigrator disabling types; calling Configure.";
+  bool configure_with_nigori = to_migrate_.count(syncable::NIGORI) == 0;
+  SVLOG(1) << "BackendMigrator disabling types "
+           << ModelTypeSetToString(to_migrate_) << "; configuring "
+           << ModelTypeSetToString(difference)
+           << (configure_with_nigori ? " with nigori" : " without nigori");
 
   // Add nigori for config or not based upon if the server told us to migrate
   // nigori or not.
-  if (to_migrate_.count(syncable::NIGORI) == 0) {
+  if (configure_with_nigori) {
     manager_->Configure(difference, sync_api::CONFIGURE_REASON_MIGRATION);
   } else {
     manager_->ConfigureWithoutNigori(difference,
@@ -81,76 +128,70 @@ void BackendMigrator::MigrateTypes(const syncable::ModelTypeSet& types) {
   }
 }
 
-void BackendMigrator::OnStateChanged() {
-  if (restart_migration_ == true) {
-    VLOG(1) << "BackendMigrator restarting migration in OnStateChanged.";
-    state_ = WAITING_TO_START;
-    restart_migration_ = false;
-    MigrateTypes(to_migrate_);
+void BackendMigrator::Observe(int type,
+                              const NotificationSource& source,
+                              const NotificationDetails& details) {
+  DCHECK_EQ(chrome::NOTIFICATION_SYNC_CONFIGURE_DONE, type);
+  if (state_ == IDLE)
     return;
-  }
 
-  if (state_ != WAITING_FOR_PURGE)
-    return;
+  // |manager_|'s methods aren't re-entrant, and we're notified from
+  // them, so post a task to avoid problems.
+  SVLOG(1) << "Posting OnConfigureDone from Observer";
+  MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&BackendMigrator::OnConfigureDone,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 *Details<DataTypeManager::ConfigureResult>(details).ptr()));
+}
 
-  size_t num_empty_migrated_markers = 0;
-  const SyncSessionSnapshot* snap = service_->GetLastSessionSnapshot();
-  for (ModelTypeSet::const_iterator it = to_migrate_.begin();
-       it != to_migrate_.end(); ++it) {
-    if (snap->download_progress_markers[*it].empty())
-      num_empty_migrated_markers++;
+namespace {
+
+syncable::ModelTypeSet GetUnsyncedDataTypes(sync_api::UserShare* user_share) {

[tim (not reviewing), 2011/09/01 01:50:56]

Ugh :(  It's pretty unfortunate that we have to hook everything up directly to
syncapi here.  Especially stuff that isn't even in glue, since that's kind of
why we have glue to begin with :\

That and I've always thought it was weird that we need the UserShare at all and
wanted to get rid of it fully.. seems silly since there is only ever one of
them...

[akalin, 2011/09/01 02:19:40]

Okay, so should I put this code back into SyncBackendHost?  I don't care which
method I do, but I'd like you guys to agree on which one. :)

[akalin, 2011/09/01 03:39:50]

Added a TODO in the header file to remove dep on user_share.

+  sync_api::ReadTransaction trans(FROM_HERE, user_share);
+  syncable::ModelTypeSet unsynced_data_types;
+  for (int i = syncable::FIRST_REAL_MODEL_TYPE;
+       i < syncable::MODEL_TYPE_COUNT; ++i) {
+    syncable::ModelType type = syncable::ModelTypeFromInt(i);
+    sync_pb::DataTypeProgressMarker progress_marker;
+    trans.GetLookup()->GetDownloadProgress(type, &progress_marker);
+    if (progress_marker.token().empty()) {
+      unsynced_data_types.insert(type);
+    }
   }
-
-  if (num_empty_migrated_markers < to_migrate_.size())
-    return;
-
-  state_ = REENABLING_TYPES;
-  ModelTypeSet full_set;
-  service_->GetPreferredDataTypes(&full_set);
-  VLOG(1) << "BackendMigrator re-enabling types.";
-
-  // Don't use |to_migrate_| for the re-enabling because the user may have
-  // chosen to disable types during the migration.
-  manager_->Configure(full_set, sync_api::CONFIGURE_REASON_MIGRATION);
+  return unsynced_data_types;
 }
 
-void BackendMigrator::Observe(int type,
-                              const NotificationSource& source,
-                              const NotificationDetails& details) {
-  DCHECK_EQ(chrome::NOTIFICATION_SYNC_CONFIGURE_DONE, type);
-  if (state_ == IDLE)
+}  // namespace
+
+void BackendMigrator::OnConfigureDone(
+    const DataTypeManager::ConfigureResult& result) {
+  SVLOG(1) << "OnConfigureDone with requested types "
+           << ModelTypeSetToString(result.requested_types)
+           << ", status " << result.status
+           << ", and to_migrate_ = " << ModelTypeSetToString(to_migrate_);
+  if (state_ == WAITING_TO_START) {
+    if (!TryStart())
+      SVLOG(1) << "Manager still not configured; still waiting";
     return;
+  }
 
-  const DataTypeManager::ConfigureResult* result =
-      Details<DataTypeManager::ConfigureResult>(details).ptr();
+  DCHECK_GT(state_, WAITING_TO_START);
 
   ModelTypeSet intersection;
-  std::set_intersection(result->requested_types.begin(),
-      result->requested_types.end(), to_migrate_.begin(), to_migrate_.end(),
+  std::set_intersection(
+      result.requested_types.begin(), result.requested_types.end(),
+      to_migrate_.begin(), to_migrate_.end(),
       std::inserter(intersection, intersection.end()));
-
-  // The intersection check is to determine if our disable request was
-  // interrupted by a user changing preferred types.  May still need to purge.
-  // It's pretty wild if we're in WAITING_FOR_PURGE here, because it would mean
-  // that after our disable-config finished but before the purge, another config
-  // was posted externally _and completed_, which means somehow the nudge to
-  // purge was dropped, yet nudges are reliable.
-  if (state_ == WAITING_TO_START || state_ == WAITING_FOR_PURGE ||
-      (state_ == DISABLING_TYPES && !intersection.empty())) {
-    state_ = WAITING_TO_START;
-    restart_migration_ = false;
-    VLOG(1) << "BackendMigrator::Observe posting MigrateTypes.";
-    if (!BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-        method_factory_.NewRunnableMethod(&BackendMigrator::MigrateTypes,
-                                          to_migrate_))) {
-      // Unittests need this.
-      // TODO(tim): Clean this up.
-      MigrateTypes(to_migrate_);
-    }
+  // This intersection check is to determine if our disable request
+  // was interrupted by a user changing preferred types.
+  if (state_ == DISABLING_TYPES && !intersection.empty()) {
+    SVLOG(1) << "Disable request interrupted by user changing types";
+    RestartMigration();
     return;
   }
 
-  if (result->status != DataTypeManager::OK) {
+  if (result.status != DataTypeManager::OK) {
     // If this fails, and we're disabling types, a type may or may not be
     // disabled until the user restarts the browser.  If this wasn't an abort,
     // any failure will be reported as an unrecoverable error to the UI. If it
@@ -158,23 +199,39 @@ void BackendMigrator::Observe(int type,
     // much we can do in any case besides wait until a restart to try again.
     // The server will send down MIGRATION_DONE again for types needing
     // migration as the type will still be enabled on restart.
-    LOG(WARNING) << "Unable to migrate, configuration failed!";
-    state_ = IDLE;
+    SLOG(WARNING) << "Unable to migrate, configuration failed!";
+    ChangeState(IDLE);
     to_migrate_.clear();
     return;
   }
 
   if (state_ == DISABLING_TYPES) {
-    state_ = WAITING_FOR_PURGE;
-    VLOG(1) << "BackendMigrator waiting for purge.";
+    const syncable::ModelTypeSet& unsynced_types =
+        GetUnsyncedDataTypes(user_share_);
+    if (!std::includes(unsynced_types.begin(), unsynced_types.end(),
+                       to_migrate_.begin(), to_migrate_.end())) {
+      SLOG(WARNING) << "Set of unsynced types: "
+                    << syncable::ModelTypeSetToString(unsynced_types)
+                    << " does not contain types to migrate: "
+                    << syncable::ModelTypeSetToString(to_migrate_)
+                    << "; not re-enabling yet";
+      return;
+    }
+
+    ChangeState(REENABLING_TYPES);
+    // Don't use |to_migrate_| for the re-enabling because the user
+    // may have chosen to disable types during the migration.
+    ModelTypeSet full_set;
+    service_->GetPreferredDataTypes(&full_set);
+    SVLOG(1) << "BackendMigrator re-enabling types: "
+             << syncable::ModelTypeSetToString(full_set);
+    manager_->Configure(full_set, sync_api::CONFIGURE_REASON_MIGRATION);
   } else if (state_ == REENABLING_TYPES) {
     // We're done!
-    state_ = IDLE;
+    ChangeState(IDLE);
 
-    std::stringstream ss;
-    std::copy(to_migrate_.begin(), to_migrate_.end(),
-              std::ostream_iterator<syncable::ModelType>(ss, ","));
-    VLOG(1) << "BackendMigrator: Migration complete for: " << ss.str();
+    SVLOG(1) << "BackendMigrator: Migration complete for: "
+             << syncable::ModelTypeSetToString(to_migrate_);
     to_migrate_.clear();
   }
 }
@@ -183,4 +240,13 @@ BackendMigrator::State BackendMigrator::state() const {
   return state_;
 }
 
+syncable::ModelTypeSet
+    BackendMigrator::GetPendingMigrationTypesForTest() const {
+  return to_migrate_;
+}
+
+#undef SVLOG
+
+#undef SLOG
+
 };  // namespace browser_sync