Do not delete a connection in the turn port with permission error or refresh error.

webrtc/p2p/base/turnport.cc

 /*
  *  Copyright 2012 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 451 matching lines @@
   for (size_t index = 0; index < Candidates().size(); ++index) {
     if (Candidates()[index].type() == RELAY_PORT_TYPE) {
       ProxyConnection* conn = new ProxyConnection(this, index, address);
       AddOrReplaceConnection(conn);
       return conn;
     }
   }
   return NULL;
 }
 
-bool TurnPort::DestroyConnection(const rtc::SocketAddress& address) {
+bool TurnPort::FailAndPruneConnection(const rtc::SocketAddress& address) {
   Connection* conn = GetConnection(address);
   if (conn != nullptr) {
-    conn->Destroy();
+    conn->FailAndPrune();
     return true;
   }
   return false;
 }
 
 int TurnPort::SetOption(rtc::Socket::Option opt, int value) {
   if (!socket_) {
     // If socket is not created yet, these options will be applied during socket
     // creation.
     socket_options_[opt] = value;
@@ skipping 68 matching lines @@
   }
 
   // The message must be at least the size of a channel header.
   if (size < TURN_CHANNEL_HEADER_SIZE) {
     LOG_J(LS_WARNING, this) << "Received TURN message that was too short";
     return false;
   }
 
   if (state_ == STATE_DISCONNECTED) {
     LOG_J(LS_WARNING, this)
         << "Received TURN message while the Turn port is disconnected";

[pthatcher1, 2016/06/16 22:34:21]

Why would we still process packets if we're STATE_DISCONNECTED?

[honghaiz3, 2016/06/16 23:22:53]

Turn port becomes disconnected if there is a refresh error (by calling Close()).
Just like what we do for the permission error, if it received packets, we should
just still process it instead of dropping it. 

[pthatcher1, 2016/06/21 06:58:26]

See my comment blow about making a refresh error not call Close().

[honghaiz3, 2016/06/22 01:50:40]

Changed it back. 

-    return false;
   }
 
   // Check the message type, to see if is a Channel Data message.
   // The message will either be channel data, a TURN data indication, or
   // a response to a previous request.
   uint16_t msg_type = rtc::GetBE16(data);
   if (IsTurnChannelData(msg_type)) {
     HandleChannelData(msg_type, data, size, packet_time);
     return true;
 
@@ skipping 172 matching lines @@
 
 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.
   for (auto kv : connections()) {
-    kv.second->Destroy();
+    kv.second->FailAndPrune();

[pthatcher1, 2016/06/16 22:34:21]

Shouldn't this Destroy() still, since the whole Port is going away?

[honghaiz3, 2016/06/16 23:22:53]

We may enter this in two cases: 
1. Socket error, maybe we should Destroy it. but it won't hurt to just
FailAndPrune them. 
2. Refresh error. In this case we may run into the same issue as the permission
error. 

Do you think we should handle the two cases differently?

[pthatcher1, 2016/06/21 06:58:26]

Yes.  A socket close is fatal.  A refresh is perhaps not.  We should treat fatal
things with Destroy() and non-fatal things differently.  The comment for
STATE_DISCONNECTED indicates it's fatal.  I think we should keep it that way.

In that case, we should just make a refresh error not Close() the port, at least
not immediately.

[honghaiz3, 2016/06/22 01:50:40]

Done.

   }
 }
 
 void TurnPort::OnMessage(rtc::Message* message) {
   switch (message->message_id) {
     case MSG_ALLOCATE_ERROR:
       SignalPortError(this);
       break;
     case MSG_ALLOCATE_MISMATCH:
       OnAllocateMismatch();
@@ skipping 715 matching lines @@
                           << delay << "ms.";
   }
 }
 
 void TurnEntry::OnCreatePermissionError(StunMessage* response, int code) {
   if (code == STUN_ERROR_STALE_NONCE) {
     if (port_->UpdateNonce(response)) {
       SendCreatePermissionRequest(0);
     }
   } else {
-    port_->DestroyConnection(ext_addr_);
+    bool found = port_->FailAndPruneConnection(ext_addr_);
+    if (found) {
+      LOG(LS_ERROR) << "Received TURN CreatePermission error response, "
+                    << "code=" << code << "; pruned connection.";
+    }
     // Send signal with error code.
     port_->SignalCreatePermissionResult(port_, ext_addr_, code);
-    Connection* c = port_->GetConnection(ext_addr_);
-    if (c) {
-      LOG_J(LS_ERROR, c) << "Received TURN CreatePermission error response, "
-                         << "code=" << code << "; killing connection.";
-      c->FailAndDestroy();
-    }
   }
 }
 
 void TurnEntry::OnCreatePermissionTimeout() {
-  port_->DestroyConnection(ext_addr_);
+  port_->FailAndPruneConnection(ext_addr_);
 }
 
 void TurnEntry::OnChannelBindSuccess() {
   LOG_J(LS_INFO, port_) << "Channel bind for " << ext_addr_.ToSensitiveString()
                         << " succeeded";
   ASSERT(state_ == STATE_BINDING || state_ == STATE_BOUND);
   state_ = STATE_BOUND;
 }
 
 void TurnEntry::OnChannelBindError(StunMessage* response, int code) {
   // 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.
+  // we will fail and prune the connection and rely on ICE restart to
+  // re-establish a new connection if needed.
   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_);
+    port_->FailAndPruneConnection(ext_addr_);

[pthatcher1, 2016/06/16 22:34:21]

Shouldn't we differentiate between different error types?  Some types of errors
are pretty fatal (bind error/timeout) and some are not (create
permission/error).  It seems like for the fatal ones, we should
DestroyConnection and for the non-fatal ones we should FailAndPruneConnection().

[honghaiz3, 2016/06/16 23:22:53]

I guess the question is if there is a binding error, is there still chance for a
connection to receive packet?
I think it is possible though data indications. 
If that is the case, I suggest we keep it in case packets received on that
address. 

[pthatcher1, 2016/06/21 06:58:26]

Good point.  These all seem non-fatal.  But the socket closing is still fatal.

[honghaiz3, 2016/06/22 01:50:40]

Acknowledged.

   }
 }
 void TurnEntry::OnChannelBindTimeout() {
   state_ = STATE_UNBOUND;
-  port_->DestroyConnection(ext_addr_);
+  port_->FailAndPruneConnection(ext_addr_);
 }
 }  // namespace cricket