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}

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

@@ -54,8 +54,7 @@ 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,
-                       PRE_ShouldUsePollIntervalFromPrefs) {
+IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest, ShouldUpdatePollPrefs) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   sync_pb::ClientCommand client_command;
@@ -77,12 +76,17 @@ IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest,
 
 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(67));
+              Eq(123));
 }
 
 // This test simulates the poll interval expiring between restarts.

components/sync/base/sync_prefs.cc

@@ -293,10 +293,6 @@ 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());
@@ -305,6 +301,8 @@ 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);
@@ -349,7 +347,7 @@ void SyncPrefs::RemoveSyncPrefObserver(SyncPrefObserver* sync_pref_observer) {
   sync_pref_observers_.RemoveObserver(sync_pref_observer);
 }
 
-void SyncPrefs::ClearLocalSyncTransportData() {
+void SyncPrefs::ClearPreferences() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // Clear user's birth year and gender.
@@ -359,19 +357,31 @@ void SyncPrefs::ClearLocalSyncTransportData() {
   // 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 {
@@ -384,11 +394,6 @@ 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);
@@ -551,11 +556,6 @@ 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,15 +77,19 @@ 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 ClearLocalSyncTransportData();
+  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();
 
   // 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);
@@ -135,14 +139,9 @@ class SyncPrefs : public CryptoSyncPrefs,
   // policy, is handled directly in ProfileSyncService.
   bool IsManaged() const;
 
-  // 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()).
+  // Use this encryption bootstrap token if we're using an explicit passphrase.
   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.
@@ -152,6 +151,10 @@ 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,7 +122,9 @@ TEST_F(SyncPrefsTest, ObservedPrefs) {
 
   sync_prefs_->SetFirstSetupComplete();
   EXPECT_TRUE(sync_prefs_->IsFirstSetupComplete());
-  sync_prefs_->ClearFirstSetupComplete();
+  // There's no direct way to clear the first-setup-complete bit, so just reset
+  // all prefs instead.
+  sync_prefs_->ClearPreferences();
   EXPECT_FALSE(sync_prefs_->IsFirstSetupComplete());
 
   sync_prefs_->SetSyncRequested(true);
@@ -146,32 +148,24 @@ TEST_F(SyncPrefsTest, SetSelectedOsTypesTriggersPreferredDataTypesPrefChange) {
 }
 #endif
 
-TEST_F(SyncPrefsTest, ClearLocalSyncTransportData) {
-  ASSERT_FALSE(sync_prefs_->IsFirstSetupComplete());
-  ASSERT_EQ(base::Time(), sync_prefs_->GetLastSyncedTime());
-  ASSERT_TRUE(sync_prefs_->GetEncryptionBootstrapToken().empty());
+TEST_F(SyncPrefsTest, ClearPreferences) {
+  EXPECT_FALSE(sync_prefs_->IsFirstSetupComplete());
+  EXPECT_EQ(base::Time(), sync_prefs_->GetLastSyncedTime());
+  EXPECT_TRUE(sync_prefs_->GetEncryptionBootstrapToken().empty());
 
   sync_prefs_->SetFirstSetupComplete();
   sync_prefs_->SetLastSyncedTime(base::Time::Now());
-  sync_prefs_->SetEncryptionBootstrapToken("explicit_passphrase_token");
-  sync_prefs_->SetKeystoreEncryptionBootstrapToken("keystore_token");
+  sync_prefs_->SetEncryptionBootstrapToken("token");
 
-  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());
+  EXPECT_TRUE(sync_prefs_->IsFirstSetupComplete());
+  EXPECT_NE(base::Time(), sync_prefs_->GetLastSyncedTime());
+  EXPECT_EQ("token", sync_prefs_->GetEncryptionBootstrapToken());
 
-  sync_prefs_->ClearLocalSyncTransportData();
+  sync_prefs_->ClearPreferences();
 
+  EXPECT_FALSE(sync_prefs_->IsFirstSetupComplete());
   EXPECT_EQ(base::Time(), sync_prefs_->GetLastSyncedTime());
-  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());
+  EXPECT_TRUE(sync_prefs_->GetEncryptionBootstrapToken().empty());
 }
 
 TEST_F(SyncPrefsTest, ReadDemographicsWithRandomOffset) {
@@ -227,7 +221,7 @@ TEST_F(SyncPrefsTest, ReadAndClearUserDemographicPreferences) {
     ASSERT_TRUE(demographics_result.IsSuccess());
   }
 
-  sync_prefs_->ClearLocalSyncTransportData();
+  sync_prefs_->ClearPreferences();
 
   // 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

@@ -518,7 +518,9 @@ 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()) {
-    sync_prefs_.ClearLocalSyncTransportData();
+    // 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_.SetCacheGuid(GenerateCacheGUID());
   }
 
@@ -625,7 +627,6 @@ void ProfileSyncService::ShutdownImpl(ShutdownReason reason) {
             base::BindOnce(&syncable::Directory::DeleteDirectoryFiles,
                            sync_client_->GetSyncDataPath()));
       }
-      sync_prefs_.ClearLocalSyncTransportData();
     }
     return;
   }
@@ -680,10 +681,6 @@ void ProfileSyncService::ShutdownImpl(ShutdownReason reason) {
     auth_manager_->ConnectionClosed();
   }
 
-  if (reason == ShutdownReason::DISABLE_SYNC) {
-    sync_prefs_.ClearLocalSyncTransportData();
-  }
-
   NotifyObservers();
 }
 
@@ -695,15 +692,11 @@ void ProfileSyncService::StopImpl(SyncStopDataFate data_fate) {
     case CLEAR_DATA:
       ClearUnrecoverableError();
       ShutdownImpl(DISABLE_SYNC);
-      // 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();
+      // 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();
       break;
   }
 }
@@ -863,6 +856,15 @@ 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,9 +1062,27 @@ 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,7 +54,6 @@ 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";
@@ -232,8 +231,6 @@ 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(
@@ -390,18 +387,13 @@ 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());
@@ -412,9 +404,10 @@ TEST_F(ProfileSyncServiceTest, NeedsConfirmation) {
   EXPECT_FALSE(service()->IsSyncFeatureActive());
   EXPECT_FALSE(service()->IsSyncFeatureEnabled());
 
-  // The local sync data shouldn't be cleared.
-  EXPECT_EQ(kTestCacheGuid, sync_prefs.GetCacheGuid());
-  EXPECT_EQ(kLastSyncedTime, sync_prefs.GetLastSyncedTime());
+  // 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());
 }
 
 // Verify that the SetSetupInProgress function call updates state