IdentityTestEnvironment: explicitly track pending & available token requests

... Rather than relying on them to be sufficiently close to each other
that there isn't much event loop activity.

This is needed for upcoming port of GaiaOauthClient to Network Service
(and hence mojo), which requires tests to spin the event loop in a lot
more places than before to deliver fetch results, making it hard to
ensure that HandleOnAccessTokenRequested and WaitForAccessTokenRequestIfNecessary
get delivered in right order.

Bug: 865018
Change-Id: I7acf6427be7be5b00c8e003dd687169c4b8a7577
Reviewed-on: https://chromium-review.googlesource.com/1167904Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582176}

services/identity/public/cpp/identity_test_environment.cc

@@ -222,6 +222,16 @@ void IdentityTestEnvironment::SetCallbackForNextAccessTokenRequest(
   on_access_token_requested_callback_ = std::move(callback);
 }
 
+IdentityTestEnvironment::AccessTokenRequestState::AccessTokenRequestState() =
+    default;
+IdentityTestEnvironment::AccessTokenRequestState::~AccessTokenRequestState() =
+    default;
+IdentityTestEnvironment::AccessTokenRequestState::AccessTokenRequestState(
+    AccessTokenRequestState&& other) = default;
+IdentityTestEnvironment::AccessTokenRequestState&
+IdentityTestEnvironment::AccessTokenRequestState::operator=(
+    AccessTokenRequestState&& other) = default;
+
 void IdentityTestEnvironment::OnAccessTokenRequested(
     const std::string& account_id,
     const std::string& consumer_id,
@@ -239,36 +249,56 @@ void IdentityTestEnvironment::OnAccessTokenRequested(
 
 void IdentityTestEnvironment::HandleOnAccessTokenRequested(
     std::string account_id) {
-  if (pending_access_token_requester_ &&
-      *pending_access_token_requester_ != account_id) {
-    // An access token request came in for a different account than the one for
-    // which we are waiting. Some unittests make access token requests for
-    // multiple accounts and interleave their responses in an order different
-    // from the requests. To accommodate this case, defer the handling of this
-    // access token request until the next iteration of the run loop, where it
-    // may then be being waited for.
-    base::SequencedTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::BindOnce(&IdentityTestEnvironment::HandleOnAccessTokenRequested,
-                       base::Unretained(this), account_id));
+  if (on_access_token_requested_callback_) {
+    std::move(on_access_token_requested_callback_).Run();
     return;
   }
 
-  pending_access_token_requester_.reset();
+  for (auto it = requesters_.begin(); it != requesters_.end(); ++it) {
+    if (!it->account_id || (it->account_id.value() == account_id)) {
+      if (it->state == AccessTokenRequestState::kAvailable)
+        return;
+      if (it->on_available)
+        std::move(it->on_available).Run();
+      requesters_.erase(it);
+      return;
+    }
+  }
 
-  if (on_access_token_requested_callback_)
-    std::move(on_access_token_requested_callback_).Run();
+  // A requests came in for a request for which we are not waiting. Record that
+  // it's available.
+  requesters_.emplace_back();
+  requesters_.back().state = AccessTokenRequestState::kAvailable;
+  requesters_.back().account_id = account_id;
 }
 
 void IdentityTestEnvironment::WaitForAccessTokenRequestIfNecessary(
-    base::Optional<std::string> pending_access_token_requester) {
-  DCHECK(!pending_access_token_requester_);
-  pending_access_token_requester_ = std::move(pending_access_token_requester);
+    base::Optional<std::string> account_id) {
+  // Handle HandleOnAccessTokenRequested getting called before
+  // WaitForAccessTokenRequestIfNecessary.
+  if (account_id) {
+    for (auto it = requesters_.begin(); it != requesters_.end(); ++it) {
+      if (it->account_id && it->account_id.value() == account_id.value()) {
+        // Can't wait twice for same thing.
+        DCHECK_EQ(AccessTokenRequestState::kAvailable, it->state);
+        requesters_.erase(it);
+        return;
+      }
+    }
+  } else {
+    for (auto it = requesters_.begin(); it != requesters_.end(); ++it) {
+      if (it->state == AccessTokenRequestState::kAvailable) {
+        requesters_.erase(it);
+        return;
+      }
+    }
+  }
 
-  DCHECK(!on_access_token_requested_callback_);
   base::RunLoop run_loop;
-  on_access_token_requested_callback_ = run_loop.QuitClosure();
-
+  requesters_.emplace_back();
+  requesters_.back().state = AccessTokenRequestState::kPending;
+  requesters_.back().account_id = std::move(account_id);
+  requesters_.back().on_available = run_loop.QuitClosure();
   run_loop.Run();
 }
 

services/identity/public/cpp/identity_test_environment.h

@@ -85,9 +85,9 @@ class IdentityTestEnvironment : public IdentityManager::DiagnosticsObserver {
   void SetAutomaticIssueOfAccessTokens(bool grant);
 
   // Issues |token| in response to any access token request that either has (a)
-  // just occurred in the current iteration of the run loop, or (b) will occur
-  // in the future via a task that was posted in the current iteration of the
-  // run loop. In the latter case, waits until the access token request occurs.
+  // already occurred and has not been matched by a previous call to this or
+  // other WaitFor... method, or (b) will occur in the future. In the latter
+  // case, waits until the access token request occurs.
   // NOTE: This method behaves this way to allow IdentityTestEnvironment to be
   // agnostic with respect to whether access token requests are handled
   // synchronously or asynchronously in the production code.
@@ -99,16 +99,9 @@ class IdentityTestEnvironment : public IdentityManager::DiagnosticsObserver {
       const base::Time& expiration);
 
   // Issues |token| in response to an access token request for |account_id| that
-  // either has (a) just occurred in the current iteration of the run loop, or
-  // (b) will occur in the future via a task that was posted in the current
-  // iteration of the run loop. In the latter case, waits until the access token
-  // request occurs.
-  // NOTE: Any access token request for a *different* account
-  // that is seen before an access token request for the given account will
-  // be deferred to be handled on the next iteration of the runloop; this
-  // behavior accommodates tests that interleave handling of production access
-  // token requests for multiple accounts in an order different from the order
-  // in which the requests are made.
+  // either already occurred and has not been matched by a previous call to this
+  // or other WaitFor... method , or (b) will occur in the future. In the latter
+  // case, waits until the access token request occurs.
   // NOTE: This method behaves this way to allow
   // IdentityTestEnvironment to be agnostic with respect to whether access token
   // requests are handled synchronously or asynchronously in the production
@@ -119,9 +112,9 @@ class IdentityTestEnvironment : public IdentityManager::DiagnosticsObserver {
       const base::Time& expiration);
 
   // Issues |error| in response to any access token request that either has (a)
-  // just occurred in the current iteration of the run loop, or (b) will occur
-  // in the future via a task that was posted in the current iteration of the
-  // run loop. In the latter case, waits until the access token request occurs.
+  // already occurred and has not been matched by a previous call to this or
+  // other WaitFor... method, or (b) will occur in the future via  In the latter
+  // case, waits until the access token request occurs.
   // NOTE: This method behaves this way to allow IdentityTestEnvironment to be
   // agnostic with respect to whether access token requests are handled
   // synchronously or asynchronously in the production code.
@@ -132,16 +125,9 @@ class IdentityTestEnvironment : public IdentityManager::DiagnosticsObserver {
       const GoogleServiceAuthError& error);
 
   // Issues |error| in response to an access token request for |account_id| that
-  // either has (a) just occurred in the current iteration of the run loop, or
-  // (b) will occur in the future via a task that was posted in the current
-  // iteration of the run loop. In the latter case, waits until the access token
-  // request occurs.
-  // NOTE: Any access token request for a *different* account
-  // that is seen before an access token request for the given account will
-  // be deferred to be handled on the next iteration of the runloop; this
-  // behavior accommodates tests that interleave handling of production access
-  // token requests for multiple accounts in an order different from the order
-  // in which the requests are made.
+  // either has (a) already occurred and has not been matched by a previous call
+  // to this or other WaitFor... method, or (b) will occur in the future. In the
+  // latter case, waits until the access token request occurs.
   // NOTE: This method behaves this way to allow
   // IdentityTestEnvironment to be agnostic with respect to whether access token
   // requests are handled synchronously or asynchronously in the production
@@ -158,6 +144,20 @@ class IdentityTestEnvironment : public IdentityManager::DiagnosticsObserver {
   void SetCallbackForNextAccessTokenRequest(base::OnceClosure callback);
 
  private:
+  struct AccessTokenRequestState {
+    AccessTokenRequestState();
+    ~AccessTokenRequestState();
+    AccessTokenRequestState(AccessTokenRequestState&& other);
+    AccessTokenRequestState& operator=(AccessTokenRequestState&& other);
+
+    enum {
+      kPending,
+      kAvailable,
+    } state;
+    base::Optional<std::string> account_id;
+    base::OnceClosure on_available;
+  };
+
   // IdentityManager::DiagnosticsObserver:
   void OnAccessTokenRequested(
       const std::string& account_id,
@@ -170,15 +170,16 @@ class IdentityTestEnvironment : public IdentityManager::DiagnosticsObserver {
   // |account_id| or |pending_access_token_requester_| is empty.
   void HandleOnAccessTokenRequested(std::string account_id);
 
-  // Sets |pending_access_token_requester_| to
-  // |pending_access_token_requester| and runs a nested runloop until an access
-  // token request is observed.
+  // If a token request for |account_id| (or any account if nullopt) has already
+  // been made and not matched by a different call, returns immediately.
+  // Otherwise and runs a nested runloop until a matching access token request
+  // is observed.
   void WaitForAccessTokenRequestIfNecessary(
-      base::Optional<std::string> pending_access_token_requester);
+      base::Optional<std::string> account_id);
 
   std::unique_ptr<IdentityTestEnvironmentInternal> internals_;
   base::OnceClosure on_access_token_requested_callback_;
-  base::Optional<std::string> pending_access_token_requester_;
+  std::vector<AccessTokenRequestState> requesters_;
 
   DISALLOW_COPY_AND_ASSIGN(IdentityTestEnvironment);
 };