When a remote candidate is added, update all prflx candidates.
Previously they were only being updated for connections using the
most current "generation" of ports. This results in the older-
generation prflx candidate pair being prioritized above newer-
generation candidate pairs.
Review-Url: https://codereview.webrtc.org/2087713002
Cr-Commit-Position: refs/heads/master@{#13245}
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index c55b310..1e565ec 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -783,6 +783,12 @@
}
}
+ // If this candidate matches what was thought to be a peer reflexive
+ // candidate, we need to update the candidate priority/etc.
+ for (Connection* conn : connections_) {
+ conn->MaybeUpdatePeerReflexiveCandidate(new_remote_candidate);
+ }
+
// Create connections to this remote candidate.
CreateConnections(new_remote_candidate, NULL);
@@ -881,9 +887,6 @@
}
// No new connection was created.
- // 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())) {
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
index 227e913..b13fac2 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -150,7 +150,6 @@
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;
@@ -327,9 +326,7 @@
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());
@@ -706,7 +703,7 @@
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("");
}
@@ -787,8 +784,13 @@
return GetChannelData(ch)->ch_packets_;
}
- void set_clear_remote_candidates_ufrag_pwd(bool clear) {
- 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) {
@@ -809,7 +811,7 @@
rtc::SocksProxyServer socks_server2_;
Endpoint ep1_;
Endpoint ep2_;
- bool clear_remote_candidates_ufrag_pwd_;
+ RemoteIceCredentialSource remote_ice_credential_source_ = FROM_CANDIDATE;
bool force_relay_;
};
@@ -888,7 +890,7 @@
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) {
@@ -1106,7 +1108,7 @@
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]);
@@ -1158,7 +1160,7 @@
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]);
@@ -1201,9 +1203,67 @@
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);
