When a remote candidate is added, update all prflx candidates.

webrtc/p2p/base/p2ptransportchannel_unittest.cc

 /*
  *  Copyright 2009 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 131 matching lines @@
         ss_scope_(ss_.get()),
         stun_server_(cricket::TestStunServer::Create(main_, kStunAddr)),
         turn_server_(main_, kTurnUdpIntAddr, kTurnUdpExtAddr),
         relay_server_(main_, kRelayUdpIntAddr, kRelayUdpExtAddr,
                       kRelayTcpIntAddr, kRelayTcpExtAddr,
                       kRelaySslTcpIntAddr, kRelaySslTcpExtAddr),
         socks_server1_(ss_.get(), kSocksProxyAddrs[0],
                        ss_.get(), kSocksProxyAddrs[0]),
         socks_server2_(ss_.get(), kSocksProxyAddrs[1],
                        ss_.get(), kSocksProxyAddrs[1]),
-        clear_remote_candidates_ufrag_pwd_(false),
         force_relay_(false) {
     ep1_.role_ = cricket::ICEROLE_CONTROLLING;
     ep2_.role_ = cricket::ICEROLE_CONTROLLED;
 
     ServerAddresses stun_servers;
     stun_servers.insert(kStunAddr);
     ep1_.allocator_.reset(new cricket::BasicPortAllocator(
         &ep1_.network_manager_,
         stun_servers, kRelayUdpIntAddr, kRelayTcpIntAddr, kRelaySslTcpIntAddr));
     ep2_.allocator_.reset(new cricket::BasicPortAllocator(
@@ skipping 156 matching lines @@
         "test content name", component, GetAllocator(endpoint));
     channel->SignalCandidateGathered.connect(
         this, &P2PTransportChannelTestBase::OnCandidateGathered);
     channel->SignalCandidatesRemoved.connect(
         this, &P2PTransportChannelTestBase::OnCandidatesRemoved);
     channel->SignalReadPacket.connect(
         this, &P2PTransportChannelTestBase::OnReadPacket);
     channel->SignalRoleConflict.connect(
         this, &P2PTransportChannelTestBase::OnRoleConflict);
     channel->SetIceCredentials(local_ice_ufrag, local_ice_pwd);
-    if (clear_remote_candidates_ufrag_pwd_) {
-      // This only needs to be set if we're clearing them from the
-      // candidates.  Some unit tests rely on this not being set.
+    if (remote_ice_credential_source_ == FROM_SETICECREDENTIALS) {
       channel->SetRemoteIceCredentials(remote_ice_ufrag, remote_ice_pwd);
     }
     channel->SetIceRole(GetEndpoint(endpoint)->ice_role());
     channel->SetIceTiebreaker(GetEndpoint(endpoint)->GetIceTiebreaker());
     channel->Connect();
     return channel;
   }
   void DestroyChannels() {
     ep1_.cd1_.ch_.reset();
     ep2_.cd1_.ch_.reset();
@@ skipping 356 matching lines @@
     ed->save_candidates_ = false;
   }
 
   void OnMessage(rtc::Message* msg) {
     switch (msg->message_id) {
       case MSG_ADD_CANDIDATES: {
         std::unique_ptr<CandidatesData> data(
             static_cast<CandidatesData*>(msg->pdata));
         cricket::P2PTransportChannel* rch = GetRemoteChannel(data->channel);
         for (auto& c : data->candidates) {
-          if (clear_remote_candidates_ufrag_pwd_) {
+          if (remote_ice_credential_source_ != FROM_CANDIDATE) {
             c.set_username("");
             c.set_password("");
           }
           LOG(LS_INFO) << "Candidate(" << data->channel->component() << "->"
                        << rch->component() << "): " << c.ToString();
           rch->AddRemoteCandidate(c);
         }
         break;
       }
       case MSG_REMOVE_CANDIDATES: {
@@ skipping 60 matching lines @@
       return ep1_ch1();
     else if (ch == ep2_ch2())
       return ep1_ch2();
     else
       return NULL;
   }
   std::list<std::string>& GetPacketList(cricket::TransportChannel* ch) {
     return GetChannelData(ch)->ch_packets_;
   }
 
-  void set_clear_remote_candidates_ufrag_pwd(bool clear) {

[Taylor Brandstetter, 2016/06/21 00:33:05]

I changed this method for readability. It wasn't clear that
"set_clear_remote_candidates_ufrag_pwd(false)" means SetRemoteIceCredentials is
never used, but several tests depend on this behavior.

-    clear_remote_candidates_ufrag_pwd_ = clear;
+  enum RemoteIceCredentialSource { FROM_CANDIDATE, FROM_SETICECREDENTIALS };
+
+  // How does the test pass ICE credentials to the P2PTransportChannel?
+  // On the candidate itself, or through SetIceCredentials?
+  // Goes through the candidate itself by default.
+  void set_remote_ice_credential_source(RemoteIceCredentialSource source) {
+    remote_ice_credential_source_ = source;
   }
 
   void set_force_relay(bool relay) {
     force_relay_ = relay;
   }
 
  private:
   rtc::Thread* main_;
   std::unique_ptr<rtc::PhysicalSocketServer> pss_;
   std::unique_ptr<rtc::VirtualSocketServer> vss_;
   std::unique_ptr<rtc::NATSocketServer> nss_;
   std::unique_ptr<rtc::FirewallSocketServer> ss_;
   rtc::SocketServerScope ss_scope_;
   std::unique_ptr<cricket::TestStunServer> stun_server_;
   cricket::TestTurnServer turn_server_;
   cricket::TestRelayServer relay_server_;
   rtc::SocksProxyServer socks_server1_;
   rtc::SocksProxyServer socks_server2_;
   Endpoint ep1_;
   Endpoint ep2_;
-  bool clear_remote_candidates_ufrag_pwd_;
+  RemoteIceCredentialSource remote_ice_credential_source_ = FROM_CANDIDATE;
   bool force_relay_;
 };
 
 // The tests have only a few outcomes, which we predefine.
 const P2PTransportChannelTestBase::Result P2PTransportChannelTestBase::
     kLocalUdpToLocalUdp("local", "udp", "local", "udp",
                         "local", "udp", "local", "udp", 1000);
 const P2PTransportChannelTestBase::Result P2PTransportChannelTestBase::
     kLocalUdpToStunUdp("local", "udp", "stun", "udp",
                        "local", "udp", "stun", "udp", 1000);
@@ skipping 58 matching lines @@
     GetEndpoint(1)->allocator_->AddTurnServer(turn_server);
 
     int delay = kMinimumStepDelay;
     ConfigureEndpoint(0, config1);
     SetAllocatorFlags(0, allocator_flags1);
     SetAllocationStepDelay(0, delay);
     ConfigureEndpoint(1, config2);
     SetAllocatorFlags(1, allocator_flags2);
     SetAllocationStepDelay(1, delay);
 
-    set_clear_remote_candidates_ufrag_pwd(true);
+    set_remote_ice_credential_source(FROM_SETICECREDENTIALS);
   }
   void ConfigureEndpoint(int endpoint, Config config) {
     switch (config) {
       case OPEN:
         AddAddress(endpoint, kPublicAddrs[endpoint]);
         break;
       case NAT_FULL_CONE:
       case NAT_ADDR_RESTRICTED:
       case NAT_PORT_RESTRICTED:
       case NAT_SYMMETRIC:
@@ skipping 197 matching lines @@
   EXPECT_GT(best_conn_info->rtt, 0U);
   DestroyChannels();
 }
 
 // Test that we properly create a connection on a STUN ping from unknown address
 // when the signaling is slow.
 TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignaling) {
   ConfigureEndpoints(OPEN, OPEN, kDefaultPortAllocatorFlags,
                      kDefaultPortAllocatorFlags);
   // Emulate no remote credentials coming in.
-  set_clear_remote_candidates_ufrag_pwd(false);
+  set_remote_ice_credential_source(FROM_CANDIDATE);
   CreateChannels(1);
   // Only have remote credentials come in for ep2, not ep1.
   ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[0], kIcePwd[0]);
 
   // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive
   // candidate.
   PauseCandidates(1);
 
   // The caller should have the best connection connected to the peer reflexive
   // candidate.
@@ skipping 31 matching lines @@
   EXPECT_EQ(best_connection, ep1_ch1()->best_connection());
   DestroyChannels();
 }
 
 // Test that we properly create a connection on a STUN ping from unknown address
 // when the signaling is slow and the end points are behind NAT.
 TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignalingWithNAT) {
   ConfigureEndpoints(OPEN, NAT_SYMMETRIC, kDefaultPortAllocatorFlags,
                      kDefaultPortAllocatorFlags);
   // Emulate no remote credentials coming in.
-  set_clear_remote_candidates_ufrag_pwd(false);
+  set_remote_ice_credential_source(FROM_CANDIDATE);
   CreateChannels(1);
   // Only have remote credentials come in for ep2, not ep1.
   ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[0], kIcePwd[0]);
   // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive
   // candidate.
   PauseCandidates(1);
 
   // The caller should have the best connection connected to the peer reflexive
   // candidate.
   WAIT(ep1_ch1()->best_connection() != NULL, 2000);
@@ skipping 22 matching lines @@
   const cricket::Connection* best_connection = NULL;
   WAIT((best_connection = ep2_ch1()->best_connection()) != NULL, 2000);
 
   // Wait to verify the connection is not culled.
   WAIT(ep1_ch1()->writable(), 2000);
   EXPECT_EQ(ep2_ch1()->best_connection(), best_connection);
   EXPECT_EQ("prflx", ep1_ch1()->best_connection()->remote_candidate().type());
   DestroyChannels();
 }
 
+// Test that we properly create a connection on a STUN ping from unknown address
+// when the signaling is slow, even if the new candidate is created due to the
+// remote peer doing an ICE restart, pairing this candidate across generations.
+//
+// Previously this wasn't working due to a bug where the peer reflexive
+// candidate was only updated for the newest generation candidate pairs, and
+// not older-generation candidate pairs created by pairing candidates across
+// generations. This resulted in the old-generation prflx candidate being
+// prioritized above new-generation candidate pairs.
+TEST_F(P2PTransportChannelTest,
+       PeerReflexiveCandidateBeforeSignalingWithIceRestart) {
+  ConfigureEndpoints(OPEN, OPEN, kDefaultPortAllocatorFlags,
+                     kDefaultPortAllocatorFlags);
+  // Only gather relay candidates, so that when the prflx candidate arrives
+  // it's prioritized above the current candidate pair.
+  GetEndpoint(0)->allocator_->set_candidate_filter(cricket::CF_RELAY);
+  GetEndpoint(1)->allocator_->set_candidate_filter(cricket::CF_RELAY);
+  // Setting this allows us to control when SetRemoteIceCredentials is called.
+  set_remote_ice_credential_source(FROM_CANDIDATE);
+  CreateChannels(1);
+  // Wait for the initial connection to be made.
+  ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]);
+  ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[0], kIcePwd[0]);
+  EXPECT_TRUE_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
+                       ep2_ch1()->receiving() && ep2_ch1()->writable(),
+                   kDefaultTimeout);
+
+  // Simulate an ICE restart on ep2, but don't signal the candidate or new
+  // ICE credentials until after a prflx connection has been made.
+  PauseCandidates(1);
+  ep2_ch1()->SetIceCredentials(kIceUfrag[3], kIcePwd[3]);
+  ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[3], kIcePwd[3]);
+  ep2_ch1()->MaybeStartGathering();
+
+  // The caller should have the best connection connected to the peer reflexive
+  // candidate.
+  EXPECT_EQ_WAIT("prflx",
+                 ep1_ch1()->best_connection()->remote_candidate().type(),
+                 kDefaultTimeout);
+  const cricket::Connection* prflx_best_connection =
+      ep1_ch1()->best_connection();
+
+  // Now simulate the ICE restart on ep1.
+  ep1_ch1()->SetIceCredentials(kIceUfrag[2], kIcePwd[2]);
+  ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[2], kIcePwd[2]);
+  ep1_ch1()->MaybeStartGathering();
+
+  // Finally send the candidates from ep2's ICE restart and verify that ep1 uses
+  // their information to update the peer reflexive candidate.
+  ResumeCandidates(1);
+
+  EXPECT_EQ_WAIT("relay",
+                 ep1_ch1()->best_connection()->remote_candidate().type(),
+                 kDefaultTimeout);
+  EXPECT_EQ(prflx_best_connection, ep1_ch1()->best_connection());
+  DestroyChannels();
+}
+
 // Test that if remote candidates don't have ufrag and pwd, we still work.
 TEST_F(P2PTransportChannelTest, RemoteCandidatesWithoutUfragPwd) {
-  set_clear_remote_candidates_ufrag_pwd(true);
+  set_remote_ice_credential_source(FROM_SETICECREDENTIALS);
   ConfigureEndpoints(OPEN, OPEN, kDefaultPortAllocatorFlags,
                      kDefaultPortAllocatorFlags);
   CreateChannels(1);
   const cricket::Connection* best_connection = NULL;
   // Wait until the callee's connections are created.
   WAIT((best_connection = ep2_ch1()->best_connection()) != NULL, 1000);
   // Wait to see if they get culled; they shouldn't.
   WAIT(ep2_ch1()->best_connection() != best_connection, 1000);
   EXPECT_TRUE(ep2_ch1()->best_connection() == best_connection);
   DestroyChannels();
@@ skipping 1752 matching lines @@
 
   // TCP Relay/Relay is the next.
   VerifyNextPingableConnection(cricket::RELAY_PORT_TYPE,
                                cricket::RELAY_PORT_TYPE,
                                cricket::TCP_PROTOCOL_NAME);
 
   // Finally, Local/Relay will be pinged.
   VerifyNextPingableConnection(cricket::LOCAL_PORT_TYPE,
                                cricket::RELAY_PORT_TYPE);
 }