Fix a Turn TCP port issue.

webrtc/p2p/base/turnport.cc

Index: webrtc/p2p/base/turnport.cc
diff --git a/webrtc/p2p/base/turnport.cc b/webrtc/p2p/base/turnport.cc
index 27f88dc7b8fb61e6380c4ca6465c0773f2a1a336..3bb3a654f486eba781a20bc78470369e51506b7a 100644
--- a/webrtc/p2p/base/turnport.cc
+++ b/webrtc/p2p/base/turnport.cc
@@ -172,8 +172,12 @@ TurnPort::TurnPort(rtc::Thread* thread,
                    const RelayCredentials& credentials,
                    int server_priority,
                    const std::string& origin)
-    : Port(thread, factory, network, socket->GetLocalAddress().ipaddr(),
-           username, password),
+    : Port(thread,
+           factory,
+           network,
+           socket->GetLocalAddress().ipaddr(),
+           username,
+           password),
       server_address_(server_address),
       credentials_(credentials),
       socket_(socket),
@@ -181,7 +185,7 @@ TurnPort::TurnPort(rtc::Thread* thread,
       error_(0),
       request_manager_(thread),
       next_channel_number_(TURN_CHANNEL_NUMBER_START),
-      connected_(false),
+      port_state_(DISCONNECTED),
       server_priority_(server_priority),
       allocate_mismatch_retries_(0) {
   request_manager_.SignalSendPacket.connect(this, &TurnPort::OnSendStunPacket);
@@ -200,8 +204,15 @@ TurnPort::TurnPort(rtc::Thread* thread,
                    const RelayCredentials& credentials,
                    int server_priority,
                    const std::string& origin)
-    : Port(thread, RELAY_PORT_TYPE, factory, network, ip, min_port, max_port,
-           username, password),
+    : Port(thread,
+           RELAY_PORT_TYPE,
+           factory,
+           network,
+           ip,
+           min_port,
+           max_port,
+           username,
+           password),
       server_address_(server_address),
       credentials_(credentials),
       socket_(NULL),
@@ -209,7 +220,7 @@ TurnPort::TurnPort(rtc::Thread* thread,
       error_(0),
       request_manager_(thread),
       next_channel_number_(TURN_CHANNEL_NUMBER_START),
-      connected_(false),
+      port_state_(DISCONNECTED),
       server_priority_(server_priority),
       allocate_mismatch_retries_(0) {
   request_manager_.SignalSendPacket.connect(this, &TurnPort::OnSendStunPacket);
@@ -221,7 +232,7 @@ TurnPort::~TurnPort() {
 
   // release the allocation by sending a refresh with
   // lifetime 0.
-  if (connected_) {
+  if (connected()) {
     TurnRefreshRequest bye(this);
     bye.set_lifetime(0);
     SendRequest(&bye, 0);
@@ -261,8 +272,9 @@ void TurnPort::PrepareAddress() {
   } else {
     // If protocol family of server address doesn't match with local, return.
     if (!IsCompatibleAddress(server_address_.address)) {
-      LOG(LS_ERROR) << "Server IP address family does not match with "
-                    << "local host address family type";
+      LOG(LS_ERROR) << "IP Address family does not match: "

[juberti1, 2015/07/29 05:28:13]

Address -> address

[honghaiz3, 2015/07/29 21:44:13]

Done.

+                    << "server: " << server_address_.address.family()
+                    << "local: " << ip().family();
       OnAllocateError();
       return;
     }
@@ -274,8 +286,11 @@ void TurnPort::PrepareAddress() {
                          << ProtoToString(server_address_.proto) << " @ "
                          << server_address_.address.ToSensitiveString();
     if (!CreateTurnClientSocket()) {
+      LOG(LS_ERROR) << "Failed to create TURN client socket";
       OnAllocateError();
-    } else if (server_address_.proto == PROTO_UDP) {
+      return;
+    }
+    if (server_address_.proto == PROTO_UDP) {
       // If its UDP, send AllocateRequest now.
       // For TCP and TLS AllcateRequest will be sent by OnSocketConnect.
       SendRequest(new TurnAllocateRequest(this), 0);
@@ -283,6 +298,10 @@ void TurnPort::PrepareAddress() {
   }
 }
 
+void TurnPort::SendAllocateRequest(int delay) {
+  SendRequest(new TurnAllocateRequest(this), delay);
+}
+
 bool TurnPort::CreateTurnClientSocket() {
   ASSERT(!socket_ || SharedSocket());
 
@@ -319,9 +338,13 @@ bool TurnPort::CreateTurnClientSocket() {
 
   socket_->SignalReadyToSend.connect(this, &TurnPort::OnReadyToSend);
 
+  // TCP port becomes CONNECTING after the socket is connected,

[juberti1, 2015/07/29 05:28:13]

The port becomes connecting after the socket is connected... that seems
confusing.

[pthatcher1, 2015/07/29 20:51:50]

It's already like that in the existing code, where connected_ = false even after
the socket is connected.   It's because port connected != socket connected. 
It's a consequence of using the word "connected" for both the port and the
socket, even though they have different meanings.   The only way around it is to
think of a different name than "connected" for a port that is ready to send. 
"ready"?

[honghaiz3, 2015/07/29 21:44:13]

If we change the name, we'd better change all three. 
For now I changed the comments to make it less confusing. Please let me know if
we should change the enum names. 

[juberti1, 2015/07/30 20:47:40]

how about STATE_CONNECTING (init), STATE_CONNECTED (ready to send allocate),
STATE_READY (received allocate success), STATE_DISCONNECTED (TCP connection
died)

[pthatcher1, 2015/07/30 21:29:16]

That sounds fine.  The only strange part is that connected() will be implemented
with "state_ == STATE_READY", which is confusing.

[honghaiz3, 2015/07/30 21:35:05]

Well, I can change the method to ready() :) 

STATE_CONNECTING and STATE_DISCONNECTED is essentially the same state in which
nothing should be sent out. Guess we just want the logical difference? 

+  // while UDP port becomes CONNECTING once the socket is created.
   if (server_address_.proto == PROTO_TCP) {
     socket_->SignalConnect.connect(this, &TurnPort::OnSocketConnect);
     socket_->SignalClose.connect(this, &TurnPort::OnSocketClose);
+  } else {
+    port_state_ = CONNECTING;
   }
   return true;
 }
@@ -360,6 +383,7 @@ void TurnPort::OnSocketConnect(rtc::AsyncPacketSocket* socket) {
     }
   }
 
+  port_state_ = CONNECTING;  // It is ready to send stun requests.
   if (server_address_.address.IsUnresolved()) {
     server_address_.address = socket_->GetRemoteAddress();
   }
@@ -372,10 +396,10 @@ void TurnPort::OnSocketConnect(rtc::AsyncPacketSocket* socket) {
 void TurnPort::OnSocketClose(rtc::AsyncPacketSocket* socket, int error) {
   LOG_J(LS_WARNING, this) << "Connection with server failed, error=" << error;
   ASSERT(socket == socket_);
-  if (!connected_) {
+  if (port_state_ == DISCONNECTED) {
     OnAllocateError();
   }
-  connected_ = false;
+  port_state_ = DISCONNECTED;
 }
 
 void TurnPort::OnAllocateMismatch() {
@@ -462,8 +486,8 @@ int TurnPort::SendTo(const void* data, size_t size,
                      bool payload) {
   // Try to find an entry for this specific address; we should have one.
   TurnEntry* entry = FindEntry(addr);
-  ASSERT(entry != NULL);
   if (!entry) {
+    LOG(LS_ERROR) << "Did not find the TurnEntry for address " << addr;
     return 0;
   }
 
@@ -536,7 +560,7 @@ void TurnPort::OnReadPacket(
 }
 
 void TurnPort::OnReadyToSend(rtc::AsyncPacketSocket* socket) {
-  if (connected_) {
+  if (connected()) {
     Port::OnReadyToSend();
   }
 }
@@ -616,6 +640,10 @@ void TurnPort::OnResolveResult(rtc::AsyncResolverInterface* resolver) {
 
 void TurnPort::OnSendStunPacket(const void* data, size_t size,
                                 StunRequest* request) {
+  if (port_state_ == DISCONNECTED) {

[juberti1, 2015/07/29 05:28:13]

Could we just flush pending packets when the connection drops, to avoid this
problem?

[pthatcher1, 2015/07/29 20:51:50]

We could add a StunRequestManager::RemoveAll that would then call Thread::Clear.
 And we could call request_manager_->RemoveAll() here.  That sounds like a good
approach.

[honghaiz3, 2015/07/29 21:44:13]

Actually there is a request_manager_->Clear method there and I tried that, but
that does not eliminate all requests. Some the requests are the permission
requests which are generated when TurnEntry is created, which is created from
Connection, which is created when 
a new candidate is received in the signaling path...

[pthatcher1, 2015/07/29 22:00:10]

But if RequestManager called thread_->Clear(), I think it would remove all of
the messages.

[juberti1, 2015/07/30 18:22:33]

I think Clear() + a check during permission creation would be the best approach.

[honghaiz3, 2015/07/30 18:38:27]

That means we will keep all the changes on the PortState and check the state at
the time of creating permission instead of sending the packet here, right?

[juberti1, 2015/07/30 20:47:40]

right. We would ASSERT if we get a packet when STATE_DISCONNECTED.

[honghaiz3, 2015/07/31 00:17:06]

Done.

+    LOG(LS_WARNING) << "Dropping STUN request because port is disconnected";
+    return;
+  }
   rtc::PacketOptions options(DefaultDscpValue());
   if (Send(data, size, options) < 0) {
     LOG_J(LS_ERROR, this) << "Failed to send TURN message, err="
@@ -635,7 +663,7 @@ void TurnPort::OnStunAddress(const rtc::SocketAddress& address) {
 
 void TurnPort::OnAllocateSuccess(const rtc::SocketAddress& address,
                                  const rtc::SocketAddress& stun_address) {
-  connected_ = true;
+  port_state_ = CONNECTED;
 
   rtc::SocketAddress related_address = stun_address;
     if (!(candidate_filter() & CF_REFLEXIVE)) {