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}
diff --git a/p2p/base/basic_ice_controller.cc b/p2p/base/basic_ice_controller.cc
index 81fb324..9025fbe 100644
--- a/p2p/base/basic_ice_controller.cc
+++ b/p2p/base/basic_ice_controller.cc
@@ -83,6 +83,8 @@
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 {
diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index c530af1..9fe9cac 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -228,6 +228,7 @@
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 @@
// 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 @@
}
}
+// 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 @@
// 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
diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h
index 174cbc2..d062346 100644
--- a/p2p/base/p2p_transport_channel.h
+++ b/p2p/base/p2p_transport_channel.h
@@ -273,6 +273,7 @@
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
diff --git a/p2p/base/port.cc b/p2p/base/port.cc
index 51297c4..c616658 100644
--- a/p2p/base/port.cc
+++ b/p2p/base/port.cc
@@ -194,8 +194,10 @@
++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 @@
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
diff --git a/p2p/base/port.h b/p2p/base/port.h
index 9918729..1ec82f7 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -435,6 +435,8 @@
// 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 {
diff --git a/p2p/base/turn_port.cc b/p2p/base/turn_port.cc
index 07c1060..e622b62 100644
--- a/p2p/base/turn_port.cc
+++ b/p2p/base/turn_port.cc
@@ -922,9 +922,7 @@
// 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 @@
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);
diff --git a/p2p/base/turn_port.h b/p2p/base/turn_port.h
index 7b8e3b9..891af76 100644
--- a/p2p/base/turn_port.h
+++ b/p2p/base/turn_port.h
@@ -361,7 +361,6 @@
// 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.
