Create a new connection if a candidate reuses an address

webrtc/p2p/base/p2ptransportchannel.cc

 /*
  *  Copyright 2004 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 514 matching lines @@
   port->SignalRoleConflict.connect(
       this, &P2PTransportChannel::OnRoleConflict);
   port->SignalSentPacket.connect(this, &P2PTransportChannel::OnSentPacket);
 
   // Attempt to create a connection from this new port to all of the remote
   // candidates that we were given so far.
 
   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();
 }
 
 // A new candidate is available, let listeners know
 void P2PTransportChannel::OnCandidatesReady(
     PortAllocatorSession* session,
     const std::vector<Candidate>& candidates) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
@@ skipping 275 matching lines @@
   }
 
   // Add a new connection for this candidate to every port that allows such a
   // connection (i.e., if they have compatible protocols) and that does not
   // already have a connection to an equivalent candidate.  We must be careful
   // to make sure that the origin port is included, even if it was pruned,
   // since that may be the only port that can create this connection.
   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;
     }
   }
 
   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;
   }
 
   // Remember this remote candidate so that we can add it to future ports.
   RememberRemoteCandidate(remote_candidate, origin_port);
 
   return created;
 }
 
 // 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);
-
-    // 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;
-
-    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)";
+  if (connection == nullptr ||
+      connection->remote_candidate().generation() <
+          remote_candidate.generation()) {
+    return CreateConnection(port, remote_candidate, origin_port);
   }
 
+  // Check if this is a peer reflexive candidate.
+  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();
+  }
+  // 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);
+
+  // 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;
+
+  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;
 }
 
 bool P2PTransportChannel::FindConnection(Connection* connection) const {
   std::vector<Connection*>::const_iterator citer =
       std::find(connections_.begin(), connections_.end(), connection);
   return citer != connections_.end();
 }
 
 uint32_t P2PTransportChannel::GetRemoteCandidateGeneration(
@@ skipping 794 matching lines @@
 
   // During the initial state when nothing has been pinged yet, return the first
   // one in the ordered |connections_|.
   return *(std::find_if(connections_.begin(), connections_.end(),
                         [conn1, conn2](Connection* conn) {
                           return conn == conn1 || conn == conn2;
                         }));
 }
 
 }  // namespace cricket