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}

pc/data_channel_controller.cc

@@ -22,17 +22,10 @@
 namespace webrtc {
 
 DataChannelController::~DataChannelController() {
-#if RTC_DCHECK_IS_ON
+  RTC_DCHECK(sctp_data_channels_n_.empty())
-  // `sctp_data_channels_n_` might be empty while `sctp_data_channels_` is
+      << "Missing call to TeardownDataChannelTransport_n?";
-  // not. An example of that is when the `DataChannelController` goes out of
+  RTC_DCHECK(!signaling_safety_.flag()->alive())
-  // scope with outstanding channels that have been properly terminated on the
+      << "Missing call to PrepareForShutdown?";
-  // 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
 }
 
 bool DataChannelController::HasDataChannelsForTest() const {
@@ -167,7 +160,7 @@ void DataChannelController::SetupDataChannelTransport_n() {
 
 void DataChannelController::PrepareForShutdown() {
   RTC_DCHECK_RUN_ON(signaling_thread());
-  signaling_safety_.reset();
+  signaling_safety_.reset(PendingTaskSafetyFlag::CreateDetachedInactive());
 }
 
 void DataChannelController::TeardownDataChannelTransport_n() {

pc/data_channel_controller.h

@@ -102,6 +102,10 @@ class DataChannelController : public SctpDataChannelControllerInterface,
 
   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 @@ class DataChannelController : public SctpDataChannelControllerInterface,
   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.

pc/data_channel_controller_unittest.cc

@@ -43,6 +43,23 @@ class MockDataChannelTransport : public webrtc::DataChannelTransportInterface {
   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 @@ class DataChannelControllerTest : public ::testing::Test {
 };
 
 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, CreateDataChannelEarlyRelease) {
 }
 
 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 @@ TEST_F(DataChannelControllerTest, CreateDataChannelEarlyClose) {
   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, CreateDataChannelLateRelease) {
 }
 
 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, CloseAfterControllerDestroyed) {
 }
 
 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 @@ TEST_F(DataChannelControllerTest, MaxChannels) {
                                                          : 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 @@ TEST_F(DataChannelControllerTest, NoStreamIdReuseWhileClosing) {
     return rtc::SSL_CLIENT;
   });
 
-  DataChannelController dcc(pc_.get());
+  NiceMock<MockDataChannelTransport> transport;  // Wider scope than `dcc`.
-  NiceMock<MockDataChannelTransport> transport;
+  DataChannelControllerForTest dcc(pc_.get());
   pc_->network_thread()->BlockingCall(
       [&] { dcc.set_data_channel_transport(&transport); });
 

pc/peer_connection.cc

@@ -588,6 +588,8 @@ PeerConnection::~PeerConnection() {
     // The event log must outlive call (and any other object that uses it).
     event_log_.reset();
   });
+
+  data_channel_controller_.PrepareForShutdown();
 }
 
 RTCError PeerConnection::Initialize(