Remove DataChannelController::sctp_data_channels_to_free_

Instead, just use the posted task to release the reference to a
pending data channel object.

Bug: none
Change-Id: I34f0bfd604cab88587a892eaa218856c890fc907
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/296767
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39527}

pc/BUILD.gn

@@ -2573,6 +2573,7 @@ if (rtc_include_tests && !build_with_chromium) {
       ":pc_test_utils",
       ":peer_connection_internal",
       ":sctp_data_channel",
+      "../test:run_loop",
       "../test:test_support",
     ]
   }

pc/data_channel_controller.cc

@@ -331,14 +331,13 @@ void DataChannelController::OnSctpDataChannelClosed(SctpDataChannel* channel) {
         // another data channel.
         sid_allocator_.ReleaseSid(channel->id());
       }
+
       // Since this method is triggered by a signal from the DataChannel,
       // we can't free it directly here; we need to free it asynchronously.
-      sctp_data_channels_to_free_.push_back(*it);
+      rtc::scoped_refptr<SctpDataChannel> release = std::move(*it);
       sctp_data_channels_.erase(it);
-      signaling_thread()->PostTask(SafeTask(signaling_safety_.flag(), [this] {
-        RTC_DCHECK_RUN_ON(signaling_thread());
-        sctp_data_channels_to_free_.clear();
-      }));
+      signaling_thread()->PostTask(SafeTask(signaling_safety_.flag(),
+                                            [release = std::move(release)] {}));
       return;
     }
   }

pc/data_channel_controller.h

@@ -150,8 +150,6 @@ class DataChannelController : public SctpDataChannelControllerInterface,
   SctpSidAllocator sid_allocator_ /* RTC_GUARDED_BY(signaling_thread()) */;
   std::vector<rtc::scoped_refptr<SctpDataChannel>> sctp_data_channels_
       RTC_GUARDED_BY(signaling_thread());
-  std::vector<rtc::scoped_refptr<SctpDataChannel>> sctp_data_channels_to_free_
-      RTC_GUARDED_BY(signaling_thread());
 
   // Signals from `data_channel_transport_`.  These are invoked on the
   // signaling thread.

pc/data_channel_controller_unittest.cc

@@ -17,6 +17,7 @@
 #include "pc/test/mock_peer_connection_internal.h"
 #include "test/gmock.h"
 #include "test/gtest.h"
+#include "test/run_loop.h"
 
 namespace webrtc {
 
@@ -33,7 +34,9 @@ class DataChannelControllerTest : public ::testing::Test {
         .WillByDefault(Return(rtc::Thread::Current()));
   }
 
-  rtc::AutoThread main_thread_;
+  ~DataChannelControllerTest() override { run_loop_.Flush(); }
+
+  test::RunLoop run_loop_;
   rtc::scoped_refptr<NiceMock<MockPeerConnectionInternal>> pc_;
 };
 
@@ -72,5 +75,46 @@ TEST_F(DataChannelControllerTest, CloseAfterControllerDestroyed) {
   channel->Close();
 }
 
+TEST_F(DataChannelControllerTest, AsyncChannelCloseTeardown) {
+  DataChannelController dcc(pc_.get());
+  rtc::scoped_refptr<DataChannelInterface> channel =
+      dcc.InternalCreateDataChannelWithProxy(
+          "label",
+          std::make_unique<InternalDataChannelInit>(DataChannelInit()).get());
+  SctpDataChannel* inner_channel =
+      DowncastProxiedDataChannelInterfaceToSctpDataChannelForTesting(
+          channel.get());
+  // Grab a reference for testing purposes.
+  inner_channel->AddRef();
+
+  channel = nullptr;  // dcc still holds a reference to `channel`.
+  EXPECT_TRUE(dcc.HasDataChannels());
+
+  // Make sure callbacks to dcc are set up.
+  dcc.ConnectDataChannel(inner_channel);
+
+  // Trigger a Close() for the channel. This will send events back to dcc,
+  // eventually reaching `OnSctpDataChannelClosed` where dcc removes
+  // the channel from the internal list of data channels, but does not release
+  // the reference synchronously since that reference might be the last one.
+  inner_channel->Close();
+  // Now there should be no tracked data channels.
+  EXPECT_FALSE(dcc.HasDataChannels());
+  // But there should be an async operation queued that still holds a reference.
+  // That means that the test reference, must not be the last one.
+  ASSERT_NE(inner_channel->Release(),
+            rtc::RefCountReleaseStatus::kDroppedLastRef);
+  // Grab a reference again (using the pointer is safe since the object still
+  // exists and we control the single-threaded environment manually).
+  inner_channel->AddRef();
+  // Now run the queued up async operations on the signaling (current) thread.
+  // This time, the reference formerly owned by dcc, should be release and the
+  // truly last reference is now held by the test.
+  run_loop_.Flush();
+  // Check that this is the last reference.
+  EXPECT_EQ(inner_channel->Release(),
+            rtc::RefCountReleaseStatus::kDroppedLastRef);
+}
+
 }  // namespace
 }  // namespace webrtc