diff --git a/pc/BUILD.gn b/pc/BUILD.gn index cd91e0801e..2776f9f245 100644 --- a/pc/BUILD.gn +++ b/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", ] } diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index d0b39fc385..3011c0f5f6 100644 --- a/pc/data_channel_controller.cc +++ b/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 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; } } diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h index 691f2cd419..dcb6366ead 100644 --- a/pc/data_channel_controller.h +++ b/pc/data_channel_controller.h @@ -150,8 +150,6 @@ class DataChannelController : public SctpDataChannelControllerInterface, SctpSidAllocator sid_allocator_ /* RTC_GUARDED_BY(signaling_thread()) */; std::vector> sctp_data_channels_ RTC_GUARDED_BY(signaling_thread()); - std::vector> sctp_data_channels_to_free_ - RTC_GUARDED_BY(signaling_thread()); // Signals from `data_channel_transport_`. These are invoked on the // signaling thread. diff --git a/pc/data_channel_controller_unittest.cc b/pc/data_channel_controller_unittest.cc index 2f5c924796..097eed61ed 100644 --- a/pc/data_channel_controller_unittest.cc +++ b/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> pc_; }; @@ -72,5 +75,46 @@ TEST_F(DataChannelControllerTest, CloseAfterControllerDestroyed) { channel->Close(); } +TEST_F(DataChannelControllerTest, AsyncChannelCloseTeardown) { + DataChannelController dcc(pc_.get()); + rtc::scoped_refptr channel = + dcc.InternalCreateDataChannelWithProxy( + "label", + std::make_unique(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