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

This reverts commit 9f5f561.

Reason for revert: reland this patch after fixing of DCHECK failures at crrev.com/c/2061751.

Original change's description:
> Revert "Verify consistency between sync and sign-in account IDs"
> 
> This reverts commit caf8eeb.
> 
> 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}

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

Bug: 1046237, 1021527, 1048771
Change-Id: If1a4a8c897d6e8bb1bae3e54b24c549146063f57
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2073757
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/master@{#745410}

components/sync/base/pref_names.cc

@@ -77,6 +77,7 @@ 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,6 +42,7 @@ 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,6 +298,7 @@ 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());
@@ -366,6 +367,7 @@ 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);
@@ -628,6 +630,15 @@ 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,6 +152,8 @@ 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

@@ -161,6 +161,38 @@ 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;
@@ -501,6 +533,22 @@ 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());
@@ -510,15 +558,22 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
     last_actionable_error_ = SyncProtocolError();
   }
 
-  // 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()) {
+  // 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.
     sync_prefs_.ClearLocalSyncTransportData();
     sync_prefs_.SetCacheGuid(GenerateCacheGUID());
+    sync_prefs_.SetGaiaId(authenticated_account_info.gaia);
   }
 
   InitializeBackendTaskRunnerIfNeeded();
@@ -538,8 +593,7 @@ 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 = GetAuthenticatedAccountInfo().account_id;
-  DCHECK(!params.authenticated_account_id.empty() || IsLocalSyncEnabled());
+  params.authenticated_account_id = authenticated_account_info.account_id;
   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 InitializeForNthSync() {
+  void PopulatePrefsForNthSync() {
     // Set first sync time before initialize to simulate a complete sync setup.
     SyncPrefs sync_prefs(prefs());
     sync_prefs.SetCacheGuid(kTestCacheGuid);
@@ -241,6 +241,10 @@ class ProfileSyncServiceTest : public ::testing::Test {
         /*registered_types=*/UserSelectableTypeSet::All(),
         /*selected_types=*/UserSelectableTypeSet::All());
     sync_prefs.SetFirstSetupComplete();
+  }
+
+  void InitializeForNthSync() {
+    PopulatePrefsForNthSync();
     service_->Initialize();
   }
 
@@ -1564,5 +1568,53 @@ 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