Remove SignalDataChannelTransportChannelClosed_s

This removes one sigslot and also simplifies the teardown procedure
of a data channel when the channel is closed by the transport.
In this case we no longer need an additional async teardown task that
releases the last remaining reference to the channel.

Bug: webrtc:11943, webrtc:11547
Change-Id: I1c170349a6cbb3cb3c5a47d284e3a3d416c92b11
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/296981
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39551}

pc/data_channel_controller.cc

@@ -12,6 +12,7 @@
 
 #include <utility>
 
+#include "absl/algorithm/container.h"
 #include "api/peer_connection_interface.h"
 #include "api/rtc_error.h"
 #include "pc/peer_connection_internal.h"
@@ -52,8 +53,6 @@ bool DataChannelController::ConnectDataChannel(
       webrtc_data_channel, &SctpDataChannel::OnDataReceived);
   SignalDataChannelTransportChannelClosing_s.connect(
       webrtc_data_channel, &SctpDataChannel::OnClosingProcedureStartedRemotely);
-  SignalDataChannelTransportChannelClosed_s.connect(
-      webrtc_data_channel, &SctpDataChannel::OnClosingProcedureComplete);
   return true;
 }
 
@@ -68,7 +67,6 @@ void DataChannelController::DisconnectDataChannel(
   SignalDataChannelTransportWritable_s.disconnect(webrtc_data_channel);
   SignalDataChannelTransportReceivedData_s.disconnect(webrtc_data_channel);
   SignalDataChannelTransportChannelClosing_s.disconnect(webrtc_data_channel);
-  SignalDataChannelTransportChannelClosed_s.disconnect(webrtc_data_channel);
 }
 
 void DataChannelController::AddSctpDataStream(int sid) {
@@ -143,7 +141,22 @@ void DataChannelController::OnChannelClosed(int channel_id) {
   signaling_thread()->PostTask(
       SafeTask(signaling_safety_.flag(), [this, channel_id] {
         RTC_DCHECK_RUN_ON(signaling_thread());
-        SignalDataChannelTransportChannelClosed_s(channel_id);
+        auto it = absl::c_find_if(sctp_data_channels_, [&](const auto& c) {
+          return c->id() == channel_id;
+        });
+
+        // Remove the channel from our list, close it and free up resources.
+        if (it != sctp_data_channels_.end()) {
+          rtc::scoped_refptr<SctpDataChannel> channel = std::move(*it);
+          // Note: this causes OnSctpDataChannelClosed() to not do anything
+          // when called from within `OnClosingProcedureComplete`.
+          sctp_data_channels_.erase(it);
+
+          DisconnectDataChannel(channel.get());
+          sid_allocator_.ReleaseSid(channel->sid());
+
+          channel->OnClosingProcedureComplete();
+        }
       }));
 }
 

pc/data_channel_controller.h

@@ -163,8 +163,6 @@ class DataChannelController : public SctpDataChannelControllerInterface,
           RTC_GUARDED_BY(signaling_thread());
   sigslot::signal1<int> SignalDataChannelTransportChannelClosing_s
       RTC_GUARDED_BY(signaling_thread());
-  sigslot::signal1<int> SignalDataChannelTransportChannelClosed_s
-      RTC_GUARDED_BY(signaling_thread());
 
   // Owning PeerConnection.
   PeerConnectionInternal* const pc_;

pc/data_channel_unittest.cc

@@ -162,20 +162,21 @@ TEST_F(SctpDataChannelTest, ConnectedAfterTransportBecomesAvailable) {
 
 // Tests the state of the data channel.
 TEST_F(SctpDataChannelTest, StateTransition) {
+  AddObserver();
+
   EXPECT_EQ(DataChannelInterface::kConnecting, webrtc_data_channel_->state());
-  EXPECT_EQ(controller_->channels_opened(), 0);
-  EXPECT_EQ(controller_->channels_closed(), 0);
+  EXPECT_EQ(observer_->on_state_change_count(), 0u);
   SetChannelReady();
 
   EXPECT_EQ(DataChannelInterface::kOpen, webrtc_data_channel_->state());
-  EXPECT_EQ(controller_->channels_opened(), 1);
-  EXPECT_EQ(controller_->channels_closed(), 0);
+  EXPECT_EQ(observer_->on_state_change_count(), 1u);
 
+  // `Close()` should trigger two state changes, first `kClosing`, then
+  // `kClose`.
   webrtc_data_channel_->Close();
   EXPECT_EQ(DataChannelInterface::kClosed, webrtc_data_channel_->state());
+  EXPECT_EQ(observer_->on_state_change_count(), 3u);
   EXPECT_TRUE(webrtc_data_channel_->error().ok());
-  EXPECT_EQ(controller_->channels_opened(), 1);
-  EXPECT_EQ(controller_->channels_closed(), 1);
   // Verifies that it's disconnected from the transport.
   EXPECT_FALSE(controller_->IsConnected(webrtc_data_channel_.get()));
 }

pc/sctp_data_channel.cc

@@ -345,11 +345,7 @@ bool SctpDataChannel::Send(const DataBuffer& buffer) {
   // If the queue is non-empty, we're waiting for SignalReadyToSend,
   // so just add to the end of the queue and keep waiting.
   if (!queued_send_data_.Empty()) {
-    if (!QueueSendDataMessage(buffer)) {
-      // Queue is full
-      return false;
-    }
-    return true;
+    return QueueSendDataMessage(buffer);
   }
 
   SendDataMessage(buffer, true);
@@ -389,16 +385,14 @@ void SctpDataChannel::OnClosingProcedureStartedRemotely(int sid) {
   }
 }
 
-void SctpDataChannel::OnClosingProcedureComplete(int sid) {
+void SctpDataChannel::OnClosingProcedureComplete() {
   RTC_DCHECK_RUN_ON(signaling_thread_);
-  if (id_.stream_id_int() == sid) {
-    // If the closing procedure is complete, we should have finished sending
-    // all pending data and transitioned to kClosing already.
-    RTC_DCHECK_EQ(state_, kClosing);
-    RTC_DCHECK(queued_send_data_.Empty());
-    DisconnectFromTransport();
-    SetState(kClosed);
-  }
+  // If the closing procedure is complete, we should have finished sending
+  // all pending data and transitioned to kClosing already.
+  RTC_DCHECK_EQ(state_, kClosing);
+  RTC_DCHECK(queued_send_data_.Empty());
+
+  SetState(kClosed);
 }
 
 void SctpDataChannel::OnTransportChannelCreated() {
@@ -515,9 +509,7 @@ void SctpDataChannel::CloseAbruptlyWithError(RTCError error) {
     return;
   }
 
-  if (connected_to_transport_) {
-    DisconnectFromTransport();
-  }
+  DisconnectFromTransport();
 
   // Closing abruptly means any queued data gets thrown away.
   queued_send_data_.Clear();

pc/sctp_data_channel.h

@@ -194,7 +194,7 @@ class SctpDataChannel : public DataChannelInterface,
   // The closing procedure is complete; both incoming and outgoing stream
   // resets are done and the channel can transition to kClosed. Called
   // asynchronously after RemoveSctpDataStream.
-  void OnClosingProcedureComplete(int sid);
+  void OnClosingProcedureComplete();
   // Called when the transport channel is created.
   // Only needs to be called for SCTP data channels.
   void OnTransportChannelCreated();

pc/test/fake_data_channel_controller.h

@@ -87,8 +87,11 @@ class FakeDataChannelController
     // instantly, doing the same snapshot thing as below.
     for (webrtc::SctpDataChannel* ch : std::set<webrtc::SctpDataChannel*>(
              connected_channels_.begin(), connected_channels_.end())) {
-      if (connected_channels_.count(ch)) {
-        ch->OnClosingProcedureComplete(sid);
+      if (connected_channels_.count(ch) && ch->id() == sid) {
+        // This path mimics the DCC's OnChannelClosed handler since the FDCC
+        // (this class) doesn't have a transport that would do that.
+        DisconnectDataChannel(ch);
+        ch->OnClosingProcedureComplete();
       }
     }
   }