[GCM] Fix crash during connection races

If a connection attempt is triggered while a connection is open, but not
active (for example due to a socket error), make sure the old connection
is closed before reattempting to connect.

BUG=462319

Review URL: https://codereview.chromium.org/980433003

Cr-Commit-Position: refs/heads/master@{#319201}

google_apis/gcm/engine/connection_factory_impl.cc

@@ -145,6 +145,10 @@ void ConnectionFactoryImpl::ConnectWithBackoff() {
 
   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();
 }
 
@@ -212,7 +216,12 @@ void ConnectionFactoryImpl::SignalConnectionReset(
     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.
+    ConnectImpl();
+    return;
+  } else if (logging_in_) {
     // Failures prior to login completion just reuse the existing backoff entry.
     logging_in_ = false;
     backoff_entry_->InformOfRequest(false);
@@ -222,9 +231,6 @@ void ConnectionFactoryImpl::SignalConnectionReset(
     // 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());
@@ -287,7 +293,8 @@ net::IPEndPoint ConnectionFactoryImpl::GetPeerIP() {
 
 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

google_apis/gcm/engine/connection_factory_impl_unittest.cc

@@ -132,6 +132,9 @@ class TestConnectionFactoryImpl : public ConnectionFactoryImpl {
   // Force a login handshake to be delayed.
   void SetDelayLogin(bool delay_login);
 
+  // Simulate a socket error.
+  void SetSocketError();
+
   base::SimpleTestTickClock* tick_clock() { return &tick_clock_; }
 
  private:
@@ -184,6 +187,7 @@ TestConnectionFactoryImpl::~TestConnectionFactoryImpl() {
 
 void TestConnectionFactoryImpl::ConnectImpl() {
   ASSERT_GT(num_expected_attempts_, 0);
+  ASSERT_FALSE(GetConnectionHandler()->CanSendMessage());
   scoped_ptr<mcs_proto::LoginRequest> request(BuildLoginRequest(0, 0, ""));
   GetConnectionHandler()->Init(*request, NULL);
   OnConnectDone(connect_result_);
@@ -255,6 +259,10 @@ void TestConnectionFactoryImpl::SetDelayLogin(bool delay_login) {
   fake_handler_->set_fail_login(delay_login_);
 }
 
+void TestConnectionFactoryImpl::SetSocketError() {
+  fake_handler_->set_had_error(true);
+}
+
 }  // namespace
 
 class ConnectionFactoryImplTest
@@ -561,4 +569,26 @@ TEST_F(ConnectionFactoryImplTest, NetworkChangeBeforeFirstConnection) {
   EXPECT_TRUE(factory()->IsEndpointReachable());
 }
 
+// Test that if the client attempts to reconnect while a connection is already
+// open, we don't crash.
+TEST_F(ConnectionFactoryImplTest, ConnectionResetRace) {
+  // Initial successful connection.
+  factory()->SetConnectResult(net::OK);
+  factory()->Connect();
+  WaitForConnections();
+  EXPECT_TRUE(factory()->IsEndpointReachable());
+
+  // Trigger a connection error under the hood.
+  factory()->SetSocketError();
+  EXPECT_FALSE(factory()->IsEndpointReachable());
+
+  // Now trigger force a re-connection.
+  factory()->SetConnectResult(net::OK);
+  factory()->Connect();
+  WaitForConnections();
+
+  // Re-connection should succeed.
+  EXPECT_TRUE(factory()->IsEndpointReachable());
+}
+
 }  // namespace gcm

google_apis/gcm/engine/fake_connection_handler.cc

@@ -32,7 +32,8 @@ FakeConnectionHandler::FakeConnectionHandler(
       write_callback_(write_callback),
       fail_login_(false),
       fail_send_(false),
-      initialized_(false) {
+      initialized_(false),
+      had_error_(false) {
 }
 
 FakeConnectionHandler::~FakeConnectionHandler() {
@@ -51,10 +52,11 @@ void FakeConnectionHandler::Init(const mcs_proto::LoginRequest& login_request,
 
 void FakeConnectionHandler::Reset() {
   initialized_ = false;
+  had_error_ = false;
 }
 
 bool FakeConnectionHandler::CanSendMessage() const {
-  return initialized_;
+  return initialized_ && !had_error_;
 }
 
 void FakeConnectionHandler::SendMessage(

google_apis/gcm/engine/fake_connection_handler.h

@@ -51,6 +51,13 @@ class FakeConnectionHandler : public ConnectionHandler {
     fail_send_ = fail_send;
   }
 
+  // Whether a socket-level error was encountered or not.
+  void set_had_error(bool had_error) {
+    had_error_ = had_error;
+  }
+
+  bool initialized() const { return initialized_; }
+
  private:
   ConnectionHandler::ProtoReceivedCallback read_callback_;
   ConnectionHandler::ProtoSentCallback write_callback_;
@@ -66,6 +73,9 @@ class FakeConnectionHandler : public ConnectionHandler {
   // Whether a successful login has completed.
   bool initialized_;
 
+  // Whether an error was encountered after a successful login.
+  bool had_error_;
+
   DISALLOW_COPY_AND_ASSIGN(FakeConnectionHandler);
 };