From e3fb8122aa218bfc90b57515c6fb118679e6f297 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Mon, 12 Feb 2024 12:21:16 +0100 Subject: [PATCH] Reland "Let port allocator create ice tie breaker" This is a reland of commit 3f3f991c03bb4073a06da37c822daaa9deed9307 Original change's description: > Let port allocator create ice tie breaker > > Moves the responsibility for creating the ICE tie breaker from the JSEP transport controller to the port allocator. This will allow a future change to separate the ICE tie breaker (which is sent over the network and hence known to the peer) from the "port allocator random" (that is used to seed the ICE candidate foundation crc32 checksum) as an implementation detail. > > BUG=webrtc:14626 > > Change-Id: I3a9a0980238d6108b1b154f45de2975b08793b1c > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/281660 > Reviewed-by: Harald Alvestrand > Commit-Queue: Philipp Hancke > Cr-Commit-Position: refs/heads/main@{#41707} Bug: webrtc:14626 Change-Id: Id3c8f257c5611958551bd66d7ce7a885bf8ba2f9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339320 Reviewed-by: Mirko Bonadei Reviewed-by: Harald Alvestrand Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/heads/main@{#41720} --- p2p/base/fake_port_allocator.h | 5 +++-- p2p/base/p2p_transport_channel.cc | 8 +++----- p2p/base/p2p_transport_channel.h | 2 +- p2p/base/p2p_transport_channel_unittest.cc | 7 ------- p2p/base/port_allocator.cc | 14 ++------------ p2p/base/port_allocator.h | 12 ++---------- p2p/base/port_allocator_unittest.cc | 5 +---- p2p/base/regathering_controller_unittest.cc | 2 -- p2p/client/basic_port_allocator.cc | 8 ++++---- p2p/client/basic_port_allocator_unittest.cc | 5 ----- pc/jsep_transport_controller.cc | 4 +--- pc/jsep_transport_controller.h | 2 +- pc/peer_connection_ice_unittest.cc | 2 -- pc/peer_connection_interface_unittest.cc | 2 -- 14 files changed, 18 insertions(+), 60 deletions(-) diff --git a/p2p/base/fake_port_allocator.h b/p2p/base/fake_port_allocator.h index 0bb7dd0139..a51a7ca4ee 100644 --- a/p2p/base/fake_port_allocator.h +++ b/p2p/base/fake_port_allocator.h @@ -93,6 +93,7 @@ class FakePortAllocatorSession : public PortAllocatorSession { ice_ufrag, ice_pwd, allocator->flags()), + allocator_(allocator), network_thread_(network_thread), factory_(factory), ipv4_network_("network", @@ -110,7 +111,6 @@ class FakePortAllocatorSession : public PortAllocatorSession { field_trials_(field_trials) { ipv4_network_.AddIP(rtc::IPAddress(INADDR_LOOPBACK)); ipv6_network_.AddIP(rtc::IPAddress(in6addr_loopback)); - set_ice_tiebreaker(/*kTiebreakerDefault = */ 44444); } void SetCandidateFilter(uint32_t filter) override { @@ -127,7 +127,7 @@ class FakePortAllocatorSession : public PortAllocatorSession { username(), password(), false, field_trials_)); RTC_DCHECK(port_); - port_->SetIceTiebreaker(ice_tiebreaker()); + port_->SetIceTiebreaker(allocator_->ice_tiebreaker()); port_->SubscribePortDestroyed( [this](PortInterface* port) { OnPortDestroyed(port); }); AddPort(port_.get()); @@ -199,6 +199,7 @@ class FakePortAllocatorSession : public PortAllocatorSession { port_.release(); } + PortAllocator* allocator_; rtc::Thread* network_thread_; rtc::PacketSocketFactory* factory_; rtc::Network ipv4_network_; diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 0bccb67209..2f18f1dbb9 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -164,7 +164,7 @@ P2PTransportChannel::P2PTransportChannel( error_(0), remote_ice_mode_(ICEMODE_FULL), ice_role_(ICEROLE_UNKNOWN), - tiebreaker_(0), + ice_tiebreaker_(0), gathering_state_(kIceGatheringNew), weak_ping_interval_(GetWeakPingIntervalInFieldTrial(field_trials)), config_(RECEIVING_TIMEOUT, @@ -326,7 +326,7 @@ void P2PTransportChannel::SetIceTiebreaker(uint64_t tiebreaker) { return; } - tiebreaker_ = tiebreaker; + ice_tiebreaker_ = tiebreaker; } IceTransportState P2PTransportChannel::GetState() const { @@ -885,7 +885,6 @@ void P2PTransportChannel::MaybeStartGathering() { ice_parameters_.ufrag, ice_parameters_.pwd); if (pooled_session) { - pooled_session->set_ice_tiebreaker(tiebreaker_); AddAllocatorSession(std::move(pooled_session)); PortAllocatorSession* raw_pooled_session = allocator_sessions_.back().get(); @@ -902,7 +901,6 @@ void P2PTransportChannel::MaybeStartGathering() { AddAllocatorSession(allocator_->CreateSession( transport_name(), component(), ice_parameters_.ufrag, ice_parameters_.pwd)); - allocator_sessions_.back()->set_ice_tiebreaker(tiebreaker_); allocator_sessions_.back()->StartGettingPorts(); } } @@ -930,7 +928,7 @@ void P2PTransportChannel::OnPortReady(PortAllocatorSession* session, // if one is pending. port->SetIceRole(ice_role_); - port->SetIceTiebreaker(tiebreaker_); + port->SetIceTiebreaker(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 da7933f2e7..f7472df38a 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -439,7 +439,7 @@ 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 tiebreaker_ 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 5bcfee0473..79e984cfec 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -141,7 +141,6 @@ const cricket::IceParameters kIceParams[4] = { const uint64_t kLowTiebreaker = 11111; const uint64_t kHighTiebreaker = 22222; -const uint64_t kTiebreakerDefault = 44444; cricket::IceConfig CreateIceConfig( int receiving_timeout, @@ -299,10 +298,6 @@ class P2PTransportChannelTestBase : public ::testing::Test, &ep2_.network_manager_, socket_factory_.get(), stun_servers, kTurnUdpIntAddr, rtc::SocketAddress())); - ep1_.SetIceTiebreaker(kTiebreakerDefault); - ep1_.allocator_->SetIceTiebreaker(kTiebreakerDefault); - ep2_.SetIceTiebreaker(kTiebreakerDefault); - ep2_.allocator_->SetIceTiebreaker(kTiebreakerDefault); webrtc::metrics::Reset(); } @@ -3347,7 +3342,6 @@ class P2PTransportChannelPingTest : public ::testing::Test, protected: void PrepareChannel(P2PTransportChannel* ch) { ch->SetIceRole(ICEROLE_CONTROLLING); - ch->SetIceTiebreaker(kTiebreakerDefault); ch->SetIceParameters(kIceParams[0]); ch->SetRemoteIceParameters(kIceParams[1]); ch->SignalNetworkRouteChanged.connect( @@ -3715,7 +3709,6 @@ TEST_F(P2PTransportChannelPingTest, PingingStartedAsSoonAsPossible) { &field_trials_); P2PTransportChannel ch("TestChannel", 1, &pa, &field_trials_); ch.SetIceRole(ICEROLE_CONTROLLING); - ch.SetIceTiebreaker(kTiebreakerDefault); ch.SetIceParameters(kIceParams[0]); ch.MaybeStartGathering(); EXPECT_EQ_WAIT(IceGatheringState::kIceGatheringComplete, ch.gathering_state(), diff --git a/p2p/base/port_allocator.cc b/p2p/base/port_allocator.cc index 3745717510..52fc8c1d39 100644 --- a/p2p/base/port_allocator.cc +++ b/p2p/base/port_allocator.cc @@ -68,8 +68,7 @@ PortAllocatorSession::PortAllocatorSession(absl::string_view content_name, content_name_(content_name), component_(component), ice_ufrag_(ice_ufrag), - ice_pwd_(ice_pwd), - tiebreaker_(0) { + ice_pwd_(ice_pwd) { // Pooled sessions are allowed to be created with empty content name, // component, ufrag and password. RTC_DCHECK(ice_ufrag.empty() == ice_pwd.empty()); @@ -101,7 +100,7 @@ PortAllocator::PortAllocator() step_delay_(kDefaultStepDelay), allow_tcp_listen_(true), candidate_filter_(CF_ALL), - tiebreaker_(0) { + tiebreaker_(rtc::CreateRandomId64()) { // The allocator will be attached to a thread in Initialize. thread_checker_.Detach(); } @@ -189,7 +188,6 @@ bool PortAllocator::SetConfiguration( PortAllocatorSession* pooled_session = CreateSessionInternal("", 0, iceCredentials.ufrag, iceCredentials.pwd); pooled_session->set_pooled(true); - pooled_session->set_ice_tiebreaker(tiebreaker_); pooled_session->StartGettingPorts(); pooled_sessions_.push_back( std::unique_ptr(pooled_session)); @@ -197,13 +195,6 @@ bool PortAllocator::SetConfiguration( return true; } -void PortAllocator::SetIceTiebreaker(uint64_t tiebreaker) { - tiebreaker_ = tiebreaker; - for (auto& pooled_session : pooled_sessions_) { - pooled_session->set_ice_tiebreaker(tiebreaker_); - } -} - std::unique_ptr PortAllocator::CreateSession( absl::string_view content_name, int component, @@ -213,7 +204,6 @@ std::unique_ptr PortAllocator::CreateSession( auto session = std::unique_ptr( CreateSessionInternal(content_name, component, ice_ufrag, ice_pwd)); session->SetCandidateFilter(candidate_filter()); - session->set_ice_tiebreaker(tiebreaker_); return session; } diff --git a/p2p/base/port_allocator.h b/p2p/base/port_allocator.h index b8cffca9c0..63ecfc6904 100644 --- a/p2p/base/port_allocator.h +++ b/p2p/base/port_allocator.h @@ -212,10 +212,6 @@ class RTC_EXPORT PortAllocatorSession : public sigslot::has_slots<> { const std::string& ice_pwd() const { return ice_pwd_; } bool pooled() const { return pooled_; } - // TODO(bugs.webrtc.org/14605): move this to the constructor - void set_ice_tiebreaker(uint64_t tiebreaker) { tiebreaker_ = tiebreaker; } - uint64_t ice_tiebreaker() const { return tiebreaker_; } - // Setting this filter should affect not only candidates gathered in the // future, but candidates already gathered and ports already "ready", // which would be returned by ReadyCandidates() and ReadyPorts(). @@ -332,9 +328,6 @@ class RTC_EXPORT PortAllocatorSession : public sigslot::has_slots<> { bool pooled_ = false; - // TODO(bugs.webrtc.org/14605): move this to the constructor - uint64_t tiebreaker_; - // SetIceParameters is an implementation detail which only PortAllocator // should be able to call. friend class PortAllocator; @@ -387,9 +380,6 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> { const absl::optional& stun_candidate_keepalive_interval = absl::nullopt); - void SetIceTiebreaker(uint64_t tiebreaker); - uint64_t IceTiebreaker() const { return tiebreaker_; } - const ServerAddresses& stun_servers() const { CheckRunOnValidThreadIfInitialized(); return stun_servers_; @@ -461,6 +451,8 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> { // 3. mDNS concealment of private IPs is enabled. Candidate SanitizeCandidate(const Candidate& c) const; + uint64_t ice_tiebreaker() const { return tiebreaker_; } + uint32_t flags() const { CheckRunOnValidThreadIfInitialized(); return flags_; diff --git a/p2p/base/port_allocator_unittest.cc b/p2p/base/port_allocator_unittest.cc index 836a2fa494..2df8662f62 100644 --- a/p2p/base/port_allocator_unittest.cc +++ b/p2p/base/port_allocator_unittest.cc @@ -26,7 +26,6 @@ static const char kIceUfrag[] = "UF00"; static const char kIcePwd[] = "TESTICEPWD00000000000000"; static const char kTurnUsername[] = "test"; static const char kTurnPassword[] = "test"; -constexpr uint64_t kTiebreakerDefault = 44444; class PortAllocatorTest : public ::testing::Test, public sigslot::has_slots<> { public: @@ -38,9 +37,7 @@ class PortAllocatorTest : public ::testing::Test, public sigslot::has_slots<> { allocator_(std::make_unique( rtc::Thread::Current(), packet_socket_factory_.get(), - &field_trials_)) { - allocator_->SetIceTiebreaker(kTiebreakerDefault); - } + &field_trials_)) {} protected: void SetConfigurationWithPoolSize(int candidate_pool_size) { diff --git a/p2p/base/regathering_controller_unittest.cc b/p2p/base/regathering_controller_unittest.cc index 91b7270f77..573c0fd23f 100644 --- a/p2p/base/regathering_controller_unittest.cc +++ b/p2p/base/regathering_controller_unittest.cc @@ -40,7 +40,6 @@ const rtc::SocketAddress kTurnUdpIntAddr("99.99.99.3", const cricket::RelayCredentials kRelayCredentials("test", "test"); const char kIceUfrag[] = "UF00"; const char kIcePwd[] = "TESTICEPWD00000000000000"; -constexpr uint64_t kTiebreakerDefault = 44444; } // namespace @@ -59,7 +58,6 @@ class RegatheringControllerTest : public ::testing::Test, rtc::Thread::Current(), packet_socket_factory_.get(), &field_trials_)) { - allocator_->SetIceTiebreaker(kTiebreakerDefault); BasicRegatheringController::Config regathering_config; regathering_config.regather_on_failed_networks_interval = 0; regathering_controller_.reset(new BasicRegatheringController( diff --git a/p2p/client/basic_port_allocator.cc b/p2p/client/basic_port_allocator.cc index e95033efeb..cc38a66727 100644 --- a/p2p/client/basic_port_allocator.cc +++ b/p2p/client/basic_port_allocator.cc @@ -1483,7 +1483,7 @@ void AllocationSequence::CreateUDPPorts() { } if (port) { - port->SetIceTiebreaker(session_->ice_tiebreaker()); + port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker()); // If shared socket is enabled, STUN candidate will be allocated by the // UDPPort. if (IsFlagSet(PORTALLOCATOR_ENABLE_SHARED_SOCKET)) { @@ -1519,7 +1519,7 @@ void AllocationSequence::CreateTCPPorts() { session_->allocator()->allow_tcp_listen(), session_->allocator()->field_trials()); if (port) { - port->SetIceTiebreaker(session_->ice_tiebreaker()); + port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker()); session_->AddAllocatedPort(port.release(), this); // Since TCPPort is not created using shared socket, `port` will not be // added to the dequeue. @@ -1549,7 +1549,7 @@ void AllocationSequence::CreateStunPorts() { session_->allocator()->stun_candidate_keepalive_interval(), session_->allocator()->field_trials()); if (port) { - port->SetIceTiebreaker(session_->ice_tiebreaker()); + port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker()); session_->AddAllocatedPort(port.release(), this); // Since StunPort is not created using shared socket, `port` will not be // added to the dequeue. @@ -1652,7 +1652,7 @@ void AllocationSequence::CreateTurnPort(const RelayServerConfig& config, } } RTC_DCHECK(port != NULL); - port->SetIceTiebreaker(session_->ice_tiebreaker()); + port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker()); session_->AddAllocatedPort(port.release(), this); } } diff --git a/p2p/client/basic_port_allocator_unittest.cc b/p2p/client/basic_port_allocator_unittest.cc index 77443eedbb..f77040d128 100644 --- a/p2p/client/basic_port_allocator_unittest.cc +++ b/p2p/client/basic_port_allocator_unittest.cc @@ -112,8 +112,6 @@ static const char kTurnPassword[] = "test"; // Add some margin of error for slow bots. static const int kStunTimeoutMs = cricket::STUN_TOTAL_TIMEOUT; -constexpr uint64_t kTiebreakerDefault = 44444; - namespace { void CheckStunKeepaliveIntervalOfAllReadyPorts( @@ -176,7 +174,6 @@ class BasicPortAllocatorTestBase : public ::testing::Test, &network_manager_, &socket_factory_, stun_servers, &field_trials_); allocator_->Initialize(); allocator_->set_step_delay(kMinimumStepDelay); - allocator_->SetIceTiebreaker(kTiebreakerDefault); webrtc::metrics::Reset(); } @@ -214,7 +211,6 @@ class BasicPortAllocatorTestBase : public ::testing::Test, allocator_.reset( new BasicPortAllocator(&network_manager_, &socket_factory_)); allocator_->Initialize(); - allocator_->SetIceTiebreaker(kTiebreakerDefault); allocator_->set_step_delay(kMinimumStepDelay); } // Endpoint is behind a NAT, with STUN specified. @@ -299,7 +295,6 @@ class BasicPortAllocatorTestBase : public ::testing::Test, this, &BasicPortAllocatorTestBase::OnCandidatesRemoved); session->SignalCandidatesAllocationDone.connect( this, &BasicPortAllocatorTestBase::OnCandidatesAllocationDone); - session->set_ice_tiebreaker(kTiebreakerDefault); return session; } diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 9473d962ef..d5eb0c633d 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -55,6 +55,7 @@ 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); @@ -62,9 +63,6 @@ JsepTransportController::JsepTransportController( RTC_DCHECK(config_.ice_transport_factory); RTC_DCHECK(config_.on_dtls_handshake_error_); RTC_DCHECK(config_.field_trials); - if (port_allocator_) { - port_allocator_->SetIceTiebreaker(ice_tiebreaker_); - } } JsepTransportController::~JsepTransportController() { diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index 448844ac79..7f06c22969 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -507,7 +507,7 @@ class JsepTransportController : public sigslot::has_slots<> { cricket::IceConfig ice_config_; cricket::IceRole ice_role_ = cricket::ICEROLE_CONTROLLING; - uint64_t ice_tiebreaker_ = rtc::CreateRandomId64(); + uint64_t ice_tiebreaker_; rtc::scoped_refptr certificate_; BundleManager bundles_; diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc index 492e108cbc..bd4848cf8b 100644 --- a/pc/peer_connection_ice_unittest.cc +++ b/pc/peer_connection_ice_unittest.cc @@ -94,7 +94,6 @@ using ::testing::Values; constexpr int kIceCandidatesTimeout = 10000; constexpr int64_t kWaitTimeout = 10000; -constexpr uint64_t kTiebreakerDefault = 44444; class PeerConnectionWrapperForIceTest : public PeerConnectionWrapper { public: @@ -1448,7 +1447,6 @@ class PeerConnectionIceConfigTest : public ::testing::Test { packet_socket_factory_.get(), &field_trials_)); port_allocator_ = port_allocator.get(); - port_allocator_->SetIceTiebreaker(kTiebreakerDefault); PeerConnectionDependencies pc_dependencies(&observer_); pc_dependencies.allocator = std::move(port_allocator); auto result = pc_factory_->CreatePeerConnectionOrError( diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc index 08fb1632d6..71c85d4c6f 100644 --- a/pc/peer_connection_interface_unittest.cc +++ b/pc/peer_connection_interface_unittest.cc @@ -114,7 +114,6 @@ static const char kVideoTracks[][32] = {"videotrack0", "videotrack1"}; static const char kRecvonly[] = "recvonly"; static const char kSendrecv[] = "sendrecv"; -constexpr uint64_t kTiebreakerDefault = 44444; // Reference SDP with a MediaStream with label "stream1" and audio track with // id "audio_1" and a video track with id "video_1; @@ -711,7 +710,6 @@ class PeerConnectionInterfaceBaseTest : public ::testing::Test { std::make_unique(vss_.get()), &field_trials_)); port_allocator_ = port_allocator.get(); - port_allocator_->SetIceTiebreaker(kTiebreakerDefault); // Create certificate generator unless DTLS constraint is explicitly set to // false.