Add ufrag to the ICE candidate signaling.

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 da8b5f76c2f7b85d6684975f1c66881fec25e2f0..7efbb9a3232b89239345459623a1b7e2ecf3bce3 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -218,7 +218,6 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
       remote_ice_mode_(ICEMODE_FULL),
       ice_role_(ICEROLE_UNKNOWN),
       tiebreaker_(0),
-      remote_candidate_generation_(0),
       gathering_state_(kIceGatheringNew),
       check_receiving_delay_(MIN_CHECK_RECEIVING_DELAY * 5),
       receiving_timeout_(MIN_CHECK_RECEIVING_DELAY * 50),
@@ -347,26 +346,18 @@ void P2PTransportChannel::SetIceCredentials(const std::string& ice_ufrag,
 void P2PTransportChannel::SetRemoteIceCredentials(const std::string& ice_ufrag,
                                                   const std::string& ice_pwd) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
-  bool ice_restart = false;
-  if (!remote_ice_ufrag_.empty() && !remote_ice_pwd_.empty()) {
-    ice_restart = (remote_ice_ufrag_ != ice_ufrag) ||
-                  (remote_ice_pwd_!= ice_pwd);
+  if (remote_ice_generations_.empty() || (remote_ice_ufrag() != ice_ufrag) ||
+      (remote_ice_pwd() != ice_pwd)) {
+    // Keep the old-generation ICE credentials so that newer connections
+    // are prioritized over the older ones.
+    remote_ice_generations_.push_back(IceGeneration(ice_ufrag, ice_pwd));
   }

[pthatcher1, 2015/12/10 22:08:06]

I think this might be a little better using an optional and operator==:

Optional<IceParameters> gen1 = remote_ice();
IceParaemeters gen2 = IceParameters(ice_ufrag, ice_pwd);
if (!gen1 || *gen1 != gen2) {
  remote_ice_generations_.push_back(gen2);
}

[honghaiz3, 2015/12/11 04:47:29]

Done.

 
-  remote_ice_ufrag_ = ice_ufrag;
-  remote_ice_pwd_ = ice_pwd;
-
   // We need to update the credentials for any peer reflexive candidates.
   std::vector<Connection*>::iterator it = connections_.begin();
   for (; it != connections_.end(); ++it) {
     (*it)->MaybeSetRemoteIceCredentials(ice_ufrag, ice_pwd);
   }
-
-  if (ice_restart) {
-    // We need to keep track of the remote ice restart so newer
-    // connections are prioritized over the older.
-    ++remote_candidate_generation_;
-  }
 }
 
 void P2PTransportChannel::SetRemoteIceMode(IceMode mode) {
@@ -536,8 +527,9 @@ void P2PTransportChannel::OnUnknownAddress(
   // The STUN binding request may arrive after setRemoteDescription and before
   // adding remote candidate, so we need to set the password to the shared
   // password if the user name matches.
-  if (remote_password.empty() && remote_username == remote_ice_ufrag_) {
-    remote_password = remote_ice_pwd_;
+  if (remote_password.empty() && !remote_ice_generations_.empty() &&
+      remote_username == remote_ice_ufrag()) {
+    remote_password = remote_ice_pwd();
   }

[pthatcher1, 2015/12/10 22:08:06]

Same here, with optional:

if (remote_password.empty() && remote_ice()) {
  remote_password = remote_ice().pwd;
}

[honghaiz3, 2015/12/11 04:47:29]

Done.

 
   Candidate remote_candidate;
@@ -655,19 +647,39 @@ void P2PTransportChannel::OnNominated(Connection* conn) {
 void P2PTransportChannel::AddRemoteCandidate(const Candidate& candidate) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
-  uint32_t generation = candidate.generation();
-  // Network may not guarantee the order of the candidate delivery. If a
-  // remote candidate with an older generation arrives, drop it.
-  if (generation != 0 && generation < remote_candidate_generation_) {
-    LOG(LS_WARNING) << "Dropping a remote candidate because its generation "
-                    << generation
-                    << " is lower than the current remote generation "
-                    << remote_candidate_generation_;
+  int generation = GetRemoteCandidateGeneration(candidate);
+  // If a remote candidate with a previous generation arrives, drop it.
+  if (generation < remote_ice_generation()) {
+    LOG(LS_WARNING) << "Dropping a remote candidate because its ufrag "
+                    << candidate.username()
+                    << " indicates it was for an previous generation.";
     return;
   }
 
+  Candidate new_remote_candidate(candidate);
+  new_remote_candidate.set_generation(generation);
+  // ICE candidates don't need to have username and password set, but
+  // the code below this (specifically, ConnectionRequest::Prepare in
+  // port.cc) uses the remote candidates's username.  So, we set it
+  // here.
+  if (!remote_ice_generations_.empty()) {
+    if (candidate.username().empty()) {
+      new_remote_candidate.set_username(remote_ice_ufrag());
+    }
+    if (new_remote_candidate.username() == remote_ice_ufrag()) {
+      if (candidate.password().empty()) {
+        new_remote_candidate.set_password(remote_ice_pwd());
+      }
+    } else {
+      // The candidate belongs to the next generation. Its pwd will be set
+      // when the new remote ICE credentials arrive.
+      LOG(LS_WARNING) << "A remote candidate arrives with an unknown ufrag: "
+                      << candidate.username();
+    }
+  }
+
   // Create connections to this remote candidate.
-  CreateConnections(candidate, NULL);
+  CreateConnections(new_remote_candidate, NULL);
 
   // Resort the connections list, which may have new elements.
   SortConnections();
@@ -680,20 +692,6 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
                                             PortInterface* origin_port) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
-  Candidate new_remote_candidate(remote_candidate);
-  new_remote_candidate.set_generation(
-      GetRemoteCandidateGeneration(remote_candidate));
-  // ICE candidates don't need to have username and password set, but
-  // the code below this (specifically, ConnectionRequest::Prepare in
-  // port.cc) uses the remote candidates's username.  So, we set it
-  // here.
-  if (remote_candidate.username().empty()) {
-    new_remote_candidate.set_username(remote_ice_ufrag_);
-  }
-  if (remote_candidate.password().empty()) {
-    new_remote_candidate.set_password(remote_ice_pwd_);
-  }
-
   // If we've already seen the new remote candidate (in the current candidate
   // generation), then we shouldn't try creating connections for it.
   // We either already have a connection for it, or we previously created one
@@ -702,7 +700,7 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
   // immediately be re-pruned, churning the network for no purpose.
   // This only applies to candidates received over signaling (i.e. origin_port
   // is NULL).
-  if (!origin_port && IsDuplicateRemoteCandidate(new_remote_candidate)) {
+  if (!origin_port && IsDuplicateRemoteCandidate(remote_candidate)) {
     // return true to indicate success, without creating any new connections.
     return true;
   }
@@ -715,7 +713,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, new_remote_candidate, origin_port)) {
+    if (CreateConnection(*it, remote_candidate, origin_port)) {
       if (*it == origin_port)
         created = true;
     }
@@ -723,12 +721,12 @@ 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, new_remote_candidate, origin_port))
+    if (CreateConnection(origin_port, remote_candidate, origin_port))
       created = true;
   }
 
   // Remember this remote candidate so that we can add it to future ports.
-  RememberRemoteCandidate(new_remote_candidate, origin_port);
+  RememberRemoteCandidate(remote_candidate, origin_port);
 
   return created;
 }
@@ -786,9 +784,28 @@ uint32_t P2PTransportChannel::GetRemoteCandidateGeneration(
     const Candidate& candidate) {
   // We need to keep track of the remote ice restart so newer
   // connections are prioritized over the older.
-  ASSERT(candidate.generation() == 0 ||
-         candidate.generation() == remote_candidate_generation_);
-  return remote_candidate_generation_;
+  const std::string& ufrag = candidate.username();
+  int generation = 0;
+  if (!ufrag.empty()) {
+    // If remote side sets the ufrag, we use that to determine the candidate
+    // generation.
+    generation = remote_ice_generations_.size() - 1;
+    for (; generation >= 0; --generation) {
+      if (ufrag == remote_ice_generations_[generation].ufrag) {
+        return generation;
+      }
+    }
+    // If not found, this is the next generation candidate.
+    return remote_ice_generations_.size();
+  }

[pthatcher1, 2015/12/10 22:08:06]

I think this could be more readable by using std::find_if;

const auto& params = remote_ice_parameters_;
if (!candidate.username().empty()) {
  auto it = std::find_if(params.begin(), params.end(), [candidate](const
IceParameters& ice) {
    return ice.ufrag = candidate.username();
  });
  if (it == params.end()) {
    // If not found, assume it's the next (future) generation;
    return remote_ice_generation() + 1;
  }
  return it - params.begin();
}

[honghaiz3, 2015/12/11 04:47:29]

Done.

+  // Use the generation set in the candidate.
+  generation = candidate.generation();
+  int cur_remote_generation = remote_ice_generation();
+  if (generation == 0 && cur_remote_generation > 0) {
+    // 0 indicates it may not be set by the peer.
+    generation = cur_remote_generation;
+  }
+  return generation;

[pthatcher1, 2015/12/10 22:08:06]

I think this would be readable as:

// If candidate generation is set, use that.
if (candidate.generation() > 0) {
  return candidate.generation();
}

// If we have remote parameters, use that.  
if (!params.empty()) {
  return remote_ice_generation();
}

// Otherwise, assume it's generation 0.
return 0;

[honghaiz3, 2015/12/11 04:47:29]

Done.

 }
 
 // Check if remote candidate is already cached.