Change IdentityManager to forward SigninManager callbacks synchronously

When we first built out the IdentityManager C++ class and backed it via
//components/signin classes, we wanted its interaction with those
classes to mimic eventual interaction with the Identity Service as much
as possible. As part of this design goal, we implemented it to handle
callbacks from SigninManager by posting tasks to set its internal state
and notify observers; this mimics the fact of receiving these
notifications asynchronously from the Identity Service.

This mimicking has proven useful, as it has ferreted out issues with
tests assuming that signin state can be changed and have those changes
observed synchronously. However, it has also proven problematic:

1. By delaying the change to IdentityManager's internal state, we could
end up with clients getting inconsistent views of the user's signin
state from SigninManager and IdentityManager (cf.  crbug.com/804410).

2. We then changed IdentityManager to change its internal state before
SigninManager sends its callbacks, but still delay notifying its
(IdentityManager's) observers by posting a task. However, we have
gotten feedback from developers of clients of IdentityManager that this
behavior is problematic semantically: these developers want to be able
to make changes to internal state in response to IdentityManager
callbacks and have the invariant that those changes to internal state
are made atomically with the change in IdentityManager's internal state.
By decoupling the change in IdentityManager's internal state from the
notification of its observers, these developers are forced to handle
additional intermediate states in their code (e.g., "IdentityManager
has changed to have the user be signed in but hasn't yet notified its
observers").

In consequence of (1) and (2), we are abandoning the asynchronous
notification at this time. However, once the codebase is converted away
from direct usage of SigninManager entirely to IdentityManager, it could
be a good idea to reintroduce the implementation described in the top
paragraph above; reintroducing that implementation would be a good
intermediate move to switching in the Identity Service-backed
implementation.

This change should not introduce any behavioral changes, as nothing in
the codebase relies on these callbacks being fired asynchronously (or
is even aware that they're fired asynchronously).

Bug: 843510
Change-Id: Id1bc40fccbe1c4481d0c4968f90d7c15d1af154a
Reviewed-on: https://chromium-review.googlesource.com/1061494
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559134}

services/identity/public/cpp/identity_manager.cc

@@ -144,10 +144,16 @@ void IdentityManager::SetPrimaryAccountSynchronously(
 #if !defined(OS_CHROMEOS)
 void IdentityManager::WillFireGoogleSigninSucceeded(
     const AccountInfo& account_info) {
+  // TODO(843510): Consider setting this info and notifying observers
+  // asynchronously in response to GoogleSigninSucceeded() once there are no
+  // direct clients of SigninManager.
   primary_account_info_ = account_info;
 }
 
 void IdentityManager::WillFireGoogleSignedOut(const AccountInfo& account_info) {
+  // TODO(843510): Consider setting this info and notifying observers
+  // asynchronously in response to GoogleSigninSucceeded() once there are no
+  // direct clients of SigninManager.
   DCHECK(account_info.account_id == primary_account_info_.account_id);
   DCHECK(account_info.gaia == primary_account_info_.gaia);
   DCHECK(account_info.email == primary_account_info_.email);
@@ -156,25 +162,19 @@ void IdentityManager::WillFireGoogleSignedOut(const AccountInfo& account_info) {
 #endif
 
 void IdentityManager::GoogleSigninSucceeded(const AccountInfo& account_info) {
-  // Fire observer callbacks asynchronously to mimic this callback itself coming
-  // in asynchronously from the Identity Service rather than synchronously from
-  // SigninManager.
   DCHECK(account_info.account_id == primary_account_info_.account_id);
   DCHECK(account_info.gaia == primary_account_info_.gaia);
   DCHECK(account_info.email == primary_account_info_.email);
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(&IdentityManager::HandleGoogleSigninSucceeded,
-                                weak_ptr_factory_.GetWeakPtr(), account_info));
+  for (auto& observer : observer_list_) {
+    observer.OnPrimaryAccountSet(account_info);
+  }
 }
 
 void IdentityManager::GoogleSignedOut(const AccountInfo& account_info) {
-  // Fire observer callbacks asynchronously to mimic this callback itself coming
-  // in asynchronously from the Identity Service rather than synchronously from
-  // SigninManager.
   DCHECK(!HasPrimaryAccount());
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(&IdentityManager::HandleGoogleSignedOut,
-                                weak_ptr_factory_.GetWeakPtr(), account_info));
+  for (auto& observer : observer_list_) {
+    observer.OnPrimaryAccountCleared(account_info);
+  }
 }
 
 void IdentityManager::OnAccessTokenRequested(
@@ -197,19 +197,6 @@ void IdentityManager::HandleRemoveAccessTokenFromCache(
   token_service_->InvalidateAccessToken(account_id, scopes, access_token);
 }
 
-void IdentityManager::HandleGoogleSigninSucceeded(
-    const AccountInfo& account_info) {
-  for (auto& observer : observer_list_) {
-    observer.OnPrimaryAccountSet(account_info);
-  }
-}
-
-void IdentityManager::HandleGoogleSignedOut(const AccountInfo& account_info) {
-  for (auto& observer : observer_list_) {
-    observer.OnPrimaryAccountCleared(account_info);
-  }
-}
-
 void IdentityManager::HandleOnAccessTokenRequested(
     const std::string& account_id,
     const std::string& consumer_id,

services/identity/public/cpp/identity_manager.h

@@ -161,6 +161,10 @@ class IdentityManager : public SigninManagerBase::Observer,
 
 #if !defined(OS_CHROMEOS)
   // SigninManager::DiagnosticsClient:
+  // Override these to update |primary_account_info_| before any observers of
+  // SigninManager are notified of the signin state change, ensuring that any
+  // such observer flows that eventually interact with IdentityManager observe
+  // its state as being consistent with that of SigninManager.
   void WillFireGoogleSigninSucceeded(const AccountInfo& account_info) override;
   void WillFireGoogleSignedOut(const AccountInfo& account_info) override;
 #endif
@@ -177,16 +181,6 @@ class IdentityManager : public SigninManagerBase::Observer,
       const OAuth2TokenService::ScopeSet& scopes,
       const std::string& access_token);
 
-  // Updates |primary_account_info_| and notifies observers. Invoked
-  // asynchronously from GoogleSigninSucceeded() to mimic the effect of
-  // receiving this call asynchronously from the Identity Service.
-  void HandleGoogleSigninSucceeded(const AccountInfo& account_info);
-
-  // Clears |primary_account_info_| and notifies observers. Invoked
-  // asynchronously from GoogleSignedOut() to mimic the effect of
-  // receiving this call asynchronously from the Identity Service.
-  void HandleGoogleSignedOut(const AccountInfo& account_info);
-
   // Notifies diagnostics observers. Invoked asynchronously from
   // OnAccessTokenRequested() to mimic the effect of receiving this call
   // asynchronously from the Identity Service.