Create a new connection if a candidate reuses an address

webrtc/p2p/base/port.cc

Index: webrtc/p2p/base/port.cc
diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc
index b08cc4c91039f8149fac115a17e70f70236698ff..a33caac12bc9afcc802aaf4c3391f19be13ead06 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -271,7 +271,17 @@ void Port::AddAddress(const rtc::SocketAddress& address,
 }
 
 void Port::AddConnection(Connection* conn) {

[pthatcher1, 2016/05/27 18:42:54]

If we're going to destroy existing connections when we add ones, we should
rename this AddOrReplaceConnection, and comment that the decision of whether to
add or replace is based on remote address.

[honghaiz3, 2016/05/31 18:57:31]

Done.

-  connections_[conn->remote_candidate().address()] = conn;
+  auto ret = connections_.insert(
+      std::make_pair(conn->remote_candidate().address(), conn));
+  // If there is a different connection on the same remote address, replace
+  // it with the new one and destroy the old one.
+  if (ret.second == false && ret.first->second != conn) {
+    LOG_J(LS_WARNING, this)
+        << "A new connection was created on an existing remote address. "
+        << "New remote candidate: " << conn->remote_candidate().ToString();
+    ret.first->second->Destroy();
+    ret.first->second = conn;

[pthatcher1, 2016/05/27 18:42:54]

Instead of putting logic in OnConnectionDestroyed to check if it's the
connection we're looking for, why not just

ret.first->SignalDestroyed.disconnect(this); 

Before destroying it?

[honghaiz3, 2016/05/31 18:57:31]

That would also do. 
But in the OnConnectionDestroyed method, deleting a connection without checking
whether the connection in the map is actually the one destroyed has a potential
risk. It is perhaps slightly safer to check it there.
I could also disconnect here and also do a check there although one of them is
really not necessary. 
WDYT?

[pthatcher1, 2016/05/31 20:09:29]

I think the check is overkill.  We don't have the check now, and it's been fine.
 I think the disconnect from the signal is all we need.

[honghaiz3, 2016/06/01 16:37:59]

Done. You are right. We only hit the replace-connection very rarely. 

+  }
   conn->SignalDestroyed.connect(this, &Port::OnConnectionDestroyed);
   SignalConnectionCreated(this, conn);
 }
@@ -687,7 +697,12 @@ void Port::OnConnectionDestroyed(Connection* conn) {
   AddressMap::iterator iter =
       connections_.find(conn->remote_candidate().address());
   ASSERT(iter != connections_.end());
+  if (iter->second != conn) {
+    // If the deleted connection is different than what we have now, ignore it.
+    return;
+  }
   connections_.erase(iter);
+  HandleConnectionDestroyed(conn);
 
   // On the controlled side, ports time out after all connections fail.
   // Note: If a new connection is added after this message is posted, but it