diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index 0ebfdfa4cc..1cad4eacd6 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -12,6 +12,7 @@ #include +#include "absl/algorithm/container.h" #include "api/peer_connection_interface.h" #include "api/rtc_error.h" #include "pc/peer_connection_internal.h" @@ -52,8 +53,6 @@ bool DataChannelController::ConnectDataChannel( webrtc_data_channel, &SctpDataChannel::OnDataReceived); SignalDataChannelTransportChannelClosing_s.connect( webrtc_data_channel, &SctpDataChannel::OnClosingProcedureStartedRemotely); - SignalDataChannelTransportChannelClosed_s.connect( - webrtc_data_channel, &SctpDataChannel::OnClosingProcedureComplete); return true; } @@ -68,7 +67,6 @@ void DataChannelController::DisconnectDataChannel( SignalDataChannelTransportWritable_s.disconnect(webrtc_data_channel); SignalDataChannelTransportReceivedData_s.disconnect(webrtc_data_channel); SignalDataChannelTransportChannelClosing_s.disconnect(webrtc_data_channel); - SignalDataChannelTransportChannelClosed_s.disconnect(webrtc_data_channel); } void DataChannelController::AddSctpDataStream(int sid) { @@ -143,7 +141,22 @@ void DataChannelController::OnChannelClosed(int channel_id) { signaling_thread()->PostTask( SafeTask(signaling_safety_.flag(), [this, channel_id] { RTC_DCHECK_RUN_ON(signaling_thread()); - SignalDataChannelTransportChannelClosed_s(channel_id); + auto it = absl::c_find_if(sctp_data_channels_, [&](const auto& c) { + return c->id() == channel_id; + }); + + // Remove the channel from our list, close it and free up resources. + if (it != sctp_data_channels_.end()) { + rtc::scoped_refptr channel = std::move(*it); + // Note: this causes OnSctpDataChannelClosed() to not do anything + // when called from within `OnClosingProcedureComplete`. + sctp_data_channels_.erase(it); + + DisconnectDataChannel(channel.get()); + sid_allocator_.ReleaseSid(channel->sid()); + + channel->OnClosingProcedureComplete(); + } })); } diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h index dcb6366ead..5fd0afb1df 100644 --- a/pc/data_channel_controller.h +++ b/pc/data_channel_controller.h @@ -163,8 +163,6 @@ class DataChannelController : public SctpDataChannelControllerInterface, RTC_GUARDED_BY(signaling_thread()); sigslot::signal1 SignalDataChannelTransportChannelClosing_s RTC_GUARDED_BY(signaling_thread()); - sigslot::signal1 SignalDataChannelTransportChannelClosed_s - RTC_GUARDED_BY(signaling_thread()); // Owning PeerConnection. PeerConnectionInternal* const pc_; diff --git a/pc/data_channel_unittest.cc b/pc/data_channel_unittest.cc index 973c6943df..529cb6b44b 100644 --- a/pc/data_channel_unittest.cc +++ b/pc/data_channel_unittest.cc @@ -162,20 +162,21 @@ TEST_F(SctpDataChannelTest, ConnectedAfterTransportBecomesAvailable) { // Tests the state of the data channel. TEST_F(SctpDataChannelTest, StateTransition) { + AddObserver(); + EXPECT_EQ(DataChannelInterface::kConnecting, webrtc_data_channel_->state()); - EXPECT_EQ(controller_->channels_opened(), 0); - EXPECT_EQ(controller_->channels_closed(), 0); + EXPECT_EQ(observer_->on_state_change_count(), 0u); SetChannelReady(); EXPECT_EQ(DataChannelInterface::kOpen, webrtc_data_channel_->state()); - EXPECT_EQ(controller_->channels_opened(), 1); - EXPECT_EQ(controller_->channels_closed(), 0); + EXPECT_EQ(observer_->on_state_change_count(), 1u); + // `Close()` should trigger two state changes, first `kClosing`, then + // `kClose`. webrtc_data_channel_->Close(); EXPECT_EQ(DataChannelInterface::kClosed, webrtc_data_channel_->state()); + EXPECT_EQ(observer_->on_state_change_count(), 3u); EXPECT_TRUE(webrtc_data_channel_->error().ok()); - EXPECT_EQ(controller_->channels_opened(), 1); - EXPECT_EQ(controller_->channels_closed(), 1); // Verifies that it's disconnected from the transport. EXPECT_FALSE(controller_->IsConnected(webrtc_data_channel_.get())); } diff --git a/pc/sctp_data_channel.cc b/pc/sctp_data_channel.cc index 81668dd4bc..be2a480a1a 100644 --- a/pc/sctp_data_channel.cc +++ b/pc/sctp_data_channel.cc @@ -345,11 +345,7 @@ bool SctpDataChannel::Send(const DataBuffer& buffer) { // If the queue is non-empty, we're waiting for SignalReadyToSend, // so just add to the end of the queue and keep waiting. if (!queued_send_data_.Empty()) { - if (!QueueSendDataMessage(buffer)) { - // Queue is full - return false; - } - return true; + return QueueSendDataMessage(buffer); } SendDataMessage(buffer, true); @@ -389,16 +385,14 @@ void SctpDataChannel::OnClosingProcedureStartedRemotely(int sid) { } } -void SctpDataChannel::OnClosingProcedureComplete(int sid) { +void SctpDataChannel::OnClosingProcedureComplete() { RTC_DCHECK_RUN_ON(signaling_thread_); - if (id_.stream_id_int() == sid) { - // If the closing procedure is complete, we should have finished sending - // all pending data and transitioned to kClosing already. - RTC_DCHECK_EQ(state_, kClosing); - RTC_DCHECK(queued_send_data_.Empty()); - DisconnectFromTransport(); - SetState(kClosed); - } + // If the closing procedure is complete, we should have finished sending + // all pending data and transitioned to kClosing already. + RTC_DCHECK_EQ(state_, kClosing); + RTC_DCHECK(queued_send_data_.Empty()); + + SetState(kClosed); } void SctpDataChannel::OnTransportChannelCreated() { @@ -515,9 +509,7 @@ void SctpDataChannel::CloseAbruptlyWithError(RTCError error) { return; } - if (connected_to_transport_) { - DisconnectFromTransport(); - } + DisconnectFromTransport(); // Closing abruptly means any queued data gets thrown away. queued_send_data_.Clear(); diff --git a/pc/sctp_data_channel.h b/pc/sctp_data_channel.h index 7fa7173b59..17cad2698e 100644 --- a/pc/sctp_data_channel.h +++ b/pc/sctp_data_channel.h @@ -194,7 +194,7 @@ class SctpDataChannel : public DataChannelInterface, // The closing procedure is complete; both incoming and outgoing stream // resets are done and the channel can transition to kClosed. Called // asynchronously after RemoveSctpDataStream. - void OnClosingProcedureComplete(int sid); + void OnClosingProcedureComplete(); // Called when the transport channel is created. // Only needs to be called for SCTP data channels. void OnTransportChannelCreated(); diff --git a/pc/test/fake_data_channel_controller.h b/pc/test/fake_data_channel_controller.h index f7a2afe1df..b0c16b7f52 100644 --- a/pc/test/fake_data_channel_controller.h +++ b/pc/test/fake_data_channel_controller.h @@ -87,8 +87,11 @@ class FakeDataChannelController // instantly, doing the same snapshot thing as below. for (webrtc::SctpDataChannel* ch : std::set( connected_channels_.begin(), connected_channels_.end())) { - if (connected_channels_.count(ch)) { - ch->OnClosingProcedureComplete(sid); + if (connected_channels_.count(ch) && ch->id() == sid) { + // This path mimics the DCC's OnChannelClosed handler since the FDCC + // (this class) doesn't have a transport that would do that. + DisconnectDataChannel(ch); + ch->OnClosingProcedureComplete(); } } }