From 0a1bc53758862014343c8186ea7c270f0cdd57c6 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Tue, 19 Apr 2016 18:03:26 -0700 Subject: [PATCH] Update prflx candidates' generation when setting ICE credentials. If a STUN ping arrives before the remote description does, a prflx candidate will be created with an unknown generation. Once the remote description does arrive, the candidate's generation should be set so it can be sorted properly, and replaced by a non-prflx candidate once the candidate is signaled. BUG=webrtc:5752 R=honghaiz@webrtc.org, pthatcher@webrtc.org Review URL: https://codereview.webrtc.org/1868353004 . Cr-Commit-Position: refs/heads/master@{#12433} --- webrtc/api/peerconnection_unittest.cc | 24 ++++----------- webrtc/p2p/base/candidate.h | 3 +- webrtc/p2p/base/p2ptransportchannel.cc | 9 ++++-- .../p2p/base/p2ptransportchannel_unittest.cc | 29 ++++++++++++++----- webrtc/p2p/base/port.cc | 14 +++++++-- webrtc/p2p/base/port.h | 12 +++++--- 6 files changed, 56 insertions(+), 35 deletions(-) diff --git a/webrtc/api/peerconnection_unittest.cc b/webrtc/api/peerconnection_unittest.cc index 4817477035..99be22f459 100644 --- a/webrtc/api/peerconnection_unittest.cc +++ b/webrtc/api/peerconnection_unittest.cc @@ -1187,10 +1187,7 @@ class P2PTestConductor : public testing::Test { // This test sets up a call between two parties. Both parties send static // frames to each other. Once the test is finished the number of sent frames // is compared to the number of received frames. - void LocalP2PTest() { LocalP2PTest(false); } - // TODO(perkj); Remove the flag bug5752 when - // https://bugs.chromium.org/p/webrtc/issues/detail?id=5752 is fixed. - void LocalP2PTest(bool bug5752) { + void LocalP2PTest() { if (initiating_client_->NumberOfLocalMediaStreams() == 0) { initiating_client_->AddMediaStream(true, true); } @@ -1219,18 +1216,9 @@ class P2PTestConductor : public testing::Test { // Completed. // Note: These tests have been observed to fail under heavy load at // shorter timeouts, so they may be flaky. - if (bug5752) { - EXPECT_TRUE_WAIT( - initiating_client_->ice_connection_state() == - webrtc::PeerConnectionInterface::kIceConnectionCompleted || - initiating_client_->ice_connection_state() == - webrtc::PeerConnectionInterface::kIceConnectionConnected, - kMaxWaitForFramesMs); - } else { - EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionCompleted, - initiating_client_->ice_connection_state(), - kMaxWaitForFramesMs); - } + EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionCompleted, + initiating_client_->ice_connection_state(), + kMaxWaitForFramesMs); EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionConnected, receiving_client_->ice_connection_state(), kMaxWaitForFramesMs); @@ -1461,9 +1449,7 @@ TEST_F(P2PTestConductor, LocalP2PTestDtlsTransferCaller) { SetSignalingReceivers(); initializing_client()->IceRestart(); - // TODO(perkj): Remove the flag bug5752 when - // https://bugs.chromium.org/p/webrtc/issues/detail?id=5752 is fixed. - LocalP2PTest(true /* bug5752 */); + LocalP2PTest(); VerifyRenderedSize(640, 480); } diff --git a/webrtc/p2p/base/candidate.h b/webrtc/p2p/base/candidate.h index 4ab1b6f235..ec758d2aa5 100644 --- a/webrtc/p2p/base/candidate.h +++ b/webrtc/p2p/base/candidate.h @@ -249,7 +249,8 @@ class Candidate { ost << "Cand[" << transport_name_ << ":" << foundation_ << ":" << component_ << ":" << protocol_ << ":" << priority_ << ":" << address << ":" << type_ << ":" << related_address_ << ":" << username_ << ":" - << password_ << ":" << network_id_ << ":" << network_cost_ << "]"; + << password_ << ":" << network_id_ << ":" << network_cost_ << ":" + << generation_ << "]"; return ost.str(); } diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 66532411b2..acbcfd77e7 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -390,10 +390,15 @@ void P2PTransportChannel::SetRemoteIceCredentials(const std::string& ice_ufrag, candidate.set_password(ice_pwd); } } - // We need to update the credentials for any peer reflexive candidates. + // We need to update the credentials and generation for any peer reflexive + // candidates. for (Connection* conn : connections_) { - conn->MaybeSetRemoteIceCredentials(ice_ufrag, ice_pwd); + conn->MaybeSetRemoteIceCredentialsAndGeneration( + ice_ufrag, ice_pwd, + static_cast(remote_ice_parameters_.size() - 1)); } + // Updating the remote ICE candidate generation could change the sort order. + RequestSort(); } void P2PTransportChannel::SetRemoteIceMode(IceMode mode) { diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 87e92a5b30..d921a1e063 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1193,7 +1193,7 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignaling) { set_clear_remote_candidates_ufrag_pwd(false); CreateChannels(1); // Only have remote credentials come in for ep2, not ep1. - ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[3], kIcePwd[3]); + ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[0], kIcePwd[0]); // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive // candidate. @@ -1209,15 +1209,23 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignaling) { EXPECT_EQ(kIceUfrag[1], ep1_ch1()->best_connection()->remote_candidate().username()); EXPECT_EQ("", ep1_ch1()->best_connection()->remote_candidate().password()); + // Because we don't have ICE credentials yet, we don't know the generation. + EXPECT_EQ(0u, ep1_ch1()->best_connection()->remote_candidate().generation()); EXPECT_TRUE(nullptr == ep1_ch1()->FindNextPingableConnection()); + // Add two sets of remote ICE credentials, so that the ones used by the + // candidate will be generation 1 instead of 0. + ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[3], kIcePwd[3]); ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]); - ResumeCandidates(1); - + // After setting the remote ICE credentials, the password and generation + // of the peer reflexive candidate should be updated. EXPECT_EQ(kIcePwd[1], ep1_ch1()->best_connection()->remote_candidate().password()); + EXPECT_EQ(1u, ep1_ch1()->best_connection()->remote_candidate().generation()); EXPECT_TRUE(nullptr != ep1_ch1()->FindNextPingableConnection()); + ResumeCandidates(1); + WAIT(ep2_ch1()->best_connection() != NULL, 2000); // Verify ep1's best connection is updated to use the 'local' candidate. EXPECT_EQ_WAIT( @@ -1237,7 +1245,7 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignalingWithNAT) { set_clear_remote_candidates_ufrag_pwd(false); CreateChannels(1); // Only have remote credentials come in for ep2, not ep1. - ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[3], kIcePwd[3]); + ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[0], kIcePwd[0]); // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive // candidate. PauseCandidates(1); @@ -1251,14 +1259,21 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignalingWithNAT) { EXPECT_EQ(kIceUfrag[1], ep1_ch1()->best_connection()->remote_candidate().username()); EXPECT_EQ("", ep1_ch1()->best_connection()->remote_candidate().password()); + // Because we don't have ICE credentials yet, we don't know the generation. + EXPECT_EQ(0u, ep1_ch1()->best_connection()->remote_candidate().generation()); EXPECT_TRUE(nullptr == ep1_ch1()->FindNextPingableConnection()); + // Add two sets of remote ICE credentials, so that the ones used by the + // candidate will be generation 1 instead of 0. + ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[3], kIcePwd[3]); ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]); - ResumeCandidates(1); - + // After setting the remote ICE credentials, the password and generation + // of the peer reflexive candidate should be updated. EXPECT_EQ(kIcePwd[1], ep1_ch1()->best_connection()->remote_candidate().password()); - EXPECT_TRUE(nullptr != ep1_ch1()->FindNextPingableConnection()); + EXPECT_EQ(1u, ep1_ch1()->best_connection()->remote_candidate().generation()); + + ResumeCandidates(1); const cricket::Connection* best_connection = NULL; WAIT((best_connection = ep2_ch1()->best_connection()) != NULL, 2000); diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index d26a9302a8..0238aa2ba2 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -1298,12 +1298,22 @@ void Connection::HandleRoleConflictFromPeer() { port_->SignalRoleConflict(port_); } -void Connection::MaybeSetRemoteIceCredentials(const std::string& ice_ufrag, - const std::string& ice_pwd) { +void Connection::MaybeSetRemoteIceCredentialsAndGeneration( + const std::string& ice_ufrag, + const std::string& ice_pwd, + int generation) { if (remote_candidate_.username() == ice_ufrag && remote_candidate_.password().empty()) { remote_candidate_.set_password(ice_pwd); } + // TODO(deadbeef): A value of '0' for the generation is used for both + // generation 0 and "generation unknown". It should be changed to an + // rtc::Optional to fix this. + if (remote_candidate_.username() == ice_ufrag && + remote_candidate_.password() == ice_pwd && + remote_candidate_.generation() == 0) { + remote_candidate_.set_generation(generation); + } } void Connection::MaybeUpdatePeerReflexiveCandidate( diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index 7c4468d2e6..2961355e3e 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -574,10 +574,14 @@ class Connection : public CandidatePairInterface, uint32_t ComputeNetworkCost() const; - // Update the ICE password of the remote candidate if |ice_ufrag| matches - // the candidate's ufrag, and the candidate's passwrod has not been set. - void MaybeSetRemoteIceCredentials(const std::string& ice_ufrag, - const std::string& ice_pwd); + // Update the ICE password and/or generation of the remote candidate if a + // ufrag in |remote_ice_parameters| matches the candidate's ufrag, and the + // candidate's password and/or ufrag has not been set. + // |remote_ice_parameters| should be a list of known ICE parameters ordered + // by generation. + void MaybeSetRemoteIceCredentialsAndGeneration(const std::string& ice_ufrag, + const std::string& ice_pwd, + int generation); // If |remote_candidate_| is peer reflexive and is equivalent to // |new_candidate| except the type, update |remote_candidate_| to