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

components/signin/core/browser/gaia_cookie_manager_service.cc

Index: components/signin/core/browser/gaia_cookie_manager_service.cc
diff --git a/components/signin/core/browser/gaia_cookie_manager_service.cc b/components/signin/core/browser/gaia_cookie_manager_service.cc
index 28f30f1d2ffab1aa02322776bfcc0253688d930f..18cccf82514e699035fc0c24a4e18ac2f5af8871 100644
--- a/components/signin/core/browser/gaia_cookie_manager_service.cc
+++ b/components/signin/core/browser/gaia_cookie_manager_service.cc
@@ -130,6 +130,7 @@ GaiaCookieManagerService::ExternalCcResultFetcher::GetExternalCcResult() {
 }
 
 void GaiaCookieManagerService::ExternalCcResultFetcher::Start() {
+  DCHECK(!helper_->external_cc_result_fetched_);
   m_external_cc_result_start_time_ = base::Time::Now();
 
   CleanupTransientState();
@@ -195,6 +196,8 @@ void GaiaCookieManagerService::ExternalCcResultFetcher::
 
 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);
@@ -204,7 +207,6 @@ void GaiaCookieManagerService::ExternalCcResultFetcher::
     return;
   }
 
-  CleanupTransientState();
   GetCheckConnectionInfoCompleted(false);
 }