Refactor how GCMAccountTracker stores pending token requests

In preparation for porting GCM to use IdentityManager to fetch access
tokens rather than ProfileOAuth2TokenService, this CL refactors how
GCMAccountTracker internally stores pending requests. Currently it
stores them as a vector of raw OAuth2TokenService::Request pointers;
when such a pointer gets passed in OnGetToken{Success, Failure}, it
looks it up in the vector to delete it. However, the IdentityManager
facilities do not pass pointers to the analogous "request" objects
(called AccessTokenFetcher), while the client is still responsible
for deleting those objects when desired.

This CL changes GCMAccountTracker to maintain a map of account IDs
to OAuth2TokenService::Request unique_ptrs. When a request is
complete, it simply erases the entry in the map by key. One hiccup
is that GCMAccountTracker currently can make two concurrent requests
for the same account:

- It makes token requests for accounts in AccountTracker's list
  of accounts that are in GCMAccountTracker's TOKEN_NEEDED state.
- When it makes a request, it transitions the state of the account to
  GETTING_TOKEN.
- When it receives a notification that an account has signed out,
  it transitions that account to ACCOUNT_REMOVED state.
- When it receives a notification that an account has signed in,
  if it has the account in ACCOUNT_REMOVED state it transitions that
  account to GETTING_TOKEN state and refetches access tokens.

However, GCMAccountTracker already short-circuits out of taking any
action on completion of access token requests if the account is in
ACCOUNT_REMOVED state (beyond deleting the request).  This
CL simply changes GCMAccountTracker to never make two concurrent
token requests for the same account by deleting a token request when
transitioning an account to ACCOUNT_REMOVED state.

Bug: 809923
Change-Id: I2bae540133ae4db3f2cfea47332b2907dd88dd6d
Reviewed-on: https://chromium-review.googlesource.com/1104689
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569599}

components/gcm_driver/gcm_account_tracker.cc

@@ -122,18 +122,14 @@ void GCMAccountTracker::OnGetTokenSuccess(
   AccountInfos::iterator iter = account_infos_.find(request->GetAccountId());
   DCHECK(iter != account_infos_.end());
   if (iter != account_infos_.end()) {
-    DCHECK(iter->second.state == GETTING_TOKEN ||
-           iter->second.state == ACCOUNT_REMOVED);
-    // If OnAccountSignedOut(..) was called most recently, account is kept in
-    // ACCOUNT_REMOVED state.
-    if (iter->second.state == GETTING_TOKEN) {
-      iter->second.state = TOKEN_PRESENT;
-      iter->second.access_token = access_token;
-      iter->second.expiration_time = expiration_time;
-    }
+    DCHECK_EQ(GETTING_TOKEN, iter->second.state);
+
+    iter->second.state = TOKEN_PRESENT;
+    iter->second.access_token = access_token;
+    iter->second.expiration_time = expiration_time;
   }
 
-  DeleteTokenRequest(request);
+  pending_token_requests_.erase(request->GetAccountId());
   ReportTokens();
 }
 
@@ -147,20 +143,16 @@ void GCMAccountTracker::OnGetTokenFailure(
   AccountInfos::iterator iter = account_infos_.find(request->GetAccountId());
   DCHECK(iter != account_infos_.end());
   if (iter != account_infos_.end()) {
-    DCHECK(iter->second.state == GETTING_TOKEN ||
-           iter->second.state == ACCOUNT_REMOVED);
-    // If OnAccountSignedOut(..) was called most recently, account is kept in
-    // ACCOUNT_REMOVED state.
-    if (iter->second.state == GETTING_TOKEN) {
-      // Given the fetcher has a built in retry logic, consider this situation
-      // to be invalid refresh token, that is only fixed when user signs in.
-      // Once the users signs in properly the minting will retry.
-      iter->second.access_token.clear();
-      iter->second.state = ACCOUNT_REMOVED;
-    }
+    DCHECK_EQ(GETTING_TOKEN, iter->second.state);
+
+    // Given the fetcher has a built in retry logic, consider this situation
+    // to be invalid refresh token, that is only fixed when user signs in.
+    // Once the users signs in properly the minting will retry.
+    iter->second.access_token.clear();
+    iter->second.state = ACCOUNT_REMOVED;
   }
 
-  DeleteTokenRequest(request);
+  pending_token_requests_.erase(request->GetAccountId());
   ReportTokens();
 }
 
@@ -297,17 +289,6 @@ base::TimeDelta GCMAccountTracker::GetTimeToNextTokenReporting() const {
   return time_till_next_reporting;
 }
 
-void GCMAccountTracker::DeleteTokenRequest(
-    const OAuth2TokenService::Request* request) {
-  auto iter = std::find_if(
-      pending_token_requests_.begin(), pending_token_requests_.end(),
-      [request](const std::unique_ptr<OAuth2TokenService::Request>& r) {
-        return request == r.get();
-      });
-  if (iter != pending_token_requests_.end())
-    pending_token_requests_.erase(iter);
-}
-
 void GCMAccountTracker::GetAllNeededTokens() {
   // Only start fetching tokens if driver is running, they have a limited
   // validity time and GCM connection is a good indication of network running.
@@ -334,7 +315,8 @@ void GCMAccountTracker::GetToken(AccountInfos::iterator& account_iter) {
   std::unique_ptr<OAuth2TokenService::Request> request =
       token_service_->StartRequest(account_iter->first, scopes, this);
 
-  pending_token_requests_.push_back(std::move(request));
+  DCHECK(pending_token_requests_.count(account_iter->first) == 0);
+  pending_token_requests_.emplace(account_iter->first, std::move(request));
   account_iter->second.state = GETTING_TOKEN;
 }
 
@@ -360,6 +342,12 @@ void GCMAccountTracker::OnAccountSignedOut(const AccountIds& ids) {
 
   iter->second.access_token.clear();
   iter->second.state = ACCOUNT_REMOVED;
+
+  // Delete any ongoing access token request now so that if the account is later
+  // re-added and a new access token request made, we do not break this class'
+  // invariant that there is at most one ongoing access token request per
+  // account.
+  pending_token_requests_.erase(ids.account_key);
   ReportTokens();
 }
 

components/gcm_driver/gcm_account_tracker.h

@@ -129,9 +129,6 @@ class GCMAccountTracker : public AccountTracker::Observer,
   bool IsTokenFetchingRequired() const;
   // Gets the time until next token reporting.
   base::TimeDelta GetTimeToNextTokenReporting() const;
-  // Deletes a token request. Should be called from OnGetTokenSuccess(..) or
-  // OnGetTokenFailure(..).
-  void DeleteTokenRequest(const OAuth2TokenService::Request* request);
   // Checks on all known accounts, and calls GetToken(..) for those with
   // |state == TOKEN_NEEDED|.
   void GetAllNeededTokens();
@@ -157,8 +154,12 @@ class GCMAccountTracker : public AccountTracker::Observer,
   // Indicates whether shutdown has been called.
   bool shutdown_called_;
 
-  std::vector<std::unique_ptr<OAuth2TokenService::Request>>
-      pending_token_requests_;
+  // Stores the ongoing access token requests for deletion either upon
+  // completion or upon signout of the account for which the request is being
+  // made.
+  using AccountIDToTokenRequestMap =
+      std::map<std::string, std::unique_ptr<OAuth2TokenService::Request>>;
+  AccountIDToTokenRequestMap pending_token_requests_;
 
   // Creates weak pointers used to postpone reporting tokens. See
   // ScheduleReportTokens.