Fill the remote pwd in ice candidates when an ICE credential is received.

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 336 matching lines @@
                                                   const std::string& ice_pwd) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   IceParameters* current_ice = remote_ice();
   IceParameters new_ice(ice_ufrag, ice_pwd);
   if (!current_ice || *current_ice != new_ice) {
     // Keep the ICE credentials so that newer connections
     // are prioritized over the older ones.
     remote_ice_parameters_.push_back(new_ice);
   }
 
+  // Update the pwd of remote candidate if needed.
+  for (RemoteCandidate& candidate : remote_candidates_) {
+    if (candidate.username() == ice_ufrag && candidate.password().empty()) {

[pthatcher1, 2015/12/28 21:33:59]

Can you put a TODO in candidate.h to change username() to either
usernameFragment() or ufrag()?  And, better yet, make a CL to do so?

[honghaiz3, 2015/12/30 18:59:33]

Added TODO... although I think there is no confusion there. A username/password
is perhaps more readable than ufrag/pwd for general people? 

[pthatcher1, 2015/12/30 19:15:44]

username() is more readable, but it's also wrong.  usernameFragment() is
correct.  I'd be happy with usernameFragment/password.  But on the other hand,
we use "ufrag" and "pwd" in all of the other code, and we should probably be
consistent.

Your TODO is sufficient for this CL.

+      candidate.set_password(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);
+  for (Connection* conn : connections_) {
+    conn->MaybeSetRemoteIceCredentials(ice_ufrag, ice_pwd);
   }
 }
 
 void P2PTransportChannel::SetRemoteIceMode(IceMode mode) {
   remote_ice_mode_ = mode;
 }
 
 void P2PTransportChannel::SetIceConfig(const IceConfig& config) {
   gather_continually_ = config.gather_continually;
   LOG(LS_INFO) << "Set gather_continually to " << gather_continually_;
@@ skipping 133 matching lines @@
     for (; it != remote_candidates_.end(); ++it) {
       if (it->username() != username) {
         ufrag_per_port = true;
         break;
       }
     }
   }
 
   const Candidate* candidate = NULL;
   std::string remote_password;
+  uint32_t remote_generation = 0;
   for (it = remote_candidates_.begin(); it != remote_candidates_.end(); ++it) {
     if (it->username() == remote_username) {
       remote_password = it->password();
       if (ufrag_per_port ||
           (it->address() == address &&
            it->protocol() == ProtoToString(proto))) {
         candidate = &(*it);
         break;
       }
       // We don't want to break here because we may find a match of the address
       // later.
     }
   }
 
   // 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()) {
-    IceParameters* current_ice = remote_ice();
-    if (current_ice && remote_username == current_ice->ufrag) {
-      remote_password = current_ice->pwd;
+    const IceParameters* ice_param =
+        FindRemoteIce(remote_username, &remote_generation);
+    if (ice_param != nullptr) {
+      remote_password = ice_param->pwd;

[pthatcher1, 2015/12/28 21:33:59]

If the remote_username is empty (not signalled), do we want to continue to
assume it means the latest ufrag/pwd?  If we don't, this is a behavior changes
which might break things.

[honghaiz3, 2015/12/30 18:59:33]

If the remote_username is not signalled, remote ufrag and pwd will be empty in
both cases, right. The behavior should not matter. 
Plus, in a stun request, the ufrag should never be empty, right?

[pthatcher1, 2015/12/30 19:15:44]

Sorry, when I said "not signalled", I meant "not signalled on the candidate". 
But looking at this more closely, I see that my comment was wrong.  This is
covering the case where we know the remote ufrag from a STUN ping and are
filling in the password.  So, your code looks good here.  Forget my previous
comment.

     }
   }
 
   Candidate remote_candidate;
   bool remote_candidate_is_new = (candidate == nullptr);
   if (!remote_candidate_is_new) {
     remote_candidate = *candidate;
     if (ufrag_per_port) {
       remote_candidate.set_address(address);
     }
@@ skipping 12 matching lines @@
       port->SendBindingErrorResponse(stun_msg, address, STUN_ERROR_BAD_REQUEST,
                                      STUN_ERROR_REASON_BAD_REQUEST);
       return;
     }
     remote_candidate_priority = priority_attr->value();
 
     // RFC 5245
     // If the source transport address of the request does not match any
     // existing remote candidates, it represents a new peer reflexive remote
     // candidate.
-    remote_candidate =
-        Candidate(component(), ProtoToString(proto), address, 0,
-                  remote_username, remote_password, PRFLX_PORT_TYPE, 0U, "");
+    remote_candidate = Candidate(component(), ProtoToString(proto), address, 0,
+                                 remote_username, remote_password,
+                                 PRFLX_PORT_TYPE, remote_generation, "");
 
     // From RFC 5245, section-7.2.1.3:
     // The foundation of the candidate is set to an arbitrary value, different
     // from the foundation for all other remote candidates.
     remote_candidate.set_foundation(
         rtc::ToString<uint32_t>(rtc::ComputeCrc32(remote_candidate.id())));
 
     remote_candidate.set_priority(remote_candidate_priority);
   }
 
