[signin] Remove inconsistent url loader factories

Several classes were taking both a SigninClient and a URLLoaderFactory.
However, SigninClient has a GetURLLoaderFactory() method, and this
was leading to a situation where code had access to two instances of
URLLoaderFactory, and would use one or the other in a random fashion.

In production, these factories were the same in practice, but in tests
this was not always the case. As a result, it was not clear what the test
were doing, and it also made the code harder to understand.

This CL removes this confusion, by always using the factory from the client
when it is available.
It also removes the unnecessary url_loader_factory parameter from
SigninClient::CreateGaiaAuthFetcher.

TBR=eugenebut

Bug: 863347
Change-Id: Ie47b56226b126fbdc753cfd170c7fead65d6952b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1569137
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#663696}

chrome/browser/signin/chrome_signin_client.cc

@@ -301,10 +301,9 @@ void ChromeSigninClient::DelayNetworkCall(base::OnceClosure callback) {
 
 std::unique_ptr<GaiaAuthFetcher> ChromeSigninClient::CreateGaiaAuthFetcher(
     GaiaAuthConsumer* consumer,
-    gaia::GaiaSource source,
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory) {
+    gaia::GaiaSource source) {
   return std::make_unique<GaiaAuthFetcher>(consumer, source,
-                                           url_loader_factory);
+                                           GetURLLoaderFactory());
 }
 
 void ChromeSigninClient::VerifySyncToken() {

chrome/browser/signin/chrome_signin_client.h

@@ -62,9 +62,7 @@ class ChromeSigninClient
   void DelayNetworkCall(base::OnceClosure callback) override;
   std::unique_ptr<GaiaAuthFetcher> CreateGaiaAuthFetcher(
       GaiaAuthConsumer* consumer,
-      gaia::GaiaSource source,
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
-      override;
+      gaia::GaiaSource source) override;
 
   // Returns a string describing the chrome version environment. Version format:
   // <Build Info> <OS> <Version number> (<Last change>)<channel or "-devel">

chrome/browser/signin/dice_response_handler.cc

@@ -176,8 +176,8 @@ DiceResponseHandler::DiceTokenFetcher::DiceTokenFetcher(
   DCHECK(dice_response_handler_);
   account_reconcilor_lock_ =
       std::make_unique<AccountReconcilor::Lock>(account_reconcilor);
-  gaia_auth_fetcher_ = signin_client->CreateGaiaAuthFetcher(
-      this, gaia::GaiaSource::kChrome, signin_client->GetURLLoaderFactory());
+  gaia_auth_fetcher_ =
+      signin_client->CreateGaiaAuthFetcher(this, gaia::GaiaSource::kChrome);
   VLOG(1) << "Start fetching token for account: " << email;
   gaia_auth_fetcher_->StartAuthCodeForOAuth2TokenExchange(authorization_code_);
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(

chrome/browser/signin/dice_response_handler_unittest.cc

@@ -52,17 +52,14 @@ class DiceTestSigninClient : public TestSigninClient, public GaiaAuthConsumer {
 
   std::unique_ptr<GaiaAuthFetcher> CreateGaiaAuthFetcher(
       GaiaAuthConsumer* consumer,
-      gaia::GaiaSource source,
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
-      override {
+      gaia::GaiaSource source) override {
     DCHECK(!consumer_ || (consumer_ == consumer));
     consumer_ = consumer;
 
     // Pass |this| as a dummy consumer to CreateGaiaAuthFetcher().
     // Since DiceTestSigninClient does not overrides any consumer method,
     // everything will be dropped on the floor.
-    return TestSigninClient::CreateGaiaAuthFetcher(this, source,
-                                                   url_loader_factory);
+    return TestSigninClient::CreateGaiaAuthFetcher(this, source);
   }
 
   GaiaAuthConsumer* consumer_;

components/signin/core/browser/account_reconcilor_unittest.cc

@@ -328,8 +328,8 @@ INSTANTIATE_TEST_SUITE_P(Dice_Mirror,
 
 AccountReconcilorTest::AccountReconcilorTest()
     : account_consistency_(signin::AccountConsistencyMethod::kDisabled),
-      test_signin_client_(&pref_service_),
-      identity_test_env_(&test_url_loader_factory_,
+      test_signin_client_(&pref_service_, &test_url_loader_factory_),
+      identity_test_env_(/*test_url_loader_factory=*/nullptr,
                          &pref_service_,
                          account_consistency_,
                          &test_signin_client_) {

components/signin/core/browser/gaia_cookie_manager_service.cc

@@ -259,7 +259,7 @@ void GaiaCookieManagerService::ExternalCcResultFetcher::Start() {
   CleanupTransientState();
   results_.clear();
   helper_->gaia_auth_fetcher_ = helper_->signin_client_->CreateGaiaAuthFetcher(
-      this, gaia::GaiaSource::kChrome, helper_->GetURLLoaderFactory());
+      this, gaia::GaiaSource::kChrome);
   helper_->gaia_auth_fetcher_->StartGetCheckConnectionInfo();
 
   // Some fetches may timeout.  Start a timer to decide when the result fetcher
@@ -451,20 +451,8 @@ void GaiaCookieManagerService::ExternalCcResultFetcher::
 GaiaCookieManagerService::GaiaCookieManagerService(
     OAuth2TokenService* token_service,
     SigninClient* signin_client)
-    : GaiaCookieManagerService(
-          token_service,
-          signin_client,
-          base::BindRepeating(&SigninClient::GetURLLoaderFactory,
-                              base::Unretained(signin_client))) {}
-
-GaiaCookieManagerService::GaiaCookieManagerService(
-    OAuth2TokenService* token_service,
-    SigninClient* signin_client,
-    base::RepeatingCallback<scoped_refptr<network::SharedURLLoaderFactory>()>
-        shared_url_loader_factory_getter)
     : token_service_(token_service),
       signin_client_(signin_client),
-      shared_url_loader_factory_getter_(shared_url_loader_factory_getter),
       external_cc_result_fetcher_(this),
       fetcher_backoff_(&kBackoffPolicy),
       fetcher_retries_(0),
@@ -684,7 +672,7 @@ void GaiaCookieManagerService::CancelAll() {
 
 scoped_refptr<network::SharedURLLoaderFactory>
 GaiaCookieManagerService::GetURLLoaderFactory() {
-  return shared_url_loader_factory_getter_.Run();
+  return signin_client_->GetURLLoaderFactory();
 }
 
 void GaiaCookieManagerService::MarkListAccountsStale() {
@@ -939,18 +927,17 @@ void GaiaCookieManagerService::StartFetchingUbertoken() {
                      base::Unretained(this)),
       base::BindRepeating(
           [](SigninClient* client,
-             scoped_refptr<network::SharedURLLoaderFactory> url_loader,
              GaiaAuthConsumer* consumer) -> std::unique_ptr<GaiaAuthFetcher> {
-            return client->CreateGaiaAuthFetcher(
-                consumer, gaia::GaiaSource::kChrome, url_loader);
+            return client->CreateGaiaAuthFetcher(consumer,
+                                                 gaia::GaiaSource::kChrome);
           },
-          base::Unretained(signin_client_), GetURLLoaderFactory()));
+          base::Unretained(signin_client_)));
 }
 
 void GaiaCookieManagerService::StartFetchingMergeSession() {
   DCHECK(!uber_token_.empty());
-  gaia_auth_fetcher_ = signin_client_->CreateGaiaAuthFetcher(
-      this, requests_.front().source(), GetURLLoaderFactory());
+  gaia_auth_fetcher_ =
+      signin_client_->CreateGaiaAuthFetcher(this, requests_.front().source());
 
   gaia_auth_fetcher_->StartMergeSession(
       uber_token_, external_cc_result_fetcher_.GetExternalCcResult());
@@ -967,15 +954,15 @@ void GaiaCookieManagerService::StartGaiaLogOut() {
 
 void GaiaCookieManagerService::StartFetchingLogOut() {
   RecordLogoutRequestState(LogoutRequestState::kStarted);
-  gaia_auth_fetcher_ = signin_client_->CreateGaiaAuthFetcher(
-      this, requests_.front().source(), GetURLLoaderFactory());
+  gaia_auth_fetcher_ =
+      signin_client_->CreateGaiaAuthFetcher(this, requests_.front().source());
   gaia_auth_fetcher_->StartLogOut();
 }
 
 void GaiaCookieManagerService::StartFetchingListAccounts() {
   VLOG(1) << "GaiaCookieManagerService::ListAccounts";
-  gaia_auth_fetcher_ = signin_client_->CreateGaiaAuthFetcher(
-      this, requests_.front().source(), GetURLLoaderFactory());
+  gaia_auth_fetcher_ =
+      signin_client_->CreateGaiaAuthFetcher(this, requests_.front().source());
   gaia_auth_fetcher_->StartListAccounts();
 }
 

components/signin/core/browser/gaia_cookie_manager_service.h

@@ -228,19 +228,6 @@ class GaiaCookieManagerService : public GaiaAuthConsumer,
   GaiaCookieManagerService(OAuth2TokenService* token_service,
                            SigninClient* signin_client);
 
-  // Creates a GaiaCookieManagerService that uses the provided
-  // |shared_url_loader_factory_getter| to determine the SharedUrlLoaderFactory
-  // used for cookie-related requests.
-  // Note: SharedUrlLoaderFactory is passed via callback, so that if the
-  // callback has side-effects (e.g. network initialization), they do not occur
-  // until the first time GaiaCookieManagerService::GetSharedUrlLoaderFactory is
-  // called.
-  GaiaCookieManagerService(
-      OAuth2TokenService* token_service,
-      SigninClient* signin_client,
-      base::RepeatingCallback<scoped_refptr<network::SharedURLLoaderFactory>()>
-          shared_url_loader_factory_getter);
-
   ~GaiaCookieManagerService() override;
 
   void InitCookieListener();
@@ -373,8 +360,6 @@ class GaiaCookieManagerService : public GaiaAuthConsumer,
   OAuth2TokenService* token_service_;
   SigninClient* signin_client_;
 
-  base::RepeatingCallback<scoped_refptr<network::SharedURLLoaderFactory>()>
-      shared_url_loader_factory_getter_;
   std::unique_ptr<GaiaAuthFetcher> gaia_auth_fetcher_;
   std::unique_ptr<signin::UbertokenFetcherImpl> uber_token_fetcher_;
   ExternalCcResultFetcher external_cc_result_fetcher_;

components/signin/core/browser/gaia_cookie_manager_service_unittest.cc

@@ -96,11 +96,7 @@ class InstrumentedGaiaCookieManagerService : public GaiaCookieManagerService {
  public:
   InstrumentedGaiaCookieManagerService(OAuth2TokenService* token_service,
                                        SigninClient* signin_client)
-      : GaiaCookieManagerService(
-            token_service,
-            signin_client,
-            base::BindRepeating(&SigninClient::GetURLLoaderFactory,
-                                base::Unretained(signin_client))) {
+      : GaiaCookieManagerService(token_service, signin_client) {
     total++;
   }
 

components/signin/core/browser/oauth_multilogin_helper.cc

@@ -100,8 +100,8 @@ void OAuthMultiloginHelper::OnAccessTokensFailure(
 
 void OAuthMultiloginHelper::StartFetchingMultiLogin() {
   DCHECK_EQ(token_id_pairs_.size(), account_ids_.size());
-  gaia_auth_fetcher_ = signin_client_->CreateGaiaAuthFetcher(
-      this, gaia::GaiaSource::kChrome, signin_client_->GetURLLoaderFactory());
+  gaia_auth_fetcher_ =
+      signin_client_->CreateGaiaAuthFetcher(this, gaia::GaiaSource::kChrome);
   gaia_auth_fetcher_->StartOAuthMultilogin(token_id_pairs_);
 }
 

components/signin/core/browser/signin_client.h

@@ -95,8 +95,7 @@ class SigninClient : public KeyedService {
   // Creates a new platform-specific GaiaAuthFetcher.
   virtual std::unique_ptr<GaiaAuthFetcher> CreateGaiaAuthFetcher(
       GaiaAuthConsumer* consumer,
-      gaia::GaiaSource source,
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory) = 0;
+      gaia::GaiaSource source) = 0;
 
   // Schedules migration to happen at next startup.
   virtual void SetReadyForDiceMigration(bool is_ready) {}

components/signin/core/browser/test_signin_client.cc

@@ -12,8 +12,11 @@
 #include "services/network/test/test_cookie_manager.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-TestSigninClient::TestSigninClient(PrefService* pref_service)
-    : pref_service_(pref_service),
+TestSigninClient::TestSigninClient(
+    PrefService* pref_service,
+    network::TestURLLoaderFactory* test_url_loader_factory)
+    : test_url_loader_factory_(test_url_loader_factory),
+      pref_service_(pref_service),
       are_signin_cookies_allowed_(true),
       network_calls_delayed_(false),
       is_signout_allowed_(true),
@@ -37,7 +40,7 @@ void TestSigninClient::PreSignOut(
 
 scoped_refptr<network::SharedURLLoaderFactory>
 TestSigninClient::GetURLLoaderFactory() {
-  return test_url_loader_factory_.GetSafeWeakWrapper();
+  return test_url_loader_factory()->GetSafeWeakWrapper();
 }
 
 network::mojom::CookieManager* TestSigninClient::GetCookieManager() {
@@ -46,6 +49,25 @@ network::mojom::CookieManager* TestSigninClient::GetCookieManager() {
   return cookie_manager_.get();
 }
 
+network::TestURLLoaderFactory* TestSigninClient::test_url_loader_factory() {
+  if (test_url_loader_factory_)
+    return test_url_loader_factory_;
+
+  if (!default_test_url_loader_factory_) {
+    default_test_url_loader_factory_ =
+        std::make_unique<network::TestURLLoaderFactory>();
+  }
+
+  return default_test_url_loader_factory_.get();
+}
+
+void TestSigninClient::OverrideTestUrlLoaderFactory(
+    network::TestURLLoaderFactory* factory) {
+  DCHECK(!default_test_url_loader_factory_);
+  DCHECK(!test_url_loader_factory_);
+  test_url_loader_factory_ = factory;
+}
+
 std::string TestSigninClient::GetProductVersion() { return ""; }
 
 void TestSigninClient::SetNetworkCallsDelayed(bool value) {
@@ -92,10 +114,9 @@ void TestSigninClient::DelayNetworkCall(base::OnceClosure callback) {
 
 std::unique_ptr<GaiaAuthFetcher> TestSigninClient::CreateGaiaAuthFetcher(
     GaiaAuthConsumer* consumer,
-    gaia::GaiaSource source,
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory) {
+    gaia::GaiaSource source) {
   return std::make_unique<GaiaAuthFetcher>(consumer, source,
-                                           url_loader_factory);
+                                           GetURLLoaderFactory());
 }
 
 void TestSigninClient::PreGaiaLogout(base::OnceClosure callback) {

components/signin/core/browser/test_signin_client.h

@@ -27,7 +27,9 @@ class PrefService;
 // part of its interface.
 class TestSigninClient : public SigninClient {
  public:
-  TestSigninClient(PrefService* pref_service);
+  TestSigninClient(
+      PrefService* pref_service,
+      network::TestURLLoaderFactory* test_url_loader_factory = nullptr);
   ~TestSigninClient() override;
 
   // SigninClient implementation that is specialized for unit tests.
@@ -57,9 +59,12 @@ class TestSigninClient : public SigninClient {
     cookie_manager_ = std::move(cookie_manager);
   }
 
-  network::TestURLLoaderFactory* test_url_loader_factory() {
-    return &test_url_loader_factory_;
-  }
+  // Returns |test_url_loader_factory_| if it is specified. Otherwise, lazily
+  // creates a default factory and returns it.
+  network::TestURLLoaderFactory* test_url_loader_factory();
+
+  // Pass a TestURLLoader factory to use instead of the default one.
+  void OverrideTestUrlLoaderFactory(network::TestURLLoaderFactory* factory);
 
   void set_are_signin_cookies_allowed(bool value) {
     are_signin_cookies_allowed_ = value;
@@ -87,14 +92,14 @@ class TestSigninClient : public SigninClient {
   void DelayNetworkCall(base::OnceClosure callback) override;
   std::unique_ptr<GaiaAuthFetcher> CreateGaiaAuthFetcher(
       GaiaAuthConsumer* consumer,
-      gaia::GaiaSource source,
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
-      override;
+      gaia::GaiaSource source) override;
   void PreGaiaLogout(base::OnceClosure callback) override;
   void SetReadyForDiceMigration(bool ready) override;
 
  private:
-  network::TestURLLoaderFactory test_url_loader_factory_;
+  std::unique_ptr<network::TestURLLoaderFactory>
+      default_test_url_loader_factory_;
+  network::TestURLLoaderFactory* test_url_loader_factory_;
 
   PrefService* pref_service_;
   std::unique_ptr<network::mojom::CookieManager> cookie_manager_;

components/signin/ios/browser/account_consistency_service_unittest.mm

@@ -130,9 +130,10 @@ void SetUp() override {
     HostContentSettingsMap::RegisterProfilePrefs(prefs_.registry());
 
     web_view_load_expection_count_ = 0;
-    signin_client_.reset(new TestSigninClient(&prefs_));
+    signin_client_.reset(
+        new TestSigninClient(&prefs_, &test_url_loader_factory_));
     identity_test_env_.reset(new identity::IdentityTestEnvironment(
-        &test_url_loader_factory_, &prefs_,
+        /*test_url_loader_factory=*/nullptr, &prefs_,
         signin::AccountConsistencyMethod::kDisabled, signin_client_.get()));
     settings_map_ = new HostContentSettingsMap(
         &prefs_, false /* is_off_the_record */, false /* store_last_modified */,

ios/chrome/browser/signin/identity_test_environment_chrome_browser_state_adaptor.cc

@@ -110,8 +110,7 @@ std::unique_ptr<KeyedService> IdentityTestEnvironmentChromeBrowserStateAdaptor::
   return identity::IdentityTestEnvironment::BuildIdentityManagerForTests(
       SigninClientFactory::GetForBrowserState(chrome_browser_state),
       chrome_browser_state->GetPrefs(), base::FilePath(),
-      signin::AccountConsistencyMethod::kMirror,
-      /*test_url_loader_factory=*/nullptr, std::move(extra_params));
+      signin::AccountConsistencyMethod::kMirror, std::move(extra_params));
 }
 
 // static

ios/chrome/browser/signin/ios_chrome_signin_client.h

@@ -35,9 +35,7 @@ class IOSChromeSigninClient : public SigninClient {
   std::string GetProductVersion() override;
   std::unique_ptr<GaiaAuthFetcher> CreateGaiaAuthFetcher(
       GaiaAuthConsumer* consumer,
-      gaia::GaiaSource source,
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
-      override;
+      gaia::GaiaSource source) override;
   void PreGaiaLogout(base::OnceClosure callback) override;
   PrefService* GetPrefs() override;
   scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() override;

ios/chrome/browser/signin/ios_chrome_signin_client.mm

@@ -95,10 +95,9 @@
 
 std::unique_ptr<GaiaAuthFetcher> IOSChromeSigninClient::CreateGaiaAuthFetcher(
     GaiaAuthConsumer* consumer,
-    gaia::GaiaSource source,
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory) {
+    gaia::GaiaSource source) {
   return std::make_unique<GaiaAuthFetcherIOS>(
-      consumer, source, url_loader_factory, browser_state_);
+      consumer, source, GetURLLoaderFactory(), browser_state_);
 }
 
 void IOSChromeSigninClient::PreGaiaLogout(base::OnceClosure callback) {

ios/web_view/internal/signin/ios_web_view_signin_client.h

@@ -52,9 +52,7 @@ class IOSWebViewSigninClient : public SigninClient {
   void DelayNetworkCall(base::OnceClosure callback) override;
   std::unique_ptr<GaiaAuthFetcher> CreateGaiaAuthFetcher(
       GaiaAuthConsumer* consumer,
-      gaia::GaiaSource source,
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
-      override;
+      gaia::GaiaSource source) override;
 
   // CWVSyncController setter/getter.
   void SetSyncController(CWVSyncController* sync_controller);

ios/web_view/internal/signin/ios_web_view_signin_client.mm

@@ -95,10 +95,9 @@
 
 std::unique_ptr<GaiaAuthFetcher> IOSWebViewSigninClient::CreateGaiaAuthFetcher(
     GaiaAuthConsumer* consumer,
-    gaia::GaiaSource source,
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory) {
+    gaia::GaiaSource source) {
   return std::make_unique<GaiaAuthFetcher>(consumer, source,
-                                           url_loader_factory);
+                                           GetURLLoaderFactory());
 }
 
 void IOSWebViewSigninClient::SetSyncController(

services/identity/public/cpp/accounts_cookie_mutator_unittest.cc

@@ -63,7 +63,7 @@ class AccountsCookieMutatorTest : public testing::Test {
  public:
   AccountsCookieMutatorTest()
       : test_signin_client_(&prefs_),
-        identity_test_env_(test_url_loader_factory(),
+        identity_test_env_(/*test_url_loader_factory=*/nullptr,
                            &prefs_,
                            signin::AccountConsistencyMethod::kDisabled,
                            &test_signin_client_) {}