From 18408391ad4c947ad1a3eeae424663f15e6346ef Mon Sep 17 00:00:00 2001
From: Sameer Vijaykar <samvi@google.com>
Date: Wed, 14 Sep 2022 11:49:47 +0200
Subject: [PATCH] Keep state internally of connections added to
 P2PTransportChannel (#4/n)

P2PTransportChannel currently relies on the ICE controller to keep track of this, even though P2PTransportChannel is actually supposed to hold the mutable connections.

Reading connections from the ICE controller also leaks some internal state from the ICE controller through the ordering of connections, which isn't strictly part of the interface. This change is a step towards fixing this.

This change is functionally no-op for now. The internal state will be used behind a field-trial in a future CL. That is also when some tests will be updated to work with the new internal state.

Bug: webrtc:14367, webrtc:1413
Change-Id: I6f8c5d805c780411fe940926f192fd2d6ce86d29
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/275081
Commit-Queue: Sameer Vijaykar <samvi@google.com>
Reviewed-by: Jonas Oreland <jonaso@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38080}
---
 p2p/base/p2p_transport_channel.cc | 17 +++++++++++++----
 p2p/base/p2p_transport_channel.h  |  3 +++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index 213cfbde77..82d77bba4b 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -223,7 +223,7 @@ P2PTransportChannel::~P2PTransportChannel() {
   std::vector<Connection*> copy(connections().begin(), connections().end());
   for (Connection* connection : copy) {
     connection->SignalDestroyed.disconnect(this);
-    ice_controller_->OnConnectionDestroyed(connection);
+    RemoveConnection(connection);
     connection->Destroy();
   }
   resolvers_.clear();
@@ -281,6 +281,7 @@ void P2PTransportChannel::AddConnection(Connection* connection) {
   LogCandidatePairConfig(connection,
                          webrtc::IceCandidatePairConfigType::kAdded);
 
+  connections_.push_back(connection);
   ice_controller_->AddConnection(connection);
 }
 
@@ -1693,7 +1694,7 @@ void P2PTransportChannel::RemoveConnectionForTest(Connection* connection) {
   RTC_DCHECK_RUN_ON(network_thread_);
   RTC_DCHECK(FindConnection(connection));
   connection->SignalDestroyed.disconnect(this);
-  ice_controller_->OnConnectionDestroyed(connection);
+  RemoveConnection(connection);
   RTC_DCHECK(!FindConnection(connection));
   if (selected_connection_ == connection)
     selected_connection_ = nullptr;
@@ -2045,7 +2046,7 @@ void P2PTransportChannel::HandleAllTimedOut() {
       update_selected_connection = true;
     }
     connection->SignalDestroyed.disconnect(this);
-    ice_controller_->OnConnectionDestroyed(connection);
+    RemoveConnection(connection);
     connection->Destroy();
   }
 
@@ -2172,7 +2173,7 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) {
   // use it.
 
   // Remove this connection from the list.
-  ice_controller_->OnConnectionDestroyed(connection);
+  RemoveConnection(connection);
 
   RTC_LOG(LS_INFO) << ToString() << ": Removed connection " << connection
                    << " (" << connections().size() << " remaining)";
@@ -2193,6 +2194,14 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) {
   }
 }
 
+void P2PTransportChannel::RemoveConnection(const Connection* connection) {
+  RTC_DCHECK_RUN_ON(network_thread_);
+  auto it = absl::c_find(connections_, connection);
+  RTC_DCHECK(it != connections_.end());
+  connections_.erase(it);
+  ice_controller_->OnConnectionDestroyed(connection);
+}
+
 // When a port is destroyed, remove it from our list of ports to use for
 // connection attempts.
 void P2PTransportChannel::OnPortDestroyed(PortInterface* port) {
diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h
index 981bd9a064..5e56df64eb 100644
--- a/p2p/base/p2p_transport_channel.h
+++ b/p2p/base/p2p_transport_channel.h
@@ -200,6 +200,8 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal {
   int check_receiving_interval() const;
   absl::optional<rtc::NetworkRoute> network_route() const override;
 
+  void RemoveConnection(const Connection* connection);
+
   // Helper method used only in unittest.
   rtc::DiffServCodePoint DefaultDscpValue() const;
 
@@ -427,6 +429,7 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal {
   std::vector<PortInterface*> pruned_ports_ RTC_GUARDED_BY(network_thread_);
 
   Connection* selected_connection_ RTC_GUARDED_BY(network_thread_) = nullptr;
+  std::vector<Connection*> connections_ RTC_GUARDED_BY(network_thread_);
 
   std::vector<RemoteCandidate> remote_candidates_
       RTC_GUARDED_BY(network_thread_);