Avoid re-starting a ExternalCCResultFetcher if a previous has finished or timed out.

components/signin/core/browser/gaia_cookie_manager_service.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/signin/core/browser/gaia_cookie_manager_service.h"
 
 #include <stddef.h>
 
 #include <queue>
 
@@ skipping 112 matching lines @@
 GaiaCookieManagerService::ExternalCcResultFetcher::GetExternalCcResult() {
   std::vector<std::string> results;
   for (ResultMap::const_iterator it = results_.begin(); it != results_.end();
        ++it) {
     results.push_back(it->first + ":" + it->second);
   }
   return base::JoinString(results, ",");
 }
 
 void GaiaCookieManagerService::ExternalCcResultFetcher::Start() {
+  DCHECK(!helper_->external_cc_result_fetched_);
   m_external_cc_result_start_time_ = base::Time::Now();
 
   CleanupTransientState();
   results_.clear();
   helper_->gaia_auth_fetcher_.reset(
       helper_->signin_client_->CreateGaiaAuthFetcher(
           this, helper_->GetDefaultSourceForRequest(),
           helper_->request_context()));
   helper_->gaia_auth_fetcher_->StartGetCheckConnectionInfo();
 
@@ skipping 45 matching lines @@
         net::URLFetcher* fetcher = CreateFetcher(GURL(url)).release();
         fetchers_[fetcher->GetOriginalURL()] = std::make_pair(token, fetcher);
         fetcher->Start();
       }
     }
   }
 }
 
 void GaiaCookieManagerService::ExternalCcResultFetcher::
     OnGetCheckConnectionInfoError(const GoogleServiceAuthError& error) {
+  CleanupTransientState();
+

[Roger Tawa OOO till Jul 10th, 2017/02/24 17:20:15]

I would leave the call to CleanupTransientState() where it was.

I think one required change to fix 680622 is this:

GaiaCookieManagerService::ExternalCcResultFetcher::IsRunning() {
  return helper_->gaia_auth_fetcher_ || fetchers_.size() > 0u ||
      timer_.IsRunning();
}

I think I now understand your call log.  The member
|external_cc_result_fetched_| is set to false in the ctor and can only set it to
true after that.  It is never reset back to false.

In your log we see:

[0224/124407.482516:ERROR:gaia_cookie_manager_service.cc(279)]
ExternalCcResultFetcher::Timeout

which unconditionally sets |external_cc_result_fetched_| to true. Yet 3 seconds
later see see:

[0224/124410.337061:FATAL:gaia_cookie_manager_service.cc(134)]
ExternalCcResultFetcher::Start 

There are only two places in the code that could call Start().  One is line 560
below.  However when |external_cc_result_fetched_| is true this call will not be
made.  So this is not the cause of the call.

The other place is line 206, in the case of a retry.  If a retry is pending when
the timeout expires, |gaia_auth_fetcher_timer_| is not reset.  This can happen
if the retry delay is longer than the timeout. This seems to make sense here,
since the DCHECK was hit 3 seconds after the expiry of a 5 second timer.  Or 8
seconds, which could makes sense for the 3rd retry of an exponential retry timer
with the parameters set here.

So I think another fix is also required, see comment below.

[msarda, 2017/03/03 14:25:00]

I guess it all depends on what we want the behavior to be:
* with my patch, we keep on retrying for 15 minutes (basically reaching the end
of the backoff retry logic)
* with your proposed solution, it stops retrying after 5 minutes.


Here is the log I get with your proposed patch:
[5065:5065:0303/150940.553079:VERBOSE1:gaia_cookie_manager_service.cc(353)]
GaiaCookieManagerService::AddAccountToCookie: 116550294018676870905
[5065:5065:0303/150940.553395:VERBOSE1:gaia_cookie_manager_service.cc(730)]
GaiaCookieManagerService::StartFetchingUbertoken
account_id=116550294018676870905
[5065:5065:0303/150942.414714:VERBOSE1:gaia_cookie_manager_service.cc(560)]
GaiaCookieManagerService::OnUbertokenSuccess account=116550294018676870905
[5065:5065:0303/150942.582054:WARNING:gaia_auth_fetcher.cc(742)]
Incomprehensible response from Google Accounts servers.
[5065:5065:0303/150942.582100:VERBOSE1:gaia_cookie_manager_service.cc(200)]
GaiaCookieManagerService::ExternalCcResultFetcher::OnGetCheckConnectionInfoError
[5065:5065:0303/150942.582152:VERBOSE1:gaia_cookie_manager_service.cc(205)]
Schedule retry in 0ms
[5065:5065:0303/150942.750395:WARNING:gaia_auth_fetcher.cc(742)]
Incomprehensible response from Google Accounts servers.
[5065:5065:0303/150942.750453:VERBOSE1:gaia_cookie_manager_service.cc(200)]
GaiaCookieManagerService::ExternalCcResultFetcher::OnGetCheckConnectionInfoError
[5065:5065:0303/150942.750515:VERBOSE1:gaia_cookie_manager_service.cc(205)]
Schedule retry in 735ms
[5065:5065:0303/150943.735907:WARNING:gaia_auth_fetcher.cc(742)]
Incomprehensible response from Google Accounts servers.
[5065:5065:0303/150943.735979:VERBOSE1:gaia_cookie_manager_service.cc(200)]
GaiaCookieManagerService::ExternalCcResultFetcher::OnGetCheckConnectionInfoError
[5065:5065:0303/150943.736043:VERBOSE1:gaia_cookie_manager_service.cc(205)]
Schedule retry in 1756ms
[5065:5065:0303/150945.659336:WARNING:gaia_auth_fetcher.cc(742)]
Incomprehensible response from Google Accounts servers.
[5065:5065:0303/150945.659422:VERBOSE1:gaia_cookie_manager_service.cc(200)]
GaiaCookieManagerService::ExternalCcResultFetcher::OnGetCheckConnectionInfoError
[5065:5065:0303/150945.659511:VERBOSE1:gaia_cookie_manager_service.cc(205)]
Schedule retry in 6281ms
[5065:5065:0303/150950.496680:VERBOSE1:gaia_cookie_manager_service.cc(266)] 
GaiaCookieManagerService::ExternalCcResultFetcher::Timeout
[5065:5065:0303/150951.260146:VERBOSE1:gaia_cookie_manager_service.cc(588)]
MergeSession successful account=116550294018676870905

Basically, on a call to OnGetCheckConnectionInfoError, and in case we retry,
|timer_| is not reset right away. So if
helper_->fetcher_backoff_.GetTimeUntilRelease() is longer than 5 seconds, then
|timer_| will fire and the timeout method will be called.

I think we need to call CleanupTransientState before scheduling a next time. I
agree with all the rest of you proposal.

   if (++helper_->fetcher_retries_ < kMaxFetcherRetries &&
       error.IsTransientError()) {
     helper_->fetcher_backoff_.InformOfRequest(false);
     gaia_auth_fetcher_timer_.Start(
         FROM_HERE, helper_->fetcher_backoff_.GetTimeUntilRelease(),
         this, &GaiaCookieManagerService::ExternalCcResultFetcher::Start);
     return;
   }
 
-  CleanupTransientState();
   GetCheckConnectionInfoCompleted(false);
 }
 
 std::unique_ptr<net::URLFetcher>
 GaiaCookieManagerService::ExternalCcResultFetcher::CreateFetcher(
     const GURL& url) {
   std::unique_ptr<net::URLFetcher> fetcher =
       net::URLFetcher::Create(0, url, net::URLFetcher::GET, this);
   fetcher->SetRequestContext(helper_->request_context());
   fetcher->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
@@ skipping 38 matching lines @@
 }
 
 void GaiaCookieManagerService::ExternalCcResultFetcher::Timeout() {
   CleanupTransientState();
   GetCheckConnectionInfoCompleted(false);
 }
 
 void GaiaCookieManagerService::ExternalCcResultFetcher::
     CleanupTransientState() {
   timer_.Stop();
   helper_->gaia_auth_fetcher_.reset();

[Roger Tawa OOO till Jul 10th, 2017/02/24 17:20:15]

Also stop |gaia_auth_fetcher_timer_|.

[msarda, 2017/03/03 14:25:00]

Done.

 
   for (URLToTokenAndFetcher::const_iterator it = fetchers_.begin();
        it != fetchers_.end(); ++it) {
     delete it->second.second;
   }
   fetchers_.clear();
 }
 
 void GaiaCookieManagerService::ExternalCcResultFetcher::
     GetCheckConnectionInfoCompleted(bool succeeded) {
@@ skipping 517 matching lines @@
         break;
       case GaiaCookieRequestType::LIST_ACCOUNTS:
         uber_token_fetcher_.reset();
         signin_client_->DelayNetworkCall(
             base::Bind(&GaiaCookieManagerService::StartFetchingListAccounts,
                        base::Unretained(this)));
         break;
     }
   }
 }