From 897d08ef1b314e8d8fcd24d1dcfba820cb2501b7 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Thu, 20 Apr 2017 00:57:25 -0700 Subject: [PATCH] Fixing bug that results in incorrect ICE role with ICE lite endpoints. There's some code that resets the ICE role on an ICE restart (behavior that's specified in ICE, but removed from ICEbis). And it wasn't taking into account that the remote endpoint may be an ICE lite endpoint, in which case the WebRTC endpoint's role should always be "controlling". BUG=chromium:710760 Review-Url: https://codereview.webrtc.org/2812173003 Cr-Commit-Position: refs/heads/master@{#17779} --- webrtc/p2p/base/transportcontroller.cc | 6 +++- .../p2p/base/transportcontroller_unittest.cc | 31 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/webrtc/p2p/base/transportcontroller.cc b/webrtc/p2p/base/transportcontroller.cc index f06a180088..df85a7c358 100644 --- a/webrtc/p2p/base/transportcontroller.cc +++ b/webrtc/p2p/base/transportcontroller.cc @@ -602,7 +602,11 @@ bool TransportController::SetLocalTransportDescription_n( if (redetermine_role_on_ice_restart_ && transport->local_description() && IceCredentialsChanged(transport->local_description()->ice_ufrag, transport->local_description()->ice_pwd, - tdesc.ice_ufrag, tdesc.ice_pwd)) { + tdesc.ice_ufrag, tdesc.ice_pwd) && + // Don't change the ICE role if the remote endpoint is ICE lite; we + // should always be controlling in that case. + (!transport->remote_description() || + transport->remote_description()->ice_mode != ICEMODE_LITE)) { IceRole new_ice_role = (action == CA_OFFER) ? ICEROLE_CONTROLLING : ICEROLE_CONTROLLED; SetIceRole(new_ice_role); diff --git a/webrtc/p2p/base/transportcontroller_unittest.cc b/webrtc/p2p/base/transportcontroller_unittest.cc index 44ad54dcbe..239bb2a235 100644 --- a/webrtc/p2p/base/transportcontroller_unittest.cc +++ b/webrtc/p2p/base/transportcontroller_unittest.cc @@ -834,6 +834,37 @@ TEST_F(TransportControllerTest, TestSetRemoteIceLiteInAnswer) { EXPECT_EQ(ICEMODE_LITE, transport->fake_ice_transport()->remote_ice_mode()); } +// Tests that the ICE role remains "controlling" if a subsequent offer that +// does an ICE restart is received from an ICE lite endpoint. Regression test +// for: https://crbug.com/710760 +TEST_F(TransportControllerTest, + IceRoleIsControllingAfterIceRestartFromIceLiteEndpoint) { + FakeDtlsTransport* transport = CreateFakeDtlsTransport("audio", 1); + ASSERT_NE(nullptr, transport); + std::string err; + + // Initial offer/answer. + TransportDescription remote_desc(std::vector(), kIceUfrag1, + kIcePwd1, ICEMODE_LITE, + CONNECTIONROLE_ACTPASS, nullptr); + TransportDescription local_desc(kIceUfrag1, kIcePwd1); + ASSERT_TRUE(transport_controller_->SetRemoteTransportDescription( + "audio", remote_desc, CA_OFFER, &err)); + ASSERT_TRUE(transport_controller_->SetLocalTransportDescription( + "audio", local_desc, CA_ANSWER, nullptr)); + // Subsequent ICE restart offer/answer. + remote_desc.ice_ufrag = kIceUfrag2; + remote_desc.ice_pwd = kIcePwd2; + local_desc.ice_ufrag = kIceUfrag2; + local_desc.ice_pwd = kIcePwd2; + ASSERT_TRUE(transport_controller_->SetRemoteTransportDescription( + "audio", remote_desc, CA_OFFER, &err)); + ASSERT_TRUE(transport_controller_->SetLocalTransportDescription( + "audio", local_desc, CA_ANSWER, nullptr)); + + EXPECT_EQ(ICEROLE_CONTROLLING, transport->fake_ice_transport()->GetIceRole()); +} + // Tests SetNeedsIceRestartFlag and NeedsIceRestart, setting the flag and then // initiating an ICE restart for one of the transports. TEST_F(TransportControllerTest, NeedsIceRestart) {