[Signin] Fetcher should retry on Multilogin failure.

Also retry fetching access token if it failed but count retries for all tokens together.

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I1c8d131faa7fba5a6d4c43714b998bc140642864
Reviewed-on: https://chromium-review.googlesource.com/1199424
Commit-Queue: Valeriya Sinevich <valeriyas@google.com>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593170}

components/signin/core/browser/gaia_cookie_manager_service.cc

@@ -63,9 +63,6 @@ const net::BackoffEntry::Policy kBackoffPolicy = {
     false,
 };
 
-// The maximum number of retries for a fetcher used in this class.
-const int kMaxFetcherRetries = 8;
-
 // Name of the GAIA cookie that is being observed to detect when available
 // accounts have changed in the content-area.
 const char* const kGaiaCookieName = "APISID";
@@ -411,19 +408,17 @@ void GaiaCookieManagerService::SetAccountsInCookie(
     const std::string& source) {
   VLOG(1) << "GaiaCookieManagerService::SetAccountsInCookie: "
           << base::JoinString(account_ids, " ");
-  // TODO(valeriyas): clear access_tokens_ here (introduced in follow-up cls)
   if (!signin_client_->AreSigninCookiesAllowed()) {
-    for (const std::string account_id : account_ids) {
-      SignalComplete(account_id, GoogleServiceAuthError(
-                                     GoogleServiceAuthError::REQUEST_CANCELED));
-    }
+    OnSetAccountsFinished(
+        GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
     return;
   }
   requests_.push_back(
       GaiaCookieRequest::CreateSetAccountsRequest(account_ids, source));
   if (requests_.size() == 1) {
+    fetcher_retries_ = 0;
     signin_client_->DelayNetworkCall(
-        base::Bind(&GaiaCookieManagerService::StartFetchingAccesstokens,
+        base::Bind(&GaiaCookieManagerService::StartFetchingAccessTokens,
                    base::Unretained(this)));
   }
 }
@@ -728,9 +723,11 @@ void GaiaCookieManagerService::OnGetTokenSuccess(
     const OAuth2AccessTokenConsumer::TokenResponse& token_response) {
   DCHECK(requests_.front().request_type() ==
          GaiaCookieRequestType::SET_ACCOUNTS);
+  fetcher_backoff_.InformOfRequest(true);
   access_tokens_.insert(
       std::make_pair(request->GetAccountId(), token_response.access_token));
   if (access_tokens_.size() == requests_.front().account_ids().size()) {
+    fetcher_retries_ = 0;
     token_requests_.clear();
     signin_client_->DelayNetworkCall(
         base::Bind(&GaiaCookieManagerService::SetAccountsInCookieWithTokens,
@@ -744,7 +741,20 @@ void GaiaCookieManagerService::OnGetTokenFailure(
   VLOG(1) << "Failed to retrieve accesstoken"
           << " account=" << request->GetAccountId()
           << " error=" << error.ToString();
-  OnOAuthMultiloginFailure(error);
+  if (++fetcher_retries_ < signin::kMaxFetcherRetries &&
+      error.IsTransientError()) {
+    fetcher_backoff_.InformOfRequest(false);
+    OAuth2TokenService::ScopeSet scopes;
+    scopes.insert(GaiaConstants::kOAuth1LoginScope);
+    fetcher_timer_.Start(
+        FROM_HERE, fetcher_backoff_.GetTimeUntilRelease(),
+        base::BindOnce(
+            &SigninClient::DelayNetworkCall, base::Unretained(signin_client_),
+            base::Bind(&GaiaCookieManagerService::StartFetchingAccessToken,
+                       base::Unretained(this), request->GetAccountId())));
+    return;
+  }
+  OnSetAccountsFinished(error);
 }
 
 void GaiaCookieManagerService::OnMergeSessionSuccess(const std::string& data) {
@@ -769,7 +779,8 @@ void GaiaCookieManagerService::OnMergeSessionFailure(
   const std::string account_id = requests_.front().GetAccountID();
   VLOG(1) << "Failed MergeSession"
           << " account=" << account_id << " error=" << error.ToString();
-  if (++fetcher_retries_ < kMaxFetcherRetries && error.IsTransientError()) {
+  if (++fetcher_retries_ < signin::kMaxFetcherRetries &&
+      error.IsTransientError()) {
     fetcher_backoff_.InformOfRequest(false);
     UMA_HISTOGRAM_ENUMERATION("OAuth2Login.MergeSessionRetry",
         error.state(), GoogleServiceAuthError::NUM_STATES);
@@ -798,6 +809,7 @@ void GaiaCookieManagerService::OnOAuthMultiloginSuccess(
           << base::JoinString(requests_.front().account_ids(), " ");
   std::vector<std::string> account_ids = requests_.front().account_ids();
   access_tokens_.clear();
+  fetcher_backoff_.InformOfRequest(true);
   StartSettingCookies(result);
 }
 
@@ -806,6 +818,17 @@ void GaiaCookieManagerService::OnOAuthMultiloginFailure(
   VLOG(1) << "Failed Multilogin for accounts: "
           << base::JoinString(requests_.front().account_ids(), " ")
           << " error=" << error.ToString();
+  if (++fetcher_retries_ < signin::kMaxFetcherRetries &&
+      error.IsTransientError()) {
+    fetcher_backoff_.InformOfRequest(false);
+    fetcher_timer_.Start(
+        FROM_HERE, fetcher_backoff_.GetTimeUntilRelease(),
+        base::BindOnce(
+            &SigninClient::DelayNetworkCall, base::Unretained(signin_client_),
+            base::Bind(&GaiaCookieManagerService::SetAccountsInCookieWithTokens,
+                       base::Unretained(this))));
+    return;
+  }
   OnSetAccountsFinished(error);
 }
 
@@ -850,7 +873,8 @@ void GaiaCookieManagerService::OnListAccountsFailure(
   VLOG(1) << "ListAccounts failed";
   DCHECK(requests_.front().request_type() ==
          GaiaCookieRequestType::LIST_ACCOUNTS);
-  if (++fetcher_retries_ < kMaxFetcherRetries && error.IsTransientError()) {
+  if (++fetcher_retries_ < signin::kMaxFetcherRetries &&
+      error.IsTransientError()) {
     fetcher_backoff_.InformOfRequest(false);
     UMA_HISTOGRAM_ENUMERATION("Signin.ListAccountsRetry",
         error.state(), GoogleServiceAuthError::NUM_STATES);
@@ -889,7 +913,7 @@ void GaiaCookieManagerService::OnLogOutFailure(
   DCHECK(requests_.front().request_type() == GaiaCookieRequestType::LOG_OUT);
   VLOG(1) << "GaiaCookieManagerService::OnLogOutFailure";
 
-  if (++fetcher_retries_ < kMaxFetcherRetries) {
+  if (++fetcher_retries_ < signin::kMaxFetcherRetries) {
     fetcher_backoff_.InformOfRequest(false);
     fetcher_timer_.Start(
         FROM_HERE, fetcher_backoff_.GetTimeUntilRelease(),
@@ -905,14 +929,22 @@ void GaiaCookieManagerService::OnLogOutFailure(
   HandleNextRequest();
 }
 
-void GaiaCookieManagerService::StartFetchingAccesstokens() {
-  VLOG(1) << "GaiaCookieManagerService::StartFetchingAccesstoken account_id ="
-          << base::JoinString(requests_.front().account_ids(), " ");
+void GaiaCookieManagerService::StartFetchingAccessToken(
+    const std::string& account_id) {
   OAuth2TokenService::ScopeSet scopes;
   scopes.insert(GaiaConstants::kOAuth1LoginScope);
+  token_requests_.push_back(
+      token_service_->StartRequest(account_id, scopes, this));
+}
+
+void GaiaCookieManagerService::StartFetchingAccessTokens() {
+  DCHECK_EQ(SET_ACCOUNTS, requests_.front().request_type());
+  VLOG(1) << "GaiaCookieManagerService::StartFetchingAccessToken account_id ="
+          << base::JoinString(requests_.front().account_ids(), " ");
+  token_requests_.clear();
+  access_tokens_.clear();
   for (const std::string& account_id : requests_.front().account_ids()) {
-    token_requests_.push_back(
-        token_service_->StartRequest(account_id, scopes, this));
+    StartFetchingAccessToken(account_id);
   }
 }
 
@@ -1002,13 +1034,14 @@ void GaiaCookieManagerService::StartSettingCookies(
   const std::vector<net::CanonicalCookie>& cookies = result.cookies();
 
   for (const net::CanonicalCookie& cookie : cookies) {
-    const auto& inserted =
-        cookies_to_set_.insert(std::make_pair(cookie.Name(), cookie.Domain()));
-    if (inserted.second) {
+    cookies_to_set_.insert(std::make_pair(cookie.Name(), cookie.Domain()));
+  }
+  for (const net::CanonicalCookie& cookie : cookies) {
+    if (cookies_to_set_.find(std::make_pair(cookie.Name(), cookie.Domain())) !=
+        cookies_to_set_.end()) {
       base::OnceCallback<void(bool success)> callback = base::Bind(
           &GaiaCookieManagerService::OnCookieSet,
           weak_ptr_factory_.GetWeakPtr(), cookie.Name(), cookie.Domain());
-      // It is assumed that OnCookieSet() is not run synchronously.
       cookie_manager->SetCanonicalCookie(
           cookie, true, true,
           mojo::WrapCallbackWithDefaultInvokeIfNotRun(std::move(callback),
@@ -1050,7 +1083,7 @@ void GaiaCookieManagerService::HandleNextRequest() {
       case GaiaCookieRequestType::SET_ACCOUNTS:
         DCHECK(!requests_.front().account_ids().empty());
         signin_client_->DelayNetworkCall(
-            base::Bind(&GaiaCookieManagerService::StartFetchingAccesstokens,
+            base::Bind(&GaiaCookieManagerService::StartFetchingAccessTokens,
                        base::Unretained(this)));
         break;
       case GaiaCookieRequestType::LOG_OUT:

components/signin/core/browser/gaia_cookie_manager_service.h

@@ -36,6 +36,11 @@ class SharedURLLoaderFactory;
 class SimpleURLLoader;
 }
 
+namespace signin {
+// The maximum number of retries for a fetcher used in this class.
+constexpr int kMaxFetcherRetries = 8;
+}  // namespace signin
+
 // Merges a Google account known to Chrome into the cookie jar.  When merging
 // multiple accounts, one instance of the helper is better than multiple
 // instances if there is the possibility that they run concurrently, since
@@ -209,7 +214,8 @@ class GaiaCookieManagerService : public KeyedService,
 
   // Takes list of account_ids from the front request, matches them with a
   // corresponding stored access_token and calls StartMultilogin.
-  void SetAccountsInCookieWithTokens();
+  // Virtual for testing purposes.
+  virtual void SetAccountsInCookieWithTokens();
 
   // Returns if the listed accounts are up to date or not. The out parameter
   // will be assigned the current cached accounts (whether they are not up to
@@ -270,6 +276,14 @@ class GaiaCookieManagerService : public KeyedService,
   virtual scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory();
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(GaiaCookieManagerServiceTest,
+                           MultiloginSuccessAndCookiesSet);
+  FRIEND_TEST_ALL_PREFIXES(GaiaCookieManagerServiceTest,
+                           MultiloginFailurePersistentError);
+  FRIEND_TEST_ALL_PREFIXES(GaiaCookieManagerServiceTest,
+                           MultiloginFailureMaxRetriesReached);
+  FRIEND_TEST_ALL_PREFIXES(GaiaCookieManagerServiceTest,
+                           FetcherRetriesZeroedBetweenCalls);
   // Returns the source value to use for GaiaFetcher requests.  This is
   // virtual to allow tests and fake classes to override.
   virtual std::string GetSourceForRequest(
@@ -307,30 +321,34 @@ class GaiaCookieManagerService : public KeyedService,
   void OnLogOutSuccess() override;
   void OnLogOutFailure(const GoogleServiceAuthError& error) override;
 
-  // Final call in the Setting accounts in cookie procedure.
-  void OnSetAccountsFinished(const GoogleServiceAuthError& error);
-
   // Callback for CookieManager::SetCanonicalCookie.
   void OnCookieSet(const std::string& cookie_name,
                    const std::string& cookie_domain,
                    bool success);
 
+  // Final call in the Setting accounts in cookie procedure. Virtual for testing
+  // purposes.
+  virtual void OnSetAccountsFinished(const GoogleServiceAuthError& error);
+
   // Helper method for AddAccountToCookie* methods.
   void AddAccountToCookieInternal(const std::string& account_id,
                                   const std::string& source);
 
+  // Helper function to trigger fetching retry in case of failure for only
+  // failed account id. Virtual for testing purposes.
+  virtual void StartFetchingAccessToken(const std::string& account_id);
+
   // Starts the process of fetching the access token with OauthLogin scope and
   // performing SetAccountsInCookie on success.  Virtual so that it can be
   // overridden in tests.
-  virtual void StartFetchingAccesstokens();
+  virtual void StartFetchingAccessTokens();
 
   // Starts the proess of fetching the uber token and performing a merge session
   // for the next account.  Virtual so that it can be overriden in tests.
   virtual void StartFetchingUbertoken();
 
-  // Starts the process of setting accounts in cookie. Virtual for testing
-  // purposes.
-  virtual void StartFetchingMultiLogin(
+  // Starts the process of setting accounts in cookie.
+  void StartFetchingMultiLogin(
       const std::vector<GaiaAuthFetcher::MultiloginTokenIDPair>& accounts);
 
   // Virtual for testing purposes.
@@ -346,7 +364,7 @@ class GaiaCookieManagerService : public KeyedService,
   // Virtual for testing purpose.
   virtual void StartFetchingLogOut();
 
-  // Starts setting parsed cookies in browser;
+  // Starts setting parsed cookies in browser.
   void StartSettingCookies(const OAuthMultiloginResult& result);
 
   // Start the next request, if needed.

components/signin/core/browser/test_signin_client.cc

@@ -12,6 +12,37 @@
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+class FakeCookieManager : public network::mojom::CookieManager {
+ public:
+  void SetCanonicalCookie(const net::CanonicalCookie& cookie,
+                          bool secure_source,
+                          bool modify_http_only,
+                          SetCanonicalCookieCallback callback) override;
+  void GetAllCookies(GetAllCookiesCallback callback) override {}
+  void GetCookieList(const GURL& url,
+                     const net::CookieOptions& cookie_options,
+                     GetCookieListCallback callback) override {}
+  void DeleteCanonicalCookie(const net::CanonicalCookie& cookie,
+                             DeleteCanonicalCookieCallback callback) override {}
+  void DeleteCookies(network::mojom::CookieDeletionFilterPtr filter,
+                     DeleteCookiesCallback callback) override {}
+  void AddCookieChangeListener(
+      const GURL& url,
+      const std::string& name,
+      network::mojom::CookieChangeListenerPtr listener) override {}
+  void AddGlobalChangeListener(
+      network::mojom::CookieChangeListenerPtr notification_pointer) override {}
+  void CloneInterface(
+      network::mojom::CookieManagerRequest new_interface) override {}
+  void FlushCookieStore(FlushCookieStoreCallback callback) override {}
+  void SetContentSettings(
+      const std::vector<::ContentSettingPatternSource>& settings) override {}
+  void SetForceKeepSessionState() override {}
+  void BlockThirdPartyCookies(bool block) override {}
+};
+}  // namespace
+
 TestSigninClient::TestSigninClient(PrefService* pref_service)
     : shared_factory_(
           base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
@@ -28,6 +59,14 @@ PrefService* TestSigninClient::GetPrefs() {
   return pref_service_;
 }
 
+void FakeCookieManager::SetCanonicalCookie(
+    const net::CanonicalCookie& cookie,
+    bool secure_source,
+    bool modify_http_only,
+    SetCanonicalCookieCallback callback) {
+  std::move(callback).Run(false);
+}
+
 void TestSigninClient::OnSignedOut() {}
 
 void TestSigninClient::PostSignedIn(const std::string& account_id,
@@ -42,7 +81,9 @@ TestSigninClient::GetURLLoaderFactory() {
 }
 
 network::mojom::CookieManager* TestSigninClient::GetCookieManager() {
-  return nullptr;
+  if (!cookie_manager_)
+    cookie_manager_ = std::make_unique<FakeCookieManager>();
+  return cookie_manager_.get();
 }
 
 std::string TestSigninClient::GetProductVersion() { return ""; }

components/signin/core/browser/test_signin_client.h

@@ -16,6 +16,7 @@
 #include "components/signin/core/browser/signin_client.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "services/network/public/mojom/cookie_manager.mojom.h"
 #include "services/network/test/test_url_loader_factory.h"
 
 class PrefService;
@@ -53,7 +54,7 @@ class TestSigninClient : public SigninClient {
   // Wraps the test_url_loader_factory().
   scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() override;
 
-  // Returns nullptr.
+  // Returns FakeCookieManager.
   network::mojom::CookieManager* GetCookieManager() override;
 
   network::TestURLLoaderFactory* test_url_loader_factory() {
@@ -91,6 +92,7 @@ class TestSigninClient : public SigninClient {
   scoped_refptr<network::SharedURLLoaderFactory> shared_factory_;
 
   PrefService* pref_service_;
+  std::unique_ptr<network::mojom::CookieManager> cookie_manager_;
   bool are_signin_cookies_allowed_;
   bool network_calls_delayed_;
   std::vector<base::OnceClosure> delayed_network_calls_;