Revert "Revert 142517 - [Sync] Refactor sync configuration logic."

Relanding with improved tests and support for cleaning up partial nigori.

The original patch was reverted due to users who needed to download a new nigori
node, but who had since been migrated. We do not handle migration before the
backend is initialized, so we now go ahead and detect+delete partial nigori
nodes before redownloading them. By blowing away the old progress marker,
we're no longer at risk of receiving the migration done response.

In order to test this the SyncBackendHost unit tests have been substantially upgraded.

original review at: https://chromiumcodereview.appspot.com/10483015

BUG=129665,133061,133219
TEST=unit/integration tests

Review URL: https://chromiumcodereview.appspot.com/10701085

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@146262 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 262 insertions, 71 deletions

diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h
index 75e55aa..d31888c6 100644
--- a/sync/internal_api/public/sync_manager.h
+++ b/sync/internal_api/public/sync_manager.h
@@ -31,6 +31,7 @@ struct Experiments;
 class ExtensionsActivityMonitor;
 class JsBackend;
 class JsEventHandler;
+class SyncScheduler;
 
 namespace sessions {
 class SyncSessionSnapshot;
@@ -401,6 +402,16 @@ class SyncManager {
   // Returns the set of types for which we have stored some sync data.
   syncer::ModelTypeSet InitialSyncEndedTypes();
 
+  // Returns those types within |types| that have an empty progress marker
+  // token.
+  syncer::ModelTypeSet GetTypesWithEmptyProgressMarkerToken(
+      syncer::ModelTypeSet types);
+
+  // Purge from the directory those types with non-empty progress markers
+  // but without initial synced ended set.
+  // Returns false if an error occurred, true otherwise.
+  bool PurgePartiallySyncedTypes();
+
   // Update tokens that we're using in Sync. Email must stay the same.
   void UpdateCredentials(const SyncCredentials& credentials);
 
@@ -428,20 +439,20 @@ class SyncManager {
   // error to call this when we don't have pending keys.
   void SetDecryptionPassphrase(const std::string& passphrase);
 
-  // Puts the SyncScheduler into a mode where no normal nudge or poll traffic
-  // will occur, but calls to RequestConfig will be supported.  If |callback|
-  // is provided, it will be invoked (from the internal SyncScheduler) when
-  // the thread has changed to configuration mode.
-  void StartConfigurationMode(const base::Closure& callback);
-
-  // Switches the mode of operation to CONFIGURATION_MODE and
-  // schedules a config task to fetch updates for |types|.
-  void RequestConfig(const syncer::ModelSafeRoutingInfo& routing_info,
-                     const syncer::ModelTypeSet& types,
-                     syncer::ConfigureReason reason);
-
-  void RequestCleanupDisabledTypes(
-      const syncer::ModelSafeRoutingInfo& routing_info);
+  // Switches the mode of operation to CONFIGURATION_MODE and performs
+  // any configuration tasks needed as determined by the params. Once complete,
+  // syncer will remain in CONFIGURATION_MODE until StartSyncingNormally is
+  // called.
+  // |ready_task| is invoked when the configuration completes.
+  // |retry_task| is invoked if the configuration job could not immediately
+  //              execute. |ready_task| will still be called when it eventually
+  //              does finish.
+  void ConfigureSyncer(
+      ConfigureReason reason,
+      const syncer::ModelTypeSet& types_to_config,
+      const syncer::ModelSafeRoutingInfo& new_routing_info,
+      const base::Closure& ready_task,
+      const base::Closure& retry_task);
 
   // Adds a listener to be notified of sync events.
   // NOTE: It is OK (in fact, it's probably a good idea) to call this before
@@ -538,11 +549,17 @@ class SyncManager {
   static const FilePath::CharType kSyncDatabaseFilename[];
 
  private:
+  friend class SyncManagerTest;
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, NudgeDelayTest);
 
   // For unit tests.
   base::TimeDelta GetNudgeDelayTimeDelta(const syncer::ModelType& model_type);
 
+  // Set the internal scheduler for testing purposes.
+  // TODO(sync): Use dependency injection instead. crbug.com/133061
+  void SetSyncSchedulerForTest(
+      scoped_ptr<syncer::SyncScheduler> scheduler);
+
   base::ThreadChecker thread_checker_;
 
   // An opaque pointer to the nested private class.
@@ -553,10 +570,6 @@ class SyncManager {
 
 bool InitialSyncEndedForTypes(syncer::ModelTypeSet types, UserShare* share);
 
-syncer::ModelTypeSet GetTypesWithEmptyProgressMarkerToken(
-    syncer::ModelTypeSet types,
-    syncer::UserShare* share);
-
 const char* ConnectionStatusToString(ConnectionStatus status);
 
 // Returns the string representation of a PassphraseRequiredReason value.
diff --git a/sync/internal_api/sync_manager.cc b/sync/internal_api/sync_manager.cc
index c8a293e..e8ba9dd 100644
--- a/sync/internal_api/sync_manager.cc
+++ b/sync/internal_api/sync_manager.cc
@@ -197,6 +197,11 @@ class SyncManager::SyncInternal
   // went wrong.
   bool SignIn(const SyncCredentials& credentials);
 
+  // Purge from the directory those types with non-empty progress markers
+  // but without initial synced ended set.
+  // Returns false if an error occurred, true otherwise.
+  bool PurgePartiallySyncedTypes();
+
   // Update tokens that we're using in Sync. Email must stay the same.
   void UpdateCredentials(const SyncCredentials& credentials);
 
@@ -357,6 +362,21 @@ class SyncManager::SyncInternal
   // Called only by our NetworkChangeNotifier.
   virtual void OnIPAddressChanged() OVERRIDE;
 
+  ModelTypeSet GetTypesWithEmptyProgressMarkerToken(ModelTypeSet types) {
+    DCHECK(initialized_);
+    syncer::ModelTypeSet result;
+    for (syncer::ModelTypeSet::Iterator i = types.First();
+         i.Good(); i.Inc()) {
+      sync_pb::DataTypeProgressMarker marker;
+      directory()->GetDownloadProgress(i.Get(), &marker);
+
+      if (marker.token().empty())
+        result.Put(i.Get());
+
+    }
+    return result;
+  }
+
   syncer::ModelTypeSet InitialSyncEndedTypes() {
     DCHECK(initialized_);
     return directory()->initial_sync_ended_types();
@@ -376,6 +396,8 @@ class SyncManager::SyncInternal
       const std::string& name, const JsArgList& args,
       const WeakHandle<JsReplyHandler>& reply_handler) OVERRIDE;
 
+  void SetSyncSchedulerForTest(scoped_ptr<SyncScheduler> scheduler);
+
  private:
   struct NotificationInfo {
     int total_count;
@@ -749,6 +771,15 @@ syncer::ModelTypeSet SyncManager::InitialSyncEndedTypes() {
   return data_->InitialSyncEndedTypes();
 }
 
+syncer::ModelTypeSet SyncManager::GetTypesWithEmptyProgressMarkerToken(
+    syncer::ModelTypeSet types) {
+  return data_->GetTypesWithEmptyProgressMarkerToken(types);
+}
+
+bool SyncManager::PurgePartiallySyncedTypes() {
+  return data_->PurgePartiallySyncedTypes();
+}
+
 void SyncManager::StartSyncingNormally(
     const syncer::ModelSafeRoutingInfo& routing_info) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -793,40 +824,39 @@ bool SyncManager::IsUsingExplicitPassphrase() {
   return data_ && data_->IsUsingExplicitPassphrase();
 }
 
-void SyncManager::RequestCleanupDisabledTypes(
-    const syncer::ModelSafeRoutingInfo& routing_info) {
+void SyncManager::ConfigureSyncer(
+    ConfigureReason reason,
+    const syncer::ModelTypeSet& types_to_config,
+    const syncer::ModelSafeRoutingInfo& new_routing_info,
+    const base::Closure& ready_task,
+    const base::Closure& retry_task) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (data_->scheduler()) {
-    data_->session_context()->set_routing_info(routing_info);
-    data_->scheduler()->CleanupDisabledTypes();
-  }
-}
+  DCHECK(!ready_task.is_null());
+  DCHECK(!retry_task.is_null());
 
-void SyncManager::RequestConfig(
-    const syncer::ModelSafeRoutingInfo& routing_info,
-    const ModelTypeSet& types, ConfigureReason reason) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (!data_->scheduler()) {
-    LOG(INFO)
-        << "SyncManager::RequestConfig: bailing out because scheduler is "
-        << "null";
-    return;
-  }
-  StartConfigurationMode(base::Closure());
-  data_->session_context()->set_routing_info(routing_info);
-  data_->scheduler()->ScheduleConfiguration(types, GetSourceFromReason(reason));
-}
+  // TODO(zea): set this based on whether cryptographer has keystore
+  // encryption key or not (requires opening a transaction). crbug.com/129665.
+  ConfigurationParams::KeystoreKeyStatus keystore_key_status =
+      ConfigurationParams::KEYSTORE_KEY_UNNECESSARY;
+
+  ConfigurationParams params(GetSourceFromReason(reason),
+                             types_to_config,
+                             new_routing_info,
+                             keystore_key_status,
+                             ready_task);
 
-void SyncManager::StartConfigurationMode(const base::Closure& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
   if (!data_->scheduler()) {
     LOG(INFO)
-        << "SyncManager::StartConfigurationMode: could not start "
-        << "configuration mode because because scheduler is null";
+        << "SyncManager::ConfigureSyncer: could not configure because "
+        << "scheduler is null";
+    params.ready_task.Run();
     return;
   }
-  data_->scheduler()->Start(
-      syncer::SyncScheduler::CONFIGURATION_MODE, callback);
+
+  data_->scheduler()->Start(syncer::SyncScheduler::CONFIGURATION_MODE);
+  if (!data_->scheduler()->ScheduleConfiguration(params))
+    retry_task.Run();
+
 }
 
 bool SyncManager::SyncInternal::Init(
@@ -923,16 +953,29 @@ bool SyncManager::SyncInternal::Init(
     scheduler_.reset(new SyncScheduler(name_, session_context(), new Syncer()));
   }
 
-  bool signed_in = SignIn(credentials);
+  bool success = SignIn(credentials);
 
-  if (signed_in) {
+  if (success) {
     if (scheduler()) {
-      scheduler()->Start(
-          syncer::SyncScheduler::CONFIGURATION_MODE, base::Closure());
+      scheduler()->Start(syncer::SyncScheduler::CONFIGURATION_MODE);
     }
 
     initialized_ = true;
 
+    // Unapplied datatypes (those that do not have initial sync ended set) get
+    // re-downloaded during any configuration. But, it's possible for a datatype
+    // to have a progress marker but not have initial sync ended yet, making
+    // it a candidate for migration. This is a problem, as the DataTypeManager
+    // does not support a migration while it's already in the middle of a
+    // configuration. As a result, any partially synced datatype can stall the
+    // DTM, waiting for the configuration to complete, which it never will due
+    // to the migration error. In addition, a partially synced nigori will
+    // trigger the migration logic before the backend is initialized, resulting
+    // in crashes. We therefore detect and purge any partially synced types as
+    // part of initialization.
+    if (!PurgePartiallySyncedTypes())
+      success = false;
+
     // Cryptographer should only be accessed while holding a
     // transaction.  Grabbing the user share for the transaction
     // checks the initialization state, so this must come after
@@ -950,14 +993,14 @@ bool SyncManager::SyncInternal::Init(
   FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                     OnInitializationComplete(
                         MakeWeakHandle(weak_ptr_factory_.GetWeakPtr()),
-                        signed_in));
+                        success));
 
-  if (!signed_in && testing_mode_ == NON_TEST)
+  if (!success && testing_mode_ == NON_TEST)
     return false;
 
   sync_notifier_->AddObserver(this);
 
-  return signed_in;
+  return success;
 }
 
 void SyncManager::SyncInternal::UpdateCryptographerAndNigori(
@@ -1102,9 +1145,13 @@ void SyncManager::SyncInternal::NotifyCryptographerState(
 void SyncManager::SyncInternal::StartSyncingNormally(
     const syncer::ModelSafeRoutingInfo& routing_info) {
   // Start the sync scheduler.
-  if (scheduler()) { // NULL during certain unittests.
+  if (scheduler()) {  // NULL during certain unittests.
+    // TODO(sync): We always want the newest set of routes when we switch back
+    // to normal mode. Figure out how to enforce set_routing_info is always
+    // appropriately set and that it's only modified when switching to normal
+    // mode.
     session_context()->set_routing_info(routing_info);
-    scheduler()->Start(SyncScheduler::NORMAL_MODE, base::Closure());
+    scheduler()->Start(SyncScheduler::NORMAL_MODE);
   }
 }
 
@@ -1158,6 +1205,20 @@ bool SyncManager::SyncInternal::SignIn(const SyncCredentials& credentials) {
   return true;
 }
 
+bool SyncManager::SyncInternal::PurgePartiallySyncedTypes() {
+  syncer::ModelTypeSet partially_synced_types =
+      syncer::ModelTypeSet::All();
+  partially_synced_types.RemoveAll(InitialSyncEndedTypes());
+  partially_synced_types.RemoveAll(GetTypesWithEmptyProgressMarkerToken(
+      syncer::ModelTypeSet::All()));
+
+  UMA_HISTOGRAM_COUNTS("Sync.PartiallySyncedTypes",
+                       partially_synced_types.Size());
+  if (partially_synced_types.Empty())
+    return true;
+  return directory()->PurgeEntriesWithTypeIn(partially_synced_types);
+}
+
 void SyncManager::SyncInternal::UpdateCredentials(
     const SyncCredentials& credentials) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -2340,6 +2401,11 @@ void SyncManager::SyncInternal::RemoveObserver(
   observers_.RemoveObserver(observer);
 }
 
+void SyncManager::SyncInternal::SetSyncSchedulerForTest(
+    scoped_ptr<SyncScheduler> sync_scheduler) {
+  scheduler_ = sync_scheduler.Pass();
+}
+
 SyncStatus SyncManager::GetDetailedStatus() const {
   return data_->GetStatus();
 }
@@ -2370,6 +2436,10 @@ TimeDelta SyncManager::GetNudgeDelayTimeDelta(
   return data_->GetNudgeDelayTimeDelta(model_type);
 }
 
+void SyncManager::SetSyncSchedulerForTest(scoped_ptr<SyncScheduler> scheduler) {
+  data_->SetSyncSchedulerForTest(scheduler.Pass());
+}
+
 syncer::ModelTypeSet SyncManager::GetEncryptedDataTypesForTest() const {
   ReadTransaction trans(FROM_HERE, GetUserShare());
   return GetEncryptedTypes(&trans);
@@ -2459,20 +2529,4 @@ bool InitialSyncEndedForTypes(syncer::ModelTypeSet types,
   return true;
 }
 
-syncer::ModelTypeSet GetTypesWithEmptyProgressMarkerToken(
-    syncer::ModelTypeSet types,
-    syncer::UserShare* share) {
-  syncer::ModelTypeSet result;
-  for (syncer::ModelTypeSet::Iterator i = types.First();
-       i.Good(); i.Inc()) {
-    sync_pb::DataTypeProgressMarker marker;
-    share->directory->GetDownloadProgress(i.Get(), &marker);
-
-    if (marker.token().empty())
-      result.Put(i.Get());
-
-  }
-  return result;
-}
-
 }  // namespace syncer
diff --git a/sync/internal_api/syncapi_unittest.cc b/sync/internal_api/syncapi_unittest.cc
index 6fff8c5..b22b585 100644
--- a/sync/internal_api/syncapi_unittest.cc
+++ b/sync/internal_api/syncapi_unittest.cc
@@ -24,6 +24,7 @@
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "sync/internal_api/public/base/model_type_test_util.h"
+#include "sync/engine/sync_scheduler.h"
 #include "sync/internal_api/public/change_record.h"
 #include "sync/internal_api/public/engine/model_safe_worker.h"
 #include "sync/internal_api/public/engine/polling_constants.h"
@@ -57,6 +58,7 @@
 #include "sync/syncable/nigori_util.h"
 #include "sync/syncable/syncable_id.h"
 #include "sync/syncable/write_transaction.h"
+#include "sync/test/callback_counter.h"
 #include "sync/test/fake_encryptor.h"
 #include "sync/test/fake_extensions_activity_monitor.h"
 #include "sync/util/cryptographer.h"
@@ -70,8 +72,10 @@ using base::ExpectDictStringValue;
 using testing::_;
 using testing::AnyNumber;
 using testing::AtLeast;
+using testing::DoAll;
 using testing::InSequence;
 using testing::Invoke;
+using testing::Return;
 using testing::SaveArg;
 using testing::StrictMock;
 
@@ -906,6 +910,10 @@ class SyncManagerTest : public testing::Test,
     return true;
   }
 
+  void SetScheduler(scoped_ptr<SyncScheduler> scheduler) {
+    sync_manager_.SetSyncSchedulerForTest(scheduler.Pass());
+  }
+
  private:
   // Needed by |sync_manager_|.
   MessageLoop message_loop_;
@@ -2485,4 +2493,120 @@ TEST_F(SyncManagerTest, SetPreviouslyEncryptedSpecifics) {
   }
 }
 
-}  // namespace syncer
+class MockSyncScheduler : public SyncScheduler {
+ public:
+  MockSyncScheduler() : SyncScheduler("name", NULL, NULL) {}
+  virtual ~MockSyncScheduler() {}
+
+  MOCK_METHOD1(Start, void(SyncScheduler::Mode));
+  MOCK_METHOD1(ScheduleConfiguration, bool(const ConfigurationParams&));
+};
+
+// Test that the configuration params are properly created and sent to
+// ScheduleConfigure. No callback should be invoked.
+TEST_F(SyncManagerTest, BasicConfiguration) {
+  ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION;
+  syncer::ModelTypeSet types_to_download(syncer::BOOKMARKS,
+                                         syncer::PREFERENCES);
+  syncer::ModelSafeRoutingInfo new_routing_info;
+  GetModelSafeRoutingInfo(&new_routing_info);
+
+  scoped_ptr<MockSyncScheduler> scheduler(new MockSyncScheduler());
+  ConfigurationParams params;
+  EXPECT_CALL(*scheduler, Start(SyncScheduler::CONFIGURATION_MODE));
+  EXPECT_CALL(*scheduler, ScheduleConfiguration(_)).
+      WillOnce(DoAll(SaveArg<0>(&params), Return(true)));
+  SetScheduler(scheduler.PassAs<SyncScheduler>());
+
+  CallbackCounter ready_task_counter, retry_task_counter;
+  sync_manager_.ConfigureSyncer(
+      reason,
+      types_to_download,
+      new_routing_info,
+      base::Bind(&CallbackCounter::Callback,
+                 base::Unretained(&ready_task_counter)),
+      base::Bind(&CallbackCounter::Callback,
+                 base::Unretained(&retry_task_counter)));
+  EXPECT_EQ(0, ready_task_counter.times_called());
+  EXPECT_EQ(0, retry_task_counter.times_called());
+  EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
+            params.source);
+  EXPECT_TRUE(types_to_download.Equals(params.types_to_download));
+  EXPECT_EQ(new_routing_info, params.routing_info);
+}
+
+// Test that the retry callback is invoked on configuration failure.
+TEST_F(SyncManagerTest, ConfigurationRetry) {
+  ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION;
+  syncer::ModelTypeSet types_to_download(syncer::BOOKMARKS,
+                                         syncer::PREFERENCES);
+  syncer::ModelSafeRoutingInfo new_routing_info;
+  GetModelSafeRoutingInfo(&new_routing_info);
+
+  scoped_ptr<MockSyncScheduler> scheduler(new MockSyncScheduler());
+  ConfigurationParams params;
+  EXPECT_CALL(*scheduler, Start(SyncScheduler::CONFIGURATION_MODE));
+  EXPECT_CALL(*scheduler, ScheduleConfiguration(_)).
+      WillOnce(DoAll(SaveArg<0>(&params), Return(false)));
+  SetScheduler(scheduler.PassAs<SyncScheduler>());
+
+  CallbackCounter ready_task_counter, retry_task_counter;
+  sync_manager_.ConfigureSyncer(
+      reason,
+      types_to_download,
+      new_routing_info,
+      base::Bind(&CallbackCounter::Callback,
+                 base::Unretained(&ready_task_counter)),
+      base::Bind(&CallbackCounter::Callback,
+                 base::Unretained(&retry_task_counter)));
+  EXPECT_EQ(0, ready_task_counter.times_called());
+  EXPECT_EQ(1, retry_task_counter.times_called());
+  EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
+            params.source);
+  EXPECT_TRUE(types_to_download.Equals(params.types_to_download));
+  EXPECT_EQ(new_routing_info, params.routing_info);
+}
+
+// Test that PurgePartiallySyncedTypes purges only those types that don't
+// have empty progress marker and don't have initial sync ended set.
+TEST_F(SyncManagerTest, PurgePartiallySyncedTypes) {
+  UserShare* share = sync_manager_.GetUserShare();
+
+  // Set Nigori and Bookmarks to be partial types.
+  sync_pb::DataTypeProgressMarker nigori_marker;
+  nigori_marker.set_data_type_id(
+      GetSpecificsFieldNumberFromModelType(NIGORI));
+  nigori_marker.set_token("token");
+  sync_pb::DataTypeProgressMarker bookmark_marker;
+  bookmark_marker.set_data_type_id(
+      GetSpecificsFieldNumberFromModelType(BOOKMARKS));
+  bookmark_marker.set_token("token");
+  share->directory->SetDownloadProgress(NIGORI, nigori_marker);
+  share->directory->SetDownloadProgress(BOOKMARKS, bookmark_marker);
+
+  // Set Preferences to be a full type.
+  sync_pb::DataTypeProgressMarker pref_marker;
+  pref_marker.set_data_type_id(
+      GetSpecificsFieldNumberFromModelType(PREFERENCES));
+  pref_marker.set_token("token");
+  share->directory->SetDownloadProgress(PREFERENCES, pref_marker);
+  share->directory->set_initial_sync_ended_for_type(PREFERENCES, true);
+
+  ModelTypeSet partial_types =
+      sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All());
+  EXPECT_FALSE(partial_types.Has(NIGORI));
+  EXPECT_FALSE(partial_types.Has(BOOKMARKS));
+  EXPECT_FALSE(partial_types.Has(PREFERENCES));
+
+  EXPECT_TRUE(sync_manager_.PurgePartiallySyncedTypes());
+
+  // Ensure only bookmarks and nigori lost their progress marker. Preferences
+  // should still have it.
+  partial_types =
+      sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All());
+  EXPECT_TRUE(partial_types.Has(NIGORI));
+  EXPECT_TRUE(partial_types.Has(BOOKMARKS));
+  EXPECT_FALSE(partial_types.Has(PREFERENCES));
+}
+
+}  // namespace