From b5df2ba10db3cd04febcde8727e782457708f2fa Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Wed, 22 May 2024 06:22:16 -0700 Subject: [PATCH] p2p: reduce visibility of ICE tiebreaker further since the tie breaker is owned by the allocator now. BUG=webrtc:42224914 Change-Id: I76bd5ae714fb2a6df38e014991242f390ae87e6a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/351180 Reviewed-by: Harald Alvestrand Reviewed-by: Tomas Gunnarsson Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/heads/main@{#42371} --- p2p/base/dtls_transport_unittest.cc | 2 - p2p/base/fake_ice_transport.h | 9 --- p2p/base/ice_transport_internal.h | 2 - p2p/base/mock_ice_transport.h | 1 - p2p/base/p2p_transport_channel.cc | 14 +---- p2p/base/p2p_transport_channel.h | 2 - p2p/base/p2p_transport_channel_unittest.cc | 72 ++++++++-------------- pc/jsep_transport_controller.cc | 2 - pc/jsep_transport_controller.h | 1 - 9 files changed, 27 insertions(+), 78 deletions(-) diff --git a/p2p/base/dtls_transport_unittest.cc b/p2p/base/dtls_transport_unittest.cc index 93653c116f..f870c1e207 100644 --- a/p2p/base/dtls_transport_unittest.cc +++ b/p2p/base/dtls_transport_unittest.cc @@ -92,8 +92,6 @@ class DtlsTestClient : public sigslot::has_slots<> { fake_ice_transport_->SetAsync(true); fake_ice_transport_->SetAsyncDelay(async_delay_ms); fake_ice_transport_->SetIceRole(role); - fake_ice_transport_->SetIceTiebreaker((role == ICEROLE_CONTROLLING) ? 1 - : 2); // Hook the raw packets so that we can verify they are encrypted. fake_ice_transport_->RegisterReceivedPacketCallback( this, [&](rtc::PacketTransportInternal* transport, diff --git a/p2p/base/fake_ice_transport.h b/p2p/base/fake_ice_transport.h index 285bfff59c..788299bb38 100644 --- a/p2p/base/fake_ice_transport.h +++ b/p2p/base/fake_ice_transport.h @@ -147,10 +147,6 @@ class FakeIceTransport : public IceTransportInternal { // Fake IceTransportInternal implementation. const std::string& transport_name() const override { return name_; } int component() const override { return component_; } - uint64_t IceTiebreaker() const { - RTC_DCHECK_RUN_ON(network_thread_); - return tiebreaker_; - } IceMode remote_ice_mode() const { RTC_DCHECK_RUN_ON(network_thread_); return remote_ice_mode_; @@ -212,10 +208,6 @@ class FakeIceTransport : public IceTransportInternal { RTC_DCHECK_RUN_ON(network_thread_); return role_; } - void SetIceTiebreaker(uint64_t tiebreaker) override { - RTC_DCHECK_RUN_ON(network_thread_); - tiebreaker_ = tiebreaker; - } void SetIceParameters(const IceParameters& ice_params) override { RTC_DCHECK_RUN_ON(network_thread_); ice_parameters_ = ice_params; @@ -406,7 +398,6 @@ class FakeIceTransport : public IceTransportInternal { Candidates remote_candidates_ RTC_GUARDED_BY(network_thread_); IceConfig ice_config_ RTC_GUARDED_BY(network_thread_); IceRole role_ RTC_GUARDED_BY(network_thread_) = ICEROLE_UNKNOWN; - uint64_t tiebreaker_ RTC_GUARDED_BY(network_thread_) = 0; IceParameters ice_parameters_ RTC_GUARDED_BY(network_thread_); IceParameters remote_ice_parameters_ RTC_GUARDED_BY(network_thread_); IceMode remote_ice_mode_ RTC_GUARDED_BY(network_thread_) = ICEMODE_FULL; diff --git a/p2p/base/ice_transport_internal.h b/p2p/base/ice_transport_internal.h index 46717cc230..48f12f9f50 100644 --- a/p2p/base/ice_transport_internal.h +++ b/p2p/base/ice_transport_internal.h @@ -254,8 +254,6 @@ class RTC_EXPORT IceTransportInternal : public rtc::PacketTransportInternal { virtual void SetIceRole(IceRole role) = 0; - virtual void SetIceTiebreaker(uint64_t tiebreaker) = 0; - virtual void SetIceCredentials(absl::string_view ice_ufrag, absl::string_view ice_pwd); diff --git a/p2p/base/mock_ice_transport.h b/p2p/base/mock_ice_transport.h index ef6bdce3c0..b7b986e022 100644 --- a/p2p/base/mock_ice_transport.h +++ b/p2p/base/mock_ice_transport.h @@ -57,7 +57,6 @@ class MockIceTransport : public IceTransportInternal { const std::string& transport_name() const override { return transport_name_; } int component() const override { return 0; } void SetIceRole(IceRole role) override {} - void SetIceTiebreaker(uint64_t tiebreaker) override {} // The ufrag and pwd in `ice_params` must be set // before candidate gathering can start. void SetIceParameters(const IceParameters& ice_params) override {} diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 241b423be8..dcea5b0e47 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -162,7 +162,6 @@ P2PTransportChannel::P2PTransportChannel( error_(0), remote_ice_mode_(ICEMODE_FULL), ice_role_(ICEROLE_UNKNOWN), - ice_tiebreaker_(0), gathering_state_(kIceGatheringNew), weak_ping_interval_(GetWeakPingIntervalInFieldTrial(field_trials)), config_(RECEIVING_TIMEOUT, @@ -316,17 +315,6 @@ IceRole P2PTransportChannel::GetIceRole() const { return ice_role_; } -void P2PTransportChannel::SetIceTiebreaker(uint64_t tiebreaker) { - RTC_DCHECK_RUN_ON(network_thread_); - if (!ports_.empty() || !pruned_ports_.empty()) { - RTC_LOG(LS_ERROR) - << "Attempt to change tiebreaker after Port has been allocated."; - return; - } - - ice_tiebreaker_ = tiebreaker; -} - IceTransportState P2PTransportChannel::GetState() const { RTC_DCHECK_RUN_ON(network_thread_); return state_; @@ -926,7 +914,7 @@ void P2PTransportChannel::OnPortReady(PortAllocatorSession* session, // if one is pending. port->SetIceRole(ice_role_); - port->SetIceTiebreaker(ice_tiebreaker_); + port->SetIceTiebreaker(allocator_->ice_tiebreaker()); ports_.push_back(port); port->SignalUnknownAddress.connect(this, &P2PTransportChannel::OnUnknownAddress); diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h index f7472df38a..2750af1332 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -128,7 +128,6 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal, bool receiving() const override; void SetIceRole(IceRole role) override; IceRole GetIceRole() const override; - void SetIceTiebreaker(uint64_t tiebreaker) override; void SetIceParameters(const IceParameters& ice_params) override; void SetRemoteIceParameters(const IceParameters& ice_params) override; void SetRemoteIceMode(IceMode mode) override; @@ -439,7 +438,6 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal, RTC_GUARDED_BY(network_thread_); IceMode remote_ice_mode_ RTC_GUARDED_BY(network_thread_); IceRole ice_role_ RTC_GUARDED_BY(network_thread_); - uint64_t ice_tiebreaker_ RTC_GUARDED_BY(network_thread_); IceGatheringState gathering_state_ RTC_GUARDED_BY(network_thread_); std::unique_ptr regathering_controller_ RTC_GUARDED_BY(network_thread_); diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index 79ca2a5f26..29bdd990f3 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -134,9 +134,6 @@ const cricket::IceParameters kIceParams[4] = { {kIceUfrag[2], kIcePwd[2], false}, {kIceUfrag[3], kIcePwd[3], false}}; -const uint64_t kLowTiebreaker = 11111; -const uint64_t kHighTiebreaker = 22222; - cricket::IceConfig CreateIceConfig( int receiving_timeout, cricket::ContinualGatheringPolicy continual_gathering_policy, @@ -377,8 +374,6 @@ class P2PTransportChannelTestBase : public ::testing::Test, void SetIceRole(IceRole role) { role_ = role; } IceRole ice_role() { return role_; } - void SetIceTiebreaker(uint64_t tiebreaker) { tiebreaker_ = tiebreaker; } - uint64_t GetIceTiebreaker() { return tiebreaker_; } void OnRoleConflict(bool role_conflict) { role_conflict_ = role_conflict; } bool role_conflict() { return role_conflict_; } void SetAllocationStepDelay(uint32_t delay) { @@ -487,7 +482,6 @@ class P2PTransportChannelTestBase : public ::testing::Test, channel->SetRemoteIceParameters(remote_ice); } channel->SetIceRole(GetEndpoint(endpoint)->ice_role()); - channel->SetIceTiebreaker(GetEndpoint(endpoint)->GetIceTiebreaker()); return channel; } @@ -562,9 +556,6 @@ class P2PTransportChannelTestBase : public ::testing::Test, void SetIceRole(int endpoint, IceRole role) { GetEndpoint(endpoint)->SetIceRole(role); } - void SetIceTiebreaker(int endpoint, uint64_t tiebreaker) { - GetEndpoint(endpoint)->SetIceTiebreaker(tiebreaker); - } bool GetRoleConflict(int endpoint) { return GetEndpoint(endpoint)->role_conflict(); } @@ -778,31 +769,6 @@ class P2PTransportChannelTestBase : public ::testing::Test, EXPECT_EQ(1u, RemoteCandidate(ep1_ch1())->generation()); } - void TestSignalRoleConflict() { - rtc::ScopedFakeClock clock; - // Default EP1 is in controlling state. - SetIceTiebreaker(0, kLowTiebreaker); - - SetIceRole(1, ICEROLE_CONTROLLING); - SetIceTiebreaker(1, kHighTiebreaker); - - // Creating channels with both channels role set to CONTROLLING. - CreateChannels(); - // Since both the channels initiated with controlling state and channel2 - // has higher tiebreaker value, channel1 should receive SignalRoleConflict. - EXPECT_TRUE_SIMULATED_WAIT(GetRoleConflict(0), kShortTimeout, clock); - EXPECT_FALSE(GetRoleConflict(1)); - - EXPECT_TRUE_SIMULATED_WAIT(CheckConnected(ep1_ch1(), ep2_ch1()), - kShortTimeout, clock); - - EXPECT_TRUE(ep1_ch1()->selected_connection() && - ep2_ch1()->selected_connection()); - - TestSendRecv(&clock); - DestroyChannels(); - } - void TestPacketInfoIsSet(rtc::PacketInfo info) { EXPECT_NE(info.packet_type, rtc::PacketType::kUnknown); EXPECT_NE(info.protocol, rtc::PacketInfoProtocolType::kUnknown); @@ -1839,13 +1805,36 @@ TEST_F(P2PTransportChannelTest, TestTcpConnectionTcptypeSet) { } TEST_F(P2PTransportChannelTest, TestIceRoleConflict) { + rtc::ScopedFakeClock clock; AddAddress(0, kPublicAddrs[0]); AddAddress(1, kPublicAddrs[1]); - TestSignalRoleConflict(); + + // Creating channels with both channels role set to CONTROLLING. + SetIceRole(0, ICEROLE_CONTROLLING); + SetIceRole(1, ICEROLE_CONTROLLING); + + CreateChannels(); + bool first_endpoint_has_lower_tiebreaker = + GetEndpoint(0)->allocator_->ice_tiebreaker() < + GetEndpoint(1)->allocator_->ice_tiebreaker(); + // Since both the channels initiated with controlling state, the channel with + // the lower tiebreaker should receive SignalRoleConflict. + EXPECT_TRUE_SIMULATED_WAIT( + GetRoleConflict(first_endpoint_has_lower_tiebreaker ? 0 : 1), + kShortTimeout, clock); + EXPECT_FALSE(GetRoleConflict(first_endpoint_has_lower_tiebreaker ? 1 : 0)); + + EXPECT_TRUE_SIMULATED_WAIT(CheckConnected(ep1_ch1(), ep2_ch1()), + kShortTimeout, clock); + + EXPECT_TRUE(ep1_ch1()->selected_connection() && + ep2_ch1()->selected_connection()); + + TestSendRecv(&clock); + DestroyChannels(); } -// Tests that the ice configs (protocol, tiebreaker and role) can be passed -// down to ports. +// Tests that the ice configs (protocol and role) can be passed down to ports. TEST_F(P2PTransportChannelTest, TestIceConfigWillPassDownToPort) { rtc::ScopedFakeClock clock; AddAddress(0, kPublicAddrs[0]); @@ -1854,9 +1843,7 @@ TEST_F(P2PTransportChannelTest, TestIceConfigWillPassDownToPort) { // Give the first connection the higher tiebreaker so its role won't // change unless we tell it to. SetIceRole(0, ICEROLE_CONTROLLING); - SetIceTiebreaker(0, kHighTiebreaker); SetIceRole(1, ICEROLE_CONTROLLING); - SetIceTiebreaker(1, kLowTiebreaker); CreateChannels(); @@ -1865,18 +1852,13 @@ TEST_F(P2PTransportChannelTest, TestIceConfigWillPassDownToPort) { const std::vector ports_before = ep1_ch1()->ports(); for (size_t i = 0; i < ports_before.size(); ++i) { EXPECT_EQ(ICEROLE_CONTROLLING, ports_before[i]->GetIceRole()); - EXPECT_EQ(kHighTiebreaker, ports_before[i]->IceTiebreaker()); } ep1_ch1()->SetIceRole(ICEROLE_CONTROLLED); - ep1_ch1()->SetIceTiebreaker(kLowTiebreaker); const std::vector ports_after = ep1_ch1()->ports(); for (size_t i = 0; i < ports_after.size(); ++i) { EXPECT_EQ(ICEROLE_CONTROLLED, ports_before[i]->GetIceRole()); - // SetIceTiebreaker after ports have been created will fail. So expect the - // original value. - EXPECT_EQ(kHighTiebreaker, ports_before[i]->IceTiebreaker()); } EXPECT_TRUE_SIMULATED_WAIT(CheckConnected(ep1_ch1(), ep2_ch1()), @@ -2262,9 +2244,7 @@ TEST_F(P2PTransportChannelTest, // With conflicting ICE roles, endpoint 1 has the higher tie breaker and will // send a binding error response. SetIceRole(0, ICEROLE_CONTROLLING); - SetIceTiebreaker(0, kHighTiebreaker); SetIceRole(1, ICEROLE_CONTROLLING); - SetIceTiebreaker(1, kLowTiebreaker); // We want the remote TURN candidate to show up as prflx. To do this we need // to configure the server to accept packets from an address we haven't // explicitly installed permission for. diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 7152826725..505ed5059e 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -58,7 +58,6 @@ JsepTransportController::JsepTransportController( }), config_(std::move(config)), active_reset_srtp_params_(config.active_reset_srtp_params), - ice_tiebreaker_(port_allocator ? port_allocator->ice_tiebreaker() : 0), bundles_(config.bundle_policy) { // The `transport_observer` is assumed to be non-null. RTC_DCHECK(config_.transport_observer); @@ -404,7 +403,6 @@ JsepTransportController::CreateIceTransport(const std::string& transport_name, transport_name, component, std::move(init)); RTC_DCHECK(transport); transport->internal()->SetIceRole(ice_role_); - transport->internal()->SetIceTiebreaker(ice_tiebreaker_); transport->internal()->SetIceConfig(ice_config_); return transport; } diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index cf2b411f42..da0b921473 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -507,7 +507,6 @@ class JsepTransportController : public sigslot::has_slots<> { cricket::IceConfig ice_config_; cricket::IceRole ice_role_ = cricket::ICEROLE_CONTROLLING; - uint64_t ice_tiebreaker_; rtc::scoped_refptr certificate_; BundleManager bundles_;