Revert "move relay server priority assignment to port_allocator"

This reverts commit b395f5bd5c.

Reason for revert: Breaks downstream project. Jonas will help to reland this CL.

Original change's description:
> move relay server priority assignment to port_allocator
>
> which knows more about the internals of ICE.
> Remove the relay server config priority field which was used to
> specify the relative priority of TURN servers. This is now handled
> internally by CreateRelayPortArgs without being exposed.
>
> Also rename BasicPortAllocator::AddTurnServer to
> BasicPortAllocator::AddTurnServerForTesting since it is a test-only
> method.
>
> BUG=webrtc:13195,webrtc:14539
>
> Change-Id: Id36cbf0187b7a84d1a9b53860f31994f3c7589f0
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/280224
> Commit-Queue: Philipp Hancke <phancke@microsoft.com>
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Reviewed-by: Jonas Oreland <jonaso@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#38520}

Bug: webrtc:13195,webrtc:14539
Change-Id: I7ca087a272793908f003cea6c32efe6214e54028
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/281340
Owners-Override: Artem Titov <titovartem@webrtc.org>
Commit-Queue: Artem Titov <titovartem@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#38524}

p2p/base/p2p_transport_channel_unittest.cc

@@ -2737,8 +2737,8 @@ TEST_P(P2PTransportChannelMultihomedTest, TestFailoverWithManyConnections) {
   RelayServerConfig turn_server;
   turn_server.credentials = kRelayCredentials;
   turn_server.ports.push_back(ProtocolAddress(kTurnTcpIntAddr, PROTO_TCP));
-  GetAllocator(0)->AddTurnServerForTesting(turn_server);
-  GetAllocator(1)->AddTurnServerForTesting(turn_server);
+  GetAllocator(0)->AddTurnServer(turn_server);
+  GetAllocator(1)->AddTurnServer(turn_server);
   // Enable IPv6
   SetAllocatorFlags(
       0, PORTALLOCATOR_ENABLE_IPV6 | PORTALLOCATOR_ENABLE_IPV6_ON_WIFI);
@@ -5238,7 +5238,7 @@ TEST_P(P2PTransportChannelMostLikelyToWorkFirstTest, TestTcpTurn) {
   RelayServerConfig config;
   config.credentials = kRelayCredentials;
   config.ports.push_back(ProtocolAddress(kTurnTcpIntAddr, PROTO_TCP));
-  allocator()->AddTurnServerForTesting(config);
+  allocator()->AddTurnServer(config);
 
   P2PTransportChannel& ch = StartTransportChannel(true, 500, &field_trials_);
   EXPECT_TRUE_WAIT(ch.ports().size() == 3, kDefaultTimeout);

p2p/base/port_allocator.h

@@ -171,12 +171,14 @@ struct RTC_EXPORT RelayServerConfig {
   ~RelayServerConfig();
 
   bool operator==(const RelayServerConfig& o) const {
-    return ports == o.ports && credentials == o.credentials;
+    return ports == o.ports && credentials == o.credentials &&
+           priority == o.priority;
   }
   bool operator!=(const RelayServerConfig& o) const { return !(*this == o); }
 
   PortList ports;
   RelayCredentials credentials;
+  int priority = 0;
   TlsCertPolicy tls_cert_policy = TlsCertPolicy::TLS_CERT_POLICY_SECURE;
   std::vector<std::string> tls_alpn_protocols;
   std::vector<std::string> tls_elliptic_curves;

p2p/base/turn_port.h

@@ -81,7 +81,7 @@ class TurnPort : public Port {
     return absl::WrapUnique(
         new TurnPort(args.network_thread, args.socket_factory, args.network,
                      socket, args.username, args.password, *args.server_address,
-                     args.config->credentials, args.relative_priority,
+                     args.config->credentials, args.config->priority,
                      args.config->tls_alpn_protocols,
                      args.config->tls_elliptic_curves, args.turn_customizer,
                      args.config->tls_cert_verifier, args.field_trials));
@@ -100,7 +100,7 @@ class TurnPort : public Port {
         new TurnPort(args.network_thread, args.socket_factory, args.network,
                      min_port, max_port, args.username, args.password,
                      *args.server_address, args.config->credentials,
-                     args.relative_priority, args.config->tls_alpn_protocols,
+                     args.config->priority, args.config->tls_alpn_protocols,
                      args.config->tls_elliptic_curves, args.turn_customizer,
                      args.config->tls_cert_verifier, args.field_trials));
   }

p2p/client/basic_port_allocator.cc

@@ -298,8 +298,7 @@ PortAllocatorSession* BasicPortAllocator::CreateSessionInternal(
   return session;
 }
 
-void BasicPortAllocator::AddTurnServerForTesting(
-    const RelayServerConfig& turn_server) {
+void BasicPortAllocator::AddTurnServer(const RelayServerConfig& turn_server) {
   CheckRunOnValidThreadAndInitialized();
   std::vector<RelayServerConfig> new_turn_servers = turn_servers();
   new_turn_servers.push_back(turn_server);
@@ -1648,17 +1647,12 @@ void AllocationSequence::CreateRelayPorts() {
     return;
   }
 
-  // Relative priority of candidates from this TURN server in relation
-  // to the candidates from other servers. Required because ICE priorities
-  // need to be unique.
-  int relative_priority = config_->relays.size();
   for (RelayServerConfig& relay : config_->relays) {
-    CreateTurnPort(relay, relative_priority--);
+    CreateTurnPort(relay);
   }
 }
 
-void AllocationSequence::CreateTurnPort(const RelayServerConfig& config,
-                                        int relative_priority) {
+void AllocationSequence::CreateTurnPort(const RelayServerConfig& config) {
   PortList::const_iterator relay_port;
   for (relay_port = config.ports.begin(); relay_port != config.ports.end();
        ++relay_port) {
@@ -1691,7 +1685,6 @@ void AllocationSequence::CreateTurnPort(const RelayServerConfig& config,
     args.config = &config;
     args.turn_customizer = session_->allocator()->turn_customizer();
     args.field_trials = session_->allocator()->field_trials();
-    args.relative_priority = relative_priority;
 
     std::unique_ptr<cricket::Port> port;
     // Shared socket mode must be enabled only for UDP based ports. Hence

p2p/client/basic_port_allocator.h

@@ -79,7 +79,7 @@ class RTC_EXPORT BasicPortAllocator : public PortAllocator {
       absl::string_view ice_pwd) override;
 
   // Convenience method that adds a TURN server to the configuration.
-  void AddTurnServerForTesting(const RelayServerConfig& turn_server);
+  void AddTurnServer(const RelayServerConfig& turn_server);
 
   RelayPortFactoryInterface* relay_port_factory() {
     CheckRunOnValidThreadIfInitialized();
@@ -386,9 +386,11 @@ class AllocationSequence : public sigslot::has_slots<> {
   void Start();
   void Stop();
 
- private:
-  void CreateTurnPort(const RelayServerConfig& config, int relative_priority);
+ protected:
+  // For testing.
+  void CreateTurnPort(const RelayServerConfig& config);
 
+ private:
   typedef std::vector<ProtocolType> ProtocolList;
 
   void Process(int epoch);

p2p/client/basic_port_allocator_unittest.cc

@@ -251,7 +251,7 @@ class BasicPortAllocatorTestBase : public ::testing::Test,
   void AddTurnServers(const rtc::SocketAddress& udp_turn,
                       const rtc::SocketAddress& tcp_turn) {
     RelayServerConfig turn_server = CreateTurnServers(udp_turn, tcp_turn);
-    allocator_->AddTurnServerForTesting(turn_server);
+    allocator_->AddTurnServer(turn_server);
   }
 
   bool CreateSession(int component) {
@@ -1818,7 +1818,7 @@ TEST_F(BasicPortAllocatorTestWithRealClock,
   turn_server.credentials = credentials;
   turn_server.ports.push_back(
       ProtocolAddress(rtc::SocketAddress("localhost", 3478), PROTO_UDP));
-  allocator_->AddTurnServerForTesting(turn_server);
+  allocator_->AddTurnServer(turn_server);
 
   allocator_->set_step_delay(kMinimumStepDelay);
   allocator_->set_flags(allocator().flags() |
@@ -2496,29 +2496,6 @@ TEST_F(BasicPortAllocatorTest, TestDoNotUseTurnServerAsStunSever) {
   EXPECT_EQ(1U, port_config.StunServers().size());
 }
 
-// Test that candidates from different servers get assigned a unique local
-// preference (the middle 16 bits of the priority)
-TEST_F(BasicPortAllocatorTest, AssignsUniqueLocalPreferencetoRelayCandidates) {
-  allocator_->SetCandidateFilter(CF_RELAY);
-  allocator_->AddTurnServerForTesting(
-      CreateTurnServers(kTurnUdpIntAddr, SocketAddress()));
-  allocator_->AddTurnServerForTesting(
-      CreateTurnServers(kTurnUdpIntAddr, SocketAddress()));
-  allocator_->AddTurnServerForTesting(
-      CreateTurnServers(kTurnUdpIntAddr, SocketAddress()));
-
-  AddInterface(kClientAddr);
-  ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP));
-  session_->StartGettingPorts();
-  ASSERT_TRUE_SIMULATED_WAIT(candidate_allocation_done_,
-                             kDefaultAllocationTimeout, fake_clock);
-  EXPECT_EQ(3u, candidates_.size());
-  EXPECT_GT((candidates_[0].priority() >> 8) & 0xFFFF,
-            (candidates_[1].priority() >> 8) & 0xFFFF);
-  EXPECT_GT((candidates_[1].priority() >> 8) & 0xFFFF,
-            (candidates_[2].priority() >> 8) & 0xFFFF);
-}
-
 // Test that no more than allocator.max_ipv6_networks() IPv6 networks are used
 // to gather candidates.
 TEST_F(BasicPortAllocatorTest, TwoIPv6AreSelectedBecauseOfMaxIpv6Limit) {

p2p/client/relay_port_factory_interface.h

@@ -45,10 +45,6 @@ struct CreateRelayPortArgs {
   std::string password;
   webrtc::TurnCustomizer* turn_customizer = nullptr;
   const webrtc::FieldTrialsView* field_trials = nullptr;
-  // Relative priority of candidates from this TURN server in relation
-  // to the candidates from other servers. Required because ICE priorities
-  // need to be unique.
-  int relative_priority = 0;
 };
 
 // A factory for creating RelayPort's.

pc/ice_server_parsing.cc

@@ -338,6 +338,13 @@ RTCError ParseIceServersOrError(
                            "ICE server parsing failed: Empty uri.");
     }
   }
+  // Candidates must have unique priorities, so that connectivity checks
+  // are performed in a well-defined order.
+  int priority = static_cast<int>(turn_servers->size() - 1);
+  for (cricket::RelayServerConfig& turn_server : *turn_servers) {
+    // First in the list gets highest priority.
+    turn_server.priority = priority--;
+  }
   return RTCError::OK();
 }
 

pc/ice_server_parsing_unittest.cc

@@ -237,4 +237,22 @@ TEST_F(IceServerParsingTest, ParseMultipleUrls) {
   EXPECT_EQ(1U, turn_servers_.size());
 }
 
+// Ensure that TURN servers are given unique priorities,
+// so that their resulting candidates have unique priorities.
+TEST_F(IceServerParsingTest, TurnServerPrioritiesUnique) {
+  PeerConnectionInterface::IceServers servers;
+  PeerConnectionInterface::IceServer server;
+  server.urls.push_back("turn:hostname");
+  server.urls.push_back("turn:hostname2");
+  server.username = "foo";
+  server.password = "bar";
+  servers.push_back(server);
+
+  EXPECT_TRUE(
+      webrtc::ParseIceServersOrError(servers, &stun_servers_, &turn_servers_)
+          .ok());
+  EXPECT_EQ(2U, turn_servers_.size());
+  EXPECT_NE(turn_servers_[0].priority, turn_servers_[1].priority);
+}
+
 }  // namespace webrtc