From 407367d053dffe4653732e3f6b5acac28caa1382 Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Mon, 12 Feb 2024 08:52:04 +0000 Subject: [PATCH] Revert "Let port allocator create ice tie breaker" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 3f3f991c03bb4073a06da37c822daaa9deed9307. Reason for revert: API breaking change on PortAllocatorSession. Is it possible to duplicate the ctor of PortAllocatorSession and remove the deprecated one (the one without ice_tiebreaker) in another CL? 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: I342c9a96ac1909244aedea6a7779f5682088a5fc Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339280 Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Commit-Queue: Mirko Bonadei Reviewed-by: Björn Terelius Owners-Override: Mirko Bonadei Cr-Commit-Position: refs/heads/main@{#41715} --- p2p/base/fake_port_allocator.h | 2 +- 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 | 12 +++++++++--- p2p/base/port_allocator.h | 16 +++++++++------- p2p/base/port_allocator_unittest.cc | 5 ++++- p2p/base/regathering_controller_unittest.cc | 2 ++ p2p/client/basic_port_allocator.cc | 19 ++----------------- p2p/client/basic_port_allocator.h | 7 ------- 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 ++ 15 files changed, 53 insertions(+), 42 deletions(-) diff --git a/p2p/base/fake_port_allocator.h b/p2p/base/fake_port_allocator.h index 706635778b..0bb7dd0139 100644 --- a/p2p/base/fake_port_allocator.h +++ b/p2p/base/fake_port_allocator.h @@ -92,7 +92,6 @@ class FakePortAllocatorSession : public PortAllocatorSession { component, ice_ufrag, ice_pwd, - /*kTiebreakerDefault = */ 44444, allocator->flags()), network_thread_(network_thread), factory_(factory), @@ -111,6 +110,7 @@ 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 { diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 2f18f1dbb9..0bccb67209 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), - ice_tiebreaker_(0), + 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; } - ice_tiebreaker_ = tiebreaker; + tiebreaker_ = tiebreaker; } IceTransportState P2PTransportChannel::GetState() const { @@ -885,6 +885,7 @@ 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(); @@ -901,6 +902,7 @@ 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(); } } @@ -928,7 +930,7 @@ void P2PTransportChannel::OnPortReady(PortAllocatorSession* session, // if one is pending. port->SetIceRole(ice_role_); - port->SetIceTiebreaker(ice_tiebreaker_); + port->SetIceTiebreaker(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..da7933f2e7 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 ice_tiebreaker_ RTC_GUARDED_BY(network_thread_); + uint64_t 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 79e984cfec..5bcfee0473 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -141,6 +141,7 @@ 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, @@ -298,6 +299,10 @@ 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(); } @@ -3342,6 +3347,7 @@ 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( @@ -3709,6 +3715,7 @@ 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 30bcfb1732..3745717510 100644 --- a/p2p/base/port_allocator.cc +++ b/p2p/base/port_allocator.cc @@ -62,7 +62,6 @@ PortAllocatorSession::PortAllocatorSession(absl::string_view content_name, int component, absl::string_view ice_ufrag, absl::string_view ice_pwd, - uint64_t ice_tiebreaker, uint32_t flags) : flags_(flags), generation_(0), @@ -70,7 +69,7 @@ PortAllocatorSession::PortAllocatorSession(absl::string_view content_name, component_(component), ice_ufrag_(ice_ufrag), ice_pwd_(ice_pwd), - ice_tiebreaker_(ice_tiebreaker) { + tiebreaker_(0) { // Pooled sessions are allowed to be created with empty content name, // component, ufrag and password. RTC_DCHECK(ice_ufrag.empty() == ice_pwd.empty()); @@ -102,7 +101,7 @@ PortAllocator::PortAllocator() step_delay_(kDefaultStepDelay), allow_tcp_listen_(true), candidate_filter_(CF_ALL), - tiebreaker_(rtc::CreateRandomId64()) { + tiebreaker_(0) { // The allocator will be attached to a thread in Initialize. thread_checker_.Detach(); } @@ -198,6 +197,13 @@ 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, diff --git a/p2p/base/port_allocator.h b/p2p/base/port_allocator.h index e9630df143..b8cffca9c0 100644 --- a/p2p/base/port_allocator.h +++ b/p2p/base/port_allocator.h @@ -199,7 +199,6 @@ class RTC_EXPORT PortAllocatorSession : public sigslot::has_slots<> { int component, absl::string_view ice_ufrag, absl::string_view ice_pwd, - uint64_t ice_tiebreaker, uint32_t flags); // Subclasses should clean up any ports created. @@ -213,9 +212,9 @@ 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): remove once downstream code has been updated. - void set_ice_tiebreaker(uint64_t tiebreaker) { ice_tiebreaker_ = tiebreaker; } - uint64_t ice_tiebreaker() const { return ice_tiebreaker_; } + // 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", @@ -330,10 +329,12 @@ class RTC_EXPORT PortAllocatorSession : public sigslot::has_slots<> { int component_; std::string ice_ufrag_; std::string ice_pwd_; - uint64_t ice_tiebreaker_; 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; @@ -386,6 +387,9 @@ 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_; @@ -457,8 +461,6 @@ 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 2df8662f62..836a2fa494 100644 --- a/p2p/base/port_allocator_unittest.cc +++ b/p2p/base/port_allocator_unittest.cc @@ -26,6 +26,7 @@ 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: @@ -37,7 +38,9 @@ class PortAllocatorTest : public ::testing::Test, public sigslot::has_slots<> { allocator_(std::make_unique( rtc::Thread::Current(), packet_socket_factory_.get(), - &field_trials_)) {} + &field_trials_)) { + allocator_->SetIceTiebreaker(kTiebreakerDefault); + } protected: void SetConfigurationWithPoolSize(int candidate_pool_size) { diff --git a/p2p/base/regathering_controller_unittest.cc b/p2p/base/regathering_controller_unittest.cc index 573c0fd23f..91b7270f77 100644 --- a/p2p/base/regathering_controller_unittest.cc +++ b/p2p/base/regathering_controller_unittest.cc @@ -40,6 +40,7 @@ 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 @@ -58,6 +59,7 @@ 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 92c0ac37f6..e95033efeb 100644 --- a/p2p/client/basic_port_allocator.cc +++ b/p2p/client/basic_port_allocator.cc @@ -243,7 +243,7 @@ PortAllocatorSession* BasicPortAllocator::CreateSessionInternal( CheckRunOnValidThreadAndInitialized(); return new BasicPortAllocatorSession(this, std::string(content_name), component, std::string(ice_ufrag), - std::string(ice_pwd), ice_tiebreaker()); + std::string(ice_pwd)); } void BasicPortAllocator::AddTurnServerForTesting( @@ -261,13 +261,11 @@ BasicPortAllocatorSession::BasicPortAllocatorSession( absl::string_view content_name, int component, absl::string_view ice_ufrag, - absl::string_view ice_pwd, - uint64_t ice_tiebreaker) + absl::string_view ice_pwd) : PortAllocatorSession(content_name, component, ice_ufrag, ice_pwd, - ice_tiebreaker, allocator->flags()), allocator_(allocator), network_thread_(rtc::Thread::Current()), @@ -283,19 +281,6 @@ BasicPortAllocatorSession::BasicPortAllocatorSession( allocator_->network_manager()->StartUpdating(); } -BasicPortAllocatorSession::BasicPortAllocatorSession( - BasicPortAllocator* allocator, - absl::string_view content_name, - int component, - absl::string_view ice_ufrag, - absl::string_view ice_pwd) - : BasicPortAllocatorSession(allocator, - content_name, - component, - ice_ufrag, - ice_pwd, - 0) {} - BasicPortAllocatorSession::~BasicPortAllocatorSession() { TRACE_EVENT0("webrtc", "BasicPortAllocatorSession::~BasicPortAllocatorSession"); diff --git a/p2p/client/basic_port_allocator.h b/p2p/client/basic_port_allocator.h index 1612f77d35..25201fd016 100644 --- a/p2p/client/basic_port_allocator.h +++ b/p2p/client/basic_port_allocator.h @@ -116,13 +116,6 @@ enum class SessionState { // destroyed on the network thread. class RTC_EXPORT BasicPortAllocatorSession : public PortAllocatorSession { public: - BasicPortAllocatorSession(BasicPortAllocator* allocator, - absl::string_view content_name, - int component, - absl::string_view ice_ufrag, - absl::string_view ice_pwd, - uint64_t ice_tiebreaker); - // TODO(bugs.webrtc.org/14605): remove this constructor BasicPortAllocatorSession(BasicPortAllocator* allocator, absl::string_view content_name, int component, diff --git a/p2p/client/basic_port_allocator_unittest.cc b/p2p/client/basic_port_allocator_unittest.cc index f77040d128..77443eedbb 100644 --- a/p2p/client/basic_port_allocator_unittest.cc +++ b/p2p/client/basic_port_allocator_unittest.cc @@ -112,6 +112,8 @@ 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( @@ -174,6 +176,7 @@ 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(); } @@ -211,6 +214,7 @@ 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. @@ -295,6 +299,7 @@ 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 d5eb0c633d..9473d962ef 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -55,7 +55,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); @@ -63,6 +62,9 @@ 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 7f06c22969..448844ac79 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_; + uint64_t ice_tiebreaker_ = rtc::CreateRandomId64(); rtc::scoped_refptr certificate_; BundleManager bundles_; diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc index bd4848cf8b..492e108cbc 100644 --- a/pc/peer_connection_ice_unittest.cc +++ b/pc/peer_connection_ice_unittest.cc @@ -94,6 +94,7 @@ using ::testing::Values; constexpr int kIceCandidatesTimeout = 10000; constexpr int64_t kWaitTimeout = 10000; +constexpr uint64_t kTiebreakerDefault = 44444; class PeerConnectionWrapperForIceTest : public PeerConnectionWrapper { public: @@ -1447,6 +1448,7 @@ 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 71c85d4c6f..08fb1632d6 100644 --- a/pc/peer_connection_interface_unittest.cc +++ b/pc/peer_connection_interface_unittest.cc @@ -114,6 +114,7 @@ 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; @@ -710,6 +711,7 @@ 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.