Fix the generation mismatch assertion error.
BUG=4860
Review URL: https://codereview.webrtc.org/1248063002
Cr-Commit-Position: refs/heads/master@{#9667}
diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc
index 2533328..b6a178c 100644
--- a/talk/app/webrtc/webrtcsession.cc
+++ b/talk/app/webrtc/webrtcsession.cc
@@ -432,11 +432,10 @@
}
}
- void CheckForRemoteIceRestart(
- const SessionDescriptionInterface* old_desc,
- const SessionDescriptionInterface* new_desc) {
+ bool CheckForRemoteIceRestart(const SessionDescriptionInterface* old_desc,
+ const SessionDescriptionInterface* new_desc) {
if (!old_desc || new_desc->type() != SessionDescriptionInterface::kOffer) {
- return;
+ return false;
}
const SessionDescription* new_sd = new_desc->description();
const SessionDescription* old_sd = old_desc->description();
@@ -462,9 +461,10 @@
new_transport_desc->ice_pwd)) {
LOG(LS_INFO) << "Remote peer request ice restart.";
ice_restart_ = true;
- break;
+ return true;
}
}
+ return false;
}
private:
@@ -838,13 +838,19 @@
// Copy all saved candidates.
CopySavedCandidates(desc);
- // We retain all received candidates.
- WebRtcSessionDescriptionFactory::CopyCandidatesFromSessionDescription(
- remote_desc_.get(), desc);
+
// Check if this new SessionDescription contains new ice ufrag and password
// that indicates the remote peer requests ice restart.
- ice_restart_latch_->CheckForRemoteIceRestart(remote_desc_.get(),
- desc);
+ bool ice_restart =
+ ice_restart_latch_->CheckForRemoteIceRestart(remote_desc_.get(), desc);
+ // We retain all received candidates only if ICE is not restarted.
+ // When ICE is restarted, all previous candidates belong to an old generation
+ // and should not be kept.
+ if (!ice_restart) {
+ WebRtcSessionDescriptionFactory::CopyCandidatesFromSessionDescription(
+ remote_desc_.get(), desc);
+ }
+
remote_desc_.reset(desc_temp.release());
rtc::SSLRole role;
@@ -1522,7 +1528,6 @@
}
continue;
}
-
ret = UseCandidate(candidate);
if (!ret)
break;
diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc
index de10bc4..a269546 100644
--- a/talk/app/webrtc/webrtcsession_unittest.cc
+++ b/talk/app/webrtc/webrtcsession_unittest.cc
@@ -2580,6 +2580,63 @@
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) {
+ Init();
+ scoped_ptr<SessionDescriptionInterface> offer(CreateRemoteOffer());
+
+ // Create the first offer.
+ std::string sdp;
+ ModifyIceUfragPwdLines(offer.get(), "0123456789012345",
+ "abcdefghijklmnopqrstuvwx", &sdp);
+ SessionDescriptionInterface* offer1 =
+ CreateSessionDescription(JsepSessionDescription::kOffer, 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(offer1->AddCandidate(&ice_candidate1));
+ SetRemoteDescriptionWithoutError(offer1);
+ EXPECT_EQ(1, session_->remote_description()->candidates(0)->count());
+
+ // The second offer has the same ufrag and pwd but different address.
+ sdp.clear();
+ ModifyIceUfragPwdLines(offer.get(), "0123456789012345",
+ "abcdefghijklmnopqrstuvwx", &sdp);
+ SessionDescriptionInterface* offer2 =
+ CreateSessionDescription(JsepSessionDescription::kOffer, sdp, NULL);
+ candidate1.set_address(rtc::SocketAddress("1.1.1.1", 6000));
+ JsepIceCandidate ice_candidate2(kMediaContentName0, kMediaContentIndex0,
+ candidate1);
+ EXPECT_TRUE(offer2->AddCandidate(&ice_candidate2));
+ SetRemoteDescriptionWithoutError(offer2);
+ EXPECT_EQ(2, session_->remote_description()->candidates(0)->count());
+
+ // The third offer has a different ufrag and different address.
+ sdp.clear();
+ ModifyIceUfragPwdLines(offer.get(), "0123456789012333",
+ "abcdefghijklmnopqrstuvwx", &sdp);
+ SessionDescriptionInterface* offer3 =
+ CreateSessionDescription(JsepSessionDescription::kOffer, sdp, NULL);
+ candidate1.set_address(rtc::SocketAddress("1.1.1.1", 7000));
+ JsepIceCandidate ice_candidate3(kMediaContentName0, kMediaContentIndex0,
+ candidate1);
+ EXPECT_TRUE(offer3->AddCandidate(&ice_candidate3));
+ SetRemoteDescriptionWithoutError(offer3);
+ EXPECT_EQ(1, session_->remote_description()->candidates(0)->count());
+
+ // The fourth offer has no candidate but a different ufrag/pwd.
+ sdp.clear();
+ ModifyIceUfragPwdLines(offer.get(), "0123456789012444",
+ "abcdefghijklmnopqrstuvyz", &sdp);
+ SessionDescriptionInterface* offer4 =
+ CreateSessionDescription(JsepSessionDescription::kOffer, 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 using TransportProxy.
TEST_F(WebRtcSessionTest, TestIgnoreCandidatesForUnusedTransportWhenBundling) {
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index 46ed40a..94eb017 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -666,6 +666,17 @@
void P2PTransportChannel::OnCandidate(const Candidate& candidate) {
ASSERT(worker_thread_ == rtc::Thread::Current());
+ uint32 generation = candidate.generation();
+ // Network may not guarantee the order of the candidate delivery. If a
+ // remote candidate with an older generation arrives, drop it.
+ if (generation != 0 && generation < remote_candidate_generation_) {
+ LOG(LS_WARNING) << "Dropping a remote candidate because its generation "
+ << generation
+ << " is lower than the current remote generation "
+ << remote_candidate_generation_;
+ return;
+ }
+
// Create connections to this remote candidate.
CreateConnections(candidate, NULL, false);
