Reland "Unify clearing of sync prefs"

This reverts commit 232ad148.

Reason for revert: this patch was reverted in crrev.com/c/2043451 due to suspect of sync protocol violation. It was fixed in crrev.com/c/2061751.

Original change's description:
> Revert "Unify clearing of sync prefs"
> 
> This reverts commit 44d3aa82.
> 
> Reason for revert: suspect of running into sync protocol violations and the corresponding DCHECK failures.
> 
> Original change's description:
> > Unify clearing of sync prefs
> > 
> > SyncPrefs is a thin layer on top of preferences and hosts two groups
> > of preferences:
> > 1) Actual user-facing settings, such as type-selection, exposed via
> >    SyncUserSettings.
> > 2) Local "bookkeeping" sync metadata, such the last synced time or the
> >    client ID (cache GUID).
> > 
> > In addition, there are two cases that fall somewhere in the middle,
> > whose behavior is changed in this patch:
> > a) FirstSetupComplete: which roughly represents the user having
> >    consented to sync-the-feature (as opposed to transport-only upon
> >    sign-in without explicit user consent).
> > b) The encryption-bootstrap-token: which represent an explicit
> >    passphrase (usually custom passphrase) entered by the user, that
> >    allows decrypting the incoming sync changes and encrypt outgoing
> >    ones.
> > 
> > The last two preferences above fit group 1 better, so this patch stops
> > clearing them in SyncPrefs::ClearPreferences(), now renamed to
> > SyncPrefs::ClearLocalSyncTransportData().
> > 
> > With such cleanup, what used to be
> > ClearDirectoryConsistencyPreferences() is now merged into a single
> > clearing function, ClearLocalSyncTransportData(), and all calling sites
> > are unified by directly clearing prefs in ShutdownImpl(DISABLE_SYNC), as
> > opposed to individual calling sites.
> > 
> > This introduces -arguably desirable- behavioral changes because
> > codepaths like RESET_LOCAL_SYNC_DATA or STOP_SYNC_FOR_DISABLED_ACCOUNT
> > now actually clear all local metadata, which most notably includes
> > keystore keys.
> > 
> > Change-Id: I2c42f98c4e068c7e340580d0b78a5cd5b5c46171
> > Bug: 1046237
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2023649
> > Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> > Reviewed-by: Marc Treib <treib@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#735841}
> 
> TBR=treib@chromium.org,mastiz@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug: 1046237, 1048771
> Change-Id: I4ce9e941f12aa58b7b6b286e990e74a1e01dad69
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2043451
> Commit-Queue: Rushan Suleymanov <rushans@google.com>
> Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#739374}

TBR=treib@chromium.org,mastiz@chromium.org,rushans@google.com

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

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

chrome/browser/sync/test/integration/single_client_polling_sync_test.cc

@@ -54,7 +54,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest, ShouldInitializePollPrefs) {
 // This test verifies that updates of the poll interval get persisted
 // That's important make sure clients with short live times will eventually poll
 // (e.g. Android).
-IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest, ShouldUpdatePollPrefs) {
+IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest,
+                       PRE_ShouldUsePollIntervalFromPrefs) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   sync_pb::ClientCommand client_command;
@@ -76,17 +77,12 @@ IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest, ShouldUpdatePollPrefs) {
 
 IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest,
                        ShouldUsePollIntervalFromPrefs) {
-  // Setup clients and provide new poll interval via prefs.
-  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
-  SyncPrefs sync_prefs(GetProfile(0)->GetPrefs());
-  sync_prefs.SetPollInterval(base::TimeDelta::FromSeconds(123));
-
   // Execute a sync cycle and verify this cycle used that interval.
   // This test assumes the SyncScheduler reads the actual interval from the
   // context. This is covered in the SyncSchedulerImpl's unittest.
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   EXPECT_THAT(GetClient(0)->GetLastCycleSnapshot().poll_interval().InSeconds(),
-              Eq(123));
+              Eq(67));
 }
 
 // This test simulates the poll interval expiring between restarts.

components/sync/base/sync_prefs.cc

@@ -293,6 +293,10 @@ void SyncPrefs::RegisterProfilePrefs(
   }
 #endif
 
+  // The encryption bootstrap token represents a user-entered passphrase.
+  registry->RegisterStringPref(prefs::kSyncEncryptionBootstrapToken,
+                               std::string());
+
   // Internal or bookkeeping prefs.
   registry->RegisterStringPref(prefs::kSyncCacheGuid, std::string());
   registry->RegisterStringPref(prefs::kSyncBirthday, std::string());
