diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index 3545546da6..a4c8d8852b 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -146,12 +146,17 @@ void DataChannelController::OnReadyToSend() { void DataChannelController::OnTransportClosed(RTCError error) { RTC_DCHECK_RUN_ON(network_thread()); + // This loop will close all data channels and trigger a callback to - // `OnSctpDataChannelClosed` which will modify `sctp_data_channels_n_`, so - // we create a local copy while we do the fan-out. - auto copy = sctp_data_channels_n_; - for (const auto& channel : copy) + // `OnSctpDataChannelClosed`. We'll empty `sctp_data_channels_n_`, first + // and `OnSctpDataChannelClosed` will become a noop but we'll release the + // StreamId here. + std::vector> temp_sctp_dcs; + temp_sctp_dcs.swap(sctp_data_channels_n_); + for (const auto& channel : temp_sctp_dcs) { channel->OnTransportChannelClosed(error); + sid_allocator_.ReleaseSid(channel->sid_n()); + } } void DataChannelController::SetupDataChannelTransport_n() { @@ -168,13 +173,17 @@ void DataChannelController::PrepareForShutdown() { signaling_safety_.reset(PendingTaskSafetyFlag::CreateDetachedInactive()); } -void DataChannelController::TeardownDataChannelTransport_n() { +void DataChannelController::TeardownDataChannelTransport_n(RTCError error) { RTC_DCHECK_RUN_ON(network_thread()); + + OnTransportClosed(error); + if (data_channel_transport_) { data_channel_transport_->SetDataSink(nullptr); set_data_channel_transport(nullptr); } - sctp_data_channels_n_.clear(); + + RTC_DCHECK(sctp_data_channels_n_.empty()); weak_factory_.InvalidateWeakPtrs(); } @@ -404,23 +413,6 @@ void DataChannelController::OnSctpDataChannelClosed(SctpDataChannel* channel) { sctp_data_channels_n_.erase(it); } -void DataChannelController::OnTransportChannelClosed(RTCError error) { - RTC_DCHECK_RUN_ON(network_thread()); - // Use a temporary copy of the SCTP DataChannel list because the - // DataChannel may callback to us and try to modify the list. - // TODO(tommi): `OnTransportChannelClosed` is called from - // `SdpOfferAnswerHandler::DestroyDataChannelTransport` just before - // `TeardownDataChannelTransport_n` is called (but on the network thread) from - // the same function. We can now get rid of this function - // (OnTransportChannelClosed) and run this loop from within the - // TeardownDataChannelTransport_n callback. - std::vector> temp_sctp_dcs; - temp_sctp_dcs.swap(sctp_data_channels_n_); - for (const auto& channel : temp_sctp_dcs) { - channel->OnTransportChannelClosed(error); - } -} - void DataChannelController::set_data_channel_transport( DataChannelTransportInterface* transport) { RTC_DCHECK_RUN_ON(network_thread()); diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h index edd21c2390..9e6af3fa5b 100644 --- a/pc/data_channel_controller.h +++ b/pc/data_channel_controller.h @@ -70,7 +70,7 @@ class DataChannelController : public SctpDataChannelControllerInterface, // Called from PeerConnection::SetupDataChannelTransport_n void SetupDataChannelTransport_n(); // Called from PeerConnection::TeardownDataChannelTransport_n - void TeardownDataChannelTransport_n(); + void TeardownDataChannelTransport_n(RTCError error); // Called from PeerConnection::OnTransportChanged // to make required changes to datachannels' transports. @@ -96,9 +96,6 @@ class DataChannelController : public SctpDataChannelControllerInterface, // Accessors void set_data_channel_transport(DataChannelTransportInterface* transport); - // Called when the transport for the data channels is closed or destroyed. - void OnTransportChannelClosed(RTCError error); - void OnSctpDataChannelClosed(SctpDataChannel* channel); protected: diff --git a/pc/data_channel_controller_unittest.cc b/pc/data_channel_controller_unittest.cc index fd3deae0d7..a2a9f00cd3 100644 --- a/pc/data_channel_controller_unittest.cc +++ b/pc/data_channel_controller_unittest.cc @@ -55,7 +55,8 @@ class DataChannelControllerForTest : public DataChannelController { : DataChannelController(pc) {} ~DataChannelControllerForTest() override { - network_thread()->BlockingCall([&] { TeardownDataChannelTransport_n(); }); + network_thread()->BlockingCall( + [&] { TeardownDataChannelTransport_n(RTCError::OK()); }); PrepareForShutdown(); } }; diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 82c5914a52..f76ce6396d 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -573,7 +573,7 @@ PeerConnection::~PeerConnection() { transport_controller_copy_ = nullptr; network_thread()->BlockingCall([this] { RTC_DCHECK_RUN_ON(network_thread()); - TeardownDataChannelTransport_n(); + TeardownDataChannelTransport_n(RTCError::OK()); transport_controller_.reset(); port_allocator_.reset(); if (network_thread_safety_) @@ -2544,7 +2544,7 @@ bool PeerConnection::SetupDataChannelTransport_n(const std::string& mid) { return true; } -void PeerConnection::TeardownDataChannelTransport_n() { +void PeerConnection::TeardownDataChannelTransport_n(RTCError error) { if (sctp_mid_n_) { // `sctp_mid_` may still be active through an SCTP transport. If not, unset // it. @@ -2553,7 +2553,7 @@ void PeerConnection::TeardownDataChannelTransport_n() { sctp_mid_n_.reset(); } - data_channel_controller_.TeardownDataChannelTransport_n(); + data_channel_controller_.TeardownDataChannelTransport_n(error); } // Returns false if bundle is enabled and rtcp_mux is disabled. diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 950758be07..e7cfb9a5c1 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -434,7 +434,8 @@ class PeerConnection : public PeerConnectionInternal, bool SetupDataChannelTransport_n(const std::string& mid) override RTC_RUN_ON(network_thread()); - void TeardownDataChannelTransport_n() override RTC_RUN_ON(network_thread()); + void TeardownDataChannelTransport_n(RTCError error) override + RTC_RUN_ON(network_thread()); const FieldTrialsView& trials() const override { return *trials_; } diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h index abf5825bd6..ab9789d3ab 100644 --- a/pc/peer_connection_internal.h +++ b/pc/peer_connection_internal.h @@ -118,7 +118,7 @@ class PeerConnectionSdpMethods { // this session. virtual bool SrtpRequired() const = 0; virtual bool SetupDataChannelTransport_n(const std::string& mid) = 0; - virtual void TeardownDataChannelTransport_n() = 0; + virtual void TeardownDataChannelTransport_n(RTCError error) = 0; virtual void SetSctpDataMid(const std::string& mid) = 0; virtual void ResetSctpDataMid() = 0; diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 0363042687..4a86ba80b7 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -5110,18 +5110,12 @@ bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) { void SdpOfferAnswerHandler::DestroyDataChannelTransport(RTCError error) { RTC_DCHECK_RUN_ON(signaling_thread()); - const bool has_sctp = pc_->sctp_mid().has_value(); - context_->network_thread()->BlockingCall( [&, data_channel_controller = data_channel_controller()] { RTC_DCHECK_RUN_ON(context_->network_thread()); - if (has_sctp) - data_channel_controller->OnTransportChannelClosed(error); - pc_->TeardownDataChannelTransport_n(); + pc_->TeardownDataChannelTransport_n(error); }); - - if (has_sctp) - pc_->ResetSctpDataMid(); + pc_->ResetSctpDataMid(); } void SdpOfferAnswerHandler::DestroyAllChannels() { diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h index 0508673d36..3178770f7c 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/pc/test/fake_peer_connection_base.h @@ -361,7 +361,7 @@ class FakePeerConnectionBase : public PeerConnectionInternal { bool SetupDataChannelTransport_n(const std::string& mid) override { return false; } - void TeardownDataChannelTransport_n() override {} + void TeardownDataChannelTransport_n(RTCError error) override {} void SetSctpDataMid(const std::string& mid) override {} void ResetSctpDataMid() override {} diff --git a/pc/test/mock_peer_connection_internal.h b/pc/test/mock_peer_connection_internal.h index 4647567559..b8107a3d5b 100644 --- a/pc/test/mock_peer_connection_internal.h +++ b/pc/test/mock_peer_connection_internal.h @@ -267,7 +267,7 @@ class MockPeerConnectionInternal : public PeerConnectionInternal { SetupDataChannelTransport_n, (const std::string&), (override)); - MOCK_METHOD(void, TeardownDataChannelTransport_n, (), (override)); + MOCK_METHOD(void, TeardownDataChannelTransport_n, (RTCError), (override)); MOCK_METHOD(void, SetSctpDataMid, (const std::string&), (override)); MOCK_METHOD(void, ResetSctpDataMid, (), (override)); MOCK_METHOD(const FieldTrialsView&, trials, (), (const, override));