Merge TeardownDataChannelTransport_n and OnTransportChannelClosed.

This consolidates termination logic in the DataChannelController
to make shut down consistent between when the transport notifies
of termination and when termination is initiated from the PC side.

This removes the need for `OnTransportChannelClosed` from the PC
side since we can just use TeardownDataChannelTransport_n (the two
were always being called together).

Bug: webrtc:11547
Change-Id: I1763f82cbfe1a3d5b8bfabb8d4cba0ee0fa95738
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300561
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39838}

pc/data_channel_controller.cc

@@ -146,12 +146,17 @@ void DataChannelController::OnReadyToSend() {
 
 void DataChannelController::OnTransportClosed(RTCError error) {
   RTC_DCHECK_RUN_ON(network_thread());
+
   // This loop will close all data channels and trigger a callback to
-  // `OnSctpDataChannelClosed` which will modify `sctp_data_channels_n_`, so
+  // `OnSctpDataChannelClosed`. We'll empty `sctp_data_channels_n_`, first
-  // we create a local copy while we do the fan-out.
+  // and `OnSctpDataChannelClosed` will become a noop but we'll release the
-  auto copy = sctp_data_channels_n_;
+  // StreamId here.
-  for (const auto& channel : copy)
+  std::vector<rtc::scoped_refptr<SctpDataChannel>> temp_sctp_dcs;
+  temp_sctp_dcs.swap(sctp_data_channels_n_);
+  for (const auto& channel : temp_sctp_dcs) {
     channel->OnTransportChannelClosed(error);
+    sid_allocator_.ReleaseSid(channel->sid_n());
+  }
 }
 
 void DataChannelController::SetupDataChannelTransport_n() {
@@ -168,13 +173,17 @@ void DataChannelController::PrepareForShutdown() {
   signaling_safety_.reset(PendingTaskSafetyFlag::CreateDetachedInactive());
 }
 
-void DataChannelController::TeardownDataChannelTransport_n() {
+void DataChannelController::TeardownDataChannelTransport_n(RTCError error) {
   RTC_DCHECK_RUN_ON(network_thread());
+
+  OnTransportClosed(error);
+
   if (data_channel_transport_) {
     data_channel_transport_->SetDataSink(nullptr);
     set_data_channel_transport(nullptr);
   }
-  sctp_data_channels_n_.clear();
+
+  RTC_DCHECK(sctp_data_channels_n_.empty());
   weak_factory_.InvalidateWeakPtrs();
 }
 
@@ -404,23 +413,6 @@ void DataChannelController::OnSctpDataChannelClosed(SctpDataChannel* channel) {
     sctp_data_channels_n_.erase(it);
 }
 
-void DataChannelController::OnTransportChannelClosed(RTCError error) {
-  RTC_DCHECK_RUN_ON(network_thread());
-  // Use a temporary copy of the SCTP DataChannel list because the
-  // DataChannel may callback to us and try to modify the list.
-  // TODO(tommi): `OnTransportChannelClosed` is called from
-  // `SdpOfferAnswerHandler::DestroyDataChannelTransport` just before
-  // `TeardownDataChannelTransport_n` is called (but on the network thread) from
-  // the same function. We can now get rid of this function
-  // (OnTransportChannelClosed) and run this loop from within the
-  // TeardownDataChannelTransport_n callback.
-  std::vector<rtc::scoped_refptr<SctpDataChannel>> temp_sctp_dcs;
-  temp_sctp_dcs.swap(sctp_data_channels_n_);
-  for (const auto& channel : temp_sctp_dcs) {
-    channel->OnTransportChannelClosed(error);
-  }
-}
-
 void DataChannelController::set_data_channel_transport(
     DataChannelTransportInterface* transport) {
   RTC_DCHECK_RUN_ON(network_thread());

pc/data_channel_controller.h

@@ -70,7 +70,7 @@ class DataChannelController : public SctpDataChannelControllerInterface,
   // Called from PeerConnection::SetupDataChannelTransport_n
   void SetupDataChannelTransport_n();
   // Called from PeerConnection::TeardownDataChannelTransport_n
-  void TeardownDataChannelTransport_n();
+  void TeardownDataChannelTransport_n(RTCError error);
 
   // Called from PeerConnection::OnTransportChanged
   // to make required changes to datachannels' transports.
@@ -96,9 +96,6 @@ class DataChannelController : public SctpDataChannelControllerInterface,
   // Accessors
   void set_data_channel_transport(DataChannelTransportInterface* transport);
 
-  // Called when the transport for the data channels is closed or destroyed.
-  void OnTransportChannelClosed(RTCError error);
-
   void OnSctpDataChannelClosed(SctpDataChannel* channel);
 
  protected:

pc/data_channel_controller_unittest.cc

@@ -55,7 +55,8 @@ class DataChannelControllerForTest : public DataChannelController {
       : DataChannelController(pc) {}
 
   ~DataChannelControllerForTest() override {
-    network_thread()->BlockingCall([&] { TeardownDataChannelTransport_n(); });
+    network_thread()->BlockingCall(
+        [&] { TeardownDataChannelTransport_n(RTCError::OK()); });
     PrepareForShutdown();
   }
 };

pc/peer_connection.cc

@@ -573,7 +573,7 @@ PeerConnection::~PeerConnection() {
   transport_controller_copy_ = nullptr;
   network_thread()->BlockingCall([this] {
     RTC_DCHECK_RUN_ON(network_thread());
-    TeardownDataChannelTransport_n();
+    TeardownDataChannelTransport_n(RTCError::OK());
     transport_controller_.reset();
     port_allocator_.reset();
     if (network_thread_safety_)
@@ -2544,7 +2544,7 @@ bool PeerConnection::SetupDataChannelTransport_n(const std::string& mid) {
   return true;
 }
 
-void PeerConnection::TeardownDataChannelTransport_n() {
+void PeerConnection::TeardownDataChannelTransport_n(RTCError error) {
   if (sctp_mid_n_) {
     // `sctp_mid_` may still be active through an SCTP transport.  If not, unset
     // it.
@@ -2553,7 +2553,7 @@ void PeerConnection::TeardownDataChannelTransport_n() {
     sctp_mid_n_.reset();
   }
 
-  data_channel_controller_.TeardownDataChannelTransport_n();
+  data_channel_controller_.TeardownDataChannelTransport_n(error);
 }
 
 // Returns false if bundle is enabled and rtcp_mux is disabled.

pc/peer_connection.h

@@ -434,7 +434,8 @@ class PeerConnection : public PeerConnectionInternal,
 
   bool SetupDataChannelTransport_n(const std::string& mid) override
       RTC_RUN_ON(network_thread());
-  void TeardownDataChannelTransport_n() override RTC_RUN_ON(network_thread());
+  void TeardownDataChannelTransport_n(RTCError error) override
+      RTC_RUN_ON(network_thread());
 
   const FieldTrialsView& trials() const override { return *trials_; }
 

pc/peer_connection_internal.h

@@ -118,7 +118,7 @@ class PeerConnectionSdpMethods {
   // this session.
   virtual bool SrtpRequired() const = 0;
   virtual bool SetupDataChannelTransport_n(const std::string& mid) = 0;
-  virtual void TeardownDataChannelTransport_n() = 0;
+  virtual void TeardownDataChannelTransport_n(RTCError error) = 0;
   virtual void SetSctpDataMid(const std::string& mid) = 0;
   virtual void ResetSctpDataMid() = 0;
 

pc/sdp_offer_answer.cc

@@ -5110,18 +5110,12 @@ bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) {
 
 void SdpOfferAnswerHandler::DestroyDataChannelTransport(RTCError error) {
   RTC_DCHECK_RUN_ON(signaling_thread());
-  const bool has_sctp = pc_->sctp_mid().has_value();
-
   context_->network_thread()->BlockingCall(
       [&, data_channel_controller = data_channel_controller()] {
         RTC_DCHECK_RUN_ON(context_->network_thread());
-        if (has_sctp)
+        pc_->TeardownDataChannelTransport_n(error);
-          data_channel_controller->OnTransportChannelClosed(error);
-        pc_->TeardownDataChannelTransport_n();
       });
-
+  pc_->ResetSctpDataMid();
-  if (has_sctp)
-    pc_->ResetSctpDataMid();
 }
 
 void SdpOfferAnswerHandler::DestroyAllChannels() {

pc/test/fake_peer_connection_base.h

@@ -361,7 +361,7 @@ class FakePeerConnectionBase : public PeerConnectionInternal {
   bool SetupDataChannelTransport_n(const std::string& mid) override {
     return false;
   }
-  void TeardownDataChannelTransport_n() override {}
+  void TeardownDataChannelTransport_n(RTCError error) override {}
   void SetSctpDataMid(const std::string& mid) override {}
   void ResetSctpDataMid() override {}
 

pc/test/mock_peer_connection_internal.h

@@ -267,7 +267,7 @@ class MockPeerConnectionInternal : public PeerConnectionInternal {
               SetupDataChannelTransport_n,
               (const std::string&),
               (override));
-  MOCK_METHOD(void, TeardownDataChannelTransport_n, (), (override));
+  MOCK_METHOD(void, TeardownDataChannelTransport_n, (RTCError), (override));
   MOCK_METHOD(void, SetSctpDataMid, (const std::string&), (override));
   MOCK_METHOD(void, ResetSctpDataMid, (), (override));
   MOCK_METHOD(const FieldTrialsView&, trials, (), (const, override));