Fix sync metadata being cleared upon restart for secondary accounts

The way ProfileSyncService determines whether the user is signed in
depends on refresh tokens being loaded, for secondary accounts.

This means, DISABLE_REASON_NOT_SIGNED_IN can report false positives
during startup, and should not be honored to clear sync metadata, until
refresh tokens are loaded and the signed-out state is determined
reliably.

This fixes cache GUIDs being reset upon every restart for secondary
accounts.

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

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

@@ -3,7 +3,9 @@
 // found in the LICENSE file.
 
 #include "base/callback_list.h"
+#include "base/files/file_util.h"
 #include "base/macros.h"
+#include "base/path_service.h"
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "chrome/browser/defaults.h"
@@ -12,6 +14,7 @@
 #include "chrome/browser/sync/test/integration/secondary_account_helper.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"
@@ -28,7 +31,13 @@ syncer::ModelTypeSet AllowedTypesInStandaloneTransportMode() {
   allowed_types.PutAll(syncer::ControlTypes());
   return allowed_types;
 }
-#endif
+
+base::FilePath GetTestFilePathForCacheGuid() {
+  base::FilePath user_data_path;
+  base::PathService::Get(chrome::DIR_USER_DATA, &user_data_path);
+  return user_data_path.AppendASCII("SyncTestTmpCacheGuid");
+}
+#endif  // !defined(OS_CHROMEOS)
 
 class SingleClientSecondaryAccountSyncTest : public SyncTest {
  public:
@@ -149,4 +158,61 @@ IN_PROC_BROWSER_TEST_F(SingleClientSecondaryAccountSyncTest,
 }
 #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 with
+// secondary accounts.
+//
+// The unconsented primary account (aka secondary account) isn't supported on
+// ChromeOS, see IdentityManager::ComputeUnconsentedPrimaryAccountInfo().
+#if !defined(OS_CHROMEOS)
+IN_PROC_BROWSER_TEST_F(SingleClientSecondaryAccountSyncTest,
+                       PRE_ReusesSameCacheGuid) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+  secondary_account_helper::SignInSecondaryAccount(
+      profile(), &test_url_loader_factory_, "user@email.com");
+  ASSERT_TRUE(GetClient(0)->AwaitSyncTransportActive());
+
+  ASSERT_EQ(syncer::SyncService::TransportState::ACTIVE,
+            GetSyncService(0)->GetTransportState());
+
+  ASSERT_FALSE(GetSyncService(0)->GetUserSettings()->IsFirstSetupComplete());
+  ASSERT_FALSE(GetSyncService(0)->IsSyncFeatureEnabled());
+
+  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.
+  ASSERT_NE(-1, base::WriteFile(GetTestFilePathForCacheGuid(),
+                                cache_guid.c_str(), cache_guid.size()));
+}
+#endif  // !defined(OS_CHROMEOS)
+
+// The unconsented primary account (aka secondary account) isn't supported on
+// ChromeOS, see IdentityManager::ComputeUnconsentedPrimaryAccountInfo().
+#if !defined(OS_CHROMEOS)
+IN_PROC_BROWSER_TEST_F(SingleClientSecondaryAccountSyncTest,
+                       ReusesSameCacheGuid) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+  ASSERT_TRUE(GetClient(0)->AwaitSyncTransportActive());
+
+  ASSERT_EQ(syncer::SyncService::TransportState::ACTIVE,
+            GetSyncService(0)->GetTransportState());
+
+  ASSERT_FALSE(GetSyncService(0)->GetUserSettings()->IsFirstSetupComplete());
+  ASSERT_FALSE(GetSyncService(0)->IsSyncFeatureEnabled());
+
+  syncer::SyncPrefs prefs(GetProfile(0)->GetPrefs());
+  ASSERT_FALSE(prefs.GetCacheGuid().empty());
+
+  std::string old_cache_guid;
+  ASSERT_TRUE(
+      base::ReadFileToString(GetTestFilePathForCacheGuid(), &old_cache_guid));
+  ASSERT_FALSE(old_cache_guid.empty());
+
+  EXPECT_EQ(old_cache_guid, prefs.GetCacheGuid());
+}
+#endif  // !defined(OS_CHROMEOS)
+
 }  // namespace

components/sync/driver/profile_sync_service.cc

@@ -235,7 +235,8 @@ void ProfileSyncService::Initialize() {
   // 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)) {
+      (HasDisableReason(DISABLE_REASON_NOT_SIGNED_IN) &&
+       auth_manager_->IsActiveAccountInfoFullyLoaded())) {
     StopImpl(CLEAR_DATA);
   }
 

components/sync/driver/sync_auth_manager.cc

@@ -83,6 +83,13 @@ void SyncAuthManager::RegisterForAuthNotifications() {
   sync_account_ = DetermineAccountToUse();
 }
 
+bool SyncAuthManager::IsActiveAccountInfoFullyLoaded() const {
+  // The result of DetermineAccountToUse() is influenced by refresh tokens being
+  // loaded due to how IdentityManager::ComputeUnconsentedPrimaryAccountInfo()
+  // is implemented, which requires a refresh token.
+  return identity_manager_->AreRefreshTokensLoaded();
+}
+
 SyncAccountInfo SyncAuthManager::GetActiveAccountInfo() const {
   // Note: |sync_account_| should generally be identical to the result of a
   // DetermineAccountToUse() call, but there are a few edge cases when it isn't:
@@ -369,6 +376,24 @@ void SyncAuthManager::OnRefreshTokenRemovedForAccount(
   credentials_changed_callback_.Run();
 }
 
+void SyncAuthManager::OnRefreshTokensLoaded() {
+  DCHECK(IsActiveAccountInfoFullyLoaded());
+
+  if (UpdateSyncAccountIfNecessary()) {
+    // |account_state_changed_callback_| has already been called, no need to
+    // consider calling it again.
+    return;
+  }
+
+  if (sync_account_.account_info.account_id.empty()) {
+    // Nothing actually changed, so |account_state_changed_callback_| hasn't
+    // been called yet. However, this is the first time we can reliably tell the
+    // user is signed out, exposed via IsActiveAccountInfoFullyLoaded(), so
+    // let's treat it as account state change.
+    account_state_changed_callback_.Run();
+  }
+}
+
 void SyncAuthManager::OnAccountsInCookieUpdated(
     const signin::AccountsInCookieJarInfo& accounts_in_cookie_jar_info,
     const GoogleServiceAuthError& error) {

components/sync/driver/sync_auth_manager.h

@@ -60,6 +60,10 @@ class SyncAuthManager : public signin::IdentityManager::Observer {
   // callbacks, even if there is an active account afterwards.
   void RegisterForAuthNotifications();
 
+  // Returns whether all relevant account information as returned by
+  // GetActiveAccountInfo() has been fully loaded.
+  bool IsActiveAccountInfoFullyLoaded() const;
+
   // Returns the account which should be used when communicating with the Sync
   // server. Note that this account may not be blessed for Sync-the-feature.
   SyncAccountInfo GetActiveAccountInfo() const;
@@ -107,6 +111,7 @@ class SyncAuthManager : public signin::IdentityManager::Observer {
       const CoreAccountInfo& account_info) override;
   void OnRefreshTokenRemovedForAccount(
       const CoreAccountId& account_id) override;
+  void OnRefreshTokensLoaded() override;
   void OnAccountsInCookieUpdated(
       const signin::AccountsInCookieJarInfo& accounts_in_cookie_jar_info,
       const GoogleServiceAuthError& error) override;
@@ -122,6 +127,7 @@ class SyncAuthManager : public signin::IdentityManager::Observer {
   // DetermineAccountToUse) if necessary, and notifies observers of any changes
   // (sign-in/sign-out/"primary" bit change). Note that changing from one
   // account to another is exposed to observers as a sign-out + sign-in.
+  // Returns whether the syncing account was updated.
   bool UpdateSyncAccountIfNecessary();
 
   // Invalidates any current access token, which means invalidating it with the