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_;