@@ -301,8 +305,6 @@ void SyncPrefs::RegisterProfilePrefs(
   registry->RegisterInt64Pref(prefs::kSyncLastPollTime, 0);
   registry->RegisterInt64Pref(prefs::kSyncPollIntervalSeconds, 0);
   registry->RegisterBooleanPref(prefs::kSyncManaged, false);
-  registry->RegisterStringPref(prefs::kSyncEncryptionBootstrapToken,
-                               std::string());
   registry->RegisterStringPref(prefs::kSyncKeystoreEncryptionBootstrapToken,
                                std::string());
   registry->RegisterBooleanPref(prefs::kSyncPassphrasePrompted, false);
@@ -347,7 +349,7 @@ void SyncPrefs::RemoveSyncPrefObserver(SyncPrefObserver* sync_pref_observer) {
   sync_pref_observers_.RemoveObserver(sync_pref_observer);
 }
 
-void SyncPrefs::ClearPreferences() {
+void SyncPrefs::ClearLocalSyncTransportData() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // Clear user's birth year and gender.
@@ -357,31 +359,19 @@ void SyncPrefs::ClearPreferences() {
   // even reveal their true birth year.
   pref_service_->ClearPref(prefs::kSyncDemographics);
 
-  ClearDirectoryConsistencyPreferences();
-
   pref_service_->ClearPref(prefs::kSyncLastSyncedTime);
   pref_service_->ClearPref(prefs::kSyncLastPollTime);
   pref_service_->ClearPref(prefs::kSyncPollIntervalSeconds);
-  pref_service_->ClearPref(prefs::kSyncEncryptionBootstrapToken);
   pref_service_->ClearPref(prefs::kSyncKeystoreEncryptionBootstrapToken);
   pref_service_->ClearPref(prefs::kSyncPassphrasePrompted);
   pref_service_->ClearPref(prefs::kSyncInvalidationVersions);
   pref_service_->ClearPref(prefs::kSyncLastRunVersion);
-  // No need to clear kManaged, kEnableLocalSyncBackend or kLocalSyncBackendDir,
-  // since they're never actually set as user preferences.
-
-  // Note: We do *not* clear prefs which are directly user-controlled such as
-  // the set of selected types here, so that if the user ever chooses to enable
-  // Sync again, they start off with their previous settings by default.
-  // We do however require going through first-time setup again.
-  pref_service_->ClearPref(prefs::kSyncFirstSetupComplete);
-}
-
-void SyncPrefs::ClearDirectoryConsistencyPreferences() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   pref_service_->ClearPref(prefs::kSyncCacheGuid);
   pref_service_->ClearPref(prefs::kSyncBirthday);
   pref_service_->ClearPref(prefs::kSyncBagOfChips);
+
+  // No need to clear kManaged, kEnableLocalSyncBackend or kLocalSyncBackendDir,
+  // since they're never actually set as user preferences.
 }
 
 bool SyncPrefs::IsFirstSetupComplete() const {
@@ -394,6 +384,11 @@ void SyncPrefs::SetFirstSetupComplete() {
   pref_service_->SetBoolean(prefs::kSyncFirstSetupComplete, true);
 }
 
+void SyncPrefs::ClearFirstSetupComplete() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  pref_service_->ClearPref(prefs::kSyncFirstSetupComplete);
+}
+
 bool SyncPrefs::IsSyncRequested() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return pref_service_->GetBoolean(prefs::kSyncRequested);
@@ -556,6 +551,11 @@ void SyncPrefs::SetEncryptionBootstrapToken(const std::string& token) {
   pref_service_->SetString(prefs::kSyncEncryptionBootstrapToken, token);
 }
 
+void SyncPrefs::ClearEncryptionBootstrapToken() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  pref_service_->ClearPref(prefs::kSyncEncryptionBootstrapToken);
+}
+
 std::string SyncPrefs::GetKeystoreEncryptionBootstrapToken() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return pref_service_->GetString(prefs::kSyncKeystoreEncryptionBootstrapToken);

components/sync/base/sync_prefs.h

@@ -77,19 +77,15 @@ class SyncPrefs : public CryptoSyncPrefs,
   // Clears "bookkeeping" sync preferences, such as the last synced time,
   // whether the last shutdown was clean, etc. Does *not* clear sync preferences
   // which are directly user-controlled, such as the set of selected types.
