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 ab7c32277f9f920c667b25c8f977998b5665b83d..6a6d4017c0f495f45468b398cb1b56f47f47ac75 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -348,6 +348,9 @@ void P2PTransportChannel::SetRemoteIceCredentials(const std::string& ice_ufrag,
                   (remote_ice_pwd_!= ice_pwd);
   }
 
+  if (ice_restart) {
+    old_remote_ufrags_.insert(remote_ice_ufrag_);
+  }
   remote_ice_ufrag_ = ice_ufrag;
   remote_ice_pwd_ = ice_pwd;
 
@@ -642,10 +645,19 @@ 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_) {
+  const std::string& ufrag = candidate.username();
+  uint32_t generation = candidate.generation();
+  if (!ufrag.empty()) {
+    if (old_remote_ufrags_.find(ufrag) != old_remote_ufrags_.end()) {
+      LOG(LS_WARNING) << "Dropping a remote candidate because its ICE ufrag "
+                      << ufrag << " is old.";

[pthatcher1, 2015/12/04 20:43:50]

Perhaps say "because its ufrag indicates it was for a previous ICE generation"

[honghaiz3, 2015/12/09 23:57:54]

Done.

+      return;
+    }
+    // TODO(honghaiz): Get rid of the checking using generation when all clients
+    // include ufrag in the candidate signaling.
+  } else 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 "
@@ -668,6 +680,7 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   Candidate new_remote_candidate(remote_candidate);
+  // Candidate generation is still used for determining the priority.

[pthatcher1, 2015/12/04 20:43:50]

You can remove the "is still"

[honghaiz3, 2015/12/09 23:57:54]

Moved set_generation to AddRemoteIceCandidate()

   new_remote_candidate.set_generation(
       GetRemoteCandidateGeneration(remote_candidate));
   // ICE candidates don't need to have username and password set, but
@@ -677,8 +690,14 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
   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 (new_remote_candidate.username() == remote_ice_ufrag_) {
+    if (remote_candidate.password().empty()) {
+      new_remote_candidate.set_password(remote_ice_pwd_);
+    }
+  } else {
+    LOG(LS_WARNING)
+        << "A remote candidate arrives with a ufrag that was not seen before: "
+        << remote_candidate.username();
   }

[pthatcher1, 2015/12/04 20:43:50]

When is the pwd on the candidate going to be filled in?  I see that it is in the
unit test, but it's not clear when that happens.  A comment explaining when that
happens would be useful.

[honghaiz3, 2015/12/09 23:57:54]

Done.

 
   // If we've already seen the new remote candidate (in the current candidate
@@ -773,9 +792,9 @@ 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_;
+  // The candidate generation should be one of 0, remote_candidate_generation_,
+  // and remote_candidate_generation_ + 1.
+  return std::max(remote_candidate_generation_, candidate.generation());

[pthatcher1, 2015/12/04 20:43:50]

I think this should go lookup the generation by looking in remote_generations_
for which ufrag matches.  If none do, then use candidate.generation().  If
that's not set, then assume it's the latest.

[honghaiz3, 2015/12/09 23:57:54]

Done.

 }
 
 // Check if remote candidate is already cached.
@@ -794,9 +813,10 @@ void P2PTransportChannel::RememberRemoteCandidate(
     const Candidate& remote_candidate, PortInterface* origin_port) {
   // Remove any candidates whose generation is older than this one.  The
   // presence of a new generation indicates that the old ones are not useful.
+  ASSERT(!remote_candidate.username().empty());
   size_t i = 0;
   while (i < remote_candidates_.size()) {
-    if (remote_candidates_[i].generation() < remote_candidate.generation()) {
+    if (remote_candidates_[i].username() != remote_candidate.username()) {
       LOG(INFO) << "Pruning candidate from old generation: "
                 << remote_candidates_[i].address().ToSensitiveString();
       remote_candidates_.erase(remote_candidates_.begin() + i);