TCPConnection can never be deteted if they fail to connect

webrtc/p2p/base/tcpport.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 360 matching lines @@
   if (socket_ip == port()->ip()) {
     LOG_J(LS_VERBOSE, this) << "Connection established to "
                             << socket->GetRemoteAddress().ToSensitiveString();
     set_connected(true);
     connection_pending_ = false;
   } else {
     LOG_J(LS_WARNING, this) << "Dropping connection as TCP socket bound to IP "
                             << socket_ip.ToSensitiveString()
                             << ", different from the local candidate IP "
                             << port()->ip().ToSensitiveString();
+    // This doesn't trigger OnClose. We should either fix that behavior or call
+    // Destroy() to ensure this connection doesn't linger.

[pthatcher1, 2015/08/22 05:05:22]

It seems like we should call OnClose here.  Is there a reason not to?

[guoweis_webrtc, 2015/08/22 05:29:49]

Done.

     socket_->Close();
   }
 }
 
 void TCPConnection::OnClose(rtc::AsyncPacketSocket* socket, int error) {
   ASSERT(socket == socket_);
   LOG_J(LS_INFO, this) << "Connection closed with error " << error;
 
   // Guard against the condition where IPC socket will call OnClose for every
   // packet it can't send.
   if (connected()) {
     set_connected(false);
     pretending_to_be_writable_ = true;
 
     // We don't attempt reconnect right here. This is to avoid a case where the
     // shutdown is intentional and reconnect is not necessary. We only reconnect
     // when the connection is used to Send() or Ping().
     port()->thread()->PostDelayed(reconnection_timeout(), this,
                                   MSG_TCPCONNECTION_DELAYED_ONCLOSE);
+  } else {
+    // OnClose could be called when the underneath socket times out during
+    // connect(). We have to manually destroy here as this connection, as never
+    // connected, will not be scheduled for ping to trigger destroy.
+    Destroy();

[pthatcher1, 2015/08/22 05:05:22]

I don't think this matches the state diagram at the top.  It says if we're not
connect and get OnClose, we should wait for a timeout before dying.  Is the
diagram wrong, the code wrong, or am I wrong?

[guoweis_webrtc, 2015/08/22 05:29:49]

the diagram above misses an arrow from state 1 to state 7. I'll update that.

   }
 }
 
 void TCPConnection::OnMessage(rtc::Message* pmsg) {
   switch (pmsg->message_id) {
     case MSG_TCPCONNECTION_DELAYED_ONCLOSE:
       // If this connection can't become connected and writable again in 5
       // seconds, it's time to tear this down. This is the case for the original
       // TCP connection on passive side during a reconnect.
       if (pretending_to_be_writable_) {
-        set_write_state(STATE_WRITE_TIMEOUT);
+        // Make sure this connection is destroyed. It will linger around if it
+        // has been READABLE.

[pthatcher1, 2015/08/22 05:05:22]

This part seems correct.  It matches the state diagram ("the connection is
dead"), so I don't think you need the comment.

[guoweis_webrtc, 2015/08/22 05:29:50]

Will remove.

+        Destroy();
       }
       break;
     default:
       Connection::OnMessage(pmsg);
   }
 }
 
 void TCPConnection::MaybeReconnect() {
   // Only reconnect for an outgoing TCPConnection when OnClose was signaled and
   // no outstanding reconnect is pending.
@@ skipping 46 matching lines @@
 void TCPConnection::ConnectSocketSignals(rtc::AsyncPacketSocket* socket) {
   if (outgoing_) {
     socket->SignalConnect.connect(this, &TCPConnection::OnConnect);
   }
   socket->SignalReadPacket.connect(this, &TCPConnection::OnReadPacket);
   socket->SignalReadyToSend.connect(this, &TCPConnection::OnReadyToSend);
   socket->SignalClose.connect(this, &TCPConnection::OnClose);
 }
 
 }  // namespace cricket