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-Original-Commit-Position: refs/heads/master@{#10122}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: d0b3143f0e37f5f5e0578e88cf740dd839b50c24
diff --git a/p2p/base/port.cc b/p2p/base/port.cc
index a5c7770..ce28551 100644
--- a/p2p/base/port.cc
+++ b/p2p/base/port.cc
@@ -566,7 +566,7 @@
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::OnMessage(rtc::Message *pmsg) {
- ASSERT(pmsg->message_id == MSG_CHECKTIMEOUT);
- CheckTimeout();
+ ASSERT(pmsg->message_id == MSG_DEAD);
+ if (dead()) {
+ Destroy();
+ }
}
std::string Port::ToString() const {
@@ -652,12 +654,13 @@
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)
- thread_->PostDelayed(timeout_delay_, this, MSG_CHECKTIMEOUT);
+ // On the controlled side, ports time out 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 (dead()) {
+ thread_->PostDelayed(timeout_delay_, this, MSG_DEAD);
+ }
}
void Port::Destroy() {
@@ -667,16 +670,6 @@
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 @@
} 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 @@
}
// 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 @@
// 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;
@@ -1302,7 +1294,7 @@
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;
}
diff --git a/p2p/base/port.h b/p2p/base/port.h
index 4616963..c6b7f60 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -288,7 +288,7 @@
protected:
enum {
- MSG_CHECKTIMEOUT = 0,
+ MSG_DEAD = 0,
MSG_FIRST_AVAILABLE
};
@@ -340,8 +340,11 @@
// Called when one of our connections deletes itself.
void OnConnectionDestroyed(Connection* conn);
- // Checks if this port is useless, and hence, should be destroyed.
- void CheckTimeout();
+ // Whether this port is dead, and hence, should be destroyed on the controlled
+ // side.
+ bool dead() const {
+ return ice_role_ == ICEROLE_CONTROLLED && connections_.empty();
+ }
rtc::Thread* thread_;
rtc::PacketSocketFactory* factory_;
diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc
index c4fa5f8..4c262bc 100644
--- a/p2p/base/port_unittest.cc
+++ b/p2p/base/port_unittest.cc
@@ -2410,3 +2410,38 @@
// 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());
+}
