Fix sync metadata being cleared on every transport-mode restart

Using IsFirstSetupComplete() to determine whether sync metadata should
be cleared does not work well for transport-only mode, where sync the
feature itself is not enabled and hence IsFirstSetupComplete() actually
never becomes true.

Therefore, prior to this patch, sync metadata (including the cache GUID)
were cleared on every browser restart, if in transport mode.

Instead, we leverage sync disable reasons to determine whether metadata
should be cleared.

Bug: 955989
Change-Id: I63d2b1e682193cd981c7cef72581a4c6e4057e49
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715822
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683284}

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

@@ -2,14 +2,18 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/files/file_util.h"
 #include "base/macros.h"
+#include "base/path_service.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
+#include "chrome/common/chrome_paths.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/driver/profile_sync_service.h"
 #include "components/sync/driver/sync_driver_switches.h"
@@ -25,6 +29,12 @@ syncer::ModelTypeSet AllowedTypesInStandaloneTransportMode() {
   return allowed_types;
 }
 
+base::FilePath GetTestFilePathForCacheGuid() {
+  base::FilePath user_data_path;
+  base::PathService::Get(chrome::DIR_USER_DATA, &user_data_path);
+  return user_data_path.AppendASCII("SyncTestTmpCacheGuid");
+}
+
 class SyncDisabledByUserChecker : public SingleClientStatusChangeChecker {
  public:
   explicit SyncDisabledByUserChecker(syncer::ProfileSyncService* service)
@@ -42,7 +52,10 @@ class SyncDisabledByUserChecker : public SingleClientStatusChangeChecker {
 
 class SingleClientStandaloneTransportSyncTest : public SyncTest {
  public:
-  SingleClientStandaloneTransportSyncTest() : SyncTest(SINGLE_CLIENT) {}
+  SingleClientStandaloneTransportSyncTest() : SyncTest(SINGLE_CLIENT) {
+    DisableVerifier();
+  }
+
   ~SingleClientStandaloneTransportSyncTest() override {}
 
  private:
@@ -209,4 +222,64 @@ IN_PROC_BROWSER_TEST_F(SingleClientStandaloneTransportSyncTest,
 #endif  // defined(OS_CHROMEOS)
 }
 
+// Regression test for crbug.com/955989 that verifies the cache GUID is not
+// reset upon restart of the browser, in standalone transport mode.
+IN_PROC_BROWSER_TEST_F(SingleClientStandaloneTransportSyncTest,
+                       PRE_ReusesSameCacheGuid) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+  ASSERT_TRUE(GetClient(0)->SignInPrimaryAccount());
+  ASSERT_TRUE(GetClient(0)->AwaitSyncTransportActive());
+
+  ASSERT_EQ(syncer::SyncService::TransportState::ACTIVE,
+            GetSyncService(0)->GetTransportState());
+
+  // On platforms where Sync starts automatically (in practice, Android and
+  // ChromeOS), IsFirstSetupComplete gets set automatically, and so the full
+  // Sync feature will start upon sign-in to a primary account.
+#if !defined(OS_CHROMEOS)
+  ASSERT_FALSE(GetSyncService(0)->GetUserSettings()->IsFirstSetupComplete());
+  ASSERT_FALSE(GetSyncService(0)->IsSyncFeatureEnabled());
+#endif  // !defined(OS_CHROMEOS)
+
+  syncer::SyncPrefs prefs(GetProfile(0)->GetPrefs());
+  const std::string cache_guid = prefs.GetCacheGuid();
+  ASSERT_FALSE(cache_guid.empty());
+
+  // Save the cache GUID to file to remember after restart, for test
+  // verification purposes only.
+  base::ScopedAllowBlockingForTesting allow_blocking;
+  ASSERT_NE(-1, base::WriteFile(GetTestFilePathForCacheGuid(),
+                                cache_guid.c_str(), cache_guid.size()));
+}
+
+IN_PROC_BROWSER_TEST_F(SingleClientStandaloneTransportSyncTest,
+                       ReusesSameCacheGuid) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+  ASSERT_FALSE(GetSyncService(0)->HasDisableReason(
+      syncer::SyncService::DISABLE_REASON_NOT_SIGNED_IN));
+  ASSERT_TRUE(GetClient(0)->AwaitSyncTransportActive());
+
+  ASSERT_EQ(syncer::SyncService::TransportState::ACTIVE,
+            GetSyncService(0)->GetTransportState());
+
+  // On platforms where Sync starts automatically (in practice, Android and
+  // ChromeOS), IsFirstSetupComplete gets set automatically, and so the full
+  // Sync feature will start upon sign-in to a primary account.
+#if !defined(OS_CHROMEOS)
+  ASSERT_FALSE(GetSyncService(0)->GetUserSettings()->IsFirstSetupComplete());
+  ASSERT_FALSE(GetSyncService(0)->IsSyncFeatureEnabled());
+#endif  // !defined(OS_CHROMEOS)
+
+  syncer::SyncPrefs prefs(GetProfile(0)->GetPrefs());
+  ASSERT_FALSE(prefs.GetCacheGuid().empty());
+
+  std::string old_cache_guid;
+  base::ScopedAllowBlockingForTesting allow_blocking;
+  ASSERT_TRUE(
+      base::ReadFileToString(GetTestFilePathForCacheGuid(), &old_cache_guid));
+  ASSERT_FALSE(old_cache_guid.empty());
+
+  EXPECT_EQ(old_cache_guid, prefs.GetCacheGuid());
+}
+
 }  // namespace

components/sync/driver/profile_sync_service.cc

@@ -221,13 +221,6 @@ void ProfileSyncService::Initialize() {
 
   sync_prefs_.AddSyncPrefObserver(this);
 
-  // If sync is disallowed by policy, clean up.
-  if (HasDisableReason(DISABLE_REASON_ENTERPRISE_POLICY)) {
-    // Note that this won't actually clear data, since neither |engine_| nor
-    // |sync_thread_| exist at this point. Bug or feature?
-    StopImpl(CLEAR_DATA);
-  }
-
   if (!IsLocalSyncEnabled()) {
     auth_manager_->RegisterForAuthNotifications();
     for (auto* provider : invalidations_identity_providers_) {
@@ -235,11 +228,13 @@ void ProfileSyncService::Initialize() {
         provider->SetActiveAccountId(GetAuthenticatedAccountInfo().account_id);
       }
     }
+  }
 
-    if (!IsSignedIn()) {
-      // Clean up in case of previous crash during signout.
-      StopImpl(CLEAR_DATA);
-    }
+  // If sync is disabled permanently, clean up old data that may be around (e.g.
+  // crash during signout).
+  if (HasDisableReason(DISABLE_REASON_ENTERPRISE_POLICY) ||
+      HasDisableReason(DISABLE_REASON_NOT_SIGNED_IN)) {
+    StopImpl(CLEAR_DATA);
   }
 
   // Note: We need to record the initial state *after* calling
@@ -423,6 +418,19 @@ void ProfileSyncService::OnDataTypeRequestsSyncStartup(ModelType type) {
   startup_controller_->OnDataTypeRequestsSyncStartup(type);
 }
 
+void ProfileSyncService::StartSyncThreadIfNeeded() {
+  if (sync_thread_) {
+    // Already started.
+    return;
+  }
+
+  sync_thread_ = std::make_unique<base::Thread>("Chrome_SyncThread");
+  base::Thread::Options options;
+  options.timer_slack = base::TIMER_SLACK_MAXIMUM;
+  bool success = sync_thread_->StartWithOptions(options);
+  DCHECK(success);
+}
+
 void ProfileSyncService::StartUpSlowEngineComponents() {
   DCHECK(IsEngineAllowedToStart());
 
@@ -435,13 +443,7 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
     last_actionable_error_ = SyncProtocolError();
   }
 
-  if (!sync_thread_) {
-    sync_thread_ = std::make_unique<base::Thread>("Chrome_SyncThread");
-    base::Thread::Options options;
-    options.timer_slack = base::TIMER_SLACK_MAXIMUM;
-    bool success = sync_thread_->StartWithOptions(options);
-    DCHECK(success);
-  }
+  StartSyncThreadIfNeeded();
 
   SyncEngine::InitParams params;
   params.sync_task_runner = sync_thread_->task_runner();
@@ -467,18 +469,6 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
   }
   params.sync_manager_factory =
       std::make_unique<SyncManagerFactory>(network_connection_tracker_);
-  // The first time we start up the engine we want to ensure we have a clean
-  // directory, so delete any old one that might be there.
-  params.delete_sync_data_folder = !user_settings_->IsFirstSetupComplete();
-  if (params.delete_sync_data_folder) {
-    // This looks questionable here but it mimics the old behavior of deleting
-    // the directory via Directory::DeleteDirectoryFiles(). One consecuence is
-    // that, for sync the transport users (without sync-the-feature enabled),
-    // the cache GUID and other fields are reset on every restart.
-    // TODO(crbug.com/923285): Reconsider the lifetime of the cache GUID and
-    // its persistence depending on StorageOption.
-    sync_prefs_.ClearDirectoryConsistencyPreferences();
-  }
   params.enable_local_sync_backend = sync_prefs_.IsLocalSyncEnabled();
   params.local_sync_backend_folder = sync_client_->GetLocalSyncBackendFolder();
   params.restored_key_for_bootstrapping =
@@ -533,12 +523,24 @@ void ProfileSyncService::Shutdown() {
 
 void ProfileSyncService::ShutdownImpl(ShutdownReason reason) {
   if (!engine_) {
-    if (reason == ShutdownReason::DISABLE_SYNC && sync_thread_) {
-      // If the engine is already shut down when a DISABLE_SYNC happens,
-      // the data directory needs to be cleaned up here.
-      sync_thread_->task_runner()->PostTask(
-          FROM_HERE, base::BindOnce(&syncable::Directory::DeleteDirectoryFiles,
-                                    sync_client_->GetSyncDataPath()));
+    // If the engine hasn't started or is already shut down when a DISABLE_SYNC
+    // happens, the data directory needs to be cleaned up here.
+    if (reason == ShutdownReason::DISABLE_SYNC) {
+      // Clearing the Directory via Directory::DeleteDirectoryFiles() requires
+      // the |sync_thread_| initialized. It also means there's IO involved which
+      // may we considerable overhead if triggered consistently upon browser
+      // startup (which is the case for certain codepaths such as the user being
+      // signed out). To avoid that, SyncPrefs is used to determine whether it's
+      // worth.
+      if (!sync_prefs_.GetCacheGuid().empty()) {
+        StartSyncThreadIfNeeded();
+      }
+      if (sync_thread_) {
+        sync_thread_->task_runner()->PostTask(
+            FROM_HERE,
+            base::BindOnce(&syncable::Directory::DeleteDirectoryFiles,
+                           sync_client_->GetSyncDataPath()));
+      }
     }
     return;
   }

components/sync/driver/profile_sync_service.h

@@ -331,6 +331,9 @@ class ProfileSyncService : public SyncService,
 
   void ClearUnrecoverableError();
 
+  // Initializes and starts |sync_thread_|.
+  void StartSyncThreadIfNeeded();
+
   // Kicks off asynchronous initialization of the SyncEngine.
   void StartUpSlowEngineComponents();
 

components/sync/engine/sync_engine.h

@@ -65,6 +65,7 @@ class SyncEngine : public ModelTypeConfigurer {
     CoreAccountId authenticated_account_id;
     std::string invalidator_client_id;
     std::unique_ptr<SyncManagerFactory> sync_manager_factory;
+    // TODO(crbug.com/955989): Remove since it's unused.
     bool delete_sync_data_folder = false;
     bool enable_local_sync_backend = false;
     base::FilePath local_sync_backend_folder;