commit f21354ce0efaa51bb60b0a98b63f2584852cbfc2
author Tommi <tommi@webrtc.org> Tue Mar 07 07:43:24 2023
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Mar 09 11:20:45 2023
tree 9470c45895d5422bc066ca523ecb2b07a4d10c18
parent 5a54800957b050fff9965d3608287db9c1f75348

Transition data channnel to kClosed when not connected to a transport.

If a data channel object was closed (via calling Close()) right after
construction and before attaching to a transport, it would never
transition to the `kClosed` state. This addresses that corner case,
which caused a DCHECK to trigger but might also cause a situation
whereby more than one DC instance existed for a given sctp sid.

Bug: chromium:1421534
Change-Id: Id757c0528f929f2e2daa5343236d7f62e309f6cc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/296341
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39513}

pc/data_channel_unittest.cc

diff --git a/pc/data_channel_unittest.cc b/pc/data_channel_unittest.cc
index 8099716..d9575f0 100644
--- a/pc/data_channel_unittest.cc
+++ b/pc/data_channel_unittest.cc
@@ -618,6 +618,15 @@
   webrtc_data_channel_->Close();
 }
 
+// Tests that a data channel that's not connected to a transport can transition
+// directly to the `kClosed` state when closed.
+// See also chromium:1421534.
+TEST_F(SctpDataChannelTest, UnusedTransitionsDirectlyToClosed) {
+  webrtc_data_channel_->Close();
+  EXPECT_EQ(webrtc::DataChannelInterface::kClosed,
+            webrtc_data_channel_->state());
+}
+
 // Test that the data channel goes to the "closed" state (and doesn't crash)
 // when its transport goes away, even while data is buffered.
 TEST_F(SctpDataChannelTest, TransportDestroyedWhileDataBuffered) {

pc/sctp_data_channel.cc

diff --git a/pc/sctp_data_channel.cc b/pc/sctp_data_channel.cc
index cb7c1c3..d41baaf 100644
--- a/pc/sctp_data_channel.cc
+++ b/pc/sctp_data_channel.cc
@@ -539,18 +539,25 @@
       break;
     }
     case kClosing: {
-      // Wait for all queued data to be sent before beginning the closing
-      // procedure.
-      if (queued_send_data_.Empty() && queued_control_data_.Empty()) {
-        // For SCTP data channels, we need to wait for the closing procedure
-        // to complete; after calling RemoveSctpDataStream,
-        // OnClosingProcedureComplete will end up called asynchronously
-        // afterwards.
-        if (connected_to_transport_ && !started_closing_procedure_ &&
-            controller_ && config_.id >= 0) {
-          started_closing_procedure_ = true;
-          controller_->RemoveSctpDataStream(config_.id);
+      if (connected_to_transport_) {
+        // Wait for all queued data to be sent before beginning the closing
+        // procedure.
+        if (queued_send_data_.Empty() && queued_control_data_.Empty()) {
+          // For SCTP data channels, we need to wait for the closing procedure
+          // to complete; after calling RemoveSctpDataStream,
+          // OnClosingProcedureComplete will end up called asynchronously
+          // afterwards.
+          if (!started_closing_procedure_ && controller_ && config_.id >= 0) {
+            started_closing_procedure_ = true;
+            controller_->RemoveSctpDataStream(config_.id);
+          }
         }
+      } else {
+        // When we're not connected to a transport, we'll transition
+        // directly to the `kClosed` state from here.
+        queued_send_data_.Clear();
+        queued_control_data_.Clear();
+        SetState(kClosed);
       }
       break;
     }