Do not time out a port if its role switched from controlled to controlling. Also fix some comments.

BUG=webrtc:5026

Review URL: https://codereview.webrtc.org/1376983002

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

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.
@@ -630,8 +630,10 @@ void Port::SendBindingErrorResponse(StunMessage* request,
 }
 
 void Port::OnMessage(rtc::Message *pmsg) {
-  ASSERT(pmsg->message_id == MSG_CHECKTIMEOUT);
+  ASSERT(pmsg->message_id == MSG_DEAD);
-  CheckTimeout();
+  if (dead()) {
+    Destroy();
+  }
 }
 
 std::string Port::ToString() const {
@@ -652,12 +654,13 @@ void Port::OnConnectionDestroyed(Connection* conn) {
   ASSERT(iter != connections_.end());
   connections_.erase(iter);
 
-  // On the controlled side, ports time out, but only after all connections
+  // On the controlled side, ports time out after all connections fail.
-  // fail.  Note: If a new connection is added after this message is posted,
+  // Note: If a new connection is added after this message is posted, but it
-  // but it fails and is removed before kPortTimeoutDelay, then this message
+  // fails and is removed before kPortTimeoutDelay, then this message will
-  //  will still cause the Port to be destroyed.
+  // still cause the Port to be destroyed.
-  if (ice_role_ == ICEROLE_CONTROLLED)
+  if (dead()) {
-    thread_->PostDelayed(timeout_delay_, this, MSG_CHECKTIMEOUT);
+    thread_->PostDelayed(timeout_delay_, this, MSG_DEAD);
+  }
 }
 
 void Port::Destroy() {
@@ -667,16 +670,6 @@ void Port::Destroy() {
   delete this;
 }
 
-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())
-    Destroy();
-}
-
 const std::string Port::username_fragment() const {
   return ice_username_fragment_;
 }
@@ -918,7 +911,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);
@@ -936,7 +929,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
@@ -976,10 +969,9 @@ void Connection::OnReadPacket(
         // Otherwise silently discard the response message.
         break;
 
-      // Remote end point sent an STUN indication instead of regular
+      // Remote end point sent an STUN indication instead of regular binding
-      // binding request. In this case |last_ping_received_| will be updated.
+      // request. In this case |last_ping_received_| will be updated but no
-      // Otherwise we can mark connection to read timeout. No response will be
+      // response will be sent.
-      // sent in this scenario.
       case STUN_BINDING_INDICATION:
         ReceivedPing();
         break;
@@ -1302,7 +1294,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;
 }

webrtc/p2p/base/port.h

@@ -288,7 +288,7 @@ class Port : public PortInterface, public rtc::MessageHandler,
 
  protected:
   enum {
-    MSG_CHECKTIMEOUT = 0,
+    MSG_DEAD = 0,
     MSG_FIRST_AVAILABLE
   };
 
@@ -340,8 +340,11 @@ class Port : public PortInterface, public rtc::MessageHandler,
   // Called when one of our connections deletes itself.
   void OnConnectionDestroyed(Connection* conn);
 
-  // Checks if this port is useless, and hence, should be destroyed.
+  // Whether this port is dead, and hence, should be destroyed on the controlled
-  void CheckTimeout();
+  // side.
+  bool dead() const {
+    return ice_role_ == ICEROLE_CONTROLLED && connections_.empty();
+  }
 
   rtc::Thread* thread_;
   rtc::PacketSocketFactory* factory_;

webrtc/p2p/base/port_unittest.cc

@@ -2410,3 +2410,38 @@ TEST_F(PortTest, TestControlledTimeout) {
   // The controlled port should be destroyed after 10 milliseconds.
   EXPECT_TRUE_WAIT(destroyed(), kTimeout);
 }
+
+// This test case verifies that if the role of a port changes from controlled
+// to controlling after all connections fail, the port will not be destroyed.
+TEST_F(PortTest, TestControlledToControllingNotDestroyed) {
+  UDPPort* port1 = CreateUdpPort(kLocalAddr1);
+  port1->SetIceRole(cricket::ICEROLE_CONTROLLING);
+  port1->SetIceTiebreaker(kTiebreaker1);
+
+  UDPPort* port2 = CreateUdpPort(kLocalAddr2);
+  ConnectToSignalDestroyed(port2);
+  port2->set_timeout_delay(10);  // milliseconds
+  port2->SetIceRole(cricket::ICEROLE_CONTROLLED);
+  port2->SetIceTiebreaker(kTiebreaker2);
+
+  // The connection must not be destroyed before a connection is attempted.
+  EXPECT_FALSE(destroyed());
+
+  port1->set_component(cricket::ICE_CANDIDATE_COMPONENT_DEFAULT);
+  port2->set_component(cricket::ICE_CANDIDATE_COMPONENT_DEFAULT);
+
+  // Set up channels and ensure both ports will be deleted.
+  TestChannel ch1(port1);
+  TestChannel ch2(port2);
+
+  // Simulate a connection that succeeds, and then is destroyed.
+  StartConnectAndStopChannels(&ch1, &ch2);
+  // Switch the role after all connections are destroyed.
+  EXPECT_TRUE_WAIT(ch2.conn() == nullptr, kTimeout);
+  port1->SetIceRole(cricket::ICEROLE_CONTROLLED);
+  port2->SetIceRole(cricket::ICEROLE_CONTROLLING);
+
+  // After the connection is destroyed, the port should not be destroyed.
+  rtc::Thread::Current()->ProcessMessages(kTimeout);
+  EXPECT_FALSE(destroyed());
+}