Provide correct guard for GCM connection tracking of login failures.

google_apis/gcm/engine/connection_factory_impl.cc

 // Copyright (c) 2013 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 "google_apis/gcm/engine/connection_factory_impl.h"
 
 #include <string>
 
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
@@ skipping 48 matching lines @@
       last_successful_endpoint_(0),
       backoff_policy_(backoff_policy),
       gcm_network_session_(gcm_network_session),
       http_network_session_(http_network_session),
       net_log_(
           net::NetLogWithSource::Make(net_log, net::NetLogSourceType::SOCKET)),
       pac_request_(NULL),
       connecting_(false),
       waiting_for_backoff_(false),
       waiting_for_network_online_(false),
-      logging_in_(false),
+      handshake_in_progress_(false),
       recorder_(recorder),
       listener_(NULL),
       weak_ptr_factory_(this) {
   DCHECK_GE(mcs_endpoints_.size(), 1U);
   DCHECK(!http_network_session_ ||
          (gcm_network_session_ != http_network_session_));
 }
 
 ConnectionFactoryImpl::~ConnectionFactoryImpl() {
   CloseSocket();
@@ skipping 44 matching lines @@
   ConnectWithBackoff();
 }
 
 ConnectionEventTracker* ConnectionFactoryImpl::GetEventTrackerForTesting() {
   return &event_tracker_;
 }
 
 void ConnectionFactoryImpl::ConnectWithBackoff() {
   // If a canary managed to connect while a backoff expiration was pending,
   // just cleanup the internal state.
-  if (connecting_ || logging_in_ || IsEndpointReachable()) {
+  if (connecting_ || handshake_in_progress_ || IsEndpointReachable()) {
     waiting_for_backoff_ = false;
     return;
   }
 
   if (backoff_entry_->ShouldRejectRequest()) {
     DVLOG(1) << "Delaying MCS endpoint connection for "
              << backoff_entry_->GetTimeUntilRelease().InMilliseconds()
              << " milliseconds.";
     waiting_for_backoff_ = true;
     recorder_->RecordConnectionDelayedDueToBackoff(
@@ skipping 15 matching lines @@
   ConnectImpl();
 }
 
 bool ConnectionFactoryImpl::IsEndpointReachable() const {
   return connection_handler_ && connection_handler_->CanSendMessage();
 }
 
 std::string ConnectionFactoryImpl::GetConnectionStateString() const {
   if (IsEndpointReachable())
     return "CONNECTED";
-  if (logging_in_)
+  if (handshake_in_progress_)
     return "LOGGING IN";

[harkness, 2017/01/12 10:26:30]

I couldn't find any places that might be relying on this text. @zea, do you know
of any? Or should I just leave it as LOGGING_IN?

[Nicolas Zea, 2017/01/12 18:00:57]

I'd say go ahead and update for consistency sake . This is used in
chrome://gcm-internals

[harkness, 2017/01/13 08:28:08]

Done.

   if (connecting_)
     return "CONNECTING";
   if (waiting_for_backoff_)
     return "WAITING FOR BACKOFF";
   if (waiting_for_network_online_)
     return "WAITING FOR NETWORK CHANGE";
   return "NOT CONNECTED";
 }
 
 void ConnectionFactoryImpl::SignalConnectionReset(
@@ skipping 20 matching lines @@
   if (!last_login_time_.is_null()) {
     UMA_HISTOGRAM_CUSTOM_TIMES("GCM.ConnectionUpTime",
                                NowTicks() - last_login_time_,
                                base::TimeDelta::FromSeconds(1),
                                base::TimeDelta::FromHours(24),
                                50);
     // |last_login_time_| will be reset below, before attempting the new
     // connection.
   }
 
-  if (logging_in_)
+  if (reason == LOGIN_FAILURE)
     event_tracker_.ConnectionLoginFailed();
   event_tracker_.EndConnectionAttempt();
 
   CloseSocket();
   DCHECK(!IsEndpointReachable());
 
   // TODO(zea): if the network is offline, don't attempt to connect.
   // See crbug.com/396687
 
   // Network changes get special treatment as they can trigger a one-off canary
   // request that bypasses backoff (but does nothing if a connection is in
   // progress). Other connection reset events can be ignored as a connection
   // is already awaiting backoff expiration.
   if (waiting_for_backoff_ && reason != NETWORK_CHANGE) {
     DVLOG(1) << "Backoff expiration pending, ignoring reset.";
     return;
   }
 
   if (reason == NETWORK_CHANGE) {
     // Canary attempts bypass backoff without resetting it. These will have no
     // effect if we're already in the process of connecting.
     ConnectImpl();
     return;
-  } else if (logging_in_) {
-    // Failures prior to login completion just reuse the existing backoff entry.
-    logging_in_ = false;
+  } else if (handshake_in_progress_) {
+    // Failures prior to handshake completion reuse the existing backoff entry.
+    handshake_in_progress_ = false;
     backoff_entry_->InformOfRequest(false);
   } else if (reason == LOGIN_FAILURE ||
              ShouldRestorePreviousBackoff(last_login_time_, NowTicks())) {
     // Failures due to login, or within the reset window of a login, restore
     // the backoff entry that was saved off at login completion time.
     backoff_entry_.swap(previous_backoff_);
     backoff_entry_->InformOfRequest(false);
   } else {
     // We shouldn't be in backoff in thise case.
     DCHECK_EQ(0, backoff_entry_->failure_count());
@@ skipping 156 matching lines @@
   ReportSuccessfulProxyConnection();
   recorder_->RecordConnectionSuccess();
 
   // Reset the endpoint back to the default.
   // TODO(zea): consider prioritizing endpoints more intelligently based on
   // which ones succeed most for this client? Although that will affect
   // measuring the success rate of the default endpoint vs fallback.
   last_successful_endpoint_ = next_endpoint_;
   next_endpoint_ = 0;
   connecting_ = false;
-  logging_in_ = true;
+  handshake_in_progress_ = true;
   DVLOG(1) << "MCS endpoint socket connection success, starting login.";
   InitHandler();
 }
 
 void ConnectionFactoryImpl::ConnectionHandlerCallback(int result) {
   DCHECK(!connecting_);
   if (result != net::OK) {
     // TODO(zea): Consider how to handle errors that may require some sort of
     // user intervention (login page, etc.).
     UMA_HISTOGRAM_SPARSE_SLOWLY("GCM.ConnectionDisconnectErrorCode", result);
     SignalConnectionReset(SOCKET_FAILURE);
     return;
   }
 
   // Handshake complete, reset backoff. If the login failed with an error,
   // the client should invoke SignalConnectionReset(LOGIN_FAILURE), which will
   // restore the previous backoff.
   DVLOG(1) << "Handshake complete.";
   last_login_time_ = NowTicks();
   previous_backoff_.swap(backoff_entry_);
   backoff_entry_->Reset();
-  logging_in_ = false;
+  handshake_in_progress_ = false;
 
   event_tracker_.ConnectionAttemptSucceeded();
 
   if (listener_)
     listener_->OnConnected(GetCurrentEndpoint(), GetPeerIP());
 }
 
 // This has largely been copied from
 // HttpStreamFactoryImpl::Job::DoResolveProxyComplete. This should be
 // refactored into some common place.
@@ skipping 144 matching lines @@
 
 void ConnectionFactoryImpl::RebuildNetworkSessionAuthCache() {
   if (!http_network_session_ || !http_network_session_->http_auth_cache())
     return;
 
   gcm_network_session_->http_auth_cache()->UpdateAllFrom(
       *http_network_session_->http_auth_cache());
 }
 
 }  // namespace gcm