Remove AfterPrimaryAccountChanged()

This CL prefers using a PostTask to update the unconsented primary
account and deletes the temporary AfterPrimaryAccountChanged() function
from the IdentityManager::Observer interface.

Bug: 1046746

Change-Id: Ie381841e6390969b8ea250480a82413f925e3e3f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2644800
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Monica Basta <msalama@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Auto-Submit: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#848064}

chrome/browser/signin/signin_manager.cc

@@ -104,18 +104,26 @@ SigninManager::ComputeUnconsentedPrimaryAccountInfo() const {
 }
 
 // signin::IdentityManager::Observer implementation.
-void SigninManager::AfterSyncPrimaryAccountCleared() {
+void SigninManager::OnPrimaryAccountChanged(
+    const signin::PrimaryAccountChangeEvent& event_details) {
   // This is needed for the case where the user chooses to start syncing
   // with an account that is different from the unconsented primary account
   // (not the first in cookies) but then cancels. In that case, the tokens stay
   // the same. In all the other cases, either the token will be revoked which
   // will trigger an update for the unconsented primary account or the
   // primary account stays the same but the sync consent is revoked.
-  // |OnPrimaryAccountCleared| is not used to ensure the value of the
-  // unconsented primary account doesn't change during other observers being
-  // notified. All observers should see the same value for the unconsented
-  // primary account.
-  UpdateUnconsentedPrimaryAccount();
+  if (event_details.GetEventTypeFor(signin::ConsentLevel::kSync) !=
+      signin::PrimaryAccountChangeEvent::Type::kCleared) {
+    return;
+  }
+
+  // It is important to update the primary account after all observers process
+  // the current OnPrimaryAccountChanged() as all observers should see the same
+  // value for the unconsented primary account. Schedule the potential update
+  // on the next run loop.
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(&SigninManager::UpdateUnconsentedPrimaryAccount,
+                                weak_ptr_factory_.GetWeakPtr()));
 }
 
 void SigninManager::OnRefreshTokenUpdatedForAccount(

chrome/browser/signin/signin_manager.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_SIGNIN_SIGNIN_MANAGER_H_
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 
@@ -36,7 +37,8 @@ class SigninManager : public KeyedService,
   base::Optional<CoreAccountInfo> ComputeUnconsentedPrimaryAccountInfo() const;
 
   // signin::IdentityManager::Observer implementation.
-  void AfterSyncPrimaryAccountCleared() override;
+  void OnPrimaryAccountChanged(
+      const signin::PrimaryAccountChangeEvent& event_details) override;
   void OnRefreshTokenUpdatedForAccount(
       const CoreAccountInfo& account_info) override;
   void OnRefreshTokenRemovedForAccount(
@@ -53,6 +55,8 @@ class SigninManager : public KeyedService,
   signin::IdentityManager* identity_manager_;
   bool unconsented_primary_account_revoked_during_load_ = false;
 
+  base::WeakPtrFactory<SigninManager> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(SigninManager);
 };
 

chrome/browser/signin/signin_manager_unittest.cc

@@ -215,6 +215,8 @@ TEST_F(SigninManagerTest, UnconsentedPrimaryAccountNotChangedOnSignout) {
 
   // Tests that sync primary account is cleared, but unconsented account is not.
   identity_test_env()->RevokeSyncConsent();
+  base::RunLoop().RunUntilIdle();
+
   EXPECT_EQ(account, identity_manager()->GetPrimaryAccountInfo(
                          ConsentLevel::kNotRequired));
   EXPECT_FALSE(identity_manager()->HasPrimaryAccount(ConsentLevel::kSync));
@@ -349,6 +351,8 @@ TEST_F(SigninManagerTest,
   // Clear primary account but do not delete the account. The unconsented
   // primary account should be updated to be the first account in cookies.
   identity_test_env()->RevokeSyncConsent();
+  base::RunLoop().RunUntilIdle();
+
   // Primary account is cleared, but unconsented account is not.
   EXPECT_FALSE(identity_manager()->HasPrimaryAccount());
   EXPECT_FALSE(identity_manager()->HasPrimaryAccount(ConsentLevel::kSync));
@@ -380,6 +384,7 @@ TEST_F(SigninManagerTest, ClearPrimaryAccountAndSignOut) {
   ExpectSyncPrimaryAccountSetEvent(account);
 
   identity_test_env()->ClearPrimaryAccount();
+  base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(1U, observer().events().size());
   auto event = observer().events()[0];

components/signin/public/identity_manager/identity_manager.cc

@@ -478,8 +478,18 @@ AccountInfo IdentityManager::GetAccountInfoForAccountWithRefreshToken(
 
 void IdentityManager::OnPrimaryAccountChanged(
     const PrimaryAccountChangeEvent& event_details) {
-  for (auto& observer : observer_list_)
+  CoreAccountId event_primary_account_id =
+      event_details.GetCurrentState().primary_account.account_id;
+  DCHECK_EQ(event_primary_account_id,
+            GetPrimaryAccountId(event_details.GetCurrentState().consent_level));
+  for (auto& observer : observer_list_) {
     observer.OnPrimaryAccountChanged(event_details);
+    // Ensure that |observer| did not change the primary account as otherwise
+    // |event_details| would not longer be correct.
+    DCHECK_EQ(
+        event_primary_account_id,
+        GetPrimaryAccountId(event_details.GetCurrentState().consent_level));
+  }
 
 #if defined(OS_ANDROID)
   if (java_identity_manager_) {
@@ -489,13 +499,6 @@ void IdentityManager::OnPrimaryAccountChanged(
         ConvertToJavaPrimaryAccountChangeEvent(env, event_details));
   }
 #endif
-
-  if (event_details.GetEventTypeFor(ConsentLevel::kSync) ==
-      PrimaryAccountChangeEvent::Type::kCleared) {
-    for (auto& observer : observer_list_) {
-      observer.AfterSyncPrimaryAccountCleared();
-    }
-  }
 }
 
 void IdentityManager::OnRefreshTokenAvailable(const CoreAccountId& account_id) {

components/signin/public/identity_manager/identity_manager.h

@@ -77,17 +77,13 @@ class IdentityManager : public KeyedService,
 
     // Called when there is a change in the primary account or in the consent
     // level for the primary account.
+    //
+    // Note: Observers are not allowed to change the primary account directly
+    // from this methood as that would lead to |event_details| not being correct
+    // for the future observers.
     virtual void OnPrimaryAccountChanged(
         const PrimaryAccountChangeEvent& event_details) {}
 
-    // TODO(crbug.com/1046746): Move to |SigninClient|.
-    // Called After notifying all the observers of |OnPrimaryAccountChanged|
-    // if the sync primary account was cleared.
-    //
-    // Note: This function is only intended to be used by the signin code.
-    // General observers should use |OnPrimaryAccountChanged|.
-    virtual void AfterSyncPrimaryAccountCleared() {}
-
     // Called when a new refresh token is associated with |account_info|.
     // NOTE: On a signin event, the ordering of this callback wrt the
     // OnPrimaryAccountSet() callback is undefined. If you as a client are