commit cbaf91bcf0701309f5160c30f5804f5d25d0e5f1
author Tommi <tommi@webrtc.org> Mon Sep 04 17:34:10 2023
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Sep 05 07:02:24 2023
tree 020636290c4effe013add3825025ca720235397e
parent 2cb531be7dceb665e771cdb501a8a96f2eeabd9e

Disconnect signals when destroying socket
Add thread checks to TcpPort code

Bug: chromium:1478154
Change-Id: I045106c552dfcd8a8ab79218a59873fdc1d4326f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/318061
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40693}

p2p/base/tcp_port.cc

diff --git a/p2p/base/tcp_port.cc b/p2p/base/tcp_port.cc
index 2760c02..5d40802 100644
--- a/p2p/base/tcp_port.cc
+++ b/p2p/base/tcp_port.cc
@@ -356,7 +356,11 @@
       connection_pending_(false),
       pretending_to_be_writable_(false),
       reconnection_timeout_(cricket::CONNECTION_WRITE_CONNECT_TIMEOUT) {
+  RTC_DCHECK_RUN_ON(network_thread_);
   RTC_DCHECK_EQ(port()->GetProtocol(), PROTO_TCP);  // Needs to be TCPPort.
+
+  SignalDestroyed.connect(this, &TCPConnection::OnDestroyed);
+
   if (outgoing_) {
     CreateOutgoingTcpSocket();
   } else {
@@ -496,6 +500,7 @@
 }
 
 void TCPConnection::OnClose(rtc::AsyncPacketSocket* socket, int error) {
+  RTC_DCHECK_RUN_ON(network_thread());
   RTC_DCHECK_EQ(socket, socket_.get());
   RTC_LOG(LS_INFO) << ToString() << ": Connection closed with error " << error;
 
@@ -532,12 +537,13 @@
     // initial connect() (i.e. `pretending_to_be_writable_` is false) . We have
     // to manually destroy here as this connection, as never connected, will not
     // be scheduled for ping to trigger destroy.
-    socket_->UnsubscribeClose(this);
+    DisconnectSocketSignals(socket_.get());
     port()->DestroyConnectionAsync(this);
   }
 }
 
 void TCPConnection::MaybeReconnect() {
+  RTC_DCHECK_RUN_ON(network_thread());
   // Only reconnect for an outgoing TCPConnection when OnClose was signaled and
   // no outstanding reconnect is pending.
   if (connected() || connection_pending_ || !outgoing_) {
@@ -557,15 +563,25 @@
                                  size_t size,
                                  const rtc::SocketAddress& remote_addr,
                                  const int64_t& packet_time_us) {
+  RTC_DCHECK_RUN_ON(network_thread());
   RTC_DCHECK_EQ(socket, socket_.get());
   Connection::OnReadPacket(data, size, packet_time_us);
 }
 
 void TCPConnection::OnReadyToSend(rtc::AsyncPacketSocket* socket) {
+  RTC_DCHECK_RUN_ON(network_thread());
   RTC_DCHECK_EQ(socket, socket_.get());
   Connection::OnReadyToSend();
 }
 
+void TCPConnection::OnDestroyed(Connection* c) {
+  RTC_DCHECK_RUN_ON(network_thread());
+  RTC_DCHECK_EQ(c, this);
+  if (socket_) {
+    DisconnectSocketSignals(socket_.get());
+  }
+}
+
 void TCPConnection::CreateOutgoingTcpSocket() {
   RTC_DCHECK(outgoing_);
   int opts = (remote_candidate().protocol() == SSLTCP_PROTOCOL_NAME)
@@ -573,7 +589,7 @@
                  : 0;
 
   if (socket_) {
-    socket_->UnsubscribeClose(this);
+    DisconnectSocketSignals(socket_.get());
   }
 
   rtc::PacketSocketTcpOptions tcp_opts;
@@ -616,4 +632,13 @@
   });
 }
 
+void TCPConnection::DisconnectSocketSignals(rtc::AsyncPacketSocket* socket) {
+  if (outgoing_) {
+    socket->SignalConnect.disconnect(this);
+  }
+  socket->SignalReadPacket.disconnect(this);
+  socket->SignalReadyToSend.disconnect(this);
+  socket->UnsubscribeClose(this);
+}
+
 }  // namespace cricket

p2p/base/tcp_port.h

diff --git a/p2p/base/tcp_port.h b/p2p/base/tcp_port.h
index ff69e6e..a1bbaa9 100644
--- a/p2p/base/tcp_port.h
+++ b/p2p/base/tcp_port.h
@@ -153,13 +153,19 @@
                                    StunMessage* response) override;
 
  private:
+  friend class TCPPort;  // For `MaybeReconnect()`.
+
   // Helper function to handle the case when Ping or Send fails with error
   // related to socket close.
   void MaybeReconnect();
 
-  void CreateOutgoingTcpSocket();
+  void CreateOutgoingTcpSocket() RTC_RUN_ON(network_thread());
 
-  void ConnectSocketSignals(rtc::AsyncPacketSocket* socket);
+  void ConnectSocketSignals(rtc::AsyncPacketSocket* socket)
+      RTC_RUN_ON(network_thread());
+
+  void DisconnectSocketSignals(rtc::AsyncPacketSocket* socket)
+      RTC_RUN_ON(network_thread());
 
   void OnConnect(rtc::AsyncPacketSocket* socket);
   void OnClose(rtc::AsyncPacketSocket* socket, int error);
@@ -169,6 +175,7 @@
                     const rtc::SocketAddress& remote_addr,
                     const int64_t& packet_time_us);
   void OnReadyToSend(rtc::AsyncPacketSocket* socket);
+  void OnDestroyed(Connection* c);
 
   TCPPort* tcp_port() {
     RTC_DCHECK_EQ(port()->GetProtocol(), PROTO_TCP);
@@ -177,7 +184,7 @@
 
   std::unique_ptr<rtc::AsyncPacketSocket> socket_;
   int error_;
-  bool outgoing_;
+  const bool outgoing_;
 
   // Guard against multiple outgoing tcp connection during a reconnect.
   bool connection_pending_;
@@ -194,8 +201,6 @@
   int reconnection_timeout_;
 
   webrtc::ScopedTaskSafety network_safety_;
-
-  friend class TCPPort;
 };
 
 }  // namespace cricket