Delete BasicPacketSocketFactory constructor with thread argument

In callers where it's non-trivial to explicitly pass the right
SocketFactory, pull the call to rtc::Thread::socketserver() into the
caller, with a TODO comment.

Bug: webrtc:13145
Change-Id: I029d3adca385d822180e089f016c3778e0d4fd0c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/231227
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35063}

p2p/base/basic_packet_socket_factory.cc

@@ -25,16 +25,12 @@
 #include "rtc_base/socket_adapters.h"
 #include "rtc_base/socket_server.h"
 #include "rtc_base/ssl_adapter.h"
-#include "rtc_base/thread.h"
 
 namespace rtc {
 
-BasicPacketSocketFactory::BasicPacketSocketFactory(Thread* thread)
-    : thread_(thread), socket_factory_(NULL) {}
-
 BasicPacketSocketFactory::BasicPacketSocketFactory(
     SocketFactory* socket_factory)
-    : thread_(NULL), socket_factory_(socket_factory) {}
+    : socket_factory_(socket_factory) {}
 
 BasicPacketSocketFactory::~BasicPacketSocketFactory() {}
 
@@ -43,7 +39,7 @@ AsyncPacketSocket* BasicPacketSocketFactory::CreateUdpSocket(
     uint16_t min_port,
     uint16_t max_port) {
   // UDP sockets are simple.
-  Socket* socket = socket_factory()->CreateSocket(address.family(), SOCK_DGRAM);
+  Socket* socket = socket_factory_->CreateSocket(address.family(), SOCK_DGRAM);
   if (!socket) {
     return NULL;
   }
@@ -67,7 +63,7 @@ AsyncPacketSocket* BasicPacketSocketFactory::CreateServerTcpSocket(
   }
 
   Socket* socket =
-      socket_factory()->CreateSocket(local_address.family(), SOCK_STREAM);
+      socket_factory_->CreateSocket(local_address.family(), SOCK_STREAM);
   if (!socket) {
     return NULL;
   }
@@ -104,7 +100,7 @@ AsyncPacketSocket* BasicPacketSocketFactory::CreateClientTcpSocket(
     const std::string& user_agent,
     const PacketSocketTcpOptions& tcp_options) {
   Socket* socket =
-      socket_factory()->CreateSocket(local_address.family(), SOCK_STREAM);
+      socket_factory_->CreateSocket(local_address.family(), SOCK_STREAM);
   if (!socket) {
     return NULL;
   }
@@ -215,13 +211,4 @@ int BasicPacketSocketFactory::BindSocket(Socket* socket,
   return ret;
 }
 
-SocketFactory* BasicPacketSocketFactory::socket_factory() {
-  if (thread_) {
-    RTC_DCHECK(thread_ == Thread::Current());
-    return thread_->socketserver();
-  } else {
-    return socket_factory_;
-  }
-}
-
 }  // namespace rtc

p2p/base/basic_packet_socket_factory.h

@@ -19,11 +19,9 @@
 namespace rtc {
 
 class SocketFactory;
-class Thread;
 
 class BasicPacketSocketFactory : public PacketSocketFactory {
  public:
-  explicit BasicPacketSocketFactory(Thread* thread);
   explicit BasicPacketSocketFactory(SocketFactory* socket_factory);
   ~BasicPacketSocketFactory() override;
 
@@ -49,9 +47,6 @@ class BasicPacketSocketFactory : public PacketSocketFactory {
                  uint16_t min_port,
                  uint16_t max_port);
 
-  SocketFactory* socket_factory();
-
-  Thread* thread_;
   SocketFactory* socket_factory_;
 };
 

p2p/base/fake_port_allocator.h

@@ -208,11 +208,13 @@ class FakePortAllocatorSession : public PortAllocatorSession {
 
 class FakePortAllocator : public cricket::PortAllocator {
  public:
+  // TODO(bugs.webrtc.org/13145): Require non-null `factory`.
   FakePortAllocator(rtc::Thread* network_thread,
                     rtc::PacketSocketFactory* factory)
       : network_thread_(network_thread), factory_(factory) {
     if (factory_ == NULL) {
-      owned_factory_.reset(new rtc::BasicPacketSocketFactory(network_thread_));
+      owned_factory_.reset(new rtc::BasicPacketSocketFactory(
+          network_thread_ ? network_thread_->socketserver() : nullptr));
       factory_ = owned_factory_.get();
     }
 

p2p/base/port_unittest.cc

@@ -395,7 +395,7 @@ class PortTest : public ::testing::Test, public sigslot::has_slots<> {
   PortTest()
       : ss_(new rtc::VirtualSocketServer()),
         main_(ss_.get()),
-        socket_factory_(rtc::Thread::Current()),
+        socket_factory_(ss_.get()),
         nat_factory1_(ss_.get(), kNatAddr1, SocketAddress()),
         nat_factory2_(ss_.get(), kNatAddr2, SocketAddress()),
         nat_socket_factory1_(&nat_factory1_),

p2p/base/stun_port_unittest.cc

@@ -48,7 +48,7 @@ class StunPortTestBase : public ::testing::Test, public sigslot::has_slots<> {
       : ss_(new rtc::VirtualSocketServer()),
         thread_(ss_.get()),
         network_("unittest", "unittest", kLocalAddr.ipaddr(), 32),
-        socket_factory_(rtc::Thread::Current()),
+        socket_factory_(ss_.get()),
         stun_server_1_(cricket::TestStunServer::Create(ss_.get(), kStunAddr1)),
         stun_server_2_(cricket::TestStunServer::Create(ss_.get(), kStunAddr2)),
         done_(false),

p2p/base/tcp_port_unittest.cc

@@ -61,7 +61,7 @@ class TCPPortTest : public ::testing::Test, public sigslot::has_slots<> {
   TCPPortTest()
       : ss_(new rtc::VirtualSocketServer()),
         main_(ss_.get()),
-        socket_factory_(rtc::Thread::Current()),
+        socket_factory_(ss_.get()),
         username_(rtc::CreateRandomString(ICE_UFRAG_LENGTH)),
         password_(rtc::CreateRandomString(ICE_PWD_LENGTH)) {}
 

p2p/base/test_turn_server.h

@@ -58,7 +58,10 @@ class TestTurnServer : public TurnAuthInterface {
                  const std::string& common_name = "test turn server")
       : server_(thread), thread_(thread) {
     AddInternalSocket(int_addr, int_protocol, ignore_bad_cert, common_name);
-    server_.SetExternalSocketFactory(new rtc::BasicPacketSocketFactory(thread),
+    // TODO(bugs.webrtc.org/13145): Take a SocketFactory as argument, so we
+    // don't need thread_->socketserver().
+    server_.SetExternalSocketFactory(
+        new rtc::BasicPacketSocketFactory(thread_->socketserver()),
         udp_ext_addr);
     server_.set_realm(kTestRealm);
     server_.set_software(kTestSoftware);

p2p/base/turn_port_unittest.cc

@@ -172,7 +172,7 @@ class TurnPortTest : public ::testing::Test,
   TurnPortTest()
       : ss_(new TurnPortTestVirtualSocketServer()),
         main_(ss_.get()),
-        socket_factory_(rtc::Thread::Current()),
+        socket_factory_(ss_.get()),
         turn_server_(&main_, kTurnUdpIntAddr, kTurnUdpExtAddr),
         turn_ready_(false),
         turn_error_(false),

p2p/client/basic_port_allocator.cc

@@ -378,7 +378,7 @@ void BasicPortAllocatorSession::StartGettingPorts() {
   state_ = SessionState::GATHERING;
   if (!socket_factory_) {
     owned_socket_factory_.reset(
-        new rtc::BasicPacketSocketFactory(network_thread_));
+        new rtc::BasicPacketSocketFactory(network_thread_->socketserver()));
     socket_factory_ = owned_socket_factory_.get();
   }
 

pc/connection_context.cc

@@ -121,8 +121,12 @@ ConnectionContext::ConnectionContext(
   default_network_manager_ = std::make_unique<rtc::BasicNetworkManager>(
       network_monitor_factory_.get());
 
-  default_socket_factory_ =
+  // TODO(bugs.webrtc.org/13145): Either require that a PacketSocketFactory
-      std::make_unique<rtc::BasicPacketSocketFactory>(network_thread());
+  // always is injected (with no need to construct this default factory), or get
+  // the appropriate underlying SocketFactory without going through the
+  // rtc::Thread::socketserver() accessor.
+  default_socket_factory_ = std::make_unique<rtc::BasicPacketSocketFactory>(
+      network_thread()->socketserver());
 
   worker_thread_->Invoke<void>(RTC_FROM_HERE, [&]() {
     channel_manager_ = cricket::ChannelManager::Create(

rtc_tools/network_tester/test_controller.cc

@@ -23,7 +23,9 @@ TestController::TestController(int min_port,
                                int max_port,
                                const std::string& config_file_path,
                                const std::string& log_file_path)
-    : socket_factory_(rtc::ThreadManager::Instance()->WrapCurrentThread()),
+    // TODO(bugs.webrtc.org/13145): Add a SocketFactory argument.
+    : socket_factory_(
+          rtc::ThreadManager::Instance()->WrapCurrentThread()->socketserver()),
       config_file_path_(config_file_path),
       packet_logger_(log_file_path),
       local_test_done_(false),