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 <hta@webrtc.org>
> Commit-Queue: Philipp Hancke <phancke@microsoft.com>
> 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 <mbonadei@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#41720}

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_;

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);

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<webrtc::BasicRegatheringController> regathering_controller_
       RTC_GUARDED_BY(network_thread_);

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(),

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<PortAllocatorSession>(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<PortAllocatorSession> PortAllocator::CreateSession(
     absl::string_view content_name,
     int component,
@@ -213,7 +204,6 @@ std::unique_ptr<PortAllocatorSession> PortAllocator::CreateSession(
   auto session = std::unique_ptr<PortAllocatorSession>(
       CreateSessionInternal(content_name, component, ice_ufrag, ice_pwd));
   session->SetCandidateFilter(candidate_filter());
-  session->set_ice_tiebreaker(tiebreaker_);
   return session;
 }
 

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<int>&
                             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_;

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<cricket::FakePortAllocator>(
             rtc::Thread::Current(),
             packet_socket_factory_.get(),
-            &field_trials_)) {
-    allocator_->SetIceTiebreaker(kTiebreakerDefault);
-  }
+            &field_trials_)) {}
 
  protected:
   void SetConfigurationWithPoolSize(int candidate_pool_size) {

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(

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);
   }
 }

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;
   }
 

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() {

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<rtc::RTCCertificate> certificate_;
 
   BundleManager bundles_;

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(

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<rtc::BasicPacketSocketFactory>(vss_.get()),
             &field_trials_));
     port_allocator_ = port_allocator.get();
-    port_allocator_->SetIceTiebreaker(kTiebreakerDefault);
 
     // Create certificate generator unless DTLS constraint is explicitly set to
     // false.