diff --git a/webrtc/p2p/base/faketransportcontroller.h b/webrtc/p2p/base/faketransportcontroller.h index c15e164f41..d79525490e 100644 --- a/webrtc/p2p/base/faketransportcontroller.h +++ b/webrtc/p2p/base/faketransportcontroller.h @@ -504,6 +504,13 @@ class FakeTransportController : public TransportController { nullptr), fail_create_channel_(false) {} + explicit FakeTransportController(bool redetermine_role_on_ice_restart) + : TransportController(rtc::Thread::Current(), + rtc::Thread::Current(), + nullptr, + redetermine_role_on_ice_restart), + fail_create_channel_(false) {} + explicit FakeTransportController(IceRole role) : TransportController(rtc::Thread::Current(), rtc::Thread::Current(), diff --git a/webrtc/p2p/base/transportcontroller.cc b/webrtc/p2p/base/transportcontroller.cc index 1a13ca14b4..de98df23bc 100644 --- a/webrtc/p2p/base/transportcontroller.cc +++ b/webrtc/p2p/base/transportcontroller.cc @@ -44,10 +44,20 @@ struct CandidatesData : public rtc::MessageData { TransportController::TransportController(rtc::Thread* signaling_thread, rtc::Thread* network_thread, - PortAllocator* port_allocator) + PortAllocator* port_allocator, + bool redetermine_role_on_ice_restart) : signaling_thread_(signaling_thread), network_thread_(network_thread), - port_allocator_(port_allocator) {} + port_allocator_(port_allocator), + redetermine_role_on_ice_restart_(redetermine_role_on_ice_restart) {} + +TransportController::TransportController(rtc::Thread* signaling_thread, + rtc::Thread* network_thread, + PortAllocator* port_allocator) + : TransportController(signaling_thread, + network_thread, + port_allocator, + true) {} TransportController::~TransportController() { network_thread_->Invoke( @@ -450,11 +460,12 @@ bool TransportController::SetLocalTransportDescription_n( // Older versions of Chrome expect the ICE role to be re-determined when an // ICE restart occurs, and also don't perform conflict resolution correctly, - // so for now we can't safely stop doing this. + // so for now we can't safely stop doing this, unless the application opts in + // by setting |redetermine_role_on_ice_restart_| to false. // See: https://bugs.chromium.org/p/chromium/issues/detail?id=628676 // TODO(deadbeef): Remove this when these old versions of Chrome reach a low // enough population. - if (transport->local_description() && + 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)) { diff --git a/webrtc/p2p/base/transportcontroller.h b/webrtc/p2p/base/transportcontroller.h index a32620281a..b5d9ac7d40 100644 --- a/webrtc/p2p/base/transportcontroller.h +++ b/webrtc/p2p/base/transportcontroller.h @@ -31,6 +31,14 @@ namespace cricket { class TransportController : public sigslot::has_slots<>, public rtc::MessageHandler { public: + // If |redetermine_role_on_ice_restart| is true, ICE role is redetermined + // upon setting a local transport description that indicates an ICE restart. + // For the constructor that doesn't take this parameter, it defaults to true. + TransportController(rtc::Thread* signaling_thread, + rtc::Thread* network_thread, + PortAllocator* port_allocator, + bool redetermine_role_on_ice_restart); + TransportController(rtc::Thread* signaling_thread, rtc::Thread* network_thread, PortAllocator* port_allocator); @@ -224,6 +232,7 @@ class TransportController : public sigslot::has_slots<>, // TODO(deadbeef): Move the fields below down to the transports themselves IceConfig ice_config_; IceRole ice_role_ = ICEROLE_CONTROLLING; + bool redetermine_role_on_ice_restart_; uint64_t ice_tiebreaker_ = rtc::CreateRandomId64(); rtc::scoped_refptr certificate_; rtc::AsyncInvoker invoker_; diff --git a/webrtc/p2p/base/transportcontroller_unittest.cc b/webrtc/p2p/base/transportcontroller_unittest.cc index 2b11286e99..f44f831b0e 100644 --- a/webrtc/p2p/base/transportcontroller_unittest.cc +++ b/webrtc/p2p/base/transportcontroller_unittest.cc @@ -689,7 +689,7 @@ TEST_F(TransportControllerTest, TestSignalingOccursOnSignalingThread) { // See: https://bugs.chromium.org/p/chromium/issues/detail?id=628676 // TODO(deadbeef): Remove this when these old versions of Chrome reach a low // enough population. -TEST_F(TransportControllerTest, IceRoleRedeterminedOnIceRestart) { +TEST_F(TransportControllerTest, IceRoleRedeterminedOnIceRestartByDefault) { FakeTransportChannel* channel = CreateChannel("audio", 1); ASSERT_NE(nullptr, channel); std::string err; @@ -717,4 +717,36 @@ TEST_F(TransportControllerTest, IceRoleRedeterminedOnIceRestart) { EXPECT_EQ(ICEROLE_CONTROLLING, channel->GetIceRole()); } +// Test that if the TransportController was created with the +// |redetermine_role_on_ice_restart| parameter set to false, the role is *not* +// redetermined on an ICE restart. +TEST_F(TransportControllerTest, IceRoleNotRedetermined) { + bool redetermine_role = false; + transport_controller_.reset(new TransportControllerForTest(redetermine_role)); + FakeTransportChannel* channel = CreateChannel("audio", 1); + ASSERT_NE(nullptr, channel); + std::string err; + // Do an initial offer answer, so that the next offer is an ICE restart. + transport_controller_->SetIceRole(ICEROLE_CONTROLLED); + TransportDescription remote_desc(std::vector(), kIceUfrag1, + kIcePwd1, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, nullptr); + EXPECT_TRUE(transport_controller_->SetRemoteTransportDescription( + "audio", remote_desc, CA_OFFER, &err)); + TransportDescription local_desc(std::vector(), kIceUfrag2, + kIcePwd2, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, nullptr); + EXPECT_TRUE(transport_controller_->SetLocalTransportDescription( + "audio", local_desc, CA_ANSWER, &err)); + EXPECT_EQ(ICEROLE_CONTROLLED, channel->GetIceRole()); + + // The endpoint that initiated an ICE restart should keep the existing role. + TransportDescription ice_restart_desc(std::vector(), kIceUfrag3, + kIcePwd3, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, nullptr); + EXPECT_TRUE(transport_controller_->SetLocalTransportDescription( + "audio", ice_restart_desc, CA_OFFER, &err)); + EXPECT_EQ(ICEROLE_CONTROLLED, channel->GetIceRole()); +} + } // namespace cricket {