@@ skipping 39 matching lines @@
   // after sending the response since it could (in principle) delete the
   // connection in question.
   SortConnections();
 }
 
 void P2PTransportChannel::OnRoleConflict(PortInterface* port) {
   SignalRoleConflict(this);  // STUN ping will be sent when SetRole is called
                              // from Transport.
 }
 
+const IceParameters* P2PTransportChannel::FindRemoteIce(
+    const std::string& ufrag,
+    uint32_t* generation) {
+  const auto& params = remote_ice_parameters_;
+  auto it = std::find_if(
+      params.rbegin(), params.rend(),
+      [ufrag](const IceParameters& param) { return param.ufrag == ufrag; });
+  if (it == params.rend()) {
+    // If not found, assume the next (future) generation.
+    *generation = static_cast<uint32_t>(params.size());

[pthatcher1, 2015/12/28 21:33:59]

I find it a bit confusing that we fill in the generation even when the
generation isn't found.  That's a little too magic/implicit.  Can we instead of
the caller explicitly use remote_ice_parameters_.size() when a generation isn't
found?

[honghaiz3, 2015/12/30 18:59:33]

Done.

+    return nullptr;
+  }
+  *generation = params.rend() - it - 1;
+  return &(*it);
+}
+
 void P2PTransportChannel::OnNominated(Connection* conn) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   ASSERT(ice_role_ == ICEROLE_CONTROLLED);
 
   if (conn->write_state() == Connection::STATE_WRITABLE) {
     if (best_connection_ != conn) {
       pending_best_connection_ = NULL;
       LOG(LS_INFO) << "Switching best connection on controlled side: "
                    << conn->ToString();
       SwitchBestConnectionTo(conn);
@@ skipping 142 matching lines @@
 
 bool P2PTransportChannel::FindConnection(
     cricket::Connection* connection) const {
   std::vector<Connection*>::const_iterator citer =
       std::find(connections_.begin(), connections_.end(), connection);
   return citer != connections_.end();
 }
 
 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.
-  const auto& params = remote_ice_parameters_;
+  // If the candidate contains ufrag, use that to find the generation.

[pthatcher1, 2015/12/28 21:33:59]

contains ufrag => has a ufrag
use that => use it

[honghaiz3, 2015/12/30 18:59:33]

Done.

   if (!candidate.username().empty()) {
-    // If remote side sets the ufrag, we use that to determine the candidate
-    // generation.
-    // Search backward as it is more likely to find it near the end.
-    auto it = std::find_if(params.rbegin(), params.rend(),
-                           [candidate](const IceParameters& param) {
-                             return param.ufrag == candidate.username();
-                           });
-    if (it == params.rend()) {
-      // If not found, assume it is the next (future) generation.
-      return static_cast<uint32_t>(remote_ice_parameters_.size());
-    }
-    return params.rend() - it - 1;
+    uint32_t generation = 0;
+    FindRemoteIce(candidate.username(), &generation);
+    return generation;
   }
   // If candidate generation is set, use that.
   if (candidate.generation() > 0) {
     return candidate.generation();
   }
   // Otherwise, assume the generation from remote ice parameters.
   return remote_ice_generation();
 }
 
 // Check if remote candidate is already cached.
@@ skipping 599 matching lines @@
   SignalSentPacket(this, sent_packet);
 }
 
 void P2PTransportChannel::OnReadyToSend(Connection* connection) {
   if (connection == best_connection_ && writable()) {
     SignalReadyToSend(this);
   }
 }
 
 }  // namespace cricket