Do not time out a port if its role switched from controlled to controlling.

webrtc/p2p/base/port.cc

Index: webrtc/p2p/base/port.cc
diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc
index 842a26e8260df55b740eed7527c13a22698a7309..ae9f6b46ccb41e9194e036630ff07dd5090466b3 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -566,7 +566,7 @@ void Port::SendBindingResponse(StunMessage* request,
   response.AddFingerprint();
 
   // The fact that we received a successful request means that this connection
-  // (if one exists) should now be readable.
+  // (if one exists) should now be receiving.
   Connection* conn = GetConnection(addr);
 
   // Send the response message.
@@ -652,12 +652,14 @@ void Port::OnConnectionDestroyed(Connection* conn) {
   ASSERT(iter != connections_.end());
   connections_.erase(iter);
 
-  // On the controlled side, ports time out, but only after all connections
-  // fail.  Note: If a new connection is added after this message is posted,
-  // but it fails and is removed before kPortTimeoutDelay, then this message
-  //  will still cause the Port to be destroyed.
-  if (ice_role_ == ICEROLE_CONTROLLED)
+  // On the controlled side, ports time out after all connections fail.
+  // Note: This message is only processed by the controlled side. Plus, if a
+  // new connection is added after this message is posted, but it fails and is
+  // removed before kPortTimeoutDelay, then this message will still cause the
+  // Port to be destroyed.
+  if (connections_.empty()) {

[pthatcher1, 2015/09/29 22:54:18]

I think we should make the conditions the same inside of CheckTimeout as here:
"ice_role_ == ICEROLE_CONTROLLED && connections_.empty()". In fact, I think it
would make sense to make a helper method.  And MSG_DEAD would be a better name
for the message.

bool Port::dead() {
  if (ice_role_ == ICEROLE_CONTROLLED) {
    return connections_.empty();
  }
  return false;
}

if (dead()) {
  thread_->PostDelayed(timeout_delay_, this, MSG_DEAD);
}

[honghaiz3, 2015/09/30 15:59:39]

Done. Except that using a single line for the dead method. 

     thread_->PostDelayed(timeout_delay_, this, MSG_CHECKTIMEOUT);
+  }
 }
 
 void Port::Destroy() {
@@ -668,12 +670,9 @@ void Port::Destroy() {
 }
 
 void Port::CheckTimeout() {
-  ASSERT(ice_role_ == ICEROLE_CONTROLLED);
-  // If this port has no connections, then there's no reason to keep it around.
-  // When the connections time out (both read and write), they will delete
-  // themselves, so if we have any connections, they are either readable or
-  // writable (or still connecting).
-  if (connections_.empty())
+  // On the controlled side, if this port has no connection, then there's no
+  // reason to keep it around.
+  if (ice_role_ == ICEROLE_CONTROLLED && connections_.empty())
     Destroy();
 }
 
@@ -919,7 +918,7 @@ void Connection::OnReadPacket(
   } else {
     // The packet is STUN and passed the Port checks.
     // Perform our own checks to ensure this packet is valid.
-    // If this is a STUN request, then update the readable bit and respond.
+    // If this is a STUN request, then update the receiving bit and respond.
     // If this is a STUN response, then update the writable bit.
     // Log at LS_INFO if we receive a ping on an unwritable connection.
     rtc::LoggingSeverity sev = (!writable() ? rtc::LS_INFO : rtc::LS_VERBOSE);
@@ -937,7 +936,7 @@ void Connection::OnReadPacket(
           }
 
           // Incoming, validated stun request from remote peer.
-          // This call will also set the connection readable.
+          // This call will also set the connection receiving.
           port_->SendBindingResponse(msg.get(), addr);
 
           // If timed out sending writability checks, start up again
@@ -977,10 +976,9 @@ void Connection::OnReadPacket(
         // Otherwise silently discard the response message.
         break;
 
-      // Remote end point sent an STUN indication instead of regular
-      // binding request. In this case |last_ping_received_| will be updated.
-      // Otherwise we can mark connection to read timeout. No response will be
-      // sent in this scenario.
+      // Remote end point sent an STUN indication instead of regular binding
+      // request. In this case |last_ping_received_| will be updated but no
+      // response will be sent.
       case STUN_BINDING_INDICATION:
         ReceivedPing();
         break;
@@ -1285,7 +1283,7 @@ void Connection::MaybeUpdatePeerReflexiveCandidate(
 
 void Connection::OnMessage(rtc::Message *pmsg) {
   ASSERT(pmsg->message_id == MSG_DELETE);
-  LOG_J(LS_INFO, this) << "Connection deleted due to read or write timeout";
+  LOG_J(LS_INFO, this) << "Connection deleted";
   SignalDestroyed(this);
   delete this;
 }