Add forget_learned_state to IceControllerInterface

This patch enables an IceController to use
Connection::ForgetLearnedState by returning it
in a SwitchResult, that will cause P2PTransportChannel
to call the method.

BUG: webrtc:11463
Change-Id: I098bbbd2fb2961822b165770189ac0c2225d1cb0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176511
Commit-Queue: Jonas Oreland <jonaso@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31458}

p2p/base/ice_controller_interface.h

@@ -51,12 +51,20 @@ struct IceControllerEvent {
 // - which connection to ping
 // - which connection to use
 // - which connection to prune
+// - which connection to forget learned state on
 //
-// P2PTransportChannel creates a |Connection| and adds a const pointer
-// to the IceController using |AddConnection|, i.e the IceController
-// should not call any non-const methods on a Connection.
+// The P2PTransportChannel owns (creates and destroys) Connections,
+// but P2PTransportChannel gives const pointers to the the IceController using
+// |AddConnection|, i.e the IceController should not call any non-const methods
+// on a Connection but signal back in the interface if any mutable function
+// shall be called.
 //
-// The IceController shall keeps track of all connections added
+// Current these are limited to:
+// Connection::Ping               - returned in PingResult
+// Connection::Prune              - retuned in PruneConnections
+// Connection::ForgetLearnedState - return in SwitchResult
+//
+// The IceController shall keep track of all connections added
 // (and not destroyed) and give them back using the connections()-function-
 //
 // When a Connection gets destroyed
@@ -71,6 +79,9 @@ class IceControllerInterface {
 
     // An optional recheck event for when a Switch() should be attempted again.
     absl::optional<IceControllerEvent> recheck_event;
+
+    // A vector with connection to run ForgetLearnedState on.
+    std::vector<const Connection*> connections_to_forget_state_on;
   };
 
   // This represents the result of a call to SelectConnectionToPing.

p2p/base/p2p_transport_channel.cc

@@ -275,8 +275,7 @@ bool P2PTransportChannel::MaybeSwitchSelectedConnection(
   if (result.connection.has_value()) {
     RTC_LOG(LS_INFO) << "Switching selected connection due to: "
                      << reason.ToString();
-    SwitchSelectedConnection(const_cast<Connection*>(*result.connection),
-                             reason);
+    SwitchSelectedConnection(FromIceController(*result.connection), reason);
   }
 
   if (result.recheck_event.has_value()) {
@@ -291,6 +290,10 @@ bool P2PTransportChannel::MaybeSwitchSelectedConnection(
         result.recheck_event->recheck_delay_ms);
   }
 
+  for (const auto* con : result.connections_to_forget_state_on) {
+    FromIceController(con)->ForgetLearnedState();
+  }
+
   return result.connection.has_value();
 }
 
@@ -1403,7 +1406,7 @@ bool P2PTransportChannel::CreateConnection(PortInterface* port,
   return false;
 }
 
-bool P2PTransportChannel::FindConnection(Connection* connection) const {
+bool P2PTransportChannel::FindConnection(const Connection* connection) const {
   RTC_DCHECK_RUN_ON(network_thread_);
   return absl::c_linear_search(connections(), connection);
 }
@@ -1709,7 +1712,7 @@ void P2PTransportChannel::PruneConnections() {
   std::vector<const Connection*> connections_to_prune =
       ice_controller_->PruneConnections();
   for (const Connection* conn : connections_to_prune) {
-    const_cast<Connection*>(conn)->Prune();
+    FromIceController(conn)->Prune();
   }
 }
 
@@ -1912,11 +1915,10 @@ void P2PTransportChannel::CheckAndPing() {
   UpdateConnectionStates();
 
   auto result = ice_controller_->SelectConnectionToPing(last_ping_sent_ms_);
-  Connection* conn =
-      const_cast<Connection*>(result.connection.value_or(nullptr));
   int delay = result.recheck_delay_ms;
 
-  if (conn) {
+  if (result.connection.value_or(nullptr)) {
+    Connection* conn = FromIceController(*result.connection);
     PingConnection(conn);
     MarkConnectionPinged(conn);
   }
@@ -1929,7 +1931,12 @@ void P2PTransportChannel::CheckAndPing() {
 // This method is only for unit testing.
 Connection* P2PTransportChannel::FindNextPingableConnection() {
   RTC_DCHECK_RUN_ON(network_thread_);
-  return const_cast<Connection*>(ice_controller_->FindNextPingableConnection());
+  auto* conn = ice_controller_->FindNextPingableConnection();
+  if (conn) {
+    return FromIceController(conn);
+  } else {
+    return nullptr;
+  }
 }
 
 // A connection is considered a backup connection if the channel state

p2p/base/p2p_transport_channel.h

@@ -245,7 +245,7 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal {
   bool CreateConnection(PortInterface* port,
                         const Candidate& remote_candidate,
                         PortInterface* origin_port);
-  bool FindConnection(Connection* connection) const;
+  bool FindConnection(const Connection* connection) const;
 
   uint32_t GetRemoteCandidateGeneration(const Candidate& candidate);
   bool IsDuplicateRemoteCandidate(const Candidate& candidate);
@@ -348,6 +348,16 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal {
   // 2. Peer-reflexive remote candidates.
   Candidate SanitizeRemoteCandidate(const Candidate& c) const;
 
+  // Cast a Connection returned from IceController and verify that it exists.
+  // (P2P owns all Connections, and only gives const pointers to IceController,
+  // see IceControllerInterface).
+  Connection* FromIceController(const Connection* conn) {
+    // Verify that IceController does not return a connection
+    // that we have destroyed.
+    RTC_DCHECK(FindConnection(conn));
+    return const_cast<Connection*>(conn);
+  }
+
   std::string transport_name_ RTC_GUARDED_BY(network_thread_);
   int component_ RTC_GUARDED_BY(network_thread_);
   PortAllocator* allocator_ RTC_GUARDED_BY(network_thread_);

p2p/base/p2p_transport_channel_unittest.cc

@@ -5728,6 +5728,81 @@ TEST(P2PTransportChannel, InjectIceController) {
       /* event_log = */ nullptr, &factory);
 }
 
+class ForgetLearnedStateController : public cricket::BasicIceController {
+ public:
+  explicit ForgetLearnedStateController(
+      const cricket::IceControllerFactoryArgs& args)
+      : cricket::BasicIceController(args) {}
+
+  SwitchResult SortAndSwitchConnection(IceControllerEvent reason) override {
+    auto result = cricket::BasicIceController::SortAndSwitchConnection(reason);
+    if (forget_connnection_) {
+      result.connections_to_forget_state_on.push_back(forget_connnection_);
+      forget_connnection_ = nullptr;
+    }
+    result.recheck_event =
+        IceControllerEvent(IceControllerEvent::ICE_CONTROLLER_RECHECK);
+    result.recheck_event->recheck_delay_ms = 100;
+    return result;
+  }
+
+  void ForgetThisConnectionNextTimeSortAndSwitchConnectionIsCalled(
+      Connection* con) {
+    forget_connnection_ = con;
+  }
+
+ private:
+  Connection* forget_connnection_ = nullptr;
+};
+
+class ForgetLearnedStateControllerFactory
+    : public cricket::IceControllerFactoryInterface {
+ public:
+  std::unique_ptr<cricket::IceControllerInterface> Create(
+      const cricket::IceControllerFactoryArgs& args) override {
+    auto controller = std::make_unique<ForgetLearnedStateController>(args);
+    // Keep a pointer to allow modifying calls.
+    // Must not be used after the p2ptransportchannel has been destructed.
+    controller_ = controller.get();
+    return controller;
+  }
+  virtual ~ForgetLearnedStateControllerFactory() = default;
+
+  ForgetLearnedStateController* controller_;
+};
+
+TEST_F(P2PTransportChannelPingTest, TestForgetLearnedState) {
+  ForgetLearnedStateControllerFactory factory;
+  FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+  P2PTransportChannel ch("ping sufficiently", 1, &pa, nullptr, nullptr,
+                         &factory);
+  PrepareChannel(&ch);
+  ch.MaybeStartGathering();
+  ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1));
+  ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 2));
+
+  Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
+  Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
+  ASSERT_TRUE(conn1 != nullptr);
+  ASSERT_TRUE(conn2 != nullptr);
+
+  // Wait for conn1 to be selected.
+  conn1->ReceivedPingResponse(LOW_RTT, "id");
+  EXPECT_EQ_WAIT(conn1, ch.selected_connection(), kMediumTimeout);
+
+  conn2->ReceivedPingResponse(LOW_RTT, "id");
+  EXPECT_TRUE(conn2->writable());
+
+  // Now let the ice controller signal to P2PTransportChannel that it
+  // should Forget conn2.
+  factory.controller_
+      ->ForgetThisConnectionNextTimeSortAndSwitchConnectionIsCalled(conn2);
+
+  // We don't have a mock Connection, so verify this by checking that it
+  // is no longer writable.
+  EXPECT_EQ_WAIT(false, conn2->writable(), kMediumTimeout);
+}
+
 TEST_F(P2PTransportChannelTest, DisableDnsLookupsWithTransportPolicyRelay) {
   ConfigureEndpoints(OPEN, OPEN, kDefaultPortAllocatorFlags,
                      kDefaultPortAllocatorFlags);