Avoid dangling pointers in a few Connection related classes.

Bug: webrtc:11988
Change-Id: I2db1281983396366b91666a1c2bbbcae434ed625
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/249949
Reviewed-by: Niels Moller <nisse@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35858}

p2p/base/basic_ice_controller.cc

@@ -83,6 +83,8 @@ void BasicIceController::OnConnectionDestroyed(const Connection* connection) {
   pinged_connections_.erase(connection);
   unpinged_connections_.erase(connection);
   connections_.erase(absl::c_find(connections_, connection));
+  if (selected_connection_ == connection)
+    selected_connection_ = nullptr;
 }
 
 bool BasicIceController::HasPingableConnection() const {

p2p/base/p2p_transport_channel.cc

@@ -228,6 +228,7 @@ P2PTransportChannel::~P2PTransportChannel() {
   RTC_DCHECK_RUN_ON(network_thread_);
   std::vector<Connection*> copy(connections().begin(), connections().end());
   for (Connection* con : copy) {
+    con->SignalDestroyed.disconnect(this);
     con->Destroy();
   }
   resolvers_.clear();
@@ -1674,7 +1675,11 @@ void P2PTransportChannel::UpdateConnectionStates() {
 
   // We need to copy the list of connections since some may delete themselves
   // when we call UpdateState.
-  for (Connection* c : connections()) {
+  // NOTE: We copy the connections() vector in case `UpdateState` triggers the
+  // Connection to be destroyed (which will cause a callback that alters
+  // the connections() vector).
+  std::vector<Connection*> copy(connections().begin(), connections().end());
+  for (Connection* c : copy) {
     c->UpdateState(now);
   }
 }
@@ -1985,12 +1990,31 @@ void P2PTransportChannel::MaybeStopPortAllocatorSessions() {
   }
 }
 
+// RTC_RUN_ON(network_thread_)
+void P2PTransportChannel::OnSelectedConnectionDestroyed() {
+  RTC_LOG(LS_INFO) << "Selected connection destroyed. Will choose a new one.";
+  IceControllerEvent reason = IceControllerEvent::SELECTED_CONNECTION_DESTROYED;
+  SwitchSelectedConnection(nullptr, reason);
+  RequestSortAndStateUpdate(reason);
+}
+
 // If all connections timed out, delete them all.
 void P2PTransportChannel::HandleAllTimedOut() {
   RTC_DCHECK_RUN_ON(network_thread_);
-  for (Connection* connection : connections()) {
+  bool update_selected_connection = false;
+  std::vector<Connection*> copy(connections().begin(), connections().end());
+  for (Connection* connection : copy) {
+    if (selected_connection_ == connection) {
+      selected_connection_ = nullptr;
+      update_selected_connection = true;
+    }
+    connection->SignalDestroyed.disconnect(this);
+    ice_controller_->OnConnectionDestroyed(connection);
     connection->Destroy();
   }
+
+  if (update_selected_connection)
+    OnSelectedConnectionDestroyed();
 }
 
 bool P2PTransportChannel::ReadyToSend(Connection* connection) const {
@@ -2124,11 +2148,7 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) {
   // we can just set selected to nullptr and re-choose a best assuming that
   // there was no selected connection.
   if (selected_connection_ == connection) {
-    RTC_LOG(LS_INFO) << "Selected connection destroyed. Will choose a new one.";
-    IceControllerEvent reason =
-        IceControllerEvent::SELECTED_CONNECTION_DESTROYED;
-    SwitchSelectedConnection(nullptr, reason);
-    RequestSortAndStateUpdate(reason);
+    OnSelectedConnectionDestroyed();
   } else {
     // If a non-selected connection was destroyed, we don't need to re-sort but
     // we do need to update state, because we could be switching to "failed" or

p2p/base/p2p_transport_channel.h

@@ -273,6 +273,7 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal {
   void UpdateState();
   void HandleAllTimedOut();
   void MaybeStopPortAllocatorSessions();
+  void OnSelectedConnectionDestroyed() RTC_RUN_ON(network_thread_);
 
   // ComputeIceTransportState computes the RTCIceTransportState as described in
   // https://w3c.github.io/webrtc-pc/#dom-rtcicetransportstate. ComputeState

p2p/base/port.cc

@@ -194,8 +194,10 @@ Port::~Port() {
     ++iter;
   }
 
-  for (uint32_t i = 0; i < list.size(); i++)
+  for (uint32_t i = 0; i < list.size(); i++) {
+    list[i]->SignalDestroyed.disconnect(this);
     delete list[i];
+  }
 }
 
 const std::string& Port::Type() const {
@@ -606,6 +608,15 @@ rtc::DiffServCodePoint Port::StunDscpValue() const {
   return rtc::DSCP_NO_CHANGE;
 }
 
+void Port::DestroyAllConnections() {
+  RTC_DCHECK_RUN_ON(thread_);
+  for (auto kv : connections_) {
+    kv.second->SignalDestroyed.disconnect(this);
+    kv.second->Destroy();
+  }
+  connections_.clear();
+}
+
 void Port::set_timeout_delay(int delay) {
   RTC_DCHECK_RUN_ON(thread_);
   // Although this method is meant to only be used by tests, some downstream

p2p/base/port.h

@@ -435,6 +435,8 @@ class Port : public PortInterface,
   // Extra work to be done in subclasses when a connection is destroyed.
   virtual void HandleConnectionDestroyed(Connection* conn) {}
 
+  void DestroyAllConnections();
+
   void CopyPortInformationToPacketInfo(rtc::PacketInfo* info) const;
 
   MdnsNameRegistrationStatus mdns_name_registration_status() const {

p2p/base/turn_port.cc

@@ -922,9 +922,7 @@ void TurnPort::Close() {
   // Stop the port from creating new connections.
   state_ = STATE_DISCONNECTED;
   // Delete all existing connections; stop sending data.
-  for (auto kv : connections()) {
-    kv.second->Destroy();
-  }
+  DestroyAllConnections();
 
   SignalTurnPortClosed(this);
 }
@@ -1272,10 +1270,6 @@ void TurnPort::HandleConnectionDestroyed(Connection* conn) {
   const rtc::SocketAddress& remote_address = conn->remote_candidate().address();
   TurnEntry* entry = FindEntry(remote_address);
   RTC_DCHECK(entry != NULL);
-  ScheduleEntryDestruction(entry);
-}
-
-void TurnPort::ScheduleEntryDestruction(TurnEntry* entry) {
   RTC_DCHECK(!entry->destruction_timestamp().has_value());
   int64_t timestamp = rtc::TimeMillis();
   entry->set_destruction_timestamp(timestamp);

p2p/base/turn_port.h

@@ -361,7 +361,6 @@ class TurnPort : public Port {
   // Destroys the entry only if `timestamp` matches the destruction timestamp
   // in `entry`.
   void DestroyEntryIfNotCancelled(TurnEntry* entry, int64_t timestamp);
-  void ScheduleEntryDestruction(TurnEntry* entry);
 
   // Marks the connection with remote address `address` failed and
   // pruned (a.k.a. write-timed-out). Returns true if a connection is found.