Remove sigslot signals from TurnPort

Bug: webrtc:11943
Change-Id: If07749a4fa47bd06a2a11be9d334a4a39a1026b1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272651
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Commit-Queue: Fredrik Solenberg <solenberg@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37998}

p2p/base/turn_port.cc

@@ -104,41 +104,38 @@ class TurnRefreshRequest : public StunRequest {
   TurnPort* port_;
 };
 
-class TurnCreatePermissionRequest : public StunRequest,
+class TurnCreatePermissionRequest : public StunRequest {
-                                    public sigslot::has_slots<> {
  public:
   TurnCreatePermissionRequest(TurnPort* port,
                               TurnEntry* entry,
                               const rtc::SocketAddress& ext_addr,
                               absl::string_view remote_ufrag);
+  ~TurnCreatePermissionRequest() override;
   void OnSent() override;
   void OnResponse(StunMessage* response) override;
   void OnErrorResponse(StunMessage* response) override;
   void OnTimeout() override;
 
  private:
-  void OnEntryDestroyed(TurnEntry* entry);
-
   TurnPort* port_;
   TurnEntry* entry_;
   rtc::SocketAddress ext_addr_;
   std::string remote_ufrag_;
 };
 
-class TurnChannelBindRequest : public StunRequest, public sigslot::has_slots<> {
+class TurnChannelBindRequest : public StunRequest {
  public:
   TurnChannelBindRequest(TurnPort* port,
                          TurnEntry* entry,
                          int channel_id,
                          const rtc::SocketAddress& ext_addr);
+  ~TurnChannelBindRequest() override;
   void OnSent() override;
   void OnResponse(StunMessage* response) override;
   void OnErrorResponse(StunMessage* response) override;
   void OnTimeout() override;
 
  private:
-  void OnEntryDestroyed(TurnEntry* entry);
-
   TurnPort* port_;
   TurnEntry* entry_;
   int channel_id_;
@@ -191,7 +188,7 @@ class TurnEntry : public sigslot::has_slots<> {
   void OnChannelBindError(StunMessage* response, int code);
   void OnChannelBindTimeout();
   // Signal sent when TurnEntry is destroyed.
-  sigslot::signal1<TurnEntry*> SignalDestroyed;
+  webrtc::CallbackList<TurnEntry*> destroyed_callback_list_;
 
   const std::string& get_remote_ufrag() const { return remote_ufrag_; }
   void set_remote_ufrag(absl::string_view remote_ufrag) {
@@ -843,10 +840,6 @@ void TurnPort::ResolveTurnAddress(const rtc::SocketAddress& address) {
                       "TURN host lookup received error.");
       return;
     }
-    // Signal needs both resolved and unresolved address. After signal is sent
-    // we can copy resolved address back into `server_address_`.
-    SignalResolvedServerAddress(this, server_address_.address,
-                                resolved_address);
     server_address_.address = resolved_address;
     PrepareAddress();
   });
@@ -947,8 +940,9 @@ void TurnPort::Close() {
   state_ = STATE_DISCONNECTED;
   // Delete all existing connections; stop sending data.
   DestroyAllConnections();
-
+  if (callbacks_for_test_) {
-  SignalTurnPortClosed(this);
+    callbacks_for_test_->OnTurnPortClosed();
+  }
 }
 
 rtc::DiffServCodePoint TurnPort::StunDscpValue() const {
@@ -1252,7 +1246,7 @@ bool TurnPort::CreateOrRefreshEntry(const rtc::SocketAddress& addr,
 
 void TurnPort::DestroyEntry(TurnEntry* entry) {
   RTC_DCHECK(entry != NULL);
-  entry->SignalDestroyed(entry);
+  entry->destroyed_callback_list_.Send(entry);
   entries_.remove(entry);
   delete entry;
 }
@@ -1288,6 +1282,11 @@ void TurnPort::HandleConnectionDestroyed(Connection* conn) {
                             kTurnPermissionTimeout);
 }
 
+void TurnPort::SetCallbacksForTest(CallbacksForTest* callbacks) {
+  RTC_DCHECK(!callbacks_for_test_);
+  callbacks_for_test_ = callbacks;
+}
+
 bool TurnPort::SetEntryChannelId(const rtc::SocketAddress& address,
                                  int channel_id) {
   TurnEntry* entry = FindEntry(address);
@@ -1595,7 +1594,9 @@ void TurnRefreshRequest::OnResponse(StunMessage* response) {
         SafeTask(port->task_safety_.flag(), [port] { port->Close(); }));
   }
 
-  port_->SignalTurnRefreshResult(port_, TURN_SUCCESS_RESULT_CODE);
+  if (port_->callbacks_for_test_) {
+    port_->callbacks_for_test_->OnTurnRefreshResult(TURN_SUCCESS_RESULT_CODE);
+  }
 }
 
 void TurnRefreshRequest::OnErrorResponse(StunMessage* response) {
@@ -1612,7 +1613,9 @@ void TurnRefreshRequest::OnErrorResponse(StunMessage* response) {
                         << rtc::hex_encode(id()) << ", code=" << error_code
                         << ", rtt=" << Elapsed();
     port_->OnRefreshError();
-    port_->SignalTurnRefreshResult(port_, error_code);
+    if (port_->callbacks_for_test_) {
+      port_->callbacks_for_test_->OnTurnRefreshResult(error_code);
+    }
   }
 }
 
@@ -1634,8 +1637,11 @@ TurnCreatePermissionRequest::TurnCreatePermissionRequest(
       entry_(entry),
       ext_addr_(ext_addr),
       remote_ufrag_(remote_ufrag) {
-  entry_->SignalDestroyed.connect(
+  RTC_DCHECK(entry_);
-      this, &TurnCreatePermissionRequest::OnEntryDestroyed);
+  entry_->destroyed_callback_list_.AddReceiver(this, [this](TurnEntry* entry) {
+    RTC_DCHECK(entry_ == entry);
+    entry_ = nullptr;
+  });
   StunMessage* message = mutable_msg();
   // Create the request as indicated in RFC5766, Section 9.1.
   RTC_DCHECK_EQ(message->type(), TURN_CREATE_PERMISSION_REQUEST);
@@ -1649,6 +1655,12 @@ TurnCreatePermissionRequest::TurnCreatePermissionRequest(
   port_->TurnCustomizerMaybeModifyOutgoingStunMessage(message);
 }
 
+TurnCreatePermissionRequest::~TurnCreatePermissionRequest() {
+  if (entry_) {
+    entry_->destroyed_callback_list_.RemoveReceivers(this);
+  }
+}
+
 void TurnCreatePermissionRequest::OnSent() {
   RTC_LOG(LS_INFO) << port_->ToString()
                    << ": TURN create permission request sent, id="
@@ -1689,11 +1701,6 @@ void TurnCreatePermissionRequest::OnTimeout() {
   }
 }
 
-void TurnCreatePermissionRequest::OnEntryDestroyed(TurnEntry* entry) {
-  RTC_DCHECK(entry_ == entry);
-  entry_ = NULL;
-}
-
 TurnChannelBindRequest::TurnChannelBindRequest(
     TurnPort* port,
     TurnEntry* entry,
@@ -1705,8 +1712,11 @@ TurnChannelBindRequest::TurnChannelBindRequest(
       entry_(entry),
       channel_id_(channel_id),
       ext_addr_(ext_addr) {
-  entry_->SignalDestroyed.connect(this,
+  RTC_DCHECK(entry_);
-                                  &TurnChannelBindRequest::OnEntryDestroyed);
+  entry_->destroyed_callback_list_.AddReceiver(this, [this](TurnEntry* entry) {
+    RTC_DCHECK(entry_ == entry);
+    entry_ = nullptr;
+  });
   StunMessage* message = mutable_msg();
   // Create the request as indicated in RFC5766, Section 11.1.
   RTC_DCHECK_EQ(message->type(), TURN_CHANNEL_BIND_REQUEST);
@@ -1718,6 +1728,12 @@ TurnChannelBindRequest::TurnChannelBindRequest(
   port_->TurnCustomizerMaybeModifyOutgoingStunMessage(message);
 }
 
+TurnChannelBindRequest::~TurnChannelBindRequest() {
+  if (entry_) {
+    entry_->destroyed_callback_list_.RemoveReceivers(this);
+  }
+}
+
 void TurnChannelBindRequest::OnSent() {
   RTC_LOG(LS_INFO) << port_->ToString()
                    << ": TURN channel bind request sent, id="
@@ -1765,11 +1781,6 @@ void TurnChannelBindRequest::OnTimeout() {
   }
 }
 
-void TurnChannelBindRequest::OnEntryDestroyed(TurnEntry* entry) {
-  RTC_DCHECK(entry_ == entry);
-  entry_ = NULL;
-}
-
 TurnEntry::TurnEntry(TurnPort* port,
                      int channel_id,
                      const rtc::SocketAddress& ext_addr,
@@ -1834,8 +1845,10 @@ int TurnEntry::Send(const void* data,
 void TurnEntry::OnCreatePermissionSuccess() {
   RTC_LOG(LS_INFO) << port_->ToString() << ": Create permission for "
                    << ext_addr_.ToSensitiveString() << " succeeded";
-  port_->SignalCreatePermissionResult(port_, ext_addr_,
+  if (port_->callbacks_for_test_) {
+    port_->callbacks_for_test_->OnTurnCreatePermissionResult(
         TURN_SUCCESS_RESULT_CODE);
+  }
 
   // If `state_` is STATE_BOUND, the permission will be refreshed
   // by ChannelBindRequest.
@@ -1862,8 +1875,9 @@ void TurnEntry::OnCreatePermissionError(StunMessage* response, int code) {
                            "code="
                         << code << "; pruned connection.";
     }
-    // Send signal with error code.
+  }
-    port_->SignalCreatePermissionResult(port_, ext_addr_, code);
+  if (port_->callbacks_for_test_) {
+    port_->callbacks_for_test_->OnTurnCreatePermissionResult(code);
   }
 }
 

p2p/base/turn_port.h

@@ -175,23 +175,6 @@ class TurnPort : public Port {
   rtc::AsyncPacketSocket* socket() const { return socket_; }
   StunRequestManager& request_manager() { return request_manager_; }
 
-  // Signal with resolved server address.
-  // Parameters are port, server address and resolved server address.
-  // This signal will be sent only if server address is resolved successfully.
-  sigslot::
-      signal3<TurnPort*, const rtc::SocketAddress&, const rtc::SocketAddress&>
-          SignalResolvedServerAddress;
-
-  // Signal when TurnPort is closed,
-  // e.g remote socket closed (TCP)
-  //  or receiveing a REFRESH response with lifetime 0.
-  sigslot::signal1<TurnPort*> SignalTurnPortClosed;
-
-  // All public methods/signals below are for testing only.
-  sigslot::signal2<TurnPort*, int> SignalTurnRefreshResult;
-  sigslot::signal3<TurnPort*, const rtc::SocketAddress&, int>
-      SignalCreatePermissionResult;
-
   bool HasRequests() { return !request_manager_.empty(); }
   void set_credentials(const RelayCredentials& credentials) {
     credentials_ = credentials;
@@ -204,6 +187,16 @@ class TurnPort : public Port {
 
   void CloseForTest() { Close(); }
 
+  // TODO(solenberg): Tests should be refactored to not peek at internal state.
+  class CallbacksForTest {
+   public:
+    virtual ~CallbacksForTest() {}
+    virtual void OnTurnCreatePermissionResult(int code) = 0;
+    virtual void OnTurnRefreshResult(int code) = 0;
+    virtual void OnTurnPortClosed() = 0;
+  };
+  void SetCallbacksForTest(CallbacksForTest* callbacks);
+
  protected:
   TurnPort(webrtc::TaskQueueBase* thread,
            rtc::PacketSocketFactory* factory,
@@ -371,6 +364,8 @@ class TurnPort : public Port {
 
   webrtc::ScopedTaskSafety task_safety_;
 
+  CallbacksForTest* callbacks_for_test_ = nullptr;
+
   friend class TurnEntry;
   friend class TurnAllocateRequest;
   friend class TurnRefreshRequest;

p2p/base/turn_port_unittest.cc

@@ -171,21 +171,15 @@ class TestConnectionWrapper : public sigslot::has_slots<> {
 
 // Note: This test uses a fake clock with a simulated network round trip
 // (between local port and TURN server) of kSimulatedRtt.
-class TurnPortTest : public ::testing::Test, public sigslot::has_slots<> {
+class TurnPortTest : public ::testing::Test,
+                     public TurnPort::CallbacksForTest,
+                     public sigslot::has_slots<> {
  public:
   TurnPortTest()
       : ss_(new TurnPortTestVirtualSocketServer()),
         main_(ss_.get()),
         socket_factory_(ss_.get()),
-        turn_server_(&main_, ss_.get(), kTurnUdpIntAddr, kTurnUdpExtAddr),
+        turn_server_(&main_, ss_.get(), kTurnUdpIntAddr, kTurnUdpExtAddr) {
-        turn_ready_(false),
-        turn_error_(false),
-        turn_unknown_address_(false),
-        turn_create_permission_success_(false),
-        turn_port_closed_(false),
-        turn_port_destroyed_(false),
-        udp_ready_(false),
-        test_finish_(false) {
     // Some code uses "last received time == 0" to represent "nothing received
     // so far", so we need to start the fake clock at a nonzero time...
     // TODO(deadbeef): Fix this.
@@ -206,16 +200,6 @@ class TurnPortTest : public ::testing::Test, public sigslot::has_slots<> {
                             bool /*port_muxed*/) {
     turn_unknown_address_ = true;
   }
-  void OnTurnCreatePermissionResult(TurnPort* port,
-                                    const SocketAddress& addr,
-                                    int code) {
-    // Ignoring the address.
-    turn_create_permission_success_ = (code == 0);
-  }
-
-  void OnTurnRefreshResult(TurnPort* port, int code) {
-    turn_refresh_success_ = (code == 0);
-  }
   void OnTurnReadPacket(Connection* conn,
                         const char* data,
                         size_t size,
@@ -237,9 +221,17 @@ class TurnPortTest : public ::testing::Test, public sigslot::has_slots<> {
     turn_port_->HandleIncomingPacket(socket, data, size, remote_addr,
                                      packet_time_us);
   }
-  void OnTurnPortClosed(TurnPort* port) { turn_port_closed_ = true; }
   void OnTurnPortDestroyed(PortInterface* port) { turn_port_destroyed_ = true; }
 
+  // TurnPort::TestCallbacks
+  void OnTurnCreatePermissionResult(int code) override {
+    turn_create_permission_success_ = (code == 0);
+  }
+  void OnTurnRefreshResult(int code) override {
+    turn_refresh_success_ = (code == 0);
+  }
+  void OnTurnPortClosed() override { turn_port_closed_ = true; }
+
   rtc::Socket* CreateServerSocket(const SocketAddress addr) {
     rtc::Socket* socket = ss_->CreateSocket(AF_INET, SOCK_STREAM);
     EXPECT_GE(socket->Bind(addr), 0);
@@ -352,14 +344,9 @@ class TurnPortTest : public ::testing::Test, public sigslot::has_slots<> {
                                              &TurnPortTest::OnCandidateError);
     turn_port_->SignalUnknownAddress.connect(
         this, &TurnPortTest::OnTurnUnknownAddress);
-    turn_port_->SignalCreatePermissionResult.connect(
-        this, &TurnPortTest::OnTurnCreatePermissionResult);
-    turn_port_->SignalTurnRefreshResult.connect(
-        this, &TurnPortTest::OnTurnRefreshResult);
-    turn_port_->SignalTurnPortClosed.connect(this,
-                                             &TurnPortTest::OnTurnPortClosed);
     turn_port_->SubscribePortDestroyed(
         [this](PortInterface* port) { OnTurnPortDestroyed(port); });
+    turn_port_->SetCallbacksForTest(this);
   }
 
   void CreateUdpPort() { CreateUdpPort(kLocalAddr2); }
@@ -783,14 +770,14 @@ class TurnPortTest : public ::testing::Test, public sigslot::has_slots<> {
   TestTurnServer turn_server_;
   std::unique_ptr<TurnPort> turn_port_;
   std::unique_ptr<UDPPort> udp_port_;
-  bool turn_ready_;
+  bool turn_ready_ = false;
-  bool turn_error_;
+  bool turn_error_ = false;
-  bool turn_unknown_address_;
+  bool turn_unknown_address_ = false;
-  bool turn_create_permission_success_;
+  bool turn_create_permission_success_ = false;
-  bool turn_port_closed_;
+  bool turn_port_closed_ = false;
-  bool turn_port_destroyed_;
+  bool turn_port_destroyed_ = false;
-  bool udp_ready_;
+  bool udp_ready_ = false;
-  bool test_finish_;
+  bool test_finish_ = false;
   bool turn_refresh_success_ = false;
   std::vector<rtc::Buffer> turn_packets_;
   std::vector<rtc::Buffer> udp_packets_;