Provide correct guard for GCM connection tracking of login failures.

The original implementation of GCM Connection tracking had a DCHECK in
ConnectionLoginFailed to validate that ConnectionAttemptSucceeded had
been called first to validate the expected ordering of the calls.

That was removed as a result of crbug.com/673706 because it was failing.

After investigating, the reason for the failures is that the entry check
for calling ConnectionLoginFailed was checking the logging_in_ member on
the ConnectionFactoryImpl. However, that member tracks whether the
initial connection is established, not whether the login handshake has
been completed.

The correct check for when to call ConnectionLoginFailed is if the
result passed to SignalConnectionReset is LOGIN_FAILURE, since that
indicates that the MCSClient was unable to complete the login.

BUG=673706

Review-Url: https://codereview.chromium.org/2624653003
Cr-Commit-Position: refs/heads/master@{#443531}

google_apis/gcm/engine/connection_event_tracker.cc

@@ -66,8 +66,8 @@ void ConnectionEventTracker::ConnectionAttemptSucceeded() {
 void ConnectionEventTracker::ConnectionLoginFailed() {
   // A login failure would have originally been marked as a successful
   // connection, so now that it failed, that needs to be updated.
-  // TODO(harkness): Add back DCHECK which was removed. See
-  // https://crbug.com/673706.
+  DCHECK_EQ(current_event_.type(),
+            mcs_proto::ClientEvent::SUCCESSFUL_CONNECTION);
 
   current_event_.set_type(mcs_proto::ClientEvent::FAILED_CONNECTION);
   current_event_.clear_time_connection_established_ms();

google_apis/gcm/engine/connection_factory_impl.cc

@@ -66,7 +66,7 @@ ConnectionFactoryImpl::ConnectionFactoryImpl(
       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) {
@@ -131,7 +131,7 @@ ConnectionEventTracker* ConnectionFactoryImpl::GetEventTrackerForTesting() {
 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;
   }
@@ -167,8 +167,8 @@ bool ConnectionFactoryImpl::IsEndpointReachable() const {
 std::string ConnectionFactoryImpl::GetConnectionStateString() const {
   if (IsEndpointReachable())
     return "CONNECTED";
-  if (logging_in_)
-    return "LOGGING IN";
+  if (handshake_in_progress_)
+    return "HANDSHAKE IN PROGRESS";
   if (connecting_)
     return "CONNECTING";
   if (waiting_for_backoff_)
@@ -209,7 +209,7 @@ void ConnectionFactoryImpl::SignalConnectionReset(
     // connection.
   }
 
-  if (logging_in_)
+  if (reason == LOGIN_FAILURE)
     event_tracker_.ConnectionLoginFailed();
   event_tracker_.EndConnectionAttempt();
 
@@ -233,9 +233,9 @@ void ConnectionFactoryImpl::SignalConnectionReset(
     // 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())) {
@@ -412,7 +412,7 @@ void ConnectionFactoryImpl::OnConnectDone(int result) {
   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();
 }
@@ -434,7 +434,7 @@ void ConnectionFactoryImpl::ConnectionHandlerCallback(int result) {
   last_login_time_ = NowTicks();
   previous_backoff_.swap(backoff_entry_);
   backoff_entry_->Reset();
-  logging_in_ = false;
+  handshake_in_progress_ = false;
 
   event_tracker_.ConnectionAttemptSucceeded();
 

google_apis/gcm/engine/connection_factory_impl.h

@@ -184,10 +184,10 @@ class GCM_EXPORT ConnectionFactoryImpl :
   // client is informed of a valid connection type.
   bool waiting_for_network_online_;
 
-  // Whether login successfully completed after the connection was established.
-  // If a connection reset happens while attempting to log in, the current
-  // backoff entry is reused (after incrementing with a new failure).
-  bool logging_in_;
+  // Whether handshake is in progress after the connection was established. If
+  // a connection reset happens while attempting to complete the handshake, the
+  // current backoff entry is reused (after incrementing with a new failure).
+  bool handshake_in_progress_;
 
   // The time of the last login completion. Used for calculating whether to
   // restore a previous backoff entry and for measuring uptime.