From 6b9ab9204b9e0891cb0be2595a0d7ada92945cea Mon Sep 17 00:00:00 2001 From: honghaiz Date: Tue, 5 Jan 2016 09:06:12 -0800 Subject: [PATCH] Cease all future TURN requests when a TURN refresh request fails for a given TURN port. This fixes an assert error in Turnport::OnSendStunPacket BUG=webrtc:5388 Review URL: https://codereview.webrtc.org/1547373002 Cr-Commit-Position: refs/heads/master@{#11152} --- webrtc/p2p/base/stunrequest.cc | 8 +++++--- webrtc/p2p/base/stunrequest.h | 8 ++++++-- webrtc/p2p/base/turnport.cc | 20 +++++++++++++++----- webrtc/p2p/base/turnport.h | 8 +++++--- webrtc/p2p/base/turnport_unittest.cc | 18 ++++++++++-------- 5 files changed, 41 insertions(+), 21 deletions(-) diff --git a/webrtc/p2p/base/stunrequest.cc b/webrtc/p2p/base/stunrequest.cc index 0a0b1a8997..ce0364e8db 100644 --- a/webrtc/p2p/base/stunrequest.cc +++ b/webrtc/p2p/base/stunrequest.cc @@ -53,11 +53,13 @@ void StunRequestManager::SendDelayed(StunRequest* request, int delay) { } } -void StunRequestManager::Flush() { +void StunRequestManager::Flush(int msg_type) { for (const auto kv : requests_) { StunRequest* request = kv.second; - thread_->Clear(request, MSG_STUN_SEND); - thread_->Send(request, MSG_STUN_SEND, NULL); + if (msg_type == kAllRequests || msg_type == request->type()) { + thread_->Clear(request, MSG_STUN_SEND); + thread_->Send(request, MSG_STUN_SEND, NULL); + } } } diff --git a/webrtc/p2p/base/stunrequest.h b/webrtc/p2p/base/stunrequest.h index 15ea0c73b7..44c1ebff56 100644 --- a/webrtc/p2p/base/stunrequest.h +++ b/webrtc/p2p/base/stunrequest.h @@ -21,6 +21,8 @@ namespace cricket { class StunRequest; +const int kAllRequests = 0; + // Manages a set of STUN requests, sending and resending until we receive a // response or determine that the request has timed out. class StunRequestManager { @@ -32,8 +34,10 @@ class StunRequestManager { void Send(StunRequest* request); void SendDelayed(StunRequest* request, int delay); - // Sends all pending requests right away. Only for testing. - void Flush(); + // If |msg_type| is kAllRequests, sends all pending requests right away. + // Otherwise, sends those that have a matching type right away. + // Only for testing. + void Flush(int msg_type); // Removes a stun request that was added previously. This will happen // automatically when a request succeeds, fails, or times out. diff --git a/webrtc/p2p/base/turnport.cc b/webrtc/p2p/base/turnport.cc index 328683b5ae..022207aa5b 100644 --- a/webrtc/p2p/base/turnport.cc +++ b/webrtc/p2p/base/turnport.cc @@ -409,11 +409,7 @@ void TurnPort::OnSocketConnect(rtc::AsyncPacketSocket* socket) { void TurnPort::OnSocketClose(rtc::AsyncPacketSocket* socket, int error) { LOG_J(LS_WARNING, this) << "Connection with server failed, error=" << error; ASSERT(socket == socket_); - if (!ready()) { - OnAllocateError(); - } - request_manager_.Clear(); - state_ = STATE_DISCONNECTED; + Close(); } void TurnPort::OnAllocateMismatch() { @@ -717,7 +713,18 @@ void TurnPort::OnAllocateError() { thread()->Post(this, MSG_ALLOCATE_ERROR); } +void TurnPort::OnTurnRefreshError() { + // Need to Close the port asynchronously because otherwise, the refresh + // request may be deleted twice: once at the end of the message processing + // and the other in Close(). + thread()->Post(this, MSG_REFRESH_ERROR); +} + void TurnPort::Close() { + if (!ready()) { + OnAllocateError(); + } + request_manager_.Clear(); // Stop the port from creating new connections. state_ = STATE_DISCONNECTED; // Delete all existing connections; stop sending data. @@ -734,6 +741,9 @@ void TurnPort::OnMessage(rtc::Message* message) { case MSG_ALLOCATE_MISMATCH: OnAllocateMismatch(); break; + case MSG_REFRESH_ERROR: + Close(); + break; case MSG_TRY_ALTERNATE_SERVER: if (server_address().proto == PROTO_UDP) { // Send another allocate request to alternate server, with the received diff --git a/webrtc/p2p/base/turnport.h b/webrtc/p2p/base/turnport.h index 4be9249c8f..a19f67632e 100644 --- a/webrtc/p2p/base/turnport.h +++ b/webrtc/p2p/base/turnport.h @@ -141,7 +141,8 @@ class TurnPort : public Port { sigslot::signal2 SignalTurnRefreshResult; sigslot::signal3 SignalCreatePermissionResult; - void FlushRequests() { request_manager_.Flush(); } + void FlushRequests(int msg_type) { request_manager_.Flush(msg_type); } + bool HasRequests() { return !request_manager_.empty(); } void set_credentials(RelayCredentials& credentials) { credentials_ = credentials; } @@ -178,7 +179,8 @@ class TurnPort : public Port { enum { MSG_ALLOCATE_ERROR = MSG_FIRST_AVAILABLE, MSG_ALLOCATE_MISMATCH, - MSG_TRY_ALTERNATE_SERVER + MSG_TRY_ALTERNATE_SERVER, + MSG_REFRESH_ERROR }; typedef std::list EntryList; @@ -199,7 +201,7 @@ class TurnPort : public Port { // Shuts down the turn port, usually because of some fatal errors. void Close(); - void OnTurnRefreshError() { Close(); } + void OnTurnRefreshError(); bool SetAlternateServer(const rtc::SocketAddress& address); void ResolveTurnAddress(const rtc::SocketAddress& address); void OnResolveResult(rtc::AsyncResolverInterface* resolver); diff --git a/webrtc/p2p/base/turnport_unittest.cc b/webrtc/p2p/base/turnport_unittest.cc index eac3474d26..d0dc487168 100644 --- a/webrtc/p2p/base/turnport_unittest.cc +++ b/webrtc/p2p/base/turnport_unittest.cc @@ -309,7 +309,7 @@ class TurnPortTest : public testing::Test, } bool CheckConnectionDestroyed() { - turn_port_->FlushRequests(); + turn_port_->FlushRequests(cricket::kAllRequests); rtc::Thread::Current()->ProcessMessages(50); return connection_destroyed_; } @@ -693,8 +693,9 @@ TEST_F(TurnPortTest, TestTurnTcpAllocateMismatch) { TEST_F(TurnPortTest, TestRefreshRequestGetsErrorResponse) { CreateTurnPort(kTurnUsername, kTurnPassword, kTurnUdpProtoAddr); - turn_port_->PrepareAddress(); - EXPECT_TRUE_WAIT(turn_ready_, kTimeout); + PrepareTurnAndUdpPorts(); + turn_port_->CreateConnection(udp_port_->Candidates()[0], + Port::ORIGIN_MESSAGE); // Set bad credentials. cricket::RelayCredentials bad_credentials("bad_user", "bad_pwd"); turn_port_->set_credentials(bad_credentials); @@ -702,13 +703,14 @@ TEST_F(TurnPortTest, TestRefreshRequestGetsErrorResponse) { // This sends out the first RefreshRequest with correct credentials. // When this succeeds, it will schedule a new RefreshRequest with the bad // credential. - turn_port_->FlushRequests(); + turn_port_->FlushRequests(cricket::TURN_REFRESH_REQUEST); EXPECT_TRUE_WAIT(turn_refresh_success_, kTimeout); // Flush it again, it will receive a bad response. - turn_port_->FlushRequests(); + turn_port_->FlushRequests(cricket::TURN_REFRESH_REQUEST); EXPECT_TRUE_WAIT(!turn_refresh_success_, kTimeout); + EXPECT_TRUE_WAIT(!turn_port_->connected(), kTimeout); EXPECT_TRUE(turn_port_->connections().empty()); - EXPECT_FALSE(turn_port_->connected()); + EXPECT_FALSE(turn_port_->HasRequests()); } // Test that CreateConnection will return null if port becomes disconnected. @@ -840,10 +842,10 @@ TEST_F(TurnPortTest, TestRefreshCreatePermissionRequest) { // another request with bad_ufrag and bad_pwd. cricket::RelayCredentials bad_credentials("bad_user", "bad_pwd"); turn_port_->set_credentials(bad_credentials); - turn_port_->FlushRequests(); + turn_port_->FlushRequests(cricket::kAllRequests); ASSERT_TRUE_WAIT(turn_create_permission_success_, kTimeout); // Flush the requests again; the create-permission-request will fail. - turn_port_->FlushRequests(); + turn_port_->FlushRequests(cricket::kAllRequests); EXPECT_TRUE_WAIT(!turn_create_permission_success_, kTimeout); EXPECT_TRUE_WAIT(connection_destroyed_, kTimeout); }