Revert "Verify consistency between sync and sign-in account IDs"

This reverts commit caf8eebf.

Reason for revert: suspect of running into sync protocol violations and
the corresponding DCHECK failures.

This CL reverts a preference being introduced without the corresponding
cleanup logic; but we intend to reland this patch anyway.

Original change's description:
> Verify consistency between sync and sign-in account IDs
>
> Local sync metadata belongs to one user account. Due to the distributed
> nature of the locally persisted sync metadata, the cache GUID is used
> as "epoch" to detect mismatched in edge cases like the browser crashing
> during shutdown and before I/O gets flushed.
>
> However, prior to this patch, the cache GUID itself has no safety
> mechanism to detect it maps to the intended account ID. In this patch,
> a new SyncPref is introduced to achieve that, in away that both prefs
> (cache GUID and account ID) are stored atomically.
>
> Because the pref is newly-introduced, migration logic is introduced to
> populate it for the first time if initially empty.
>
> Change-Id: I2cdd9f997077c4acd16e9283df8c025f51d40546
> Bug: 1046237,1021527
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2023528
> Reviewed-by: Marc Treib <treib@chromium.org>
> Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#736848}

TBR=msarda@chromium.org,treib@chromium.org,mastiz@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1046237, 1021527, 1048771
Change-Id: Ic9dcaf53780350984c3036f6acdb549f631893f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2041669
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739371}

components/sync/base/pref_names.cc

@@ -77,7 +77,6 @@ const char kSyncEncryptionBootstrapToken[] = "sync.encryption_bootstrap_token";
 const char kSyncKeystoreEncryptionBootstrapToken[] =
     "sync.keystore_encryption_bootstrap_token";
 
-const char kSyncGaiaId[] = "sync.gaia_id";
 const char kSyncCacheGuid[] = "sync.cache_guid";
 const char kSyncBirthday[] = "sync.birthday";
 const char kSyncBagOfChips[] = "sync.bag_of_chips";

components/sync/base/pref_names.h

@@ -42,7 +42,6 @@ extern const char kSyncRequested[];
 extern const char kSyncEncryptionBootstrapToken[];
 extern const char kSyncKeystoreEncryptionBootstrapToken[];
 
-extern const char kSyncGaiaId[];
 extern const char kSyncCacheGuid[];
 extern const char kSyncBirthday[];
 extern const char kSyncBagOfChips[];

components/sync/base/sync_prefs.cc

@@ -298,7 +298,6 @@ void SyncPrefs::RegisterProfilePrefs(
                                std::string());
 
   // Internal or bookkeeping prefs.
-  registry->RegisterStringPref(prefs::kSyncGaiaId, std::string());
   registry->RegisterStringPref(prefs::kSyncCacheGuid, std::string());
   registry->RegisterStringPref(prefs::kSyncBirthday, std::string());
   registry->RegisterStringPref(prefs::kSyncBagOfChips, std::string());
@@ -367,7 +366,6 @@ void SyncPrefs::ClearLocalSyncTransportData() {
   pref_service_->ClearPref(prefs::kSyncPassphrasePrompted);
   pref_service_->ClearPref(prefs::kSyncInvalidationVersions);
   pref_service_->ClearPref(prefs::kSyncLastRunVersion);
-  pref_service_->ClearPref(prefs::kSyncGaiaId);
   pref_service_->ClearPref(prefs::kSyncCacheGuid);
   pref_service_->ClearPref(prefs::kSyncBirthday);
   pref_service_->ClearPref(prefs::kSyncBagOfChips);
@@ -630,15 +628,6 @@ void SyncPrefs::RegisterTypeSelectedPref(
   registry->RegisterBooleanPref(pref_name, false);
 }
 
-void SyncPrefs::SetGaiaId(const std::string& gaia_id) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  pref_service_->SetString(prefs::kSyncGaiaId, gaia_id);
-}
-
-std::string SyncPrefs::GetGaiaId() const {
-  return pref_service_->GetString(prefs::kSyncGaiaId);
-}
-
 void SyncPrefs::SetCacheGuid(const std::string& cache_guid) {
   pref_service_->SetString(prefs::kSyncCacheGuid, cache_guid);
 }

components/sync/base/sync_prefs.h

@@ -152,8 +152,6 @@ class SyncPrefs : public CryptoSyncPrefs,
   // Maps |type| to its corresponding preference name.
   static const char* GetPrefNameForType(UserSelectableType type);
 
-  void SetGaiaId(const std::string& gaia_id);
-  std::string GetGaiaId() const;
   void SetCacheGuid(const std::string& cache_guid);
   std::string GetCacheGuid() const;
   void SetBirthday(const std::string& birthday);

components/sync/driver/profile_sync_service.cc

@@ -160,38 +160,6 @@ std::string GenerateCacheGUID() {
   return guid;
 }
 
-bool IsLocalSyncTransportDataValid(const SyncPrefs& sync_prefs,
-                                   const CoreAccountInfo& core_account_info) {
-  // If the cache GUID is empty, it most probably is because local sync data
-  // has been fully cleared via ClearLocalSyncTransportData() due to
-  // ShutdownReason::DISABLE_SYNC. Let's return false here anyway to make sure
-  // all prefs are cleared and a new random cache GUID generated.
-  if (sync_prefs.GetCacheGuid().empty()) {
-    return false;
-  }
-
-  // If cache GUID is initialized but the birthday isn't, it means the first
-  // sync cycle never completed (OnEngineInitialized()). This should be a rare
-  // case and theoretically harmless to resume, but as safety precaution, its
-  // simpler to regenerate the cache GUID and start from scratch, to avoid
-  // protocol violations (fetching updates requires that the request either has
-  // a birthday, or there should be no progress marker).
-  if (sync_prefs.GetBirthday().empty()) {
-    return false;
-  }
-
-  // Make sure the cached account information (gaia ID) is equal to the current
-  // one (otherwise the data may be corrupt). Note that, for local sync
-  // (IsLocalSyncEnabled()), the authenticated account is always empty.
-  if (sync_prefs.GetGaiaId() != core_account_info.gaia) {
-    DLOG(WARNING) << "Found mismatching gaia ID in sync preferences";
-    return false;
-  }
-
-  // All good: local sync data looks initialized and valid.
-  return true;
-}
-
 }  // namespace
 
 ProfileSyncService::InitParams::InitParams() = default;
@@ -534,22 +502,6 @@ void ProfileSyncService::InitializeBackendTaskRunnerIfNeeded() {
 void ProfileSyncService::StartUpSlowEngineComponents() {
   DCHECK(IsEngineAllowedToStart());
 
-  const CoreAccountInfo authenticated_account_info =
-      GetAuthenticatedAccountInfo();
-
-  if (IsLocalSyncEnabled()) {
-    // With local sync (roaming profiles) there is no identity manager and hence
-    // |authenticated_account_info| is empty. This is required for
-    // IsLocalSyncTransportDataValid() to work properly.
-    DCHECK(authenticated_account_info.gaia.empty());
-    DCHECK(authenticated_account_info.account_id.empty());
-  } else {
-    // Except for local sync (roaming profiles), the user must be signed in for
-    // sync to start.
-    DCHECK(!authenticated_account_info.gaia.empty());
-    DCHECK(!authenticated_account_info.account_id.empty());
-  }
-
   engine_ = sync_client_->GetSyncApiComponentFactory()->CreateSyncEngine(
       debug_identifier_, sync_client_->GetInvalidationService(),
       sync_prefs_.AsWeakPtr());
@@ -559,22 +511,15 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
     last_actionable_error_ = SyncProtocolError();
   }
 
-  // The gaia ID in SyncPrefs was introduced with M81, so having an empty value
-  // is legitimate and should be populated as a one-off migration.
-  // TODO(mastiz): Clean up this migration code after a grace period (e.g. 1
-  // year).
-  if (sync_prefs_.GetGaiaId().empty()) {
-    sync_prefs_.SetGaiaId(authenticated_account_info.gaia);
-  }
-
-  if (!IsLocalSyncTransportDataValid(sync_prefs_, authenticated_account_info)) {
-    // Either the local data is uninitialized or corrupt, so let's throw
-    // everything away and start from scratch with a new cache GUID, which also
-    // cascades into datatypes throwing away their dangling sync metadata due to
-    // cache GUID mismatches.
+  // If either the cache GUID or the birthday are uninitialized, it means we
+  // haven't completed a first sync cycle (OnEngineInitialized()). Theoretically
+  // both or neither should be empty, but just in case, we regenerate the cache
+  // GUID if either of them is missing, to avoid protocol violations (fetching
+  // updates requires that the request either has a birthday, or there should be
+  // no progress marker).
+  if (sync_prefs_.GetCacheGuid().empty() || sync_prefs_.GetBirthday().empty()) {
     sync_prefs_.ClearLocalSyncTransportData();
     sync_prefs_.SetCacheGuid(GenerateCacheGUID());
-    sync_prefs_.SetGaiaId(authenticated_account_info.gaia);
   }
 
   InitializeBackendTaskRunnerIfNeeded();
@@ -594,7 +539,8 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
   params.http_factory_getter = base::BindOnce(
       create_http_post_provider_factory_cb_, MakeUserAgentForSync(channel_),
       url_loader_factory_->Clone(), network_time_update_callback_);
-  params.authenticated_account_id = authenticated_account_info.account_id;
+  params.authenticated_account_id = GetAuthenticatedAccountInfo().account_id;
+  DCHECK(!params.authenticated_account_id.empty() || IsLocalSyncEnabled());
   if (!base::FeatureList::IsEnabled(switches::kSyncE2ELatencyMeasurement)) {
     invalidation::InvalidationService* invalidator =
         sync_client_->GetInvalidationService();

components/sync/driver/profile_sync_service_unittest.cc

@@ -229,7 +229,7 @@ class ProfileSyncServiceTest : public ::testing::Test {
     service_.reset();
   }
 
-  void PopulatePrefsForNthSync() {
+  void InitializeForNthSync() {
     // Set first sync time before initialize to simulate a complete sync setup.
     SyncPrefs sync_prefs(prefs());
     sync_prefs.SetCacheGuid(kTestCacheGuid);
@@ -241,10 +241,6 @@ class ProfileSyncServiceTest : public ::testing::Test {
         /*registered_types=*/UserSelectableTypeSet::All(),
         /*selected_types=*/UserSelectableTypeSet::All());
     sync_prefs.SetFirstSetupComplete();
-  }
-
-  void InitializeForNthSync() {
-    PopulatePrefsForNthSync();
     service_->Initialize();
   }
 
@@ -1568,53 +1564,5 @@ TEST_F(ProfileSyncServiceTestWithStopSyncInPausedState,
   EXPECT_FALSE(service()->GetDisableReasons().Empty());
 }
 
-TEST_F(ProfileSyncServiceTest, ShouldPopulateAccountIdCachedInPrefs) {
-  SyncPrefs sync_prefs(prefs());
-
-  SignIn();
-  CreateService(ProfileSyncService::AUTO_START);
-  InitializeForNthSync();
-  ASSERT_EQ(SyncService::TransportState::ACTIVE,
-            service()->GetTransportState());
-  ASSERT_EQ(kTestCacheGuid, sync_prefs.GetCacheGuid());
-  EXPECT_EQ(signin::GetTestGaiaIdForEmail(kTestUser), sync_prefs.GetGaiaId());
-}
-
-TEST_F(ProfileSyncServiceTest,
-       ShouldNotPopulateAccountIdCachedInPrefsWithLocalSync) {
-  SyncPrefs sync_prefs(prefs());
-
-  SignIn();
-  CreateServiceWithLocalSyncBackend();
-  InitializeForNthSync();
-  ASSERT_EQ(SyncService::TransportState::ACTIVE,
-            service()->GetTransportState());
-  ASSERT_EQ(kTestCacheGuid, sync_prefs.GetCacheGuid());
-  EXPECT_TRUE(sync_prefs.GetGaiaId().empty());
-}
-
-// Verifies that local sync transport data is thrown away if there is a mismatch
-// between the account ID cached in SyncPrefs and the actual one.
-TEST_F(ProfileSyncServiceTest,
-       ShouldClearLocalSyncTransportDataDueToAccountIdMismatch) {
-  SyncPrefs sync_prefs(prefs());
-
-  SignIn();
-  CreateService(ProfileSyncService::AUTO_START);
-  PopulatePrefsForNthSync();
-  ASSERT_EQ(kTestCacheGuid, sync_prefs.GetCacheGuid());
-
-  // Manually override the authenticated account ID, which should be detected
-  // during initialization.
-  sync_prefs.SetGaiaId("corrupt_gaia_id");
-
-  service()->Initialize();
-
-  ASSERT_EQ(SyncService::TransportState::ACTIVE,
-            service()->GetTransportState());
-  EXPECT_NE(kTestCacheGuid, sync_prefs.GetCacheGuid());
-  EXPECT_EQ(signin::GetTestGaiaIdForEmail(kTestUser), sync_prefs.GetGaiaId());
-}
-
 }  // namespace
 }  // namespace syncer