Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(774)

Unified Diff: webrtc/p2p/base/p2ptransportchannel_unittest.cc

Issue 2087713002: When a remote candidate is added, update all prflx candidates. (Closed) Base URL: https://chromium.googlesource.com/external/webrtc.git@master
Patch Set: Created 4 years, 6 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « webrtc/p2p/base/p2ptransportchannel.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: webrtc/p2p/base/p2ptransportchannel_unittest.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
index 45add0c733f03e100df0754478a406b6dc7133cd..38242a39c08e929f601cf8e5a2d1a8b59444dec3 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -149,7 +149,6 @@ class P2PTransportChannelTestBase : public testing::Test,
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;
@@ -326,9 +325,7 @@ class P2PTransportChannelTestBase : public testing::Test,
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());
@@ -705,7 +702,7 @@ class P2PTransportChannelTestBase : public testing::Test,
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("");
}
@@ -786,8 +783,13 @@ class P2PTransportChannelTestBase : public testing::Test,
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 c
- 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) {
@@ -808,7 +810,7 @@ class P2PTransportChannelTestBase : public testing::Test,
rtc::SocksProxyServer socks_server2_;
Endpoint ep1_;
Endpoint ep2_;
- bool clear_remote_candidates_ufrag_pwd_;
+ RemoteIceCredentialSource remote_ice_credential_source_ = FROM_CANDIDATE;
bool force_relay_;
};
@@ -887,7 +889,7 @@ class P2PTransportChannelTest : public P2PTransportChannelTestBase {
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) {
@@ -1105,7 +1107,7 @@ 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]);
@@ -1157,7 +1159,7 @@ 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]);
@@ -1200,9 +1202,67 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignalingWithNAT) {
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);
« no previous file with comments | « webrtc/p2p/base/p2ptransportchannel.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698