-  void ClearPreferences();
-
-  // Clears only the subset of preferences that are redundant with the sync
-  // directory and used only for verifying consistency with prefs.
-  // TODO(crbug.com/923285): Remove this function and instead rely solely on
-  // ClearPreferences() once investigations are finalized are we understand the
-  // source of discrepancies for UMA Sync.DirectoryVsPrefsConsistency.
-  void ClearDirectoryConsistencyPreferences();
+  void ClearLocalSyncTransportData();
 
   // Getters and setters for global sync prefs.
 
+  // First-Setup-Complete is conceptually similar to the user's consent to
+  // enable sync-the-feature.
   bool IsFirstSetupComplete() const;
   void SetFirstSetupComplete();
+  void ClearFirstSetupComplete();
 
   bool IsSyncRequested() const;
   void SetSyncRequested(bool is_requested);
@@ -139,9 +135,14 @@ class SyncPrefs : public CryptoSyncPrefs,
   // policy, is handled directly in ProfileSyncService.
   bool IsManaged() const;
 
-  // Use this encryption bootstrap token if we're using an explicit passphrase.
+  // The encryption bootstrap token is used for explicit passphrase users
+  // (usually custom passphrase) and represents a user-entered passphrase.
+  // Hence, it gets treated as user-controlled similarly to sync datatype
+  // selection settings (i.e. doesn't get cleared in
+  // ClearLocalSyncTransportData()).
   std::string GetEncryptionBootstrapToken() const override;
   void SetEncryptionBootstrapToken(const std::string& token) override;
+  void ClearEncryptionBootstrapToken();
 
   // Use this keystore bootstrap token if we're not using an explicit
   // passphrase.
@@ -151,10 +152,6 @@ class SyncPrefs : public CryptoSyncPrefs,
   // Maps |type| to its corresponding preference name.
   static const char* GetPrefNameForType(UserSelectableType type);
 
-  // Copy of various fields historically owned and persisted by the Directory.
-  // This is a future-proof approach to ultimately replace the Directory once
-  // most users have populated prefs and the Directory is about to be removed.
-  // TODO(crbug.com/923287): Figure out if this is an appropriate place.
   void SetCacheGuid(const std::string& cache_guid);
   std::string GetCacheGuid() const;
   void SetBirthday(const std::string& birthday);

components/sync/base/sync_prefs_unittest.cc

@@ -122,9 +122,7 @@ TEST_F(SyncPrefsTest, ObservedPrefs) {
 
   sync_prefs_->SetFirstSetupComplete();
   EXPECT_TRUE(sync_prefs_->IsFirstSetupComplete());
-  // There's no direct way to clear the first-setup-complete bit, so just reset
-  // all prefs instead.
-  sync_prefs_->ClearPreferences();
+  sync_prefs_->ClearFirstSetupComplete();
   EXPECT_FALSE(sync_prefs_->IsFirstSetupComplete());
 
   sync_prefs_->SetSyncRequested(true);
@@ -148,24 +146,32 @@ TEST_F(SyncPrefsTest, SetSelectedOsTypesTriggersPreferredDataTypesPrefChange) {
 }
 #endif
 
-TEST_F(SyncPrefsTest, ClearPreferences) {
-  EXPECT_FALSE(sync_prefs_->IsFirstSetupComplete());
-  EXPECT_EQ(base::Time(), sync_prefs_->GetLastSyncedTime());
-  EXPECT_TRUE(sync_prefs_->GetEncryptionBootstrapToken().empty());
+TEST_F(SyncPrefsTest, ClearLocalSyncTransportData) {
+  ASSERT_FALSE(sync_prefs_->IsFirstSetupComplete());
+  ASSERT_EQ(base::Time(), sync_prefs_->GetLastSyncedTime());
+  ASSERT_TRUE(sync_prefs_->GetEncryptionBootstrapToken().empty());
 
   sync_prefs_->SetFirstSetupComplete();
   sync_prefs_->SetLastSyncedTime(base::Time::Now());
-  sync_prefs_->SetEncryptionBootstrapToken("token");
+  sync_prefs_->SetEncryptionBootstrapToken("explicit_passphrase_token");
+  sync_prefs_->SetKeystoreEncryptionBootstrapToken("keystore_token");
 
-  EXPECT_TRUE(sync_prefs_->IsFirstSetupComplete());
-  EXPECT_NE(base::Time(), sync_prefs_->GetLastSyncedTime());
-  EXPECT_EQ("token", sync_prefs_->GetEncryptionBootstrapToken());
+  ASSERT_TRUE(sync_prefs_->IsFirstSetupComplete());
+  ASSERT_NE(base::Time(), sync_prefs_->GetLastSyncedTime());
+  ASSERT_EQ("explicit_passphrase_token",
+            sync_prefs_->GetEncryptionBootstrapToken());
+  ASSERT_EQ("keystore_token",
+            sync_prefs_->GetKeystoreEncryptionBootstrapToken());
 
-  sync_prefs_->ClearPreferences();
+  sync_prefs_->ClearLocalSyncTransportData();
 
-  EXPECT_FALSE(sync_prefs_->IsFirstSetupComplete());
   EXPECT_EQ(base::Time(), sync_prefs_->GetLastSyncedTime());
-  EXPECT_TRUE(sync_prefs_->GetEncryptionBootstrapToken().empty());
+  EXPECT_TRUE(sync_prefs_->GetKeystoreEncryptionBootstrapToken().empty());
+
+  // User-entered field should not have been cleared.
+  EXPECT_TRUE(sync_prefs_->IsFirstSetupComplete());
+  EXPECT_EQ("explicit_passphrase_token",
+            sync_prefs_->GetEncryptionBootstrapToken());
 }
 
 TEST_F(SyncPrefsTest, ReadDemographicsWithRandomOffset) {
@@ -221,7 +227,7 @@ TEST_F(SyncPrefsTest, ReadAndClearUserDemographicPreferences) {
     ASSERT_TRUE(demographics_result.IsSuccess());
   }
 
-  sync_prefs_->ClearPreferences();
+  sync_prefs_->ClearLocalSyncTransportData();
 
   // Verify that demographics are not provided and kSyncDemographics is cleared.
   // Note that we retain kSyncDemographicsBirthYearOffset. If the user resumes

components/sync/driver/profile_sync_service.cc

@@ -516,9 +516,7 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
   // 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()) {
-    // TODO(crbug.com/923285): This doesn't seem to belong here, or if it does,
-    // all preferences should be cleared via SyncPrefs::ClearPreferences().
-    sync_prefs_.ClearDirectoryConsistencyPreferences();
+    sync_prefs_.ClearLocalSyncTransportData();
     sync_prefs_.SetCacheGuid(GenerateCacheGUID());
   }
 
@@ -625,6 +623,7 @@ void ProfileSyncService::ShutdownImpl(ShutdownReason reason) {
             base::BindOnce(&syncable::Directory::DeleteDirectoryFiles,
                            sync_client_->GetSyncDataPath()));
       }
+      sync_prefs_.ClearLocalSyncTransportData();
     }
     return;
   }
@@ -679,6 +678,10 @@ void ProfileSyncService::ShutdownImpl(ShutdownReason reason) {
     auth_manager_->ConnectionClosed();
   }
 
+  if (reason == ShutdownReason::DISABLE_SYNC) {
+    sync_prefs_.ClearLocalSyncTransportData();
+  }
+
   NotifyObservers();
 }
 
@@ -690,11 +693,15 @@ void ProfileSyncService::StopImpl(SyncStopDataFate data_fate) {
     case CLEAR_DATA:
       ClearUnrecoverableError();
       ShutdownImpl(DISABLE_SYNC);
-      // Clear prefs (including SyncSetupHasCompleted) before shutting down so
-      // PSS clients don't think we're set up while we're shutting down.
-      // Note: We do this after shutting down, so that notifications about the
-      // changed pref values don't mess up our state.
-      sync_prefs_.ClearPreferences();
+      // Note: ShutdownImpl(DISABLE_SYNC) does *not* clear prefs which are
+      // directly user-controlled such as the set of selected types here, so
+      // that if the user ever chooses to enable Sync again, they start off
+      // with their previous settings by default. We do however require going
+      // through first-time setup again.
+      sync_prefs_.ClearFirstSetupComplete();
+      // For explicit passphrase users, clear the encryption key, such that they
+      // will need to reenter it if sync gets re-enabled.
+      sync_prefs_.ClearEncryptionBootstrapToken();
       break;
   }
 }
@@ -854,15 +861,6 @@ void ProfileSyncService::OnUnrecoverableErrorImpl(
 
   // Shut all data types down.
   ShutdownImpl(DISABLE_SYNC);
-
-  // This is the equivalent for Directory::DeleteDirectoryFiles(), guaranteed
-  // to be called, either directly in ShutdownImpl(), or later in
-  // SyncEngineBackend::DoShutdown().
-  // TODO(crbug.com/923285): This doesn't seem to belong here, or if it does,
-  // all preferences should be cleared via SyncPrefs::ClearPreferences(),
-  // which is done by some of the callers (but not all). Care must be taken
-  // however for scenarios like custom passphrase being set.
-  sync_prefs_.ClearDirectoryConsistencyPreferences();
 }
 
 void ProfileSyncService::DataTypePreconditionChanged(ModelType type) {
@@ -1060,27 +1058,9 @@ void ProfileSyncService::OnActionableError(const SyncProtocolError& error) {
       // restart.
       sync_disabled_by_admin_ = true;
       ShutdownImpl(DISABLE_SYNC);
-      // This is the equivalent for Directory::DeleteDirectoryFiles(),
-      // guaranteed to be called, either directly in ShutdownImpl(), or later in
-      // SyncEngineBackend::DoShutdown().
-      // TODO(crbug.com/923285): This doesn't seem to belong here, or if it
-      // does, all preferences should be cleared via
-      // SyncPrefs::ClearPreferences(), which is done by some of the callers
-      // (but not all). Care must be taken however for scenarios like custom
-      // passphrase being set.
-      sync_prefs_.ClearDirectoryConsistencyPreferences();
       break;
     case RESET_LOCAL_SYNC_DATA:
       ShutdownImpl(DISABLE_SYNC);
-      // This is the equivalent for Directory::DeleteDirectoryFiles(),
-      // guaranteed to be called, either directly in ShutdownImpl(), or later in
-      // SyncEngineBackend::DoShutdown().
-      // TODO(crbug.com/923285): This doesn't seem to belong here, or if it
-      // does, all preferences should be cleared via
-      // SyncPrefs::ClearPreferences(), which is done by some of the callers
-      // (but not all). Care must be taken however for scenarios like custom
-      // passphrase being set.
-      sync_prefs_.ClearDirectoryConsistencyPreferences();
       startup_controller_->TryStart(/*force_immediate=*/true);
       break;
     case UNKNOWN_ACTION:

components/sync/driver/profile_sync_service_unittest.cc

@@ -54,6 +54,7 @@ namespace {
 constexpr int kOldEnoughForDemographicsUserBirthYear = 1983;
 
 constexpr char kTestUser[] = "test_user@gmail.com";
+constexpr char kTestCacheGuid[] = "test_cache_guid";
 
 // Now time in string format.
 constexpr char kNowTimeInStringFormat[] = "23 Mar 2019 16:00:00 UDT";
@@ -231,6 +232,8 @@ class ProfileSyncServiceTest : public ::testing::Test {
   void InitializeForNthSync() {
     // Set first sync time before initialize to simulate a complete sync setup.
     SyncPrefs sync_prefs(prefs());
+    sync_prefs.SetCacheGuid(kTestCacheGuid);
+    sync_prefs.SetBirthday(FakeSyncEngine::kTestBirthday);
     sync_prefs.SetLastSyncedTime(base::Time::Now());
     sync_prefs.SetSyncRequested(true);
     sync_prefs.SetSelectedTypes(
@@ -387,13 +390,18 @@ TEST_F(ProfileSyncServiceTest, NeedsConfirmation) {
   CreateService(ProfileSyncService::MANUAL_START);
 
   SyncPrefs sync_prefs(prefs());
-  base::Time now = base::Time::Now();
-  sync_prefs.SetLastSyncedTime(now);
   sync_prefs.SetSyncRequested(true);
   sync_prefs.SetSelectedTypes(
       /*keep_everything_synced=*/true,
       /*registered_types=*/UserSelectableTypeSet::All(),
       /*selected_types=*/UserSelectableTypeSet::All());
+
+  // Mimic a sync cycle (transport-only) having completed earlier.
+  const base::Time kLastSyncedTime = base::Time::Now();
+  sync_prefs.SetLastSyncedTime(kLastSyncedTime);
+  sync_prefs.SetCacheGuid(kTestCacheGuid);
+  sync_prefs.SetBirthday(FakeSyncEngine::kTestBirthday);
+
   service()->Initialize();
 
   EXPECT_EQ(SyncService::DisableReasonSet(), service()->GetDisableReasons());
@@ -404,10 +412,9 @@ TEST_F(ProfileSyncServiceTest, NeedsConfirmation) {
   EXPECT_FALSE(service()->IsSyncFeatureActive());
   EXPECT_FALSE(service()->IsSyncFeatureEnabled());
 
-  // The last sync time shouldn't be cleared.
-  // TODO(zea): figure out a way to check that the directory itself wasn't
-  // cleared.
-  EXPECT_EQ(now, sync_prefs.GetLastSyncedTime());
+  // The local sync data shouldn't be cleared.
+  EXPECT_EQ(kTestCacheGuid, sync_prefs.GetCacheGuid());
+  EXPECT_EQ(kLastSyncedTime, sync_prefs.GetLastSyncedTime());
 }
 
 // Verify that the SetSetupInProgress function call updates state