[GCM] Fix crash during connection races

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 "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "google_apis/gcm/engine/connection_handler_impl.h"
@@ skipping 127 matching lines @@
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&ConnectionFactoryImpl::ConnectWithBackoff,
                    weak_ptr_factory_.GetWeakPtr()),
         backoff_entry_->GetTimeUntilRelease());
     return;
   }
 
   DVLOG(1) << "Attempting connection to MCS endpoint.";
   waiting_for_backoff_ = false;
+  // It's necessary to close the socket before attempting any new connection,
+  // otherwise it's possible to hit a use-after-free in the connection handler.
+  // crbug.com/462319
+  CloseSocket();
   ConnectImpl();
 }
 
 bool ConnectionFactoryImpl::IsEndpointReachable() const {
   return connection_handler_ && connection_handler_->CanSendMessage();
 }
 
 std::string ConnectionFactoryImpl::GetConnectionStateString() const {
   if (IsEndpointReachable())
     return "CONNECTED";
@@ skipping 47 matching lines @@
 
   // 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 (logging_in_) {
+  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.

[fgorski, 2015/03/04 17:43:04]

To confirm. There is no need to close socket here, right?

[Nicolas Zea, 2015/03/04 17:56:42]

We do it on line 204 above.

+    ConnectImpl();
+    return;
+  } else if (logging_in_) {
     // Failures prior to login completion just reuse the existing backoff entry.
     logging_in_ = 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 if (reason == NETWORK_CHANGE) {
-    ConnectImpl();  // Canary attempts bypass backoff without resetting it.
-    return;
   } else {
     // We shouldn't be in backoff in thise case.
     DCHECK_EQ(0, backoff_entry_->failure_count());
   }
 
   // At this point the last login time has been consumed or deemed irrelevant,
   // reset it.
   last_login_time_ = base::TimeTicks();
 
   Connect();
@@ skipping 42 matching lines @@
   net::IPEndPoint ip_endpoint;
   int result = socket_handle_.socket()->GetPeerAddress(&ip_endpoint);
   if (result != net::OK)
     return net::IPEndPoint();
 
   return ip_endpoint;
 }
 
 void ConnectionFactoryImpl::ConnectImpl() {
   DCHECK(!IsEndpointReachable());
-  DCHECK(!socket_handle_.socket());
+  // TODO(zea): Make this a dcheck again. crbug.com/462319
+  CHECK(!socket_handle_.socket());
 
   // TODO(zea): if the network is offline, don't attempt to connect.
   // See crbug.com/396687
 
   connecting_ = true;
   GURL current_endpoint = GetCurrentEndpoint();
   recorder_->RecordConnectionInitiated(current_endpoint.host());
   RebuildNetworkSessionAuthCache();
   int status = gcm_network_session_->proxy_service()->ResolveProxy(
       current_endpoint,
@@ skipping 261 matching lines @@
 
 void ConnectionFactoryImpl::RebuildNetworkSessionAuthCache() {
   if (!http_network_session_.get() || !http_network_session_->http_auth_cache())
     return;
 
   gcm_network_session_->http_auth_cache()->UpdateAllFrom(
       *http_network_session_->http_auth_cache());
 }
 
 }  // namespace gcm