Revert "Handling of multiple concurrent requests from different clients in OAuth2TokenService"

This reverts commit 2a0ff283.

Reason: BuildBot Vista fails TokenServiceUpdateClearsCache test.

BUG=
TBR=zelidrag@chromium.org

Review URL: https://codereview.chromium.org/23767004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@221350 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc

@@ -32,7 +32,6 @@
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "google_apis/gaia/google_service_auth_error.h"
-#include "google_apis/gaia/oauth2_access_token_fetcher.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -143,8 +142,6 @@ class FakeAndroidProfileOAuth2TokenService
 
 #endif
 
-}  // namespace
-
 class UserPolicySigninServiceTest : public testing::Test {
  public:
   UserPolicySigninServiceTest()
@@ -222,7 +219,6 @@ class UserPolicySigninServiceTest : public testing::Test {
     testing_browser_process->SetBrowserPolicyConnector(NULL);
     base::RunLoop run_loop;
     run_loop.RunUntilIdle();
-    ResetLastFetcherId();
   }
 
 #if defined(OS_ANDROID)
@@ -352,10 +348,6 @@ class UserPolicySigninServiceTest : public testing::Test {
     Mock::VerifyAndClearExpectations(this);
   }
 
-  void ResetLastFetcherId() {
-    OAuth2AccessTokenFetcher::ResetLastFetcherIdForTest();
-  }
-
   scoped_ptr<TestingProfile> profile_;
   // Weak pointer to a MockUserCloudPolicyStore - lifetime is managed by the
   // UserCloudPolicyManager.
@@ -794,7 +786,6 @@ TEST_F(UserPolicySigninServiceTest, SignOutThenSignInAgain) {
   ASSERT_FALSE(manager_->core()->service());
 
   // Now sign in again.
-  ResetLastFetcherId();
   ASSERT_NO_FATAL_FAILURE(TestSuccessfulSignin());
 }
 
@@ -852,4 +843,6 @@ TEST_F(UserPolicySigninServiceTest, PolicyFetchFailureDisableManagement) {
 #endif
 }
 
+}  // namespace
+
 }  // namespace policy

chrome/browser/signin/profile_oauth2_token_service.cc

@@ -168,15 +168,13 @@ GoogleServiceAuthError ProfileOAuth2TokenService::GetAuthStatus() const {
 }
 
 void ProfileOAuth2TokenService::RegisterCacheEntry(
-    const std::string& client_id,
     const std::string& refresh_token,
     const ScopeSet& scopes,
     const std::string& access_token,
     const base::Time& expiration_date) {
   if (ShouldCacheForRefreshToken(TokenServiceFactory::GetForProfile(profile_),
                                  refresh_token)) {
-    OAuth2TokenService::RegisterCacheEntry(client_id,
-                                           refresh_token,
+    OAuth2TokenService::RegisterCacheEntry(refresh_token,
                                            scopes,
                                            access_token,
                                            expiration_date);

chrome/browser/signin/profile_oauth2_token_service.h

@@ -103,8 +103,7 @@ class ProfileOAuth2TokenService : public OAuth2TokenService,
   // logs back in with a different account, then any in-flight token
   // fetches will be for the old account's refresh token.  Therefore
   // when they come back, they shouldn't be cached.
-  virtual void RegisterCacheEntry(const std::string& client_id,
-                                  const std::string& refresh_token,
+  virtual void RegisterCacheEntry(const std::string& refresh_token,
                                   const ScopeSet& scopes,
                                   const std::string& access_token,
                                   const base::Time& expiration_date) OVERRIDE;

chrome/browser/signin/profile_oauth2_token_service_unittest.cc

@@ -331,7 +331,7 @@ TEST_F(ProfileOAuth2TokenServiceTest, TokenServiceUpdateClearsCache) {
 
   request = oauth2_service_->StartRequest(scope_list, &consumer_);
   base::RunLoop().RunUntilIdle();
-  fetcher = factory_.GetFetcherByID(1);
+  fetcher = factory_.GetFetcherByID(0);
   fetcher->set_response_code(net::HTTP_OK);
   fetcher->SetResponseString(GetValidTokenResponse("another token", 3600));
   fetcher->delegate()->OnURLFetchComplete(fetcher);

chrome/browser/sync/test_profile_sync_service.cc

@@ -319,11 +319,8 @@ scoped_ptr<OAuth2TokenService::Request> FakeOAuth2TokenService::StartRequest(
     OAuth2TokenService::Consumer* consumer) {
   // Ensure token in question is cached and never expires. Request will succeed
   // without network IO.
-  RegisterCacheEntry("test_client_id",
-                     GetRefreshToken(),
-                     scopes,
-                     "access_token",
-                     base::Time::Max());
+  RegisterCacheEntry(GetRefreshToken(), scopes, "access_token",
+      base::Time::Max());
   return ProfileOAuth2TokenService::StartRequest(scopes, consumer);
 }
 

google_apis/gaia/oauth2_access_token_fetcher.cc

@@ -56,14 +56,13 @@ static GoogleServiceAuthError CreateAuthError(URLRequestStatus status) {
   }
 }
 
-static URLFetcher* CreateFetcher(int id,
-                                 URLRequestContextGetter* getter,
+static URLFetcher* CreateFetcher(URLRequestContextGetter* getter,
                                  const GURL& url,
                                  const std::string& body,
                                  URLFetcherDelegate* delegate) {
   bool empty_body = body.empty();
   URLFetcher* result = net::URLFetcher::Create(
-      id, url,
+      0, url,
       empty_body ? URLFetcher::GET : URLFetcher::POST,
       delegate);
 
@@ -83,8 +82,6 @@ static URLFetcher* CreateFetcher(int id,
 }
 }  // namespace
 
-int OAuth2AccessTokenFetcher::last_fetcher_id_ = 0;
-
 OAuth2AccessTokenFetcher::OAuth2AccessTokenFetcher(
     OAuth2AccessTokenConsumer* consumer,
     URLRequestContextGetter* getter)
@@ -113,7 +110,6 @@ void OAuth2AccessTokenFetcher::StartGetAccessToken() {
   CHECK_EQ(INITIAL, state_);
   state_ = GET_ACCESS_TOKEN_STARTED;
   fetcher_.reset(CreateFetcher(
-      last_fetcher_id_++,
       getter_,
       MakeGetAccessTokenUrl(),
       MakeGetAccessTokenBody(
@@ -235,8 +231,3 @@ bool OAuth2AccessTokenFetcher::ParseGetAccessTokenResponse(
   return dict->GetString(kAccessTokenKey, access_token) &&
       dict->GetInteger(kExpiresInKey, expires_in);
 }
-
-// static
-void OAuth2AccessTokenFetcher::ResetLastFetcherIdForTest() {
-  last_fetcher_id_ = 0;
-}

google_apis/gaia/oauth2_access_token_fetcher.h

@@ -26,10 +26,6 @@ class URLRequestContextGetter;
 class URLRequestStatus;
 }
 
-namespace policy {
-  class UserPolicySigninServiceTest;
-}
-
 // Abstracts the details to get OAuth2 access token token from
 // OAuth2 refresh token.
 // See "Using the Refresh Token" section in:
@@ -98,9 +94,6 @@ class OAuth2AccessTokenFetcher : public net::URLFetcherDelegate {
                                           std::string* access_token,
                                           int* expires_in);
 
-  // Resets |last_fetcher_id_| to 0.
-  static void ResetLastFetcherIdForTest();
-
   // State that is set during construction.
   OAuth2AccessTokenConsumer* const consumer_;
   net::URLRequestContextGetter* const getter_;
@@ -113,12 +106,7 @@ class OAuth2AccessTokenFetcher : public net::URLFetcherDelegate {
   std::string refresh_token_;
   std::vector<std::string> scopes_;
 
-  // The last fetcher id.
-  static int last_fetcher_id_;
-
   friend class OAuth2AccessTokenFetcherTest;
-  friend class OAuth2TokenServiceTest;
-  friend class policy::UserPolicySigninServiceTest;
   FRIEND_TEST_ALL_PREFIXES(OAuth2AccessTokenFetcherTest,
                            ParseGetAccessTokenResponse);
   FRIEND_TEST_ALL_PREFIXES(OAuth2AccessTokenFetcherTest,

google_apis/gaia/oauth2_token_service.h

@@ -10,16 +10,12 @@
 #include <string>
 
 #include "base/basictypes.h"
-#include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time/time.h"
-#include "base/timer/timer.h"
 #include "google_apis/gaia/google_service_auth_error.h"
-#include "google_apis/gaia/oauth2_access_token_consumer.h"
-#include "google_apis/gaia/oauth2_access_token_fetcher.h"
 
 namespace net {
 class URLRequestContextGetter;
@@ -149,23 +145,8 @@ class OAuth2TokenService : public base::NonThreadSafe {
   // Return the current number of entries in the cache.
   int cache_size_for_testing() const;
   void set_max_authorization_token_fetch_retries_for_testing(int max_retries);
-  // Returns the current number of pending fetchers matching given params.
-  size_t GetNumPendingRequestsForTesting(
-      const std::string& client_id,
-      const std::string& refresh_token,
-      const ScopeSet& scopes) const;
 
  protected:
-  struct ClientScopeSet {
-    ClientScopeSet(const std::string& client_id,
-                   const ScopeSet& scopes);
-    ~ClientScopeSet();
-    bool operator<(const ClientScopeSet& set) const;
-
-    std::string client_id;
-    ScopeSet scopes;
-  };
-
   // Implements a cancelable |OAuth2TokenService::Request|, which should be
   // operated on the UI thread.
   // TODO(davidroche): move this out of header file.
@@ -197,20 +178,19 @@ class OAuth2TokenService : public base::NonThreadSafe {
   // Add a new entry to the cache.
   // Subclasses can override if there are implementation-specific reasons
   // that an access token should ever not be cached.
-  virtual void RegisterCacheEntry(const std::string& client_id,
-                                  const std::string& refresh_token,
+  virtual void RegisterCacheEntry(const std::string& refresh_token,
                                   const ScopeSet& scopes,
                                   const std::string& access_token,
                                   const base::Time& expiration_date);
 
   // Returns true if GetCacheEntry would return a valid cache entry for the
   // given scopes.
-  bool HasCacheEntry(const ClientScopeSet& client_scopes);
+  bool HasCacheEntry(const ScopeSet& scopes);
 
   // Posts a task to fire the Consumer callback with the cached token.  Must
   // Must only be called if HasCacheEntry() returns true.
   void StartCacheLookupRequest(RequestImpl* request,
-                               const ClientScopeSet& client_scopes,
+                               const ScopeSet& scopes,
                                Consumer* consumer);
 
   // Clears the internal token cache.
@@ -228,6 +208,10 @@ class OAuth2TokenService : public base::NonThreadSafe {
   void FireRefreshTokensLoaded();
   void FireRefreshTokensCleared();
 
+  // Derived classes must provide a request context used for fetching access
+  // tokens with the |StartRequest| method.
+  virtual net::URLRequestContextGetter* GetRequestContext() = 0;
+
   // Fetches an OAuth token for the specified client/scopes. Virtual so it can
   // be overridden for tests and for platform-specific behavior on Android.
   virtual void FetchOAuth2Token(RequestImpl* request,
@@ -235,32 +219,13 @@ class OAuth2TokenService : public base::NonThreadSafe {
                                 const std::string& client_id,
                                 const std::string& client_secret,
                                 const ScopeSet& scopes);
+
  private:
+  // Class that fetches an OAuth2 access token for a given set of scopes and
+  // OAuth2 refresh token.
   class Fetcher;
   friend class Fetcher;
 
-  // The parameters used to fetch an OAuth2 access token.
-  struct FetchParameters {
-    FetchParameters(const std::string& client_id,
-                    const std::string& refresh_token,
-                    const ScopeSet& scopes);
-    ~FetchParameters();
-    bool operator<(const FetchParameters& params) const;
-
-    // OAuth2 client id.
-    std::string client_id;
-    // Refresh token used for minting access tokens within this request.
-    std::string refresh_token;
-    // URL scopes for the requested access token.
-    ScopeSet scopes;
-  };
-
-  typedef std::map<FetchParameters, Fetcher*> PendingFetcherMap;
-
-  // Derived classes must provide a request context used for fetching access
-  // tokens with the |StartRequest| method.
-  virtual net::URLRequestContextGetter* GetRequestContext() = 0;
-
   // Struct that contains the information of an OAuth2 access token.
   struct CacheEntry {
     std::string access_token;
@@ -279,14 +244,14 @@ class OAuth2TokenService : public base::NonThreadSafe {
 
   // Returns a currently valid OAuth2 access token for the given set of scopes,
   // or NULL if none have been cached. Note the user of this method should
-  // ensure no entry with the same |client_scopes| is added before the usage of
-  // the returned entry is done.
-  const CacheEntry* GetCacheEntry(const ClientScopeSet& client_scopes);
+  // ensure no entry with the same |scopes| is added before the usage of the
+  // returned entry is done.
+  const CacheEntry* GetCacheEntry(const ScopeSet& scopes);
 
 
   // Removes an access token for the given set of scopes from the cache.
   // Returns true if the entry was removed, otherwise false.
-  bool RemoveCacheEntry(const ClientScopeSet& client_scopes,
+  bool RemoveCacheEntry(const OAuth2TokenService::ScopeSet& scopes,
                         const std::string& token_to_remove);
 
 
@@ -297,12 +262,15 @@ class OAuth2TokenService : public base::NonThreadSafe {
   void CancelFetchers(std::vector<Fetcher*> fetchers_to_cancel);
 
   // The cache of currently valid tokens.
-  typedef std::map<ClientScopeSet, CacheEntry> TokenCache;
+  typedef std::map<ScopeSet, CacheEntry> TokenCache;
   TokenCache token_cache_;
 
+  // The parameters (refresh token and scope set) used to fetch an OAuth2 access
+  // token.
+  typedef std::pair<std::string, ScopeSet> FetchParameters;
   // A map from fetch parameters to a fetcher that is fetching an OAuth2 access
   // token using these parameters.
-  PendingFetcherMap pending_fetchers_;
+  std::map<FetchParameters, Fetcher*> pending_fetchers_;
 
   // List of observers to notify when token availability changes.
   // Makes sure list is empty on destruction.
@@ -311,11 +279,6 @@ class OAuth2TokenService : public base::NonThreadSafe {
   // Maximum number of retries in fetching an OAuth2 access token.
   static int max_fetch_retry_num_;
 
-  FRIEND_TEST_ALL_PREFIXES(OAuth2TokenServiceTest, ClientScopeSetOrderTest);
-  FRIEND_TEST_ALL_PREFIXES(OAuth2TokenServiceTest, FetchParametersOrderTest);
-  FRIEND_TEST_ALL_PREFIXES(OAuth2TokenServiceTest,
-                           SameScopesRequestedForDifferentClients);
-
   DISALLOW_COPY_AND_ASSIGN(OAuth2TokenService);
 };
 

net/url_request/test_url_fetcher_factory.cc

@@ -275,10 +275,6 @@ void TestURLFetcherFactory::RemoveFetcherFromMap(int id) {
   fetchers_.erase(i);
 }
 
-size_t TestURLFetcherFactory::GetFetcherCount() const {
-  return fetchers_.size();
-}
-
 void TestURLFetcherFactory::SetDelegateForTests(
     TestURLFetcherDelegateForTests* delegate_for_tests) {
   delegate_for_tests_ = delegate_for_tests;

net/url_request/test_url_fetcher_factory.h

@@ -241,7 +241,6 @@ class TestURLFetcherFactory : public URLFetcherFactory,
       URLFetcherDelegate* d) OVERRIDE;
   TestURLFetcher* GetFetcherByID(int id) const;
   void RemoveFetcherFromMap(int id);
-  size_t GetFetcherCount() const;
   void SetDelegateForTests(TestURLFetcherDelegateForTests* delegate_for_tests);
   void set_remove_fetcher_on_delete(bool remove_fetcher_on_delete) {
     remove_fetcher_on_delete_ = remove_fetcher_on_delete;