Update DataChannelControllerTests to exercise teardown path.
This updates DataChannelControllerTests to shut down the DCC in the
same way it's shut down by the owning PeerConnection instance:
* Call TeardownDataChannelTransport_n()
* Call PrepareForShutdown()
Also calling PrepareForShutdown() from PC's dtor to be consistent with
how `sdp_handler_->PrepareForShutdown()` is called since it appears
that many tests do not call PC::Close() before destruction.
Bug: b/276434297
Change-Id: I0379baa0df0e764bc255b83ae0667032acfe3db0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300220
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39756}
diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc
index 92cdba3..36e8be1 100644
--- a/pc/data_channel_controller.cc
+++ b/pc/data_channel_controller.cc
@@ -22,17 +22,10 @@
namespace webrtc {
DataChannelController::~DataChannelController() {
-#if RTC_DCHECK_IS_ON
- // `sctp_data_channels_n_` might be empty while `sctp_data_channels_` is
- // not. An example of that is when the `DataChannelController` goes out of
- // scope with outstanding channels that have been properly terminated on the
- // network thread but not yet cleared from `sctp_data_channels_`. However,
- // if `sctp_data_channels_n_` is not empty, then `sctp_data_channels_n_` and
- // sctp_data_channels_ should hold the same contents.
- if (!sctp_data_channels_n_.empty()) {
- RTC_DCHECK_EQ(sctp_data_channels_n_.size(), sctp_data_channels_.size());
- }
-#endif
+ RTC_DCHECK(sctp_data_channels_n_.empty())
+ << "Missing call to TeardownDataChannelTransport_n?";
+ RTC_DCHECK(!signaling_safety_.flag()->alive())
+ << "Missing call to PrepareForShutdown?";
}
bool DataChannelController::HasDataChannelsForTest() const {
@@ -167,7 +160,7 @@
void DataChannelController::PrepareForShutdown() {
RTC_DCHECK_RUN_ON(signaling_thread());
- signaling_safety_.reset();
+ signaling_safety_.reset(PendingTaskSafetyFlag::CreateDetachedInactive());
}
void DataChannelController::TeardownDataChannelTransport_n() {
diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h
index f71e7a4..fa6c13e 100644
--- a/pc/data_channel_controller.h
+++ b/pc/data_channel_controller.h
@@ -102,6 +102,10 @@
void OnSctpDataChannelClosed(SctpDataChannel* channel);
+ protected:
+ rtc::Thread* network_thread() const;
+ rtc::Thread* signaling_thread() const;
+
private:
// Parses and handles open messages. Returns true if the message is an open
// message and should be considered to be handled, false otherwise.
@@ -138,9 +142,6 @@
std::vector<rtc::scoped_refptr<SctpDataChannel>>::iterator FindChannel(
StreamId stream_id);
- rtc::Thread* network_thread() const;
- rtc::Thread* signaling_thread() const;
-
// Plugin transport used for data channels. Pointer may be accessed and
// checked from any thread, but the object may only be touched on the
// network thread.
diff --git a/pc/data_channel_controller_unittest.cc b/pc/data_channel_controller_unittest.cc
index 470c01c..9b66fac 100644
--- a/pc/data_channel_controller_unittest.cc
+++ b/pc/data_channel_controller_unittest.cc
@@ -43,6 +43,23 @@
MOCK_METHOD(bool, IsReadyToSend, (), (const, override));
};
+// Convenience class for tests to ensure that shutdown methods for DCC
+// are consistently called. In practice SdpOfferAnswerHandler will call
+// TeardownDataChannelTransport_n on the network thread when destroying the
+// data channel transport and PeerConnection calls PrepareForShutdown() from
+// within PeerConnection::Close(). The DataChannelControllerForTest class mimics
+// behavior by calling those methods from within its destructor.
+class DataChannelControllerForTest : public DataChannelController {
+ public:
+ explicit DataChannelControllerForTest(PeerConnectionInternal* pc)
+ : DataChannelController(pc) {}
+
+ ~DataChannelControllerForTest() override {
+ network_thread()->BlockingCall([&] { TeardownDataChannelTransport_n(); });
+ PrepareForShutdown();
+ }
+};
+
class DataChannelControllerTest : public ::testing::Test {
protected:
DataChannelControllerTest()
@@ -65,11 +82,11 @@
};
TEST_F(DataChannelControllerTest, CreateAndDestroy) {
- DataChannelController dcc(pc_.get());
+ DataChannelControllerForTest dcc(pc_.get());
}
TEST_F(DataChannelControllerTest, CreateDataChannelEarlyRelease) {
- DataChannelController dcc(pc_.get());
+ DataChannelControllerForTest dcc(pc_.get());
auto ret = dcc.InternalCreateDataChannelWithProxy(
"label", InternalDataChannelInit(DataChannelInit()));
ASSERT_TRUE(ret.ok());
@@ -79,7 +96,7 @@
}
TEST_F(DataChannelControllerTest, CreateDataChannelEarlyClose) {
- DataChannelController dcc(pc_.get());
+ DataChannelControllerForTest dcc(pc_.get());
EXPECT_FALSE(dcc.HasDataChannelsForTest());
EXPECT_FALSE(dcc.HasUsedDataChannels());
auto ret = dcc.InternalCreateDataChannelWithProxy(
@@ -89,12 +106,13 @@
EXPECT_TRUE(dcc.HasDataChannelsForTest());
EXPECT_TRUE(dcc.HasUsedDataChannels());
channel->Close();
+ run_loop_.Flush();
EXPECT_FALSE(dcc.HasDataChannelsForTest());
EXPECT_TRUE(dcc.HasUsedDataChannels());
}
TEST_F(DataChannelControllerTest, CreateDataChannelLateRelease) {
- auto dcc = std::make_unique<DataChannelController>(pc_.get());
+ auto dcc = std::make_unique<DataChannelControllerForTest>(pc_.get());
auto ret = dcc->InternalCreateDataChannelWithProxy(
"label", InternalDataChannelInit(DataChannelInit()));
ASSERT_TRUE(ret.ok());
@@ -104,7 +122,7 @@
}
TEST_F(DataChannelControllerTest, CloseAfterControllerDestroyed) {
- auto dcc = std::make_unique<DataChannelController>(pc_.get());
+ auto dcc = std::make_unique<DataChannelControllerForTest>(pc_.get());
auto ret = dcc->InternalCreateDataChannelWithProxy(
"label", InternalDataChannelInit(DataChannelInit()));
ASSERT_TRUE(ret.ok());
@@ -114,7 +132,7 @@
}
TEST_F(DataChannelControllerTest, AsyncChannelCloseTeardown) {
- DataChannelController dcc(pc_.get());
+ DataChannelControllerForTest dcc(pc_.get());
auto ret = dcc.InternalCreateDataChannelWithProxy(
"label", InternalDataChannelInit(DataChannelInit()));
ASSERT_TRUE(ret.ok());
@@ -162,7 +180,7 @@
: rtc::SSL_CLIENT);
});
- DataChannelController dcc(pc_.get());
+ DataChannelControllerForTest dcc(pc_.get());
pc_->network_thread()->BlockingCall(
[&] { dcc.set_data_channel_transport(&transport); });
@@ -189,8 +207,8 @@
return rtc::SSL_CLIENT;
});
- DataChannelController dcc(pc_.get());
- NiceMock<MockDataChannelTransport> transport;
+ NiceMock<MockDataChannelTransport> transport; // Wider scope than `dcc`.
+ DataChannelControllerForTest dcc(pc_.get());
pc_->network_thread()->BlockingCall(
[&] { dcc.set_data_channel_transport(&transport); });
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index f365601..fdbd32b 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -588,6 +588,8 @@
// The event log must outlive call (and any other object that uses it).
event_log_.reset();
});
+
+ data_channel_controller_.PrepareForShutdown();
}
RTCError PeerConnection::Initialize(
