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

webrtc/p2p/base/port.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 548 matching lines @@
           << retransmit_attr->value();
     }
   }
 
   response.AddAttribute(
       new StunXorAddressAttribute(STUN_ATTR_XOR_MAPPED_ADDRESS, addr));
   response.AddMessageIntegrity(password_);
   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.
   rtc::ByteBuffer buf;
   response.Write(&buf);
   rtc::PacketOptions options(DefaultDscpValue());
   auto err = SendTo(buf.Data(), buf.Length(), addr, options, false);
   if (err < 0) {
     LOG_J(LS_ERROR, this)
         << "Failed to send STUN ping response"
@@ skipping 44 matching lines @@
   rtc::ByteBuffer buf;
   response.Write(&buf);
   rtc::PacketOptions options(DefaultDscpValue());
   SendTo(buf.Data(), buf.Length(), addr, options, false);
   LOG_J(LS_INFO, this) << "Sending STUN binding error: reason=" << reason
                        << " to " << addr.ToSensitiveString();
 }
 
 void Port::OnMessage(rtc::Message *pmsg) {
   ASSERT(pmsg->message_id == MSG_CHECKTIMEOUT);
   CheckTimeout();

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

Can you remove CheckTimeout and just put the logic here?

if (dead()) {
  Destroy();
}

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

Done.

 }
 
 std::string Port::ToString() const {
   std::stringstream ss;
   ss << "Port[" << content_name_ << ":" << component_
      << ":" << generation_ << ":" << type_
      << ":" << network_->ToString() << "]";
   return ss.str();
 }
 
 void Port::EnablePortPackets() {
   enable_port_packets_ = true;
 }
 
 void Port::OnConnectionDestroyed(Connection* conn) {
   AddressMap::iterator iter =
       connections_.find(conn->remote_candidate().address());
   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() {
   ASSERT(connections_.empty());
   LOG_J(LS_INFO, this) << "Port deleted";
   SignalDestroyed(this);
   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())
+  // 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();
 }
 
 const std::string Port::username_fragment() const {
   return ice_username_fragment_;
 }
 
 // A ConnectionRequest is a simple STUN ping used to determine writability.
 class ConnectionRequest : public StunRequest {
  public:
@@ skipping 225 matching lines @@
     if (!pruned_ && (write_state_ == STATE_WRITE_TIMEOUT)) {
       LOG(LS_WARNING) << "Received a data packet on a timed-out Connection. "
                       << "Resetting state to STATE_WRITE_INIT.";
       set_write_state(STATE_WRITE_INIT);
     }
   } else if (!msg) {
     // The packet was STUN, but failed a check and was handled internally.
   } 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);
     switch (msg->type()) {
       case STUN_BINDING_REQUEST:
         LOG_JV(sev, this) << "Received STUN ping"
                           << ", id=" << rtc::hex_encode(msg->transaction_id());
 
         if (remote_ufrag == remote_candidate_.username()) {
           // Check for role conflicts.
           if (!port_->MaybeIceRoleConflict(addr, msg.get(), remote_ufrag)) {
             // Received conflicting role from the peer.
             LOG(LS_INFO) << "Received conflicting role from the peer.";
             return;
           }
 
           // 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
           if (!pruned_ && (write_state_ == STATE_WRITE_TIMEOUT))
             set_write_state(STATE_WRITE_INIT);
 
           if (port_->GetIceRole() == ICEROLE_CONTROLLED) {
             const StunByteStringAttribute* use_candidate_attr =
                 msg->GetByteString(STUN_ATTR_USE_CANDIDATE);
             if (use_candidate_attr) {
@@ skipping 19 matching lines @@
       // id's match.
       case STUN_BINDING_RESPONSE:
       case STUN_BINDING_ERROR_RESPONSE:
         if (msg->ValidateMessageIntegrity(
                 data, size, remote_candidate().password())) {
           requests_.CheckResponse(msg.get());
         }
         // 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;
 
       default:
         ASSERT(false);
         break;
     }
   }
 }
@@ skipping 284 matching lines @@
       remote_candidate_.address() == new_candidate.address() &&
       remote_candidate_.username() == new_candidate.username() &&
       remote_candidate_.password() == new_candidate.password() &&
       remote_candidate_.generation() == new_candidate.generation()) {
     remote_candidate_ = new_candidate;
   }
 }
 
 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;
 }
 
 uint32 Connection::last_received() {
   return std::max(last_data_received_,
              std::max(last_ping_received_, last_ping_response_received_));
 }
 
 size_t Connection::recv_bytes_second() {
@@ skipping 103 matching lines @@
     ASSERT(sent < 0);
     error_ = port_->GetError();
     sent_packets_discarded_++;
   } else {
     send_rate_tracker_.AddSamples(sent);
   }
   return sent;
 }
 
 }  // namespace cricket