Handle Turn error response to RefreshRequest, CreatePermissionRequest, and ChanelBindRequest

webrtc/p2p/base/turnport.cc

Index: webrtc/p2p/base/turnport.cc
diff --git a/webrtc/p2p/base/turnport.cc b/webrtc/p2p/base/turnport.cc
index 5e88365a83d615bdbc2e19ffe62694444ee20039..f4f40fcd0f7a55f1016f6eabd0b06f02ba0ae35b 100644
--- a/webrtc/p2p/base/turnport.cc
+++ b/webrtc/p2p/base/turnport.cc
@@ -137,6 +137,9 @@ class TurnEntry : public sigslot::has_slots<> {
   TurnPort* port() { return port_; }
 
   int channel_id() const { return channel_id_; }
+  // For testing only.
+  void set_channel_id(int channel_id) { channel_id_ = channel_id; }
+
   const rtc::SocketAddress& address() const { return ext_addr_; }
   BindState state() const { return state_; }
 
@@ -155,8 +158,10 @@ class TurnEntry : public sigslot::has_slots<> {
 
   void OnCreatePermissionSuccess();
   void OnCreatePermissionError(StunMessage* response, int code);
+  void OnCreatePermissionTimeout();
   void OnChannelBindSuccess();
   void OnChannelBindError(StunMessage* response, int code);
+  void OnChannelBindTimeout();
   // Signal sent when TurnEntry is destroyed.
   sigslot::signal1<TurnEntry*> SignalDestroyed;
 
@@ -464,6 +469,13 @@ Connection* TurnPort::CreateConnection(const Candidate& address,
   return NULL;
 }
 
+void TurnPort::DestroyConnection(const rtc::SocketAddress& address) {

[pthatcher1, 2015/12/08 20:04:20]

Should it return a bool of whether or not it was destroyed?

[honghaiz3, 2015/12/10 17:58:30]

Done

+  Connection* conn = GetConnection(address);
+  if (conn != nullptr) {
+    conn->Destroy();
+  }
+}
+
 int TurnPort::SetOption(rtc::Socket::Option opt, int value) {
   if (!socket_) {
     // If socket is not created yet, these options will be applied during socket
@@ -696,34 +708,45 @@ void TurnPort::OnAllocateError() {
   // We will send SignalPortError asynchronously as this can be sent during
   // port initialization. This way it will not be blocking other port
   // creation.
-  thread()->Post(this, MSG_ERROR);
+  thread()->Post(this, MSG_ALLOCATE_ERROR);
+}
+
+void TurnPort::OnTurnRefreshError() {
+  // Stop the port from creating new connections.
+  state_ = STATE_DISCONNECTED;
+  // Delete all existing connections; stop sending data.
+  for (auto kv : connections()) {
+    kv.second->Destroy();
+  }

[pthatcher1, 2015/12/08 20:04:20]

Should we have a TunrPort::Close method that does this?  It seems like common
functionality.

[honghaiz3, 2015/12/10 17:58:30]

Done.

 }
 
 void TurnPort::OnMessage(rtc::Message* message) {
-  if (message->message_id == MSG_ERROR) {
-    SignalPortError(this);
-    return;
-  } else if (message->message_id == MSG_ALLOCATE_MISMATCH) {
-    OnAllocateMismatch();
-    return;
-  } else if (message->message_id == MSG_TRY_ALTERNATE_SERVER) {
-    if (server_address().proto == PROTO_UDP) {
-      // Send another allocate request to alternate server, with the received
-      // realm and nonce values.
-      SendRequest(new TurnAllocateRequest(this), 0);
-    } else {
-      // Since it's TCP, we have to delete the connected socket and reconnect
-      // with the alternate server. PrepareAddress will send stun binding once
-      // the new socket is connected.
-      ASSERT(server_address().proto == PROTO_TCP);
-      ASSERT(!SharedSocket());
-      delete socket_;
-      socket_ = NULL;
-      PrepareAddress();
-    }
-    return;
+  switch (message->message_id) {
+    case MSG_ALLOCATE_ERROR:
+      SignalPortError(this);
+      break;
+    case MSG_ALLOCATE_MISMATCH:
+      OnAllocateMismatch();
+      break;
+    case MSG_TRY_ALTERNATE_SERVER:
+      if (server_address().proto == PROTO_UDP) {
+        // Send another allocate request to alternate server, with the received
+        // realm and nonce values.
+        SendRequest(new TurnAllocateRequest(this), 0);
+      } else {
+        // Since it's TCP, we have to delete the connected socket and reconnect
+        // with the alternate server. PrepareAddress will send stun binding once
+        // the new socket is connected.
+        ASSERT(server_address().proto == PROTO_TCP);
+        ASSERT(!SharedSocket());
+        delete socket_;
+        socket_ = NULL;
+        PrepareAddress();
+      }
+      break;
+    default:
+      Port::OnMessage(message);
   }
-  Port::OnMessage(message);
 }
 
 void TurnPort::OnAllocateRequestTimeout() {
@@ -958,6 +981,16 @@ void TurnPort::CancelEntryDestruction(TurnEntry* entry) {
   entry->set_destruction_timestamp(0);
 }
 
+bool TurnPort::SetEntryChannelId(const rtc::SocketAddress& address,
+                                 int channel_id) {
+  TurnEntry* entry = FindEntry(address);

[pthatcher1, 2015/12/08 20:04:19]

Can we just make FindEntry callable by the unit test and then move this logic
into the unit test?

[honghaiz3, 2015/12/10 17:58:30]

If we do that, we will need to make TurnEntry accessible outside of the TurnPort
implementation. Current it is defined in turnport.cc. 

+  if (!entry) {
+    return false;
+  }
+  entry->set_channel_id(channel_id);
+  return true;
+}
+
 TurnAllocateRequest::TurnAllocateRequest(TurnPort* port)
     : StunRequest(new TurnMessage()),
       port_(port) {
@@ -1171,16 +1204,13 @@ void TurnRefreshRequest::OnResponse(StunMessage* response) {
 
   // Schedule a refresh based on the returned lifetime value.
   port_->ScheduleRefresh(lifetime_attr->value());
+  // Result code 0 means the refresh request is successful.
+  port_->SignalTurnRefreshResult(port_, 0);

[pthatcher1, 2015/12/08 20:04:19]

Can we make a name constant for it?  kTurnRefreshSuccess?

[honghaiz3, 2015/12/10 17:58:30]

used name RESULT_SUCCESS_CODE. also used this for the permission request
success.

 }
 
 void TurnRefreshRequest::OnErrorResponse(StunMessage* response) {
   const StunErrorCodeAttribute* error_code = response->GetErrorCode();
 
-  LOG_J(LS_INFO, port_) << "Received TURN refresh error response"
-                        << ", id=" << rtc::hex_encode(id())
-                        << ", code=" << error_code->code()
-                        << ", rtt=" << Elapsed();
-
   if (error_code->code() == STUN_ERROR_STALE_NONCE) {
     if (port_->UpdateNonce(response)) {
       // Send RefreshRequest immediately.
@@ -1191,11 +1221,14 @@ void TurnRefreshRequest::OnErrorResponse(StunMessage* response) {
                              << ", id=" << rtc::hex_encode(id())
                              << ", code=" << error_code->code()
                              << ", rtt=" << Elapsed();
+    port_->OnTurnRefreshError();
+    port_->SignalTurnRefreshResult(port_, error_code->code());
   }
 }
 
 void TurnRefreshRequest::OnTimeout() {
   LOG_J(LS_WARNING, port_) << "TURN refresh timeout " << rtc::hex_encode(id());
+  port_->OnTurnRefreshError();
 }
 
 TurnCreatePermissionRequest::TurnCreatePermissionRequest(
@@ -1248,6 +1281,9 @@ void TurnCreatePermissionRequest::OnErrorResponse(StunMessage* response) {
 void TurnCreatePermissionRequest::OnTimeout() {
   LOG_J(LS_WARNING, port_) << "TURN create permission timeout "
                            << rtc::hex_encode(id());
+  if (entry_) {
+    entry_->OnCreatePermissionTimeout();
+  }
 }
 
 void TurnCreatePermissionRequest::OnEntryDestroyed(TurnEntry* entry) {
@@ -1315,6 +1351,9 @@ void TurnChannelBindRequest::OnErrorResponse(StunMessage* response) {
 void TurnChannelBindRequest::OnTimeout() {
   LOG_J(LS_WARNING, port_) << "TURN channel bind timeout "
                            << rtc::hex_encode(id());
+  if (entry_) {
+    entry_->OnChannelBindTimeout();
+  }
 }
 
 void TurnChannelBindRequest::OnEntryDestroyed(TurnEntry* entry) {
@@ -1396,11 +1435,16 @@ void TurnEntry::OnCreatePermissionError(StunMessage* response, int code) {
       SendCreatePermissionRequest(0);
     }
   } else {
+    port_->DestroyConnection(ext_addr_);
     // Send signal with error code.
     port_->SignalCreatePermissionResult(port_, ext_addr_, code);
   }
 }
 
+void TurnEntry::OnCreatePermissionTimeout() {
+  port_->DestroyConnection(ext_addr_);
+}
+
 void TurnEntry::OnChannelBindSuccess() {
   LOG_J(LS_INFO, port_) << "Channel bind for " << ext_addr_.ToSensitiveString()
                         << " succeeded";
@@ -1409,14 +1453,21 @@ void TurnEntry::OnChannelBindSuccess() {
 }
 
 void TurnEntry::OnChannelBindError(StunMessage* response, int code) {
-  // TODO(mallinath) - Implement handling of error response for channel
-  // bind request as per http://tools.ietf.org/html/rfc5766#section-11.3
+  // If the channel bind fails due to errors other than STATE_NONCE,
+  // we just destroy the connection and rely on ICE restart to re-establish
+  // the connection.
   if (code == STUN_ERROR_STALE_NONCE) {
     if (port_->UpdateNonce(response)) {
       // Send channel bind request with fresh nonce.
       SendChannelBindRequest(0);
     }
+  } else {
+    state_ = STATE_UNBOUND;
+    port_->DestroyConnection(ext_addr_);
   }
 }
-
+void TurnEntry::OnChannelBindTimeout() {
+  state_ = STATE_UNBOUND;
+  port_->DestroyConnection(ext_addr_);
+}
 }  // namespace cricket