Delete SignalQueueDestroyed

It was used only to break the circular dependency between SocketServer
and Thread at destruction time. Replaced with a method call to
SetMessageQueue(nullptr).

Bug: webrtc:11943
Change-Id: I0606d473ad79655cca28411bb02c21e21d2d7220
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215587
Reviewed-by: Tommi <tommi@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33807}

rtc_base/nat_socket_factory.cc

@@ -428,14 +428,15 @@ NATSocketServer::Translator::Translator(NATSocketServer* server,
   // Create a new private network, and a NATServer running on the private
   // network that bridges to the external network. Also tell the private
   // network to use the same message queue as us.
-  VirtualSocketServer* internal_server = new VirtualSocketServer();
-  internal_server->SetMessageQueue(server_->queue());
-  internal_factory_.reset(internal_server);
-  nat_server_.reset(new NATServer(type, internal_server, int_ip, int_ip,
-                                  ext_factory, ext_ip));
+  internal_server_ = std::make_unique<VirtualSocketServer>();
+  internal_server_->SetMessageQueue(server_->queue());
+  nat_server_ = std::make_unique<NATServer>(
+      type, internal_server_.get(), int_ip, int_ip, ext_factory, ext_ip);
 }
 
-NATSocketServer::Translator::~Translator() = default;
+NATSocketServer::Translator::~Translator() {
+  internal_server_->SetMessageQueue(nullptr);
+}
 
 NATSocketServer::Translator* NATSocketServer::Translator::GetTranslator(
     const SocketAddress& ext_ip) {

rtc_base/nat_socket_factory.h

@@ -107,7 +107,7 @@ class NATSocketServer : public SocketServer, public NATInternalSocketFactory {
                const SocketAddress& ext_addr);
     ~Translator();
 
-    SocketFactory* internal_factory() { return internal_factory_.get(); }
+    SocketFactory* internal_factory() { return internal_server_.get(); }
     SocketAddress internal_udp_address() const {
       return nat_server_->internal_udp_address();
     }
@@ -129,7 +129,7 @@ class NATSocketServer : public SocketServer, public NATInternalSocketFactory {
 
    private:
     NATSocketServer* server_;
-    std::unique_ptr<SocketFactory> internal_factory_;
+    std::unique_ptr<SocketServer> internal_server_;
     std::unique_ptr<NATServer> nat_server_;
     TranslatorMap nats_;
     std::set<SocketAddress> clients_;

rtc_base/socket_server.h

@@ -33,9 +33,10 @@ class SocketServer : public SocketFactory {
   static const int kForever = -1;
 
   static std::unique_ptr<SocketServer> CreateDefault();
-  // When the socket server is installed into a Thread, this function is
-  // called to allow the socket server to use the thread's message queue for
-  // any messaging that it might need to perform.
+  // When the socket server is installed into a Thread, this function is called
+  // to allow the socket server to use the thread's message queue for any
+  // messaging that it might need to perform. It is also called with a null
+  // argument before the thread is destroyed.
   virtual void SetMessageQueue(Thread* queue) {}
 
   // Sleeps until:

rtc_base/thread.cc

@@ -429,13 +429,11 @@ void Thread::DoDestroy() {
   // The signal is done from here to ensure
   // that it always gets called when the queue
   // is going away.
-  SignalQueueDestroyed();
-  ThreadManager::Remove(this);
-  ClearInternal(nullptr, MQID_ANY, nullptr);
-
   if (ss_) {
     ss_->SetMessageQueue(nullptr);
   }
+  ThreadManager::Remove(this);
+  ClearInternal(nullptr, MQID_ANY, nullptr);
 }
 
 SocketServer* Thread::socketserver() {

rtc_base/thread.h

@@ -336,10 +336,6 @@ class RTC_LOCKABLE RTC_EXPORT Thread : public webrtc::TaskQueueBase {
     }
   }
 
-  // When this signal is sent out, any references to this queue should
-  // no longer be used.
-  sigslot::signal0<> SignalQueueDestroyed;
-
   bool IsCurrent() const;
 
   // Sleeps the calling thread for the specified number of milliseconds, during

rtc_base/thread_unittest.cc

@@ -553,36 +553,6 @@ TEST(ThreadTest, ThreeThreadsInvoke) {
   EXPECT_TRUE_WAIT(thread_a_called.Get(), 2000);
 }
 
-// Set the name on a thread when the underlying QueueDestroyed signal is
-// triggered. This causes an error if the object is already partially
-// destroyed.
-class SetNameOnSignalQueueDestroyedTester : public sigslot::has_slots<> {
- public:
-  SetNameOnSignalQueueDestroyedTester(Thread* thread) : thread_(thread) {
-    thread->SignalQueueDestroyed.connect(
-        this, &SetNameOnSignalQueueDestroyedTester::OnQueueDestroyed);
-  }
-
-  void OnQueueDestroyed() {
-    // Makes sure that if we access the Thread while it's being destroyed, that
-    // it doesn't cause a problem because the vtable has been modified.
-    thread_->SetName("foo", nullptr);
-  }
-
- private:
-  Thread* thread_;
-};
-
-TEST(ThreadTest, SetNameOnSignalQueueDestroyed) {
-  auto thread1 = Thread::CreateWithSocketServer();
-  SetNameOnSignalQueueDestroyedTester tester1(thread1.get());
-  thread1.reset();
-
-  Thread* thread2 = new AutoThread();
-  SetNameOnSignalQueueDestroyedTester tester2(thread2);
-  delete thread2;
-}
-
 class ThreadQueueTest : public ::testing::Test, public Thread {
  public:
   ThreadQueueTest() : Thread(CreateDefaultSocketServer(), true) {}

rtc_base/virtual_socket_server.cc

@@ -613,10 +613,6 @@ VirtualSocket* VirtualSocketServer::CreateSocketInternal(int family, int type) {
 
 void VirtualSocketServer::SetMessageQueue(Thread* msg_queue) {
   msg_queue_ = msg_queue;
-  if (msg_queue_) {
-    msg_queue_->SignalQueueDestroyed.connect(
-        this, &VirtualSocketServer::OnMessageQueueDestroyed);
-  }
 }
 
 bool VirtualSocketServer::Wait(int cmsWait, bool process_io) {

rtc_base/virtual_socket_server.h

@@ -33,7 +33,7 @@ class SocketAddressPair;
 // interface can create as many addresses as you want.  All of the sockets
 // created by this network will be able to communicate with one another, unless
 // they are bound to addresses from incompatible families.
-class VirtualSocketServer : public SocketServer, public sigslot::has_slots<> {
+class VirtualSocketServer : public SocketServer {
  public:
   VirtualSocketServer();
   // This constructor needs to be used if the test uses a fake clock and
@@ -259,11 +259,6 @@ class VirtualSocketServer : public SocketServer, public sigslot::has_slots<> {
                                             uint32_t samples);
   static double Evaluate(const Function* f, double x);
 
-  // Null out our message queue if it goes away. Necessary in the case where
-  // our lifetime is greater than that of the thread we are using, since we
-  // try to send Close messages for all connected sockets when we shutdown.
-  void OnMessageQueueDestroyed() { msg_queue_ = nullptr; }
-
   // Determine if two sockets should be able to communicate.
   // We don't (currently) specify an address family for sockets; instead,
   // the currently bound address is used to infer the address family.

test/network/BUILD.gn

@@ -70,7 +70,6 @@ rtc_library("emulated_network") {
     "../../rtc_base/task_utils:pending_task_safety_flag",
     "../../rtc_base/task_utils:repeating_task",
     "../../rtc_base/task_utils:to_queued_task",
-    "../../rtc_base/third_party/sigslot",
     "../../system_wrappers",
     "../scenario:column_printer",
     "../time_controller",

test/network/fake_network_socket_server.cc

@@ -276,10 +276,6 @@ FakeNetworkSocketServer::FakeNetworkSocketServer(
       wakeup_(/*manual_reset=*/false, /*initially_signaled=*/false) {}
 FakeNetworkSocketServer::~FakeNetworkSocketServer() = default;
 
-void FakeNetworkSocketServer::OnMessageQueueDestroyed() {
-  thread_ = nullptr;
-}
-
 EmulatedEndpointImpl* FakeNetworkSocketServer::GetEndpointNode(
     const rtc::IPAddress& ip) {
   return endpoints_container_->LookupByLocalAddress(ip);
@@ -311,10 +307,6 @@ rtc::AsyncSocket* FakeNetworkSocketServer::CreateAsyncSocket(int family,
 
 void FakeNetworkSocketServer::SetMessageQueue(rtc::Thread* thread) {
   thread_ = thread;
-  if (thread_) {
-    thread_->SignalQueueDestroyed.connect(
-        this, &FakeNetworkSocketServer::OnMessageQueueDestroyed);
-  }
 }
 
 // Always returns true (if return false, it won't be invoked again...)

test/network/fake_network_socket_server.h

@@ -19,7 +19,6 @@
 #include "rtc_base/event.h"
 #include "rtc_base/socket_server.h"
 #include "rtc_base/synchronization/mutex.h"
-#include "rtc_base/third_party/sigslot/sigslot.h"
 #include "system_wrappers/include/clock.h"
 #include "test/network/network_emulation.h"
 
@@ -28,8 +27,7 @@ namespace test {
 class FakeNetworkSocket;
 
 // FakeNetworkSocketServer must outlive any sockets it creates.
-class FakeNetworkSocketServer : public rtc::SocketServer,
-                                public sigslot::has_slots<> {
+class FakeNetworkSocketServer : public rtc::SocketServer {
  public:
   explicit FakeNetworkSocketServer(EndpointsContainer* endpoints_controller);
   ~FakeNetworkSocketServer() override;
@@ -52,8 +50,6 @@ class FakeNetworkSocketServer : public rtc::SocketServer,
   void Unregister(FakeNetworkSocket* socket);
 
  private:
-  void OnMessageQueueDestroyed();
-
   const EndpointsContainer* endpoints_container_;
   rtc::Event wakeup_;
   rtc::Thread* thread_ = nullptr;