From 04e9146e58bd68339b15ad651c9ee593d781e040 Mon Sep 17 00:00:00 2001 From: Honghai Zhang Date: Fri, 11 Dec 2015 14:26:22 -0800 Subject: [PATCH] Discard old-generation candidates when ICE restarts The existing code only do so on the controlled side. BUG=webrtc:5291 R=pthatcher@webrtc.org Review URL: https://codereview.webrtc.org/1496693002 . Cr-Commit-Position: refs/heads/master@{#10993} --- talk/app/webrtc/webrtcsession.cc | 10 +++- talk/app/webrtc/webrtcsession_unittest.cc | 65 +++++++++++++++++++++-- 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index c14f720502..015531d33e 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -492,9 +492,13 @@ class IceRestartAnswerLatch { } } + // This method has two purposes: 1. Return whether |new_desc| requests + // an ICE restart (i.e., new ufrag/pwd). 2. If it requests an ICE restart + // and it is an OFFER, remember this in |ice_restart_| so that the next + // Local Answer will be created with new ufrag and pwd. bool CheckForRemoteIceRestart(const SessionDescriptionInterface* old_desc, const SessionDescriptionInterface* new_desc) { - if (!old_desc || new_desc->type() != SessionDescriptionInterface::kOffer) { + if (!old_desc) { return false; } const SessionDescription* new_sd = new_desc->description(); @@ -520,7 +524,9 @@ class IceRestartAnswerLatch { new_transport_desc->ice_ufrag, new_transport_desc->ice_pwd)) { LOG(LS_INFO) << "Remote peer request ice restart."; - ice_restart_ = true; + if (new_desc->type() == SessionDescriptionInterface::kOffer) { + ice_restart_ = true; + } return true; } } diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc index fa48fd3c16..6a38351cb5 100644 --- a/talk/app/webrtc/webrtcsession_unittest.cc +++ b/talk/app/webrtc/webrtcsession_unittest.cc @@ -2806,10 +2806,9 @@ TEST_F(WebRtcSessionTest, TestSetRemoteDescriptionInvalidIceCredentials) { EXPECT_FALSE(session_->SetRemoteDescription(modified_offer, &error)); } -// Test that if the remote description indicates the peer requested ICE restart -// (via a new ufrag or pwd), the old ICE candidates are not copied, -// and vice versa. -TEST_F(WebRtcSessionTest, TestSetRemoteDescriptionWithIceRestart) { +// Test that if the remote offer indicates the peer requested ICE restart (via +// a new ufrag or pwd), the old ICE candidates are not copied, and vice versa. +TEST_F(WebRtcSessionTest, TestSetRemoteOfferWithIceRestart) { Init(); scoped_ptr offer(CreateRemoteOffer()); @@ -2863,6 +2862,64 @@ TEST_F(WebRtcSessionTest, TestSetRemoteDescriptionWithIceRestart) { EXPECT_EQ(0, session_->remote_description()->candidates(0)->count()); } +// Test that if the remote answer indicates the peer requested ICE restart (via +// a new ufrag or pwd), the old ICE candidates are not copied, and vice versa. +TEST_F(WebRtcSessionTest, TestSetRemoteAnswerWithIceRestart) { + Init(); + SessionDescriptionInterface* offer = CreateOffer(); + SetLocalDescriptionWithoutError(offer); + scoped_ptr answer(CreateRemoteAnswer(offer)); + + // Create the first answer. + std::string sdp; + ModifyIceUfragPwdLines(answer.get(), "0123456789012345", + "abcdefghijklmnopqrstuvwx", &sdp); + SessionDescriptionInterface* answer1 = + CreateSessionDescription(JsepSessionDescription::kPrAnswer, sdp, NULL); + cricket::Candidate candidate1(1, "udp", rtc::SocketAddress("1.1.1.1", 5000), + 0, "", "", "relay", 0, ""); + JsepIceCandidate ice_candidate1(kMediaContentName0, kMediaContentIndex0, + candidate1); + EXPECT_TRUE(answer1->AddCandidate(&ice_candidate1)); + SetRemoteDescriptionWithoutError(answer1); + EXPECT_EQ(1, session_->remote_description()->candidates(0)->count()); + + // The second answer has the same ufrag and pwd but different address. + sdp.clear(); + ModifyIceUfragPwdLines(answer.get(), "0123456789012345", + "abcdefghijklmnopqrstuvwx", &sdp); + SessionDescriptionInterface* answer2 = + CreateSessionDescription(JsepSessionDescription::kPrAnswer, sdp, NULL); + candidate1.set_address(rtc::SocketAddress("1.1.1.1", 6000)); + JsepIceCandidate ice_candidate2(kMediaContentName0, kMediaContentIndex0, + candidate1); + EXPECT_TRUE(answer2->AddCandidate(&ice_candidate2)); + SetRemoteDescriptionWithoutError(answer2); + EXPECT_EQ(2, session_->remote_description()->candidates(0)->count()); + + // The third answer has a different ufrag and different address. + sdp.clear(); + ModifyIceUfragPwdLines(answer.get(), "0123456789012333", + "abcdefghijklmnopqrstuvwx", &sdp); + SessionDescriptionInterface* answer3 = + CreateSessionDescription(JsepSessionDescription::kPrAnswer, sdp, NULL); + candidate1.set_address(rtc::SocketAddress("1.1.1.1", 7000)); + JsepIceCandidate ice_candidate3(kMediaContentName0, kMediaContentIndex0, + candidate1); + EXPECT_TRUE(answer3->AddCandidate(&ice_candidate3)); + SetRemoteDescriptionWithoutError(answer3); + EXPECT_EQ(1, session_->remote_description()->candidates(0)->count()); + + // The fourth answer has no candidate but a different ufrag/pwd. + sdp.clear(); + ModifyIceUfragPwdLines(answer.get(), "0123456789012444", + "abcdefghijklmnopqrstuvyz", &sdp); + SessionDescriptionInterface* offer4 = + CreateSessionDescription(JsepSessionDescription::kPrAnswer, sdp, NULL); + SetRemoteDescriptionWithoutError(offer4); + EXPECT_EQ(0, session_->remote_description()->candidates(0)->count()); +} + // Test that candidates sent to the "video" transport do not get pushed down to // the "audio" transport channel when bundling. TEST_F(WebRtcSessionTest, TestIgnoreCandidatesForUnusedTransportWhenBundling) {