Make it so that the account reconcilor (or any other component that w…

…ants to react to token service changes in batches) only triggers its actions once all token changes are applied.

BUG=368903

Review URL: https://codereview.chromium.org/340533004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@281331 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/signin/account_reconcilor_unittest.cc

@@ -763,6 +763,41 @@ TEST_P(AccountReconcilorTest, StartReconcileWithSessionInfoExpiredDefault) {
   ASSERT_FALSE(reconcilor->is_reconcile_started_);
 }
 
+TEST_F(AccountReconcilorTest, MergeSessionCompletedWithBogusAccount) {
+  signin_manager()->SetAuthenticatedUsername("user@gmail.com");
+  token_service()->UpdateCredentials("user@gmail.com", "refresh_token");
+
+  EXPECT_CALL(*GetMockReconcilor(), PerformMergeAction("user@gmail.com"));
+
+  SetFakeResponse(GaiaUrls::GetInstance()->list_accounts_url().spec(),
+      "[\"f\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0, 0, 0]]]",
+      net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+  SetFakeResponse("https://www.googleapis.com/oauth2/v1/userinfo",
+      "{\"id\":\"foo\"}", net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+
+  AccountReconcilor* reconcilor =
+      AccountReconcilorFactory::GetForProfile(profile());
+  ASSERT_TRUE(reconcilor);
+
+  ASSERT_FALSE(reconcilor->is_reconcile_started_);
+  reconcilor->StartReconcile();
+  ASSERT_TRUE(reconcilor->is_reconcile_started_);
+
+  token_service()->IssueAllTokensForAccount("user@gmail.com", "access_token",
+      base::Time::Now() + base::TimeDelta::FromHours(1));
+  base::RunLoop().RunUntilIdle();
+
+  // If an unknown account id is sent, it should not upset the state.
+  SimulateMergeSessionCompleted(reconcilor, "bogus@gmail.com",
+                                GoogleServiceAuthError::AuthErrorNone());
+  ASSERT_TRUE(reconcilor->is_reconcile_started_);
+
+  SimulateMergeSessionCompleted(reconcilor, "user@gmail.com",
+                                GoogleServiceAuthError::AuthErrorNone());
+  ASSERT_FALSE(reconcilor->is_reconcile_started_);
+}
+
 INSTANTIATE_TEST_CASE_P(AccountReconcilorMaybeEnabled,
                         AccountReconcilorTest,
                         testing::Bool());
+

chrome/browser/signin/android_profile_oauth2_token_service.cc

@@ -212,6 +212,8 @@ void AndroidProfileOAuth2TokenService::ValidateAccounts(
     curr_ids.clear();
   }
 
+  ScopedBacthChange batch(this);
+
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jobjectArray> java_accounts(
       base::android::ToJavaArrayOfStrings(env, curr_ids));
@@ -363,6 +365,7 @@ void AndroidProfileOAuth2TokenService::FireRefreshTokensLoaded() {
 
 void AndroidProfileOAuth2TokenService::RevokeAllCredentials() {
   VLOG(1) << "AndroidProfileOAuth2TokenService::RevokeAllCredentials";
+  ScopedBacthChange batch(this);
   std::vector<std::string> accounts = GetAccounts();
   for (std::vector<std::string>::iterator it = accounts.begin();
        it != accounts.end(); it++) {

chrome/browser/signin/fake_profile_oauth2_token_service.cc

@@ -57,6 +57,7 @@ void FakeProfileOAuth2TokenService::IssueRefreshToken(
 void FakeProfileOAuth2TokenService::IssueRefreshTokenForUser(
     const std::string& account_id,
     const std::string& token) {
+  ScopedBacthChange batch(this);
   if (token.empty()) {
     refresh_tokens_.erase(account_id);
     FireRefreshTokenRevoked(account_id);

components/signin/core/browser/account_reconcilor.cc

@@ -332,17 +332,15 @@ void AccountReconcilor::OnCookieChanged(const net::CanonicalCookie* cookie) {
   }
 }
 
-void AccountReconcilor::OnRefreshTokenAvailable(const std::string& account_id) {
-  VLOG(1) << "AccountReconcilor::OnRefreshTokenAvailable: " << account_id;
-  StartReconcile();
-}
-
 void AccountReconcilor::OnRefreshTokenRevoked(const std::string& account_id) {
   VLOG(1) << "AccountReconcilor::OnRefreshTokenRevoked: " << account_id;
   PerformStartRemoveAction(account_id);
 }
 
-void AccountReconcilor::OnRefreshTokensLoaded() {}
+void AccountReconcilor::OnEndBatchChanges() {
+  VLOG(1) << "AccountReconcilor::OnEndBatchChanges";
+  StartReconcile();
+}
 
 void AccountReconcilor::GoogleSigninSucceeded(const std::string& username,
                                               const std::string& password) {
@@ -426,7 +424,9 @@ void AccountReconcilor::PerformLogoutAllAccountsAction() {
 }
 
 void AccountReconcilor::StartReconcile() {
-  if (!IsProfileConnected() || is_reconcile_started_)
+  if (!IsProfileConnected() || is_reconcile_started_ ||
+      get_gaia_accounts_callbacks_.size() > 0 ||
+      merge_session_helper_.is_running())
     return;
 
   is_reconcile_started_ = true;
@@ -529,7 +529,6 @@ void AccountReconcilor::ValidateAccountsFromTokenService() {
   DCHECK(!primary_account_.empty());
 
   chrome_accounts_ = token_service_->GetAccounts();
-  DCHECK_GT(chrome_accounts_.size(), 0u);
 
   VLOG(1) << "AccountReconcilor::ValidateAccountsFromTokenService: "
           << "Chrome " << chrome_accounts_.size() << " accounts, "
@@ -722,16 +721,17 @@ void AccountReconcilor::ScheduleStartReconcileIfChromeAccountsChanged() {
 }
 
 // Remove the account from the list that is being merged.
-void AccountReconcilor::MarkAccountAsAddedToCookie(
+bool AccountReconcilor::MarkAccountAsAddedToCookie(
     const std::string& account_id) {
   for (std::vector<std::string>::iterator i = add_to_cookie_.begin();
        i != add_to_cookie_.end();
        ++i) {
     if (account_id == *i) {
       add_to_cookie_.erase(i);
-      break;
+      return true;
     }
   }
+  return false;
 }
 
 void AccountReconcilor::MergeSessionCompleted(
@@ -740,9 +740,10 @@ void AccountReconcilor::MergeSessionCompleted(
   VLOG(1) << "AccountReconcilor::MergeSessionCompleted: account_id="
           << account_id;
 
-  MarkAccountAsAddedToCookie(account_id);
-  CalculateIfReconcileIsDone();
-  ScheduleStartReconcileIfChromeAccountsChanged();
+  if (MarkAccountAsAddedToCookie(account_id)) {
+    CalculateIfReconcileIsDone();
+    ScheduleStartReconcileIfChromeAccountsChanged();
+  }
 }
 
 void AccountReconcilor::HandleSuccessfulAccountIdCheck(

components/signin/core/browser/account_reconcilor.h

@@ -128,6 +128,8 @@ class AccountReconcilor : public KeyedService,
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileOnlyOnce);
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
                            StartReconcileWithSessionInfoExpiredDefault);
+  FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
+                           MergeSessionCompletedWithBogusAccount);
 
   // Register and unregister with dependent services.
   void RegisterForCookieChanges();
@@ -177,7 +179,7 @@ class AccountReconcilor : public KeyedService,
       const std::vector<std::pair<std::string, bool> >& accounts);
   void ValidateAccountsFromTokenService();
   // Note internally that this |account_id| is added to the cookie jar.
-  void MarkAccountAsAddedToCookie(const std::string& account_id);
+  bool MarkAccountAsAddedToCookie(const std::string& account_id);
   // Note internally that this |account_id| is added to the token service.
   void MarkAccountAsAddedToChrome(const std::string& account_id);
 
@@ -201,9 +203,8 @@ class AccountReconcilor : public KeyedService,
                                  const GoogleServiceAuthError& error) OVERRIDE;
 
   // Overriden from OAuth2TokenService::Observer.
-  virtual void OnRefreshTokenAvailable(const std::string& account_id) OVERRIDE;
   virtual void OnRefreshTokenRevoked(const std::string& account_id) OVERRIDE;
-  virtual void OnRefreshTokensLoaded() OVERRIDE;
+  virtual void OnEndBatchChanges() OVERRIDE;
 
   // Overriden from SigninManagerBase::Observer.
   virtual void GoogleSigninSucceeded(const std::string& username,

components/signin/core/browser/mutable_profile_oauth2_token_service.cc

@@ -223,34 +223,38 @@ void MutableProfileOAuth2TokenService::LoadAllCredentialsIntoMemory(
     const std::map<std::string, std::string>& db_tokens) {
   std::string old_login_token;
 
-  for (std::map<std::string, std::string>::const_iterator iter =
-           db_tokens.begin();
-       iter != db_tokens.end();
-       ++iter) {
-    std::string prefixed_account_id = iter->first;
-    std::string refresh_token = iter->second;
-
-    if (IsLegacyRefreshTokenId(prefixed_account_id) && !refresh_token.empty())
-      old_login_token = refresh_token;
-
-    if (IsLegacyServiceId(prefixed_account_id)) {
-      scoped_refptr<TokenWebData> token_web_data = client()->GetDatabase();
-      if (token_web_data.get())
-        token_web_data->RemoveTokenForService(prefixed_account_id);
-    } else {
-      DCHECK(!refresh_token.empty());
-      std::string account_id = RemoveAccountIdPrefix(prefixed_account_id);
-      refresh_tokens()[account_id].reset(
-          new AccountInfo(this, account_id, refresh_token));
-      FireRefreshTokenAvailable(account_id);
-      // TODO(fgorski): Notify diagnostic observers.
+  {
+    ScopedBacthChange batch(this);
+
+    for (std::map<std::string, std::string>::const_iterator iter =
+             db_tokens.begin();
+         iter != db_tokens.end();
+         ++iter) {
+      std::string prefixed_account_id = iter->first;
+      std::string refresh_token = iter->second;
+
+      if (IsLegacyRefreshTokenId(prefixed_account_id) && !refresh_token.empty())
+        old_login_token = refresh_token;
+
+      if (IsLegacyServiceId(prefixed_account_id)) {
+        scoped_refptr<TokenWebData> token_web_data = client()->GetDatabase();
+        if (token_web_data.get())
+          token_web_data->RemoveTokenForService(prefixed_account_id);
+      } else {
+        DCHECK(!refresh_token.empty());
+        std::string account_id = RemoveAccountIdPrefix(prefixed_account_id);
+        refresh_tokens()[account_id].reset(
+            new AccountInfo(this, account_id, refresh_token));
+        FireRefreshTokenAvailable(account_id);
+        // TODO(fgorski): Notify diagnostic observers.
+      }
     }
-  }
 
-  if (!old_login_token.empty()) {
-    DCHECK(!loading_primary_account_id_.empty());
-    if (refresh_tokens().count(loading_primary_account_id_) == 0)
-      UpdateCredentials(loading_primary_account_id_, old_login_token);
+    if (!old_login_token.empty()) {
+      DCHECK(!loading_primary_account_id_.empty());
+      if (refresh_tokens().count(loading_primary_account_id_) == 0)
+        UpdateCredentials(loading_primary_account_id_, old_login_token);
+    }
   }
 
   FireRefreshTokensLoaded();
@@ -307,6 +311,8 @@ void MutableProfileOAuth2TokenService::UpdateCredentials(
   bool refresh_token_present = refresh_tokens_.count(account_id) > 0;
   if (!refresh_token_present ||
       refresh_tokens_[account_id]->refresh_token() != refresh_token) {
+    ScopedBacthChange batch(this);
+
     // If token present, and different from the new one, cancel its requests,
     // and clear the entries in cache related to that account.
     if (refresh_token_present) {
@@ -336,6 +342,7 @@ void MutableProfileOAuth2TokenService::RevokeCredentials(
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (refresh_tokens_.count(account_id) > 0) {
+    ScopedBacthChange batch(this);
     RevokeCredentialsOnServer(refresh_tokens_[account_id]->refresh_token());
     CancelRequestsForAccount(account_id);
     ClearCacheForAccount(account_id);
@@ -366,6 +373,9 @@ void MutableProfileOAuth2TokenService::RevokeAllCredentials() {
   if (!client()->CanRevokeCredentials())
     return;
   DCHECK(thread_checker_.CalledOnValidThread());
+
+  ScopedBacthChange batch(this);
+
   CancelWebTokenFetch();
   CancelAllRequests();
   ClearCache();

components/signin/core/browser/mutable_profile_oauth2_token_service_unittest.cc

@@ -33,7 +33,9 @@ class MutableProfileOAuth2TokenServiceTest
       : factory_(NULL),
         token_available_count_(0),
         token_revoked_count_(0),
-        tokens_loaded_count_(0) {}
+        tokens_loaded_count_(0),
+        start_batch_changes_(0),
+        end_batch_changes_(0) {}
 
   virtual void SetUp() OVERRIDE {
 #if defined(OS_MACOSX)
@@ -71,10 +73,20 @@ class MutableProfileOAuth2TokenServiceTest
   }
   virtual void OnRefreshTokensLoaded() OVERRIDE { ++tokens_loaded_count_; }
 
+  virtual void OnStartBatchChanges() OVERRIDE {
+    ++start_batch_changes_;
+  }
+
+  virtual void OnEndBatchChanges() OVERRIDE {
+    ++end_batch_changes_;
+  }
+
   void ResetObserverCounts() {
     token_available_count_ = 0;
     token_revoked_count_ = 0;
     tokens_loaded_count_ = 0;
+    start_batch_changes_ = 0;
+    end_batch_changes_ = 0;
   }
 
   void ExpectNoNotifications() {
@@ -114,6 +126,8 @@ class MutableProfileOAuth2TokenServiceTest
   int token_available_count_;
   int token_revoked_count_;
   int tokens_loaded_count_;
+  int start_batch_changes_;
+  int end_batch_changes_;
 };
 
 TEST_F(MutableProfileOAuth2TokenServiceTest, PersistenceDBUpgrade) {
@@ -133,6 +147,8 @@ TEST_F(MutableProfileOAuth2TokenServiceTest, PersistenceDBUpgrade) {
   // Legacy tokens get discarded, but the old refresh token is kept.
   EXPECT_EQ(1, tokens_loaded_count_);
   EXPECT_EQ(1, token_available_count_);
+  EXPECT_EQ(1, start_batch_changes_);
+  EXPECT_EQ(1, end_batch_changes_);
   EXPECT_TRUE(oauth2_service_.RefreshTokenIsAvailable(main_account_id));
   EXPECT_EQ(1U, oauth2_service_.refresh_tokens().size());
   EXPECT_EQ(main_refresh_token,
@@ -159,6 +175,8 @@ TEST_F(MutableProfileOAuth2TokenServiceTest, PersistenceDBUpgrade) {
   // token is present it is not overwritten.
   EXPECT_EQ(2, token_available_count_);
   EXPECT_EQ(1, tokens_loaded_count_);
+  EXPECT_EQ(1, start_batch_changes_);
+  EXPECT_EQ(1, end_batch_changes_);
   EXPECT_TRUE(oauth2_service_.RefreshTokenIsAvailable(main_account_id));
   // TODO(fgorski): cover both using RefreshTokenIsAvailable() and then get the
   // tokens using GetRefreshToken()
@@ -170,6 +188,8 @@ TEST_F(MutableProfileOAuth2TokenServiceTest, PersistenceDBUpgrade) {
       oauth2_service_.refresh_tokens()[other_account_id]->refresh_token());
 
   oauth2_service_.RevokeAllCredentials();
+  EXPECT_EQ(2, start_batch_changes_);
+  EXPECT_EQ(2, end_batch_changes_);
 }
 
 TEST_F(MutableProfileOAuth2TokenServiceTest, PersistenceRevokeCredentials) {
@@ -184,13 +204,17 @@ TEST_F(MutableProfileOAuth2TokenServiceTest, PersistenceRevokeCredentials) {
 
   oauth2_service_.UpdateCredentials(account_id_1, refresh_token_1);
   oauth2_service_.UpdateCredentials(account_id_2, refresh_token_2);
+  EXPECT_EQ(2, start_batch_changes_);
+  EXPECT_EQ(2, end_batch_changes_);
 
   // TODO(fgorski): Enable below when implemented:
   // EXPECT_TRUE(oauth2_servive_->RefreshTokenIsAvailable(account_id_1));
   // EXPECT_TRUE(oauth2_service_.RefreshTokenIsAvailable(account_id_2));
 
   ResetObserverCounts();
   oauth2_service_.RevokeCredentials(account_id_1);
+  EXPECT_EQ(1, start_batch_changes_);
+  EXPECT_EQ(1, end_batch_changes_);
   ExpectOneTokenRevokedNotification();
 
   // TODO(fgorski): Enable below when implemented:
@@ -201,6 +225,8 @@ TEST_F(MutableProfileOAuth2TokenServiceTest, PersistenceRevokeCredentials) {
   EXPECT_EQ(0, token_available_count_);
   EXPECT_EQ(1, token_revoked_count_);
   EXPECT_EQ(0, tokens_loaded_count_);
+  EXPECT_EQ(1, start_batch_changes_);
+  EXPECT_EQ(1, end_batch_changes_);
   ResetObserverCounts();
 }
 
@@ -211,6 +237,8 @@ TEST_F(MutableProfileOAuth2TokenServiceTest, PersistenceLoadCredentials) {
   // Perform a load from an empty DB.
   oauth2_service_.LoadCredentials("account_id");
   base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, start_batch_changes_);
+  EXPECT_EQ(1, end_batch_changes_);
   ExpectOneTokensLoadedNotification();
   // LoadCredentials() guarantees that the account given to it as argument
   // is in the refresh_token map.
@@ -221,13 +249,17 @@ TEST_F(MutableProfileOAuth2TokenServiceTest, PersistenceLoadCredentials) {
   oauth2_service_.UpdateCredentials("account_id", "refresh_token");
   oauth2_service_.UpdateCredentials("account_id2", "refresh_token2");
   oauth2_service_.refresh_tokens().clear();
+  EXPECT_EQ(2, start_batch_changes_);
+  EXPECT_EQ(2, end_batch_changes_);
   ResetObserverCounts();
 
   oauth2_service_.LoadCredentials("account_id");
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(2, token_available_count_);
   EXPECT_EQ(0, token_revoked_count_);
   EXPECT_EQ(1, tokens_loaded_count_);
+  EXPECT_EQ(1, start_batch_changes_);
+  EXPECT_EQ(1, end_batch_changes_);
   ResetObserverCounts();
 
   // TODO(fgorski): Enable below when implemented:
@@ -238,6 +270,8 @@ TEST_F(MutableProfileOAuth2TokenServiceTest, PersistenceLoadCredentials) {
   EXPECT_EQ(0, token_available_count_);
   EXPECT_EQ(2, token_revoked_count_);
   EXPECT_EQ(0, tokens_loaded_count_);
+  EXPECT_EQ(1, start_batch_changes_);
+  EXPECT_EQ(1, end_batch_changes_);
   ResetObserverCounts();
 }