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

@@ -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);
+    }
   }
 }
 

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.

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

webrtc/p2p/base/turnport.h

@@ -141,7 +141,8 @@ class TurnPort : public Port {
   sigslot::signal2<TurnPort*, int> SignalTurnRefreshResult;
   sigslot::signal3<TurnPort*, const rtc::SocketAddress&, int>
       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<TurnEntry*> 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);

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);
 }