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..5bbba03ae204fb0d1c7433719d3bba7adc369a9c 100644
--- a/components/signin/core/browser/gaia_cookie_manager_service.cc
+++ b/components/signin/core/browser/gaia_cookie_manager_service.cc
@@ -130,6 +130,14 @@ GaiaCookieManagerService::ExternalCcResultFetcher::GetExternalCcResult() {
 }
 
 void GaiaCookieManagerService::ExternalCcResultFetcher::Start() {
+  if (helper_->external_cc_result_fetched_) {
+    // Avoid starting a net fetcher if the the external CC result was already
+    // marked as fetched.
+    // This matches the case when |timer_| has already fired and the external CC
+    // result was already marked as fetched.
+    return;
+  }
+

[Roger Tawa OOO till Jul 10th, 2017/02/23 19:00:00]

This method is only called at one place, line 568 below, and
|external_cc_result_fetched_| is already checked there before calling.  So not
sure this extra check does much.  Maybe this function should have a DCHECK
instead:

    DCHECK(!helper_->external_cc_result_fetched_);

[msarda, 2017/02/24 12:53:40]

I've added this DCHECK and it fires. Here is the list of calls:
[0224/124358.755662:ERROR:gaia_cookie_manager_service.cc(135)]
ExternalCcResultFetcher::Start
[0224/124358.755784:ERROR:gaia_cookie_manager_service.cc(287)]
ExternalCcResultFetcher::CleanupTransientState
[0224/124358.884159:WARNING:gaia_auth_fetcher.cc(721)] ClientLogin failed with 
[0224/124358.884292:WARNING:gaia_auth_fetcher.cc(742)] Incomprehensible response
from Google Accounts servers.
[0224/124358.884437:ERROR:gaia_cookie_manager_service.cc(209)]
ExternalCcResultFetcher::OnGetCheckConnectionInfoError
[0224/124358.884529:ERROR:gaia_cookie_manager_service.cc(218)] SCHEDULE
ExternalCcResultFetcher::Start in 0 s
[0224/124359.882536:ERROR:gaia_cookie_manager_service.cc(135)]
ExternalCcResultFetcher::Start
[0224/124359.882688:ERROR:gaia_cookie_manager_service.cc(287)]
ExternalCcResultFetcher::CleanupTransientState
[0224/124400.009480:WARNING:gaia_auth_fetcher.cc(721)] ClientLogin failed with 
[0224/124400.009629:WARNING:gaia_auth_fetcher.cc(742)] Incomprehensible response
from Google Accounts servers.
[0224/124400.009730:ERROR:gaia_cookie_manager_service.cc(209)]
ExternalCcResultFetcher::OnGetCheckConnectionInfoError
[0224/124400.009837:ERROR:gaia_cookie_manager_service.cc(218)] SCHEDULE
ExternalCcResultFetcher::Start in 0 s
[0224/124402.482121:ERROR:gaia_cookie_manager_service.cc(135)]
ExternalCcResultFetcher::Start
[0224/124402.482263:ERROR:gaia_cookie_manager_service.cc(287)]
ExternalCcResultFetcher::CleanupTransientState
[0224/124402.620963:WARNING:gaia_auth_fetcher.cc(721)] ClientLogin failed with 
[0224/124402.621105:WARNING:gaia_auth_fetcher.cc(742)] Incomprehensible response
from Google Accounts servers.
[0224/124402.621207:ERROR:gaia_cookie_manager_service.cc(209)]
ExternalCcResultFetcher::OnGetCheckConnectionInfoError
[0224/124402.621311:ERROR:gaia_cookie_manager_service.cc(218)] SCHEDULE
ExternalCcResultFetcher::Start in 0 s
[0224/124407.482516:ERROR:gaia_cookie_manager_service.cc(279)]
ExternalCcResultFetcher::Timeout
[0224/124407.482904:ERROR:gaia_cookie_manager_service.cc(287)]
ExternalCcResultFetcher::CleanupTransientState
[0224/124410.337061:FATAL:gaia_cookie_manager_service.cc(134)]
ExternalCcResultFetcher::Start 
[0224/124410.337061:FATAL:gaia_cookie_manager_service.cc(134)] Check failed:
!helper_->external_cc_result_fetched_

What happens is that when we get an OnGetCheckConnectionInfoError, we schedule a
retry, but we did not stop the timer (there is no call to CleanupTransientState
on the retry branch in OnGetCheckConnectionInfoError. In my case, the timer
fires between the moment the ExternalCcResultFetcher::Start  is scheduled and
the time the moment the Start method is actually called. This leads to the
DCHECK being hit.

   m_external_cc_result_start_time_ = base::Time::Now();
 
   CleanupTransientState();
@@ -195,6 +203,12 @@ void GaiaCookieManagerService::ExternalCcResultFetcher::
 
 void GaiaCookieManagerService::ExternalCcResultFetcher::
     OnGetCheckConnectionInfoError(const GoogleServiceAuthError& error) {
+  if (helper_->external_cc_result_fetched_) {
+    // Nothing else to do if the timer has already fired and the external CC
+    // result was already marked as fetched.
+    return;
+  }
+

[Roger Tawa OOO till Jul 10th, 2017/02/23 19:00:00]

If the timer already fired, CleanupTransientState() would have been called,
which resets |helper_->gaia_auth_fetcher_|.  So this should not be called.  Are
you seeing that it is called?

[msarda, 2017/02/24 12:53:40]

See comment above.

   if (++helper_->fetcher_retries_ < kMaxFetcherRetries &&
       error.IsTransientError()) {
     helper_->fetcher_backoff_.InformOfRequest(false);
@@ -280,10 +294,6 @@ void GaiaCookieManagerService::ExternalCcResultFetcher::
                                              time_to_check_connections);
 
   helper_->external_cc_result_fetched_ = true;
-  // Since the ExternalCCResultFetcher is only Started in place of calling
-  // StartFetchingMergeSession, we can assume we need to call
-  // StartFetchingMergeSession. If this assumption becomes invalid, a Callback
-  // will need to be passed to Start() and Run() here.

[Roger Tawa OOO till Jul 10th, 2017/02/23 19:00:00]

I think this comment is still valid.  Is there a reason to remove it?

[msarda, 2017/02/24 12:53:40]

Fixed.

   helper_->StartFetchingMergeSession();
 }