From 9e5aeb9d92981818f1ed6dd9719df2edf0e373f0 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Wed, 11 May 2022 09:35:36 +0000 Subject: [PATCH] Safeguard SctpDataChannel against detached controller Since the lifetime of an SctpDataChannel is not strictly controlled by its controller, the controller might go away before the channel does. This CL guards against this. Bug: webrtc:13931 Change-Id: I07046fe896d1a66bf89287429beb0587382a13a9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261940 Commit-Queue: Harald Alvestrand Reviewed-by: Florent Castelli Cr-Commit-Position: refs/heads/main@{#36852} --- pc/BUILD.gn | 3 +- pc/data_channel_controller.cc | 9 ++ pc/data_channel_controller.h | 3 +- pc/data_channel_controller_unittest.cc | 17 ++- pc/data_channel_unittest.cc | 120 +++++++++--------- pc/rtc_stats_collector_unittest.cc | 8 +- pc/sctp_data_channel.cc | 70 ++++++---- pc/sctp_data_channel.h | 27 ++-- ...vider.h => fake_data_channel_controller.h} | 14 +- pc/test/fake_peer_connection_for_stats.h | 6 +- 10 files changed, 167 insertions(+), 110 deletions(-) rename pc/test/{fake_data_channel_provider.h => fake_data_channel_controller.h} (94%) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index dcb4c2d4e5..066723547a 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -2524,6 +2524,7 @@ if (rtc_include_tests && !build_with_chromium) { ":data_channel_controller", ":pc_test_utils", ":peer_connection_internal", + ":sctp_data_channel", "../test:test_support", ] } @@ -2671,7 +2672,7 @@ if (rtc_include_tests && !build_with_chromium) { sources = [ "test/fake_audio_capture_module.cc", "test/fake_audio_capture_module.h", - "test/fake_data_channel_provider.h", + "test/fake_data_channel_controller.h", "test/fake_peer_connection_base.h", "test/fake_peer_connection_for_stats.h", "test/fake_periodic_video_source.h", diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index a9fa3dabb9..8dde41724c 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -22,6 +22,15 @@ namespace webrtc { +DataChannelController::~DataChannelController() { + // Since channels may have multiple owners, we cannot guarantee that + // they will be deallocated before destroying the controller. + // Therefore, detach them from the controller. + for (auto channel : sctp_data_channels_) { + channel->DetachFromController(); + } +} + bool DataChannelController::HasDataChannels() const { RTC_DCHECK_RUN_ON(signaling_thread()); return !sctp_data_channels_.empty(); diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h index 56a9435f9e..cec79038c6 100644 --- a/pc/data_channel_controller.h +++ b/pc/data_channel_controller.h @@ -34,10 +34,11 @@ namespace webrtc { class PeerConnectionInternal; -class DataChannelController : public SctpDataChannelProviderInterface, +class DataChannelController : public SctpDataChannelControllerInterface, public DataChannelSink { public: explicit DataChannelController(PeerConnectionInternal* pc) : pc_(pc) {} + ~DataChannelController(); // Not copyable or movable. DataChannelController(DataChannelController&) = delete; diff --git a/pc/data_channel_controller_unittest.cc b/pc/data_channel_controller_unittest.cc index 16985f9834..7a1f68a52f 100644 --- a/pc/data_channel_controller_unittest.cc +++ b/pc/data_channel_controller_unittest.cc @@ -13,6 +13,7 @@ #include #include "pc/peer_connection_internal.h" +#include "pc/sctp_data_channel.h" #include "pc/test/mock_peer_connection_internal.h" #include "test/gmock.h" #include "test/gtest.h" @@ -44,7 +45,7 @@ TEST_F(DataChannelControllerTest, CreateDataChannelEarlyRelease) { auto channel = dcc.InternalCreateDataChannelWithProxy( "label", std::make_unique(DataChannelInit()).get()); - channel = nullptr; // Should call destructor of channel + channel = nullptr; // dcc holds a reference to channel, so not destroyed yet } TEST_F(DataChannelControllerTest, CreateDataChannelLateRelease) { @@ -56,5 +57,19 @@ TEST_F(DataChannelControllerTest, CreateDataChannelLateRelease) { channel = nullptr; } +TEST_F(DataChannelControllerTest, CloseAfterControllerDestroyed) { + auto dcc = std::make_unique(pc_.get()); + auto channel = dcc->InternalCreateDataChannelWithProxy( + "label", + std::make_unique(DataChannelInit()).get()); + // Connect to provider + auto inner_channel = + DowncastProxiedDataChannelInterfaceToSctpDataChannelForTesting( + channel.get()); + dcc->ConnectDataChannel(inner_channel); + dcc.reset(); + channel->Close(); +} + } // namespace } // namespace webrtc diff --git a/pc/data_channel_unittest.cc b/pc/data_channel_unittest.cc index 5797d1da44..55f9c34fea 100644 --- a/pc/data_channel_unittest.cc +++ b/pc/data_channel_unittest.cc @@ -23,7 +23,7 @@ #include "media/sctp/sctp_transport_internal.h" #include "pc/sctp_data_channel.h" #include "pc/sctp_utils.h" -#include "pc/test/fake_data_channel_provider.h" +#include "pc/test/fake_data_channel_controller.h" #include "rtc_base/copy_on_write_buffer.h" #include "rtc_base/gunit.h" #include "rtc_base/ssl_stream_adapter.h" @@ -72,27 +72,27 @@ class FakeDataChannelObserver : public webrtc::DataChannelObserver { size_t on_buffered_amount_change_count_; }; -// TODO(deadbeef): The fact that these tests use a fake provider makes them not -// too valuable. Should rewrite using the +// TODO(deadbeef): The fact that these tests use a fake controller makes them +// not too valuable. Should rewrite using the // peerconnection_datachannel_unittest.cc infrastructure. // TODO(bugs.webrtc.org/11547): Incorporate a dedicated network thread. class SctpDataChannelTest : public ::testing::Test { protected: SctpDataChannelTest() - : provider_(new FakeDataChannelProvider()), - webrtc_data_channel_(SctpDataChannel::Create(provider_.get(), + : controller_(new FakeDataChannelController()), + webrtc_data_channel_(SctpDataChannel::Create(controller_.get(), "test", init_, rtc::Thread::Current(), rtc::Thread::Current())) {} void SetChannelReady() { - provider_->set_transport_available(true); + controller_->set_transport_available(true); webrtc_data_channel_->OnTransportChannelCreated(); if (webrtc_data_channel_->id() < 0) { webrtc_data_channel_->SetSctpSid(0); } - provider_->set_ready_to_send(true); + controller_->set_ready_to_send(true); } void AddObserver() { @@ -101,7 +101,7 @@ class SctpDataChannelTest : public ::testing::Test { } webrtc::InternalDataChannelInit init_; - std::unique_ptr provider_; + std::unique_ptr controller_; std::unique_ptr observer_; rtc::scoped_refptr webrtc_data_channel_; }; @@ -122,29 +122,29 @@ class StateSignalsListener : public sigslot::has_slots<> { // Verifies that the data channel is connected to the transport after creation. TEST_F(SctpDataChannelTest, ConnectedToTransportOnCreated) { - provider_->set_transport_available(true); + controller_->set_transport_available(true); rtc::scoped_refptr dc = - SctpDataChannel::Create(provider_.get(), "test1", init_, + SctpDataChannel::Create(controller_.get(), "test1", init_, rtc::Thread::Current(), rtc::Thread::Current()); - EXPECT_TRUE(provider_->IsConnected(dc.get())); + EXPECT_TRUE(controller_->IsConnected(dc.get())); // The sid is not set yet, so it should not have added the streams. - EXPECT_FALSE(provider_->IsSendStreamAdded(dc->id())); - EXPECT_FALSE(provider_->IsRecvStreamAdded(dc->id())); + EXPECT_FALSE(controller_->IsSendStreamAdded(dc->id())); + EXPECT_FALSE(controller_->IsRecvStreamAdded(dc->id())); dc->SetSctpSid(0); - EXPECT_TRUE(provider_->IsSendStreamAdded(dc->id())); - EXPECT_TRUE(provider_->IsRecvStreamAdded(dc->id())); + EXPECT_TRUE(controller_->IsSendStreamAdded(dc->id())); + EXPECT_TRUE(controller_->IsRecvStreamAdded(dc->id())); } // Verifies that the data channel is connected to the transport if the transport // is not available initially and becomes available later. TEST_F(SctpDataChannelTest, ConnectedAfterTransportBecomesAvailable) { - EXPECT_FALSE(provider_->IsConnected(webrtc_data_channel_.get())); + EXPECT_FALSE(controller_->IsConnected(webrtc_data_channel_.get())); - provider_->set_transport_available(true); + controller_->set_transport_available(true); webrtc_data_channel_->OnTransportChannelCreated(); - EXPECT_TRUE(provider_->IsConnected(webrtc_data_channel_.get())); + EXPECT_TRUE(controller_->IsConnected(webrtc_data_channel_.get())); } // Tests the state of the data channel. @@ -170,7 +170,7 @@ TEST_F(SctpDataChannelTest, StateTransition) { EXPECT_EQ(state_signals_listener.opened_count(), 1); EXPECT_EQ(state_signals_listener.closed_count(), 1); // Verifies that it's disconnected from the transport. - EXPECT_FALSE(provider_->IsConnected(webrtc_data_channel_.get())); + EXPECT_FALSE(controller_->IsConnected(webrtc_data_channel_.get())); } // Tests that DataChannel::buffered_amount() is correct after the channel is @@ -186,7 +186,7 @@ TEST_F(SctpDataChannelTest, BufferedAmountWhenBlocked) { EXPECT_EQ(successful_send_count, observer_->on_buffered_amount_change_count()); - provider_->set_send_blocked(true); + controller_->set_send_blocked(true); const int number_of_packets = 3; for (int i = 0; i < number_of_packets; ++i) { @@ -197,7 +197,7 @@ TEST_F(SctpDataChannelTest, BufferedAmountWhenBlocked) { EXPECT_EQ(successful_send_count, observer_->on_buffered_amount_change_count()); - provider_->set_send_blocked(false); + controller_->set_send_blocked(false); successful_send_count += number_of_packets; EXPECT_EQ(0U, webrtc_data_channel_->buffered_amount()); EXPECT_EQ(successful_send_count, @@ -210,12 +210,12 @@ TEST_F(SctpDataChannelTest, QueuedDataSentWhenUnblocked) { AddObserver(); SetChannelReady(); webrtc::DataBuffer buffer("abcd"); - provider_->set_send_blocked(true); + controller_->set_send_blocked(true); EXPECT_TRUE(webrtc_data_channel_->Send(buffer)); EXPECT_EQ(0U, observer_->on_buffered_amount_change_count()); - provider_->set_send_blocked(false); + controller_->set_send_blocked(false); SetChannelReady(); EXPECT_EQ(0U, webrtc_data_channel_->buffered_amount()); EXPECT_EQ(1U, observer_->on_buffered_amount_change_count()); @@ -227,7 +227,7 @@ TEST_F(SctpDataChannelTest, BlockedWhenSendQueuedDataNoCrash) { AddObserver(); SetChannelReady(); webrtc::DataBuffer buffer("abcd"); - provider_->set_send_blocked(true); + controller_->set_send_blocked(true); EXPECT_TRUE(webrtc_data_channel_->Send(buffer)); EXPECT_EQ(0U, observer_->on_buffered_amount_change_count()); @@ -237,7 +237,7 @@ TEST_F(SctpDataChannelTest, BlockedWhenSendQueuedDataNoCrash) { EXPECT_EQ(0U, observer_->on_buffered_amount_change_count()); // Unblock the channel to send queued data again, there should be no crash. - provider_->set_send_blocked(false); + controller_->set_send_blocked(false); SetChannelReady(); EXPECT_EQ(0U, webrtc_data_channel_->buffered_amount()); EXPECT_EQ(1U, observer_->on_buffered_amount_change_count()); @@ -262,7 +262,7 @@ TEST_F(SctpDataChannelTest, VerifyMessagesAndBytesSent) { EXPECT_EQ(0U, webrtc_data_channel_->bytes_sent()); // Send three buffers while not blocked. - provider_->set_send_blocked(false); + controller_->set_send_blocked(false); EXPECT_TRUE(webrtc_data_channel_->Send(buffers[0])); EXPECT_TRUE(webrtc_data_channel_->Send(buffers[1])); EXPECT_TRUE(webrtc_data_channel_->Send(buffers[2])); @@ -272,7 +272,7 @@ TEST_F(SctpDataChannelTest, VerifyMessagesAndBytesSent) { EXPECT_EQ(bytes_sent, webrtc_data_channel_->bytes_sent()); // Send three buffers while blocked, queuing the buffers. - provider_->set_send_blocked(true); + controller_->set_send_blocked(true); EXPECT_TRUE(webrtc_data_channel_->Send(buffers[3])); EXPECT_TRUE(webrtc_data_channel_->Send(buffers[4])); EXPECT_TRUE(webrtc_data_channel_->Send(buffers[5])); @@ -283,7 +283,7 @@ TEST_F(SctpDataChannelTest, VerifyMessagesAndBytesSent) { EXPECT_EQ(bytes_sent, webrtc_data_channel_->bytes_sent()); // Unblock and make sure everything was sent. - provider_->set_send_blocked(false); + controller_->set_send_blocked(false); EXPECT_EQ_WAIT(0U, webrtc_data_channel_->buffered_amount(), kDefaultTimeout); bytes_sent += bytes_queued; EXPECT_EQ(6U, webrtc_data_channel_->messages_sent()); @@ -298,18 +298,18 @@ TEST_F(SctpDataChannelTest, OpenMessageSent) { SetChannelReady(); EXPECT_GE(webrtc_data_channel_->id(), 0); EXPECT_EQ(webrtc::DataMessageType::kControl, - provider_->last_send_data_params().type); - EXPECT_EQ(provider_->last_sid(), webrtc_data_channel_->id()); + controller_->last_send_data_params().type); + EXPECT_EQ(controller_->last_sid(), webrtc_data_channel_->id()); } TEST_F(SctpDataChannelTest, QueuedOpenMessageSent) { - provider_->set_send_blocked(true); + controller_->set_send_blocked(true); SetChannelReady(); - provider_->set_send_blocked(false); + controller_->set_send_blocked(false); EXPECT_EQ(webrtc::DataMessageType::kControl, - provider_->last_send_data_params().type); - EXPECT_EQ(provider_->last_sid(), webrtc_data_channel_->id()); + controller_->last_send_data_params().type); + EXPECT_EQ(controller_->last_sid(), webrtc_data_channel_->id()); } // Tests that the DataChannel created after transport gets ready can enter OPEN @@ -319,7 +319,7 @@ TEST_F(SctpDataChannelTest, LateCreatedChannelTransitionToOpen) { webrtc::InternalDataChannelInit init; init.id = 1; rtc::scoped_refptr dc = - SctpDataChannel::Create(provider_.get(), "test1", init, + SctpDataChannel::Create(controller_.get(), "test1", init, rtc::Thread::Current(), rtc::Thread::Current()); EXPECT_EQ(webrtc::DataChannelInterface::kConnecting, dc->state()); EXPECT_TRUE_WAIT(webrtc::DataChannelInterface::kOpen == dc->state(), 1000); @@ -333,7 +333,7 @@ TEST_F(SctpDataChannelTest, SendUnorderedAfterReceivesOpenAck) { init.id = 1; init.ordered = false; rtc::scoped_refptr dc = - SctpDataChannel::Create(provider_.get(), "test1", init, + SctpDataChannel::Create(controller_.get(), "test1", init, rtc::Thread::Current(), rtc::Thread::Current()); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kOpen, dc->state(), 1000); @@ -341,7 +341,7 @@ TEST_F(SctpDataChannelTest, SendUnorderedAfterReceivesOpenAck) { // Sends a message and verifies it's ordered. webrtc::DataBuffer buffer("some data"); ASSERT_TRUE(dc->Send(buffer)); - EXPECT_TRUE(provider_->last_send_data_params().ordered); + EXPECT_TRUE(controller_->last_send_data_params().ordered); // Emulates receiving an OPEN_ACK message. cricket::ReceiveDataParams params; @@ -353,7 +353,7 @@ TEST_F(SctpDataChannelTest, SendUnorderedAfterReceivesOpenAck) { // Sends another message and verifies it's unordered. ASSERT_TRUE(dc->Send(buffer)); - EXPECT_FALSE(provider_->last_send_data_params().ordered); + EXPECT_FALSE(controller_->last_send_data_params().ordered); } // Tests that an unordered DataChannel sends unordered data after any DATA @@ -364,7 +364,7 @@ TEST_F(SctpDataChannelTest, SendUnorderedAfterReceiveData) { init.id = 1; init.ordered = false; rtc::scoped_refptr dc = - SctpDataChannel::Create(provider_.get(), "test1", init, + SctpDataChannel::Create(controller_.get(), "test1", init, rtc::Thread::Current(), rtc::Thread::Current()); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kOpen, dc->state(), 1000); @@ -378,7 +378,7 @@ TEST_F(SctpDataChannelTest, SendUnorderedAfterReceiveData) { // Sends a message and verifies it's unordered. ASSERT_TRUE(dc->Send(buffer)); - EXPECT_FALSE(provider_->last_send_data_params().ordered); + EXPECT_FALSE(controller_->last_send_data_params().ordered); } // Tests that the channel can't open until it's successfully sent the OPEN @@ -386,37 +386,37 @@ TEST_F(SctpDataChannelTest, SendUnorderedAfterReceiveData) { TEST_F(SctpDataChannelTest, OpenWaitsForOpenMesssage) { webrtc::DataBuffer buffer("foo"); - provider_->set_send_blocked(true); + controller_->set_send_blocked(true); SetChannelReady(); EXPECT_EQ(webrtc::DataChannelInterface::kConnecting, webrtc_data_channel_->state()); - provider_->set_send_blocked(false); + controller_->set_send_blocked(false); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kOpen, webrtc_data_channel_->state(), 1000); EXPECT_EQ(webrtc::DataMessageType::kControl, - provider_->last_send_data_params().type); + controller_->last_send_data_params().type); } // Tests that close first makes sure all queued data gets sent. TEST_F(SctpDataChannelTest, QueuedCloseFlushes) { webrtc::DataBuffer buffer("foo"); - provider_->set_send_blocked(true); + controller_->set_send_blocked(true); SetChannelReady(); EXPECT_EQ(webrtc::DataChannelInterface::kConnecting, webrtc_data_channel_->state()); - provider_->set_send_blocked(false); + controller_->set_send_blocked(false); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kOpen, webrtc_data_channel_->state(), 1000); - provider_->set_send_blocked(true); + controller_->set_send_blocked(true); webrtc_data_channel_->Send(buffer); webrtc_data_channel_->Close(); - provider_->set_send_blocked(false); + controller_->set_send_blocked(false); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kClosed, webrtc_data_channel_->state(), 1000); EXPECT_TRUE(webrtc_data_channel_->error().ok()); EXPECT_EQ(webrtc::DataMessageType::kText, - provider_->last_send_data_params().type); + controller_->last_send_data_params().type); } // Tests that messages are sent with the right id. @@ -425,7 +425,7 @@ TEST_F(SctpDataChannelTest, SendDataId) { SetChannelReady(); webrtc::DataBuffer buffer("data"); EXPECT_TRUE(webrtc_data_channel_->Send(buffer)); - EXPECT_EQ(1, provider_->last_sid()); + EXPECT_EQ(1, controller_->last_sid()); } // Tests that the incoming messages with wrong ids are rejected. @@ -468,11 +468,11 @@ TEST_F(SctpDataChannelTest, NoMsgSentIfNegotiatedAndNotFromOpenMsg) { SetChannelReady(); rtc::scoped_refptr dc = - SctpDataChannel::Create(provider_.get(), "test1", config, + SctpDataChannel::Create(controller_.get(), "test1", config, rtc::Thread::Current(), rtc::Thread::Current()); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kOpen, dc->state(), 1000); - EXPECT_EQ(0, provider_->last_sid()); + EXPECT_EQ(0, controller_->last_sid()); } // Tests that DataChannel::messages_received() and DataChannel::bytes_received() @@ -532,14 +532,14 @@ TEST_F(SctpDataChannelTest, OpenAckSentIfCreatedFromOpenMessage) { SetChannelReady(); rtc::scoped_refptr dc = - SctpDataChannel::Create(provider_.get(), "test1", config, + SctpDataChannel::Create(controller_.get(), "test1", config, rtc::Thread::Current(), rtc::Thread::Current()); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kOpen, dc->state(), 1000); - EXPECT_EQ(config.id, provider_->last_sid()); + EXPECT_EQ(config.id, controller_->last_sid()); EXPECT_EQ(webrtc::DataMessageType::kControl, - provider_->last_send_data_params().type); + controller_->last_send_data_params().type); } // Tests the OPEN_ACK role assigned by InternalDataChannelInit. @@ -565,7 +565,7 @@ TEST_F(SctpDataChannelTest, OpenWhenSendBufferFull) { memset(buffer.MutableData(), 0, buffer.size()); webrtc::DataBuffer packet(buffer, true); - provider_->set_send_blocked(true); + controller_->set_send_blocked(true); for (size_t i = 0; i < webrtc::DataChannelInterface::MaxSendQueueSize() / packetSize; ++i) { @@ -583,7 +583,7 @@ TEST_F(SctpDataChannelTest, OpenWhenSendBufferFull) { TEST_F(SctpDataChannelTest, ClosedOnTransportError) { SetChannelReady(); webrtc::DataBuffer buffer("abcd"); - provider_->set_transport_error(); + controller_->set_transport_error(); EXPECT_TRUE(webrtc_data_channel_->Send(buffer)); @@ -631,7 +631,7 @@ TEST_F(SctpDataChannelTest, SendEmptyData) { // Tests that a channel can be closed without being opened or assigned an sid. TEST_F(SctpDataChannelTest, NeverOpened) { - provider_->set_transport_available(true); + controller_->set_transport_available(true); webrtc_data_channel_->OnTransportChannelCreated(); webrtc_data_channel_->Close(); } @@ -646,7 +646,7 @@ TEST_F(SctpDataChannelTest, TransportDestroyedWhileDataBuffered) { webrtc::DataBuffer packet(buffer, true); // Send a packet while sending is blocked so it ends up buffered. - provider_->set_send_blocked(true); + controller_->set_send_blocked(true); EXPECT_TRUE(webrtc_data_channel_->Send(packet)); // Tell the data channel that its transport is being destroyed. @@ -655,7 +655,7 @@ TEST_F(SctpDataChannelTest, TransportDestroyedWhileDataBuffered) { webrtc::RTCError error(webrtc::RTCErrorType::OPERATION_ERROR_WITH_DATA, ""); error.set_error_detail(webrtc::RTCErrorDetailType::SCTP_FAILURE); webrtc_data_channel_->OnTransportChannelClosed(error); - provider_.reset(nullptr); + controller_.reset(nullptr); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kClosed, webrtc_data_channel_->state(), kDefaultTimeout); EXPECT_FALSE(webrtc_data_channel_->error().ok()); @@ -677,7 +677,7 @@ TEST_F(SctpDataChannelTest, TransportGotErrorCode) { error.set_sctp_cause_code( static_cast(cricket::SctpErrorCauseCode::kProtocolViolation)); webrtc_data_channel_->OnTransportChannelClosed(error); - provider_.reset(nullptr); + controller_.reset(nullptr); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kClosed, webrtc_data_channel_->state(), kDefaultTimeout); EXPECT_FALSE(webrtc_data_channel_->error().ok()); diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index 4fb4a7674f..39999b6a09 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -50,7 +50,7 @@ #include "p2p/base/port.h" #include "pc/media_stream.h" #include "pc/stream_collection.h" -#include "pc/test/fake_data_channel_provider.h" +#include "pc/test/fake_data_channel_controller.h" #include "pc/test/fake_peer_connection_for_stats.h" #include "pc/test/mock_data_channel.h" #include "pc/test/mock_rtp_receiver_internal.h" @@ -1649,13 +1649,13 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) { } // TODO(bugs.webrtc.org/11547): Supply a separate network thread. - FakeDataChannelProvider provider; + FakeDataChannelController controller; rtc::scoped_refptr dummy_channel_a = SctpDataChannel::Create( - &provider, "DummyChannelA", InternalDataChannelInit(), + &controller, "DummyChannelA", InternalDataChannelInit(), rtc::Thread::Current(), rtc::Thread::Current()); pc_->SignalSctpDataChannelCreated()(dummy_channel_a.get()); rtc::scoped_refptr dummy_channel_b = SctpDataChannel::Create( - &provider, "DummyChannelB", InternalDataChannelInit(), + &controller, "DummyChannelB", InternalDataChannelInit(), rtc::Thread::Current(), rtc::Thread::Current()); pc_->SignalSctpDataChannelCreated()(dummy_channel_b.get()); diff --git a/pc/sctp_data_channel.cc b/pc/sctp_data_channel.cc index 9333be96ad..e995a0e9a6 100644 --- a/pc/sctp_data_channel.cc +++ b/pc/sctp_data_channel.cc @@ -141,13 +141,13 @@ bool SctpSidAllocator::IsSidAvailable(int sid) const { } rtc::scoped_refptr SctpDataChannel::Create( - SctpDataChannelProviderInterface* provider, + SctpDataChannelControllerInterface* controller, const std::string& label, const InternalDataChannelInit& config, rtc::Thread* signaling_thread, rtc::Thread* network_thread) { auto channel = rtc::make_ref_counted( - config, provider, label, signaling_thread, network_thread); + config, controller, label, signaling_thread, network_thread); if (!channel->Init()) { return nullptr; } @@ -163,7 +163,7 @@ rtc::scoped_refptr SctpDataChannel::CreateProxy( } SctpDataChannel::SctpDataChannel(const InternalDataChannelInit& config, - SctpDataChannelProviderInterface* provider, + SctpDataChannelControllerInterface* controller, const std::string& label, rtc::Thread* signaling_thread, rtc::Thread* network_thread) @@ -173,11 +173,16 @@ SctpDataChannel::SctpDataChannel(const InternalDataChannelInit& config, label_(label), config_(config), observer_(nullptr), - provider_(provider) { + controller_(controller) { RTC_DCHECK_RUN_ON(signaling_thread_); RTC_UNUSED(network_thread_); } +void SctpDataChannel::DetachFromController() { + RTC_DCHECK_RUN_ON(signaling_thread_); + controller_detached_ = true; +} + bool SctpDataChannel::Init() { RTC_DCHECK_RUN_ON(signaling_thread_); if (config_.id < -1 || @@ -214,7 +219,8 @@ bool SctpDataChannel::Init() { // This has to be done async because the upper layer objects (e.g. // Chrome glue and WebKit) are not wired up properly until after this // function returns. - if (provider_->ReadyToSendData()) { + RTC_DCHECK(!controller_detached_); + if (controller_->ReadyToSendData()) { AddRef(); rtc::Thread::Current()->PostTask(ToQueuedTask( [this] { @@ -330,7 +336,8 @@ void SctpDataChannel::SetSctpSid(int sid) { } const_cast(config_).id = sid; - provider_->AddSctpDataStream(sid); + RTC_DCHECK(!controller_detached_); + controller_->AddSctpDataStream(sid); } void SctpDataChannel::OnClosingProcedureStartedRemotely(int sid) { @@ -356,20 +363,23 @@ void SctpDataChannel::OnClosingProcedureComplete(int sid) { // all pending data and transitioned to kClosing already. RTC_DCHECK_EQ(state_, kClosing); RTC_DCHECK(queued_send_data_.Empty()); - DisconnectFromProvider(); + DisconnectFromTransport(); SetState(kClosed); } } void SctpDataChannel::OnTransportChannelCreated() { RTC_DCHECK_RUN_ON(signaling_thread_); - if (!connected_to_provider_) { - connected_to_provider_ = provider_->ConnectDataChannel(this); + if (controller_detached_) { + return; } - // The sid may have been unassigned when provider_->ConnectDataChannel was - // done. So always add the streams even if connected_to_provider_ is true. + if (!connected_to_transport_) { + connected_to_transport_ = controller_->ConnectDataChannel(this); + } + // The sid may have been unassigned when controller_->ConnectDataChannel was + // done. So always add the streams even if connected_to_transport_ is true. if (config_.id >= 0) { - provider_->AddSctpDataStream(config_.id); + controller_->AddSctpDataStream(config_.id); } } @@ -472,8 +482,8 @@ void SctpDataChannel::CloseAbruptlyWithError(RTCError error) { return; } - if (connected_to_provider_) { - DisconnectFromProvider(); + if (connected_to_transport_) { + DisconnectFromTransport(); } // Closing abruptly means any queued data gets thrown away. @@ -503,7 +513,7 @@ void SctpDataChannel::UpdateState() { switch (state_) { case kConnecting: { - if (connected_to_provider_) { + if (connected_to_transport_) { if (handshake_state_ == kHandshakeShouldSendOpen) { rtc::CopyOnWriteBuffer payload; WriteDataChannelOpenMessage(label_, config_, &payload); @@ -534,10 +544,10 @@ void SctpDataChannel::UpdateState() { // to complete; after calling RemoveSctpDataStream, // OnClosingProcedureComplete will end up called asynchronously // afterwards. - if (connected_to_provider_ && !started_closing_procedure_ && - config_.id >= 0) { + if (connected_to_transport_ && !started_closing_procedure_ && + !controller_detached_ && config_.id >= 0) { started_closing_procedure_ = true; - provider_->RemoveSctpDataStream(config_.id); + controller_->RemoveSctpDataStream(config_.id); } } break; @@ -564,13 +574,13 @@ void SctpDataChannel::SetState(DataState state) { } } -void SctpDataChannel::DisconnectFromProvider() { +void SctpDataChannel::DisconnectFromTransport() { RTC_DCHECK_RUN_ON(signaling_thread_); - if (!connected_to_provider_) + if (!connected_to_transport_ || controller_detached_) return; - provider_->DisconnectDataChannel(this); - connected_to_provider_ = false; + controller_->DisconnectDataChannel(this); + connected_to_transport_ = false; } void SctpDataChannel::DeliverQueuedReceivedData() { @@ -609,6 +619,9 @@ bool SctpDataChannel::SendDataMessage(const DataBuffer& buffer, bool queue_if_blocked) { RTC_DCHECK_RUN_ON(signaling_thread_); SendDataParams send_params; + if (controller_detached_) { + return false; + } send_params.ordered = config_.ordered; // Send as ordered if it is still going through OPEN/ACK signaling. @@ -626,7 +639,7 @@ bool SctpDataChannel::SendDataMessage(const DataBuffer& buffer, cricket::SendDataResult send_result = cricket::SDR_SUCCESS; bool success = - provider_->SendData(config_.id, send_params, buffer.data, &send_result); + controller_->SendData(config_.id, send_params, buffer.data, &send_result); if (success) { ++messages_sent_; @@ -688,6 +701,9 @@ bool SctpDataChannel::SendControlMessage(const rtc::CopyOnWriteBuffer& buffer) { RTC_DCHECK(writable_); RTC_DCHECK_GE(config_.id, 0); + if (controller_detached_) { + return false; + } bool is_open_message = handshake_state_ == kHandshakeShouldSendOpen; RTC_DCHECK(!is_open_message || !config_.negotiated); @@ -700,7 +716,7 @@ bool SctpDataChannel::SendControlMessage(const rtc::CopyOnWriteBuffer& buffer) { cricket::SendDataResult send_result = cricket::SDR_SUCCESS; bool retval = - provider_->SendData(config_.id, send_params, buffer, &send_result); + controller_->SendData(config_.id, send_params, buffer, &send_result); if (retval) { RTC_LOG(LS_VERBOSE) << "Sent CONTROL message on channel " << config_.id; @@ -726,4 +742,10 @@ void SctpDataChannel::ResetInternalIdAllocatorForTesting(int new_value) { g_unique_id = new_value; } +SctpDataChannel* DowncastProxiedDataChannelInterfaceToSctpDataChannelForTesting( + DataChannelInterface* channel) { + return static_cast( + static_cast(channel)->internal()); +} + } // namespace webrtc diff --git a/pc/sctp_data_channel.h b/pc/sctp_data_channel.h index 56f99df3e5..a8442c59cc 100644 --- a/pc/sctp_data_channel.h +++ b/pc/sctp_data_channel.h @@ -37,7 +37,7 @@ class SctpDataChannel; // TODO(deadbeef): Get rid of this and have SctpDataChannel depend on // SctpTransportInternal (pure virtual SctpTransport interface) instead. -class SctpDataChannelProviderInterface { +class SctpDataChannelControllerInterface { public: // Sends the data to the transport. virtual bool SendData(int sid, @@ -57,7 +57,7 @@ class SctpDataChannelProviderInterface { virtual bool ReadyToSendData() const = 0; protected: - virtual ~SctpDataChannelProviderInterface() {} + virtual ~SctpDataChannelControllerInterface() {} }; // TODO(tommi): Change to not inherit from DataChannelInit but to have it as @@ -120,7 +120,7 @@ class SctpDataChannel : public DataChannelInterface, public sigslot::has_slots<> { public: static rtc::scoped_refptr Create( - SctpDataChannelProviderInterface* provider, + SctpDataChannelControllerInterface* controller, const std::string& label, const InternalDataChannelInit& config, rtc::Thread* signaling_thread, @@ -131,6 +131,9 @@ class SctpDataChannel : public DataChannelInterface, static rtc::scoped_refptr CreateProxy( rtc::scoped_refptr channel); + // Invalidate the link to the controller (DataChannelController); + void DetachFromController(); + void RegisterObserver(DataChannelObserver* observer) override; void UnregisterObserver() override; @@ -178,10 +181,10 @@ class SctpDataChannel : public DataChannelInterface, // Specializations of CloseAbruptlyWithError void CloseAbruptlyWithDataChannelFailure(const std::string& message); - // Slots for provider to connect signals to. + // Slots for controller to connect signals to. // // TODO(deadbeef): Make these private once we're hooking up signals ourselves, - // instead of relying on SctpDataChannelProviderInterface. + // instead of relying on SctpDataChannelControllerInterface. // Called when the SctpTransport's ready to use. That can happen when we've // finished negotiation, or if the channel was created after negotiation has @@ -223,7 +226,7 @@ class SctpDataChannel : public DataChannelInterface, protected: SctpDataChannel(const InternalDataChannelInit& config, - SctpDataChannelProviderInterface* client, + SctpDataChannelControllerInterface* client, const std::string& label, rtc::Thread* signaling_thread, rtc::Thread* network_thread); @@ -242,7 +245,7 @@ class SctpDataChannel : public DataChannelInterface, bool Init(); void UpdateState(); void SetState(DataState state); - void DisconnectFromProvider(); + void DisconnectFromTransport(); void DeliverQueuedReceivedData(); @@ -266,11 +269,12 @@ class SctpDataChannel : public DataChannelInterface, uint64_t bytes_sent_ RTC_GUARDED_BY(signaling_thread_) = 0; uint32_t messages_received_ RTC_GUARDED_BY(signaling_thread_) = 0; uint64_t bytes_received_ RTC_GUARDED_BY(signaling_thread_) = 0; - SctpDataChannelProviderInterface* const provider_ + SctpDataChannelControllerInterface* const controller_ RTC_GUARDED_BY(signaling_thread_); + bool controller_detached_ RTC_GUARDED_BY(signaling_thread_) = false; HandshakeState handshake_state_ RTC_GUARDED_BY(signaling_thread_) = kHandshakeInit; - bool connected_to_provider_ RTC_GUARDED_BY(signaling_thread_) = false; + bool connected_to_transport_ RTC_GUARDED_BY(signaling_thread_) = false; bool writable_ RTC_GUARDED_BY(signaling_thread_) = false; // Did we already start the graceful SCTP closing procedure? bool started_closing_procedure_ RTC_GUARDED_BY(signaling_thread_) = false; @@ -281,6 +285,11 @@ class SctpDataChannel : public DataChannelInterface, PacketQueue queued_send_data_ RTC_GUARDED_BY(signaling_thread_); }; +// Downcast a PeerConnectionInterface that points to a proxy object +// to its underlying SctpDataChannel object. For testing only. +SctpDataChannel* DowncastProxiedDataChannelInterfaceToSctpDataChannelForTesting( + DataChannelInterface* channel); + } // namespace webrtc #endif // PC_SCTP_DATA_CHANNEL_H_ diff --git a/pc/test/fake_data_channel_provider.h b/pc/test/fake_data_channel_controller.h similarity index 94% rename from pc/test/fake_data_channel_provider.h rename to pc/test/fake_data_channel_controller.h index f9e9e91d48..bdab7d2ec9 100644 --- a/pc/test/fake_data_channel_provider.h +++ b/pc/test/fake_data_channel_controller.h @@ -8,23 +8,23 @@ * be found in the AUTHORS file in the root of the source tree. */ -#ifndef PC_TEST_FAKE_DATA_CHANNEL_PROVIDER_H_ -#define PC_TEST_FAKE_DATA_CHANNEL_PROVIDER_H_ +#ifndef PC_TEST_FAKE_DATA_CHANNEL_CONTROLLER_H_ +#define PC_TEST_FAKE_DATA_CHANNEL_CONTROLLER_H_ #include #include "pc/sctp_data_channel.h" #include "rtc_base/checks.h" -class FakeDataChannelProvider - : public webrtc::SctpDataChannelProviderInterface { +class FakeDataChannelController + : public webrtc::SctpDataChannelControllerInterface { public: - FakeDataChannelProvider() + FakeDataChannelController() : send_blocked_(false), transport_available_(false), ready_to_send_(false), transport_error_(false) {} - virtual ~FakeDataChannelProvider() {} + virtual ~FakeDataChannelController() {} bool SendData(int sid, const webrtc::SendDataParams& params, @@ -157,4 +157,4 @@ class FakeDataChannelProvider std::set send_ssrcs_; std::set recv_ssrcs_; }; -#endif // PC_TEST_FAKE_DATA_CHANNEL_PROVIDER_H_ +#endif // PC_TEST_FAKE_DATA_CHANNEL_CONTROLLER_H_ diff --git a/pc/test/fake_peer_connection_for_stats.h b/pc/test/fake_peer_connection_for_stats.h index e612f0d10f..d7f97daf76 100644 --- a/pc/test/fake_peer_connection_for_stats.h +++ b/pc/test/fake_peer_connection_for_stats.h @@ -22,7 +22,7 @@ #include "pc/channel.h" #include "pc/channel_manager.h" #include "pc/stream_collection.h" -#include "pc/test/fake_data_channel_provider.h" +#include "pc/test/fake_data_channel_controller.h" #include "pc/test/fake_peer_connection_base.h" namespace webrtc { @@ -250,7 +250,7 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { void AddSctpDataChannel(const std::string& label, const InternalDataChannelInit& init) { // TODO(bugs.webrtc.org/11547): Supply a separate network thread. - AddSctpDataChannel(SctpDataChannel::Create(&data_channel_provider_, label, + AddSctpDataChannel(SctpDataChannel::Create(&data_channel_controller_, label, init, rtc::Thread::Current(), rtc::Thread::Current())); } @@ -431,7 +431,7 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { rtc::scoped_refptr>> transceivers_; - FakeDataChannelProvider data_channel_provider_; + FakeDataChannelController data_channel_controller_; std::vector> sctp_data_channels_;