Have IdentityManager always pass on refresh token removal notifications

ProfileOAuth2TokenService has a corner case wherein during startup,
it can fire token removal notifications for accounts for which it has
never previously filed a token available notification. IdentityManager
currently swallows such notifications. However, as we streamline
IdentityManager to be just a straight pass-through to its backing
classes (crbug.com/883722), IdentityManager will no longer be able
to detect this case (because it won't be maintaining any cached info
from the token available notifications).

This CL makes the behavioral change of having IdentityManager always
pass on token removal notifications from ProfileOAuth2TokenService.
The behavioral impact is that consumers of
IdentityManager::Observer::OnRefreshTokenRemovedForAccount() will now
see a removal notification in the corner case described above. However,
all such consumers were previously consumers of
ProfileOAuth2TokenService::Observer::OnRefreshTokenRevoked() before
their conversion and hence were previously seeing the removal
notification in this corner case. Thus, the behavioral change does
not seem to have a concrete impact on consumers one way or the other.

Bug: 883722
Change-Id: I131ad8dbe12d73772845fd2eb565cd3477779c36
Reviewed-on: https://chromium-review.googlesource.com/1225878Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592794}

services/identity/public/cpp/identity_manager.cc

@@ -356,18 +356,7 @@ void IdentityManager::OnRefreshTokenAvailable(const std::string& account_id) {
 }
 
 void IdentityManager::OnRefreshTokenRevoked(const std::string& account_id) {
-  // NOTE: It is possible for |pending_token_revoked_info_| to be null in the
-  // corner case of tokens being revoked during initial startup (see
-  // WillFireOnRefreshTokenRevoked() above).
-  if (!pending_token_revoked_info_) {
-    return;
-  }
-
-  DCHECK_EQ(pending_token_revoked_info_->account_id, account_id);
-
-  // Move the state out of |pending_token_revoked_info_| in case any observer
-  // callbacks fired below result in mutations of refresh tokens.
-  AccountInfo account_info = pending_token_revoked_info_.value();
+  // TODO(883722): Remove this state entirely, as it now serves no purpose.
   pending_token_revoked_info_.reset();
 
   for (auto& observer : observer_list_) {

services/identity/public/cpp/identity_manager.h

@@ -83,6 +83,10 @@ class IdentityManager : public SigninManagerBase::Observer,
     // has been removed. At the time that this callback is invoked, there is
     // no longer guaranteed to be any AccountInfo associated with
     // |account_id|.
+    // NOTE: It is not guaranteed that a call to
+    // OnRefreshTokenUpdatedForAccount() has previously occurred for this
+    // account due to corner cases.
+    // TODO(https://crbug.com/884731): Eliminate these corner cases.
     // NOTE: On a signout event, the ordering of this callback wrt the
     // OnPrimaryAccountCleared() callback is undefined.If this lack of ordering
     // is problematic for your use case, please contact blundell@chromium.org.

services/identity/public/cpp/identity_manager_unittest.cc

@@ -1463,21 +1463,23 @@ TEST_F(IdentityManagerTest,
 }
 #endif
 
-TEST_F(IdentityManagerTest,
-       CallbackNotSentOnRefreshTokenRemovalOfUnknownAccount) {
-  // RemoveCredentials expects (and DCHECKS) that either the caller passes a
-  // known account ID, or the account is unknown because the token service is
-  // still loading credentials. Our common test setup actually completes this
-  // loading, so use the *for_testing() method below to simulate the race
-  // condition.
+TEST_F(IdentityManagerTest, CallbackSentOnRefreshTokenRemovalOfUnknownAccount) {
+  // When the token service is still loading credentials, it may send token
+  // revoked callbacks for accounts that it has never sent a token available
+  // callback. Our common test setup actually completes this loading, so use the
+  // *for_testing() method below to simulate the race condition and ensure that
+  // IdentityManager passes on the callback in this case.
   token_service()->set_all_credentials_loaded_for_testing(false);
 
-  base::RunLoop run_loop;
-  identity_manager_observer()->set_on_refresh_token_removed_callback(
-      base::BindRepeating([](const std::string&) { EXPECT_TRUE(false); }));
-  token_service()->RevokeCredentials("dummy_account");
+  std::string dummy_account_id = "dummy_account";
 
+  base::RunLoop run_loop;
+  token_service()->RevokeCredentials(dummy_account_id);
   run_loop.RunUntilIdle();
+
+  EXPECT_EQ(dummy_account_id,
+            identity_manager_observer()
+                ->account_from_refresh_token_removed_callback());
 }
 
 TEST_F(IdentityManagerTest,