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

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 ff0c2922de28334f285e4592aefb9dc78d6db4ad..8db1123ae779c8dbef88eeabe30de3a39f93452e 100644
--- a/webrtc/p2p/base/turnport.cc
+++ b/webrtc/p2p/base/turnport.cc
@@ -469,10 +469,10 @@ Connection* TurnPort::CreateConnection(const Candidate& address,
   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;
@@ -562,7 +562,6 @@ bool TurnPort::HandleIncomingPacket(rtc::AsyncPacketSocket* socket,
   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.
@@ -755,7 +754,7 @@ void TurnPort::Close() {
   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.

   }
 }
 
@@ -1491,20 +1490,18 @@ void TurnEntry::OnCreatePermissionError(StunMessage* response, int code) {
       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() {
@@ -1516,8 +1513,8 @@ void TurnEntry::OnChannelBindSuccess() {
 
 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.
@@ -1525,11 +1522,11 @@ void TurnEntry::OnChannelBindError(StunMessage* response, int code) {
     }
   } 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