Have IdentityManager interact w/ ProfileOAuth2TokenService synchronously

The reasoning for this change is analogous to that described in
https://chromium-review.googlesource.com/1061494 for IdentityManager's
interactions with SigninManager. As with that change, once the codebase is
converted away from direct usage of PO2TS entirely to IdentityManager,
it could be a good idea to reintroduce the asynchronous interaction;
reintroducing that implementation would be a good intermediate move to
switching in the Identity Service-backed implementation.

As part of this change, we remove a test of PrimaryAccessTokenFetcher
that assumes that the implementation handles access token requests
asynchronously.

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

services/identity/public/cpp/identity_manager.cc

@@ -3,16 +3,14 @@
 // found in the LICENSE file.
 
 #include "services/identity/public/cpp/identity_manager.h"
-#include "base/threading/sequenced_task_runner_handle.h"
+
 #include "google_apis/gaia/gaia_auth_util.h"
 
 namespace identity {
 
 IdentityManager::IdentityManager(SigninManagerBase* signin_manager,
                                  ProfileOAuth2TokenService* token_service)
-    : signin_manager_(signin_manager),
-      token_service_(token_service),
-      weak_ptr_factory_(this) {
+    : signin_manager_(signin_manager), token_service_(token_service) {
   primary_account_info_ = signin_manager_->GetAuthenticatedAccountInfo();
   signin_manager_->AddObserver(this);
 #if !defined(OS_CHROMEOS)
@@ -95,13 +93,14 @@ void IdentityManager::RemoveAccessTokenFromCache(
     const AccountInfo& account_info,
     const OAuth2TokenService::ScopeSet& scopes,
     const std::string& access_token) {
-  // Call PO2TS asynchronously to mimic the eventual interaction with the
-  // Identity Service.
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&IdentityManager::HandleRemoveAccessTokenFromCache,
-                     weak_ptr_factory_.GetWeakPtr(), account_info.account_id,
-                     scopes, access_token));
+  // TODO(843510): Consider making the request to ProfileOAuth2TokenService
+  // asynchronously once there are no direct clients of PO2TS. This change would
+  // need to be made together with changing all callsites to
+  // ProfileOAuth2TokenService::RequestAccessToken() to be made asynchronously
+  // as well (to maintain ordering in the case where a client removes an access
+  // token from the cache and then immediately requests an access token).
+  token_service_->InvalidateAccessToken(account_info.account_id, scopes,
+                                        access_token);
 }
 
 void IdentityManager::AddObserver(Observer* observer) {
@@ -181,26 +180,8 @@ void IdentityManager::OnAccessTokenRequested(
     const std::string& account_id,
     const std::string& consumer_id,
     const OAuth2TokenService::ScopeSet& scopes) {
-  // Fire observer callbacks asynchronously to mimic this callback itself coming
-  // in asynchronously from the Identity Service rather than synchronously from
-  // ProfileOAuth2TokenService.
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(&IdentityManager::HandleOnAccessTokenRequested,
-                                weak_ptr_factory_.GetWeakPtr(), account_id,
-                                consumer_id, scopes));
-}
-
-void IdentityManager::HandleRemoveAccessTokenFromCache(
-    const std::string& account_id,
-    const OAuth2TokenService::ScopeSet& scopes,
-    const std::string& access_token) {
-  token_service_->InvalidateAccessToken(account_id, scopes, access_token);
-}
-
-void IdentityManager::HandleOnAccessTokenRequested(
-    const std::string& account_id,
-    const std::string& consumer_id,
-    const OAuth2TokenService::ScopeSet& scopes) {
+  // TODO(843510): Consider notifying observers asynchronously once there
+  // are no direct clients of ProfileOAuth2TokenService.
   for (auto& observer : diagnostics_observer_list_) {
     observer.OnAccessTokenRequested(account_id, consumer_id, scopes);
   }

services/identity/public/cpp/identity_manager.h

@@ -5,7 +5,6 @@
 #ifndef SERVICES_IDENTITY_PUBLIC_CPP_IDENTITY_MANAGER_H_
 #define SERVICES_IDENTITY_PUBLIC_CPP_IDENTITY_MANAGER_H_
 
-#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "components/signin/core/browser/account_info.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
@@ -181,19 +180,6 @@ class IdentityManager : public SigninManagerBase::Observer,
       const std::string& consumer_id,
       const OAuth2TokenService::ScopeSet& scopes) override;
 
-  // Removes synchronously token from token_service
-  void HandleRemoveAccessTokenFromCache(
-      const std::string& account_id,
-      const OAuth2TokenService::ScopeSet& scopes,
-      const std::string& access_token);
-
-  // Notifies diagnostics observers. Invoked asynchronously from
-  // OnAccessTokenRequested() to mimic the effect of receiving this call
-  // asynchronously from the Identity Service.
-  void HandleOnAccessTokenRequested(const std::string& account_id,
-                                    const std::string& consumer_id,
-                                    const OAuth2TokenService::ScopeSet& scopes);
-
   // Backing signin classes. NOTE: We strive to limit synchronous access to
   // these classes in the IdentityManager implementation, as all such
   // synchronous access will become impossible when IdentityManager is backed by
@@ -209,8 +195,6 @@ class IdentityManager : public SigninManagerBase::Observer,
   base::ObserverList<Observer, true> observer_list_;
   base::ObserverList<DiagnosticsObserver, true> diagnostics_observer_list_;
 
-  base::WeakPtrFactory<IdentityManager> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(IdentityManager);
 };
 

services/identity/public/cpp/primary_account_access_token_fetcher.cc

@@ -7,7 +7,6 @@
 #include <utility>
 
 #include "base/logging.h"
-#include "base/threading/sequenced_task_runner_handle.h"
 
 namespace identity {
 
@@ -26,8 +25,7 @@ PrimaryAccountAccessTokenFetcher::PrimaryAccountAccessTokenFetcher(
       waiting_for_sign_in_(false),
       waiting_for_refresh_token_(false),
       access_token_retried_(false),
-      mode_(mode),
-      weak_factory_(this) {
+      mode_(mode) {
   Start();
 }
 
@@ -42,7 +40,7 @@ PrimaryAccountAccessTokenFetcher::~PrimaryAccountAccessTokenFetcher() {
 
 void PrimaryAccountAccessTokenFetcher::Start() {
   if (mode_ == Mode::kImmediate) {
-    ScheduleStartAccessTokenRequest();
+    StartAccessTokenRequest();
     return;
   }
 
@@ -68,7 +66,7 @@ void PrimaryAccountAccessTokenFetcher::WaitForRefreshToken() {
   if (token_service_->RefreshTokenIsAvailable(
           signin_manager_->GetAuthenticatedAccountId())) {
     // Already have refresh token: Get the access token directly.
-    ScheduleStartAccessTokenRequest();
+    StartAccessTokenRequest();
     return;
   }
 
@@ -78,22 +76,15 @@ void PrimaryAccountAccessTokenFetcher::WaitForRefreshToken() {
   token_service_->AddObserver(this);
 }
 
-void PrimaryAccountAccessTokenFetcher::ScheduleStartAccessTokenRequest() {
-  // Fire off the request asynchronously to mimic the asynchronous flow that
-  // will occur when this request is going through the Identity Service.
-  // NOTE: Posting the task using a WeakPtr is necessary as this instance
-  // might die before the posted task runs (https://crbug.com/800263).
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&PrimaryAccountAccessTokenFetcher::StartAccessTokenRequest,
-                     weak_factory_.GetWeakPtr()));
-}
-
 void PrimaryAccountAccessTokenFetcher::StartAccessTokenRequest() {
   // Note: We might get here even in cases where we know that there's no refresh
   // token. We're requesting an access token anyway, so that the token service
   // will generate an appropriate error code that we can return to the client.
   DCHECK(!access_token_request_);
+
+  // TODO(843510): Consider making the request to ProfileOAuth2TokenService
+  // asynchronously once there are no direct clients of PO2TS (i.e., PO2TS is
+  // used only by this class and IdentityManager).
   access_token_request_ = token_service_->StartRequest(
       signin_manager_->GetAuthenticatedAccountId(), scopes_, this);
 }
@@ -135,7 +126,7 @@ void PrimaryAccountAccessTokenFetcher::OnRefreshTokenAvailable(
 
   waiting_for_refresh_token_ = false;
   token_service_->RemoveObserver(this);
-  ScheduleStartAccessTokenRequest();
+  StartAccessTokenRequest();
 }
 
 void PrimaryAccountAccessTokenFetcher::OnRefreshTokensLoaded() {
@@ -150,7 +141,7 @@ void PrimaryAccountAccessTokenFetcher::OnRefreshTokensLoaded() {
   // provide us with an appropriate error code.
   waiting_for_refresh_token_ = false;
   token_service_->RemoveObserver(this);
-  ScheduleStartAccessTokenRequest();
+  StartAccessTokenRequest();
 }
 
 void PrimaryAccountAccessTokenFetcher::OnGetTokenSuccess(
@@ -188,7 +179,7 @@ void PrimaryAccountAccessTokenFetcher::OnGetTokenFailure(
       token_service_->RefreshTokenIsAvailable(
           signin_manager_->GetAuthenticatedAccountId())) {
     access_token_retried_ = true;
-    ScheduleStartAccessTokenRequest();
+    StartAccessTokenRequest();
     return;
   }
 

services/identity/public/cpp/primary_account_access_token_fetcher.h

@@ -10,7 +10,6 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 #include "google_apis/gaia/oauth2_token_service.h"
@@ -60,7 +59,6 @@ class PrimaryAccountAccessTokenFetcher : public SigninManagerBase::Observer,
   void Start();
 
   void WaitForRefreshToken();
-  void ScheduleStartAccessTokenRequest();
   void StartAccessTokenRequest();
 
   // SigninManagerBase::Observer implementation.
@@ -94,8 +92,6 @@ class PrimaryAccountAccessTokenFetcher : public SigninManagerBase::Observer,
 
   Mode mode_;
 
-  base::WeakPtrFactory<PrimaryAccountAccessTokenFetcher> weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(PrimaryAccountAccessTokenFetcher);
 };
 

services/identity/public/cpp/primary_account_access_token_fetcher_unittest.cc

@@ -207,34 +207,6 @@ TEST_F(PrimaryAccountAccessTokenFetcherTest, ShouldNotReplyIfDestroyed) {
       base::Time::Now() + base::TimeDelta::FromHours(1));
 }
 
-TEST_F(PrimaryAccountAccessTokenFetcherTest, ShouldNotRequestIfDestroyedEarly) {
-  TestTokenCallback callback;
-
-  base::RunLoop run_loop;
-  set_on_access_token_request_callback(
-      base::BindOnce([]() { EXPECT_TRUE(false); }));
-
-  SignIn("account");
-  token_service()->UpdateCredentials("account", "refresh token");
-
-  // Signed in and refresh token already exists, so this should result in
-  // posting a task to make a request for an access token.
-  auto fetcher = CreateFetcher(
-      callback.Get(), PrimaryAccountAccessTokenFetcher::Mode::kImmediate);
-
-  // Destroy the fetcher immediately.
-  fetcher.reset();
-
-  // No access token request should occur (i.e., the posted task should not
-  // actually execute).
-  run_loop.RunUntilIdle();
-
-  // Now fulfilling the access token request should have no effect.
-  token_service()->IssueAllTokensForAccount(
-      "account", "access token",
-      base::Time::Now() + base::TimeDelta::FromHours(1));
-}
-
 TEST_F(PrimaryAccountAccessTokenFetcherTest, OneShotCallsBackWhenSignedOut) {
   base::RunLoop run_loop;