Create a new connection if a candidate reuses an address

webrtc/p2p/base/p2ptransportchannel.cc

Index: webrtc/p2p/base/p2ptransportchannel.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index b1de3e91d3d4a3ee5bca7221e515a10c4aae090c..17a3643bc5e7aa863f25d3dfeb582edde9414342 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -532,7 +532,7 @@ void P2PTransportChannel::OnPortReady(PortAllocatorSession *session,
   std::vector<RemoteCandidate>::iterator iter;
   for (iter = remote_candidates_.begin(); iter != remote_candidates_.end();
        ++iter) {
-    CreateConnection(port, *iter, iter->origin_port());
+    MaybeCreateConnection(port, *iter, iter->origin_port());
   }
 
   SortConnections();
@@ -828,7 +828,7 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
   bool created = false;
   std::vector<PortInterface *>::reverse_iterator it;
   for (it = ports_.rbegin(); it != ports_.rend(); ++it) {
-    if (CreateConnection(*it, remote_candidate, origin_port)) {
+    if (MaybeCreateConnection(*it, remote_candidate, origin_port)) {
       if (*it == origin_port)
         created = true;
     }
@@ -836,7 +836,7 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
 
   if ((origin_port != NULL) &&
       std::find(ports_.begin(), ports_.end(), origin_port) == ports_.end()) {
-    if (CreateConnection(origin_port, remote_candidate, origin_port))
+    if (MaybeCreateConnection(origin_port, remote_candidate, origin_port))
       created = true;
   }
 
@@ -848,46 +848,56 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
 
 // Setup a connection object for the local and remote candidate combination.
 // And then listen to connection object for changes.
-bool P2PTransportChannel::CreateConnection(PortInterface* port,
-                                           const Candidate& remote_candidate,
-                                           PortInterface* origin_port) {
+bool P2PTransportChannel::MaybeCreateConnection(
+    PortInterface* port,
+    const Candidate& remote_candidate,
+    PortInterface* origin_port) {
   if (!port->SupportsProtocol(remote_candidate.protocol())) {
     return false;
   }
   // Look for an existing connection with this remote address.  If one is not
-  // found, then we can create a new connection for this address.
+  // found or it is found but the existing remote candidate has an older
+  // generation, then we can create a new connection for this address.
   Connection* connection = port->GetConnection(remote_candidate.address());
-  if (connection != NULL) {
-    connection->MaybeUpdatePeerReflexiveCandidate(remote_candidate);
-
-    // It is not legal to try to change any of the parameters of an existing
-    // connection; however, the other side can send a duplicate candidate.
-    if (!remote_candidate.IsEquivalent(connection->remote_candidate())) {
-      LOG(INFO) << "Attempt to change a remote candidate."
-                << " Existing remote candidate: "
-                << connection->remote_candidate().ToString()
-                << "New remote candidate: "
-                << remote_candidate.ToString();
-      return false;
-    }
-  } else {
-    PortInterface::CandidateOrigin origin = GetOrigin(port, origin_port);
+  if (connection == nullptr ||
+      connection->remote_candidate().generation() <
+          remote_candidate.generation()) {
+    return CreateConnection(port, remote_candidate, origin_port);
+  }
 
-    // Don't create connection if this is a candidate we received in a
-    // message and we are not allowed to make outgoing connections.
-    if (origin == PortInterface::ORIGIN_MESSAGE && incoming_only_)
-      return false;
+  // Check if this is a peer reflexive candidate.
+  connection->MaybeUpdatePeerReflexiveCandidate(remote_candidate);
 
-    connection = port->CreateConnection(remote_candidate, origin);
-    if (!connection)
-      return false;
+  // It is not legal to try to change any of the parameters of an existing
+  // connection; however, the other side can send a duplicate candidate.
+  if (!remote_candidate.IsEquivalent(connection->remote_candidate())) {
+    LOG(INFO) << "Attempt to change a remote candidate."
+              << " Existing remote candidate: "
+              << connection->remote_candidate().ToString()
+              << "New remote candidate: " << remote_candidate.ToString();
+  }
+  // No new connection was created at this point.
+  return false;
+}
+
+bool P2PTransportChannel::CreateConnection(PortInterface* port,
+                                           const Candidate& remote_candidate,
+                                           PortInterface* origin_port) {

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

I don't quite understand why you split these methods into two, since both of
them don't create a connection under certain circumstances.  So how do you
decide to put the logic in one vs. the other?

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

I thought it may improve readability.
Since it caused confusion, I just put them together in one method. 

+  PortInterface::CandidateOrigin origin = GetOrigin(port, origin_port);
 
-    AddConnection(connection);
+  // Don't create connection if this is a candidate we received in a

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

create connection => create a caonnection

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

Done.

+  // message and we are not allowed to make outgoing connections.
+  if (origin == PortInterface::ORIGIN_MESSAGE && incoming_only_)

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

{}s please

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

Done.

+    return false;
 
-    LOG_J(LS_INFO, this) << "Created connection with origin=" << origin << ", ("
-                         << connections_.size() << " total)";
+  Connection* connection = port->CreateConnection(remote_candidate, origin);
+  if (!connection) {
+    return false;
   }
 
+  AddConnection(connection);
+  LOG_J(LS_INFO, this) << "Created connection with origin=" << origin << ", ("
+                       << connections_.size() << " total)";
   return true;
 }