TCPConnection can never be deteted if they fail to connect.
Since the TCPConnection has never been connected, they are not scheduled for ping hence will never be detected.
Also fix the case when reconnect fails, as it has become READABLE before, it also will not be deleted.
BUG=webrtc:4936
R=pthatcher@webrtc.org
Review URL: https://codereview.webrtc.org/1307083002 .
Cr-Commit-Position: refs/heads/master@{#9782}
diff --git a/webrtc/p2p/base/port_unittest.cc b/webrtc/p2p/base/port_unittest.cc
index b0ba89f..453b77c 100644
--- a/webrtc/p2p/base/port_unittest.cc
+++ b/webrtc/p2p/base/port_unittest.cc
@@ -200,13 +200,16 @@
class TestChannel : public sigslot::has_slots<> {
public:
// Takes ownership of |p1| (but not |p2|).
- TestChannel(Port* p1, Port* p2)
- : ice_mode_(ICEMODE_FULL), src_(p1), dst_(p2), complete_count_(0),
- conn_(NULL), remote_request_(), nominated_(false) {
- src_->SignalPortComplete.connect(
- this, &TestChannel::OnPortComplete);
- src_->SignalUnknownAddress.connect(this, &TestChannel::OnUnknownAddress);
- src_->SignalDestroyed.connect(this, &TestChannel::OnSrcPortDestroyed);
+ TestChannel(Port* p1)
+ : ice_mode_(ICEMODE_FULL),
+ port_(p1),
+ complete_count_(0),
+ conn_(NULL),
+ remote_request_(),
+ nominated_(false) {
+ port_->SignalPortComplete.connect(this, &TestChannel::OnPortComplete);
+ port_->SignalUnknownAddress.connect(this, &TestChannel::OnUnknownAddress);
+ port_->SignalDestroyed.connect(this, &TestChannel::OnSrcPortDestroyed);
}
int complete_count() { return complete_count_; }
@@ -214,11 +217,9 @@
const SocketAddress& remote_address() { return remote_address_; }
const std::string remote_fragment() { return remote_frag_; }
- void Start() {
- src_->PrepareAddress();
- }
- void CreateConnection() {
- conn_ = src_->CreateConnection(GetCandidate(dst_), Port::ORIGIN_MESSAGE);
+ void Start() { port_->PrepareAddress(); }
+ void CreateConnection(const Candidate& remote_candidate) {
+ conn_ = port_->CreateConnection(remote_candidate, Port::ORIGIN_MESSAGE);
IceMode remote_ice_mode =
(ice_mode_ == ICEMODE_FULL) ? ICEMODE_LITE : ICEMODE_FULL;
conn_->set_remote_ice_mode(remote_ice_mode);
@@ -236,13 +237,13 @@
nominated_ = true;
}
}
- void AcceptConnection() {
+ void AcceptConnection(const Candidate& remote_candidate) {
ASSERT_TRUE(remote_request_.get() != NULL);
- Candidate c = GetCandidate(dst_);
+ Candidate c = remote_candidate;
c.set_address(remote_address_);
- conn_ = src_->CreateConnection(c, Port::ORIGIN_MESSAGE);
+ conn_ = port_->CreateConnection(c, Port::ORIGIN_MESSAGE);
conn_->SignalDestroyed.connect(this, &TestChannel::OnDestroyed);
- src_->SendBindingResponse(remote_request_.get(), remote_address_);
+ port_->SendBindingResponse(remote_request_.get(), remote_address_);
remote_request_.reset();
}
void Ping() {
@@ -273,7 +274,7 @@
ProtocolType proto,
IceMessage* msg, const std::string& rf,
bool /*port_muxed*/) {
- ASSERT_EQ(src_.get(), port);
+ ASSERT_EQ(port_.get(), port);
if (!remote_address_.IsNil()) {
ASSERT_EQ(remote_address_, addr);
}
@@ -302,11 +303,11 @@
}
void OnSrcPortDestroyed(PortInterface* port) {
- Port* destroyed_src = src_.release();
+ Port* destroyed_src = port_.release();
ASSERT_EQ(destroyed_src, port);
}
- Port* src_port() { return src_.get(); }
+ Port* port() { return port_.get(); }
bool nominated() const { return nominated_; }
@@ -325,8 +326,7 @@
}
IceMode ice_mode_;
- rtc::scoped_ptr<Port> src_;
- Port* dst_;
+ rtc::scoped_ptr<Port> port_;
int complete_count_;
Connection* conn_;
@@ -565,7 +565,7 @@
WAIT(!ch2->remote_address().IsNil(), kTimeout);
// Send a ping from dst to src.
- ch2->AcceptConnection();
+ ch2->AcceptConnection(GetCandidate(ch1->port()));
ch2->Ping();
EXPECT_EQ_WAIT(Connection::STATE_WRITABLE, ch2->conn()->write_state(),
kTimeout);
@@ -579,7 +579,7 @@
ch1->Start();
ch2->Start();
- ch1->CreateConnection();
+ ch1->CreateConnection(GetCandidate(ch2->port()));
ConnectStartedChannels(ch1, ch2);
// Destroy the connections.
@@ -590,15 +590,24 @@
// This disconnects both end's Connection and make sure ch2 ready for new
// connection.
void DisconnectTcpTestChannels(TestChannel* ch1, TestChannel* ch2) {
- ASSERT_TRUE(ss_->CloseTcpConnections(
- static_cast<TCPConnection*>(ch1->conn())->socket()->GetLocalAddress(),
- static_cast<TCPConnection*>(ch2->conn())->socket()->GetLocalAddress()));
+ TCPConnection* tcp_conn1 = static_cast<TCPConnection*>(ch1->conn());
+ TCPConnection* tcp_conn2 = static_cast<TCPConnection*>(ch2->conn());
+ ASSERT_TRUE(
+ ss_->CloseTcpConnections(tcp_conn1->socket()->GetLocalAddress(),
+ tcp_conn2->socket()->GetLocalAddress()));
// Wait for both OnClose are delivered.
EXPECT_TRUE_WAIT(!ch1->conn()->connected(), kTimeout);
EXPECT_TRUE_WAIT(!ch2->conn()->connected(), kTimeout);
- // Destroy channel2 connection to get ready for new incoming TCPConnection.
+ // Ensure redundant SignalClose events on TcpConnection won't break tcp
+ // reconnection. Chromium will fire SignalClose for all outstanding IPC
+ // packets during reconnection.
+ tcp_conn1->socket()->SignalClose(tcp_conn1->socket(), 0);
+ tcp_conn2->socket()->SignalClose(tcp_conn2->socket(), 0);
+
+ // Speed up destroying ch2's connection such that the test is ready to
+ // accept a new connection from ch1 before ch1's connection destroys itself.
ch2->conn()->Destroy();
EXPECT_TRUE_WAIT(ch2->conn() == NULL, kTimeout);
}
@@ -614,8 +623,8 @@
port2->set_component(cricket::ICE_CANDIDATE_COMPONENT_DEFAULT);
// Set up channels and ensure both ports will be deleted.
- TestChannel ch1(port1, port2);
- TestChannel ch2(port2, port1);
+ TestChannel ch1(port1);
+ TestChannel ch2(port2);
EXPECT_EQ(0, ch1.complete_count());
EXPECT_EQ(0, ch2.complete_count());
@@ -625,7 +634,7 @@
ASSERT_EQ_WAIT(1, ch2.complete_count(), kTimeout);
// Initial connecting the channel, create connection on channel1.
- ch1.CreateConnection();
+ ch1.CreateConnection(GetCandidate(port2));
ConnectStartedChannels(&ch1, &ch2);
// Shorten the timeout period.
@@ -666,11 +675,10 @@
EXPECT_FALSE(ch2.connection_ready_to_send());
} else {
EXPECT_EQ(ch1.conn()->write_state(), Connection::STATE_WRITABLE);
- EXPECT_TRUE_WAIT(
- ch1.conn()->write_state() == Connection::STATE_WRITE_TIMEOUT,
- kTcpReconnectTimeout + kTimeout);
- EXPECT_FALSE(ch1.connection_ready_to_send());
- EXPECT_FALSE(ch2.connection_ready_to_send());
+ // Since the reconnection never happens, the connections should have been
+ // destroyed after the timeout.
+ EXPECT_TRUE_WAIT(!ch1.conn(), kTcpReconnectTimeout + kTimeout);
+ EXPECT_TRUE(!ch2.conn());
}
// Tear down and ensure that goes smoothly.
@@ -730,6 +738,9 @@
return &nat_socket_factory1_;
}
+ protected:
+ rtc::VirtualSocketServer* vss() { return ss_.get(); }
+
private:
rtc::Thread* main_;
rtc::scoped_ptr<rtc::PhysicalSocketServer> pss_;
@@ -761,8 +772,8 @@
port2->set_component(cricket::ICE_CANDIDATE_COMPONENT_DEFAULT);
// Set up channels and ensure both ports will be deleted.
- TestChannel ch1(port1, port2);
- TestChannel ch2(port2, port1);
+ TestChannel ch1(port1);
+ TestChannel ch2(port2);
EXPECT_EQ(0, ch1.complete_count());
EXPECT_EQ(0, ch2.complete_count());
@@ -773,7 +784,7 @@
ASSERT_EQ_WAIT(1, ch2.complete_count(), kTimeout);
// Send a ping from src to dst. This may or may not make it.
- ch1.CreateConnection();
+ ch1.CreateConnection(GetCandidate(port2));
ASSERT_TRUE(ch1.conn() != NULL);
EXPECT_TRUE_WAIT(ch1.conn()->connected(), kTimeout); // for TCP connect
ch1.Ping();
@@ -791,7 +802,7 @@
EXPECT_TRUE(same_addr2);
// Send a ping from dst to src.
- ch2.AcceptConnection();
+ ch2.AcceptConnection(GetCandidate(port1));
ASSERT_TRUE(ch2.conn() != NULL);
ch2.Ping();
EXPECT_EQ_WAIT(Connection::STATE_WRITABLE, ch2.conn()->write_state(),
@@ -803,7 +814,7 @@
EXPECT_TRUE(ch2.remote_address().IsNil());
// Send a ping from dst to src. Again, this may or may not make it.
- ch2.CreateConnection();
+ ch2.CreateConnection(GetCandidate(port1));
ASSERT_TRUE(ch2.conn() != NULL);
ch2.Ping();
WAIT(ch2.conn()->write_state() == Connection::STATE_WRITABLE, kTimeout);
@@ -834,7 +845,7 @@
EXPECT_TRUE(ch1.remote_address().IsNil());
// Pick up the actual address and establish the connection.
- ch2.AcceptConnection();
+ ch2.AcceptConnection(GetCandidate(port1));
ASSERT_TRUE(ch2.conn() != NULL);
ch2.Ping();
EXPECT_EQ_WAIT(Connection::STATE_WRITABLE, ch2.conn()->write_state(),
@@ -846,7 +857,7 @@
EXPECT_EQ(Connection::STATE_READ_INIT, ch1.conn()->read_state());
// Update our address and complete the connection.
- ch1.AcceptConnection();
+ ch1.AcceptConnection(GetCandidate(port2));
ch1.Ping();
EXPECT_EQ_WAIT(Connection::STATE_WRITABLE, ch1.conn()->write_state(),
kTimeout);
@@ -1171,6 +1182,33 @@
TestTcpReconnect(false /* ping */, false /* send */);
}
+// Test when TcpConnection never connects, the OnClose() will be called to
+// destroy the connection.
+TEST_F(PortTest, TestTcpNeverConnect) {
+ Port* port1 = CreateTcpPort(kLocalAddr1);
+ port1->SetIceRole(cricket::ICEROLE_CONTROLLING);
+ port1->set_component(cricket::ICE_CANDIDATE_COMPONENT_DEFAULT);
+
+ // Set up a channel and ensure the port will be deleted.
+ TestChannel ch1(port1);
+ EXPECT_EQ(0, ch1.complete_count());
+
+ ch1.Start();
+ ASSERT_EQ_WAIT(1, ch1.complete_count(), kTimeout);
+
+ rtc::scoped_ptr<rtc::AsyncSocket> server(
+ vss()->CreateAsyncSocket(kLocalAddr2.family(), SOCK_STREAM));
+ // Bind but not listen.
+ EXPECT_EQ(0, server->Bind(kLocalAddr2));
+
+ Candidate c = GetCandidate(port1);
+ c.set_address(server->GetLocalAddress());
+
+ ch1.CreateConnection(c);
+ EXPECT_TRUE(ch1.conn());
+ EXPECT_TRUE_WAIT(!ch1.conn(), kTimeout); // for TCP connect
+}
+
/* TODO: Enable these once testrelayserver can accept external TCP.
TEST_F(PortTest, TestTcpToTcpRelay) {
TestTcpToRelay(PROTO_TCP);
@@ -2162,8 +2200,8 @@
port2->SetIceRole(cricket::ICEROLE_CONTROLLED);
// Set up channels.
- TestChannel ch1(port1, port2);
- TestChannel ch2(port2, port1);
+ TestChannel ch1(port1);
+ TestChannel ch2(port2);
// Acquire addresses.
ch1.Start();
@@ -2172,7 +2210,7 @@
ASSERT_EQ_WAIT(1, ch2.complete_count(), kTimeout);
// Send a ping from src to dst.
- ch1.CreateConnection();
+ ch1.CreateConnection(GetCandidate(port2));
ASSERT_TRUE(ch1.conn() != NULL);
EXPECT_EQ(Connection::STATE_WRITE_INIT, ch1.conn()->write_state());
EXPECT_TRUE_WAIT(ch1.conn()->connected(), kTimeout); // for TCP connect
@@ -2187,7 +2225,7 @@
// Accept the connection to return the binding response, transition to
// writable, and allow data to be sent.
- ch2.AcceptConnection();
+ ch2.AcceptConnection(GetCandidate(port1));
EXPECT_EQ_WAIT(Connection::STATE_WRITABLE, ch1.conn()->write_state(),
kTimeout);
EXPECT_EQ(data_size, ch1.conn()->Send(data, data_size, options));
@@ -2233,14 +2271,14 @@
port2->SetIceRole(cricket::ICEROLE_CONTROLLED);
// Set up channels.
- TestChannel ch1(port1, port2);
- TestChannel ch2(port2, port1);
+ TestChannel ch1(port1);
+ TestChannel ch2(port2);
// Acquire addresses.
ch1.Start();
ch2.Start();
- ch1.CreateConnection();
+ ch1.CreateConnection(GetCandidate(port2));
ASSERT_TRUE(ch1.conn() != NULL);
EXPECT_EQ(Connection::STATE_WRITE_INIT, ch1.conn()->write_state());
@@ -2265,7 +2303,7 @@
kLocalAddr2, "rfrag", "rpass",
cricket::ICEROLE_CONTROLLED, kTiebreaker2));
// Setup TestChannel. This behaves like FULL mode client.
- TestChannel ch1(ice_full_port, ice_lite_port.get());
+ TestChannel ch1(ice_full_port);
ch1.SetIceMode(ICEMODE_FULL);
// Start gathering candidates.
@@ -2275,7 +2313,7 @@
ASSERT_EQ_WAIT(1, ch1.complete_count(), kTimeout);
ASSERT_FALSE(ice_lite_port->Candidates().empty());
- ch1.CreateConnection();
+ ch1.CreateConnection(GetCandidate(ice_lite_port.get()));
ASSERT_TRUE(ch1.conn() != NULL);
EXPECT_EQ(Connection::STATE_WRITE_INIT, ch1.conn()->write_state());
@@ -2332,8 +2370,8 @@
port2->SetIceTiebreaker(kTiebreaker2);
// Set up channels and ensure both ports will be deleted.
- TestChannel ch1(port1, port2);
- TestChannel ch2(port2, port1);
+ TestChannel ch1(port1);
+ TestChannel ch2(port2);
// Simulate a connection that succeeds, and then is destroyed.
StartConnectAndStopChannels(&ch1, &ch2);
@@ -2363,8 +2401,8 @@
port2->set_component(cricket::ICE_CANDIDATE_COMPONENT_DEFAULT);
// Set up channels and ensure both ports will be deleted.
- TestChannel ch1(port1, port2);
- TestChannel ch2(port2, port1);
+ TestChannel ch1(port1);
+ TestChannel ch2(port2);
// Simulate a connection that succeeds, and then is destroyed.
StartConnectAndStopChannels(&ch1, &ch2);
diff --git a/webrtc/p2p/base/tcpport.cc b/webrtc/p2p/base/tcpport.cc
index 7dc416f..91b9944 100644
--- a/webrtc/p2p/base/tcpport.cc
+++ b/webrtc/p2p/base/tcpport.cc
@@ -29,32 +29,32 @@
* Data could only be sent in state 3. Sening data during state 2 & 6 will get
* EWOULDBLOCK, 4 & 5 EPIPE.
*
- * 7 -------------+
- * |Connected: N |
- * Timeout |Writable: N | Timeout
- * +------------------->|Connection is |<----------------+
- * | |Dead | |
- * | +--------------+ |
- * | ^ |
- * | OnClose | |
- * | +-----------------------+ | |
- * | | | |Timeout |
- * | v | | |
- * 4 +----------+ 5 -----+--+--+ 6 -----+-----+
- * |Connected: N|Send() or |Connected: N| |Connected: Y|
- * |Writable: Y|Ping() |Writable: Y|OnConnect |Writable: Y|
- * |PendingTCP:N+--------> |PendingTCP:Y+---------> |PendingTCP:N|
- * |PretendWri:Y| |PretendWri:Y| |PretendWri:Y|
- * +-----+------+ +------------+ +---+--+-----+
- * ^ ^ | |
- * | | OnClose | |
- * | +----------------------------------------------+ |
- * | |
- * | Stun Binding Completed |
- * | |
- * | OnClose |
- * +------------------------------------------------+ |
- * | v
+ * OS Timeout 7 -------------+
+ * +----------------------->|Connected: N |
+ * | |Writable: N | Timeout
+ * | Timeout |Connection is |<----------------+
+ * | +------------------->|Dead | |
+ * | | +--------------+ |
+ * | | ^ |
+ * | | OnClose | |
+ * | | +-----------------------+ | |
+ * | | | | |Timeout |
+ * | | v | | |
+ * | 4 +----------+ 5 -----+--+--+ 6 -----+-----+
+ * | |Connected: N|Send() or |Connected: N| |Connected: Y|
+ * | |Writable: Y|Ping() |Writable: Y|OnConnect |Writable: Y|
+ * | |PendingTCP:N+--------> |PendingTCP:Y+---------> |PendingTCP:N|
+ * | |PretendWri:Y| |PretendWri:Y| |PretendWri:Y|
+ * | +-----+------+ +------------+ +---+--+-----+
+ * | ^ ^ | |
+ * | | | OnClose | |
+ * | | +----------------------------------------------+ |
+ * | | |
+ * | | Stun Binding Completed |
+ * | | |
+ * | | OnClose |
+ * | +------------------------------------------------+ |
+ * | | v
* 1 -----------+ 2 -----------+Stun 3 -----------+
* |Connected: N| |Connected: Y|Binding |Connected: Y|
* |Writable: N|OnConnect |Writable: N|Completed |Writable: Y|
@@ -384,7 +384,7 @@
<< socket_ip.ToSensitiveString()
<< ", different from the local candidate IP "
<< port()->ip().ToSensitiveString();
- socket_->Close();
+ OnClose(socket, 0);
}
}
@@ -396,6 +396,9 @@
// packet it can't send.
if (connected()) {
set_connected(false);
+
+ // Prevent the connection from being destroyed by redundant SignalClose
+ // events.
pretending_to_be_writable_ = true;
// We don't attempt reconnect right here. This is to avoid a case where the
@@ -403,6 +406,12 @@
// when the connection is used to Send() or Ping().
port()->thread()->PostDelayed(reconnection_timeout(), this,
MSG_TCPCONNECTION_DELAYED_ONCLOSE);
+ } else if (!pretending_to_be_writable_) {
+ // OnClose could be called when the underneath socket times out during the
+ // initial connect() (i.e. |pretending_to_be_writable_| is false) . We have
+ // to manually destroy here as this connection, as never connected, will not
+ // be scheduled for ping to trigger destroy.
+ Destroy();
}
}
@@ -413,7 +422,7 @@
// seconds, it's time to tear this down. This is the case for the original
// TCP connection on passive side during a reconnect.
if (pretending_to_be_writable_) {
- set_write_state(STATE_WRITE_TIMEOUT);
+ Destroy();
}
break;
default:
