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() {
+  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();
   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) {
+  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);
     gaia_auth_fetcher_timer_.Start(
         FROM_HERE, helper_->fetcher_backoff_.GetTimeUntilRelease(),
         this, &GaiaCookieManagerService::ExternalCcResultFetcher::Start);
     return;
   }
 
   CleanupTransientState();
@@ skipping 65 matching lines @@
 }
 
 void GaiaCookieManagerService::ExternalCcResultFetcher::
     GetCheckConnectionInfoCompleted(bool succeeded) {
   base::TimeDelta time_to_check_connections =
       base::Time::Now() - m_external_cc_result_start_time_;
   signin_metrics::LogExternalCcResultFetches(succeeded,
                                              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();
 }
 
 GaiaCookieManagerService::GaiaCookieManagerService(
     OAuth2TokenService* token_service,
     const std::string& source,
     SigninClient* signin_client)
     : token_service_(token_service),
       signin_client_(signin_client),
       external_cc_result_fetcher_(this),
@@ skipping 497 matching lines @@
         break;
       case GaiaCookieRequestType::LIST_ACCOUNTS:
         uber_token_fetcher_.reset();
         signin_client_->DelayNetworkCall(
             base::Bind(&GaiaCookieManagerService::StartFetchingListAccounts,
                        base::Unretained(this)));
         break;
     }
   }
 }