From 1c0d91f047e1917ce291e1e176b900505e525e71 Mon Sep 17 00:00:00 2001 From: Tommi Date: Thu, 2 Mar 2023 15:42:06 +0100 Subject: [PATCH] Use WeakPtr in SctpDataChannel. DataChannelController used WeakPtr to clear outstanding references upon destruction - except for the case of SctpDataChannel where we had a pointer+flag for the same purpose. This change updates SctpDataChannel and FakeDataChannelController to use a consistent approach. Bug: none Change-Id: I0248471c241365a2c0de76afbb37302115650194 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/295820 Reviewed-by: Harald Alvestrand Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#39464} --- pc/BUILD.gn | 2 ++ pc/data_channel_controller.cc | 14 +++------ pc/data_channel_unittest.cc | 14 ++++----- pc/rtc_stats_collector_unittest.cc | 4 +-- pc/sctp_data_channel.cc | 38 +++++++++++------------- pc/sctp_data_channel.h | 11 +++---- pc/test/fake_data_channel_controller.h | 6 ++++ pc/test/fake_peer_connection_for_stats.h | 6 ++-- pc/test/mock_data_channel.h | 2 +- 9 files changed, 46 insertions(+), 51 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 0a2c9ba788..faa2b1dda6 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -883,6 +883,7 @@ rtc_library("sctp_data_channel") { "../rtc_base:ssl", "../rtc_base:threading", "../rtc_base:threading", + "../rtc_base:weak_ptr", "../rtc_base/system:unused", "../rtc_base/third_party/sigslot:sigslot", ] @@ -2783,6 +2784,7 @@ if (rtc_include_tests && !build_with_chromium) { "../rtc_base:task_queue_for_test", "../rtc_base:threading", "../rtc_base:timeutils", + "../rtc_base:weak_ptr", "../rtc_base/synchronization:mutex", "../rtc_base/task_utils:repeating_task", "../rtc_base/third_party/sigslot", diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index 947efe52a3..a13066b27c 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -20,14 +20,7 @@ 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(); - } -} +DataChannelController::~DataChannelController() {} bool DataChannelController::HasDataChannels() const { RTC_DCHECK_RUN_ON(signaling_thread()); @@ -301,8 +294,9 @@ DataChannelController::InternalCreateSctpDataChannel( "because the id is already in use or out of range."; return nullptr; } - rtc::scoped_refptr channel(SctpDataChannel::Create( - this, label, new_config, signaling_thread(), network_thread())); + rtc::scoped_refptr channel( + SctpDataChannel::Create(weak_factory_.GetWeakPtr(), label, new_config, + signaling_thread(), network_thread())); if (!channel) { sid_allocator_.ReleaseSid(new_config.id); return nullptr; diff --git a/pc/data_channel_unittest.cc b/pc/data_channel_unittest.cc index 5ed6d5242d..80997161cf 100644 --- a/pc/data_channel_unittest.cc +++ b/pc/data_channel_unittest.cc @@ -79,7 +79,7 @@ class SctpDataChannelTest : public ::testing::Test { protected: SctpDataChannelTest() : controller_(new FakeDataChannelController()), - webrtc_data_channel_(SctpDataChannel::Create(controller_.get(), + webrtc_data_channel_(SctpDataChannel::Create(controller_->weak_ptr(), "test", init_, rtc::Thread::Current(), @@ -110,7 +110,7 @@ class SctpDataChannelTest : public ::testing::Test { TEST_F(SctpDataChannelTest, ConnectedToTransportOnCreated) { controller_->set_transport_available(true); rtc::scoped_refptr dc = - SctpDataChannel::Create(controller_.get(), "test1", init_, + SctpDataChannel::Create(controller_->weak_ptr(), "test1", init_, rtc::Thread::Current(), rtc::Thread::Current()); EXPECT_TRUE(controller_->IsConnected(dc.get())); @@ -301,7 +301,7 @@ TEST_F(SctpDataChannelTest, LateCreatedChannelTransitionToOpen) { webrtc::InternalDataChannelInit init; init.id = 1; rtc::scoped_refptr dc = - SctpDataChannel::Create(controller_.get(), "test1", init, + SctpDataChannel::Create(controller_->weak_ptr(), "test1", init, rtc::Thread::Current(), rtc::Thread::Current()); EXPECT_EQ(webrtc::DataChannelInterface::kConnecting, dc->state()); EXPECT_TRUE_WAIT(webrtc::DataChannelInterface::kOpen == dc->state(), 1000); @@ -315,7 +315,7 @@ TEST_F(SctpDataChannelTest, SendUnorderedAfterReceivesOpenAck) { init.id = 1; init.ordered = false; rtc::scoped_refptr dc = - SctpDataChannel::Create(controller_.get(), "test1", init, + SctpDataChannel::Create(controller_->weak_ptr(), "test1", init, rtc::Thread::Current(), rtc::Thread::Current()); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kOpen, dc->state(), 1000); @@ -346,7 +346,7 @@ TEST_F(SctpDataChannelTest, SendUnorderedAfterReceiveData) { init.id = 1; init.ordered = false; rtc::scoped_refptr dc = - SctpDataChannel::Create(controller_.get(), "test1", init, + SctpDataChannel::Create(controller_->weak_ptr(), "test1", init, rtc::Thread::Current(), rtc::Thread::Current()); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kOpen, dc->state(), 1000); @@ -450,7 +450,7 @@ TEST_F(SctpDataChannelTest, NoMsgSentIfNegotiatedAndNotFromOpenMsg) { SetChannelReady(); rtc::scoped_refptr dc = - SctpDataChannel::Create(controller_.get(), "test1", config, + SctpDataChannel::Create(controller_->weak_ptr(), "test1", config, rtc::Thread::Current(), rtc::Thread::Current()); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kOpen, dc->state(), 1000); @@ -514,7 +514,7 @@ TEST_F(SctpDataChannelTest, OpenAckSentIfCreatedFromOpenMessage) { SetChannelReady(); rtc::scoped_refptr dc = - SctpDataChannel::Create(controller_.get(), "test1", config, + SctpDataChannel::Create(controller_->weak_ptr(), "test1", config, rtc::Thread::Current(), rtc::Thread::Current()); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kOpen, dc->state(), 1000); diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index efc9a6ef8f..8c161a2ee0 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -2109,10 +2109,10 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) { // TODO(bugs.webrtc.org/11547): Supply a separate network thread. FakeDataChannelController controller; rtc::scoped_refptr dummy_channel_a = SctpDataChannel::Create( - &controller, "DummyChannelA", InternalDataChannelInit(), + controller.weak_ptr(), "DummyChannelA", InternalDataChannelInit(), rtc::Thread::Current(), rtc::Thread::Current()); rtc::scoped_refptr dummy_channel_b = SctpDataChannel::Create( - &controller, "DummyChannelB", InternalDataChannelInit(), + controller.weak_ptr(), "DummyChannelB", InternalDataChannelInit(), rtc::Thread::Current(), rtc::Thread::Current()); stats_->stats_collector()->OnSctpDataChannelStateChanged( diff --git a/pc/sctp_data_channel.cc b/pc/sctp_data_channel.cc index d8efb56b32..0c66e77f59 100644 --- a/pc/sctp_data_channel.cc +++ b/pc/sctp_data_channel.cc @@ -138,14 +138,15 @@ bool SctpSidAllocator::IsSidAvailable(int sid) const { return used_sids_.find(sid) == used_sids_.end(); } +// static rtc::scoped_refptr SctpDataChannel::Create( - SctpDataChannelControllerInterface* controller, + rtc::WeakPtr controller, const std::string& label, const InternalDataChannelInit& config, rtc::Thread* signaling_thread, rtc::Thread* network_thread) { auto channel = rtc::make_ref_counted( - config, controller, label, signaling_thread, network_thread); + config, std::move(controller), label, signaling_thread, network_thread); if (!channel->Init()) { return nullptr; } @@ -160,27 +161,23 @@ rtc::scoped_refptr SctpDataChannel::CreateProxy( return DataChannelProxy::Create(signaling_thread, std::move(channel)); } -SctpDataChannel::SctpDataChannel(const InternalDataChannelInit& config, - SctpDataChannelControllerInterface* controller, - const std::string& label, - rtc::Thread* signaling_thread, - rtc::Thread* network_thread) +SctpDataChannel::SctpDataChannel( + const InternalDataChannelInit& config, + rtc::WeakPtr controller, + const std::string& label, + rtc::Thread* signaling_thread, + rtc::Thread* network_thread) : signaling_thread_(signaling_thread), network_thread_(network_thread), internal_id_(GenerateUniqueId()), label_(label), config_(config), observer_(nullptr), - controller_(controller) { + controller_(std::move(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 || @@ -217,7 +214,7 @@ 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. - RTC_DCHECK(!controller_detached_); + RTC_DCHECK(controller_); if (controller_->ReadyToSendData()) { AddRef(); absl::Cleanup release = [this] { Release(); }; @@ -333,7 +330,6 @@ void SctpDataChannel::SetSctpSid(int sid) { } const_cast(config_).id = sid; - RTC_DCHECK(!controller_detached_); controller_->AddSctpDataStream(sid); } @@ -367,7 +363,7 @@ void SctpDataChannel::OnClosingProcedureComplete(int sid) { void SctpDataChannel::OnTransportChannelCreated() { RTC_DCHECK_RUN_ON(signaling_thread_); - if (controller_detached_) { + if (!controller_) { return; } if (!connected_to_transport_) { @@ -542,7 +538,7 @@ void SctpDataChannel::UpdateState() { // OnClosingProcedureComplete will end up called asynchronously // afterwards. if (connected_to_transport_ && !started_closing_procedure_ && - !controller_detached_ && config_.id >= 0) { + controller_ && config_.id >= 0) { started_closing_procedure_ = true; controller_->RemoveSctpDataStream(config_.id); } @@ -565,13 +561,13 @@ void SctpDataChannel::SetState(DataState state) { observer_->OnStateChange(); } - if (!controller_detached_) + if (controller_) controller_->OnChannelStateChanged(this, state_); } void SctpDataChannel::DisconnectFromTransport() { RTC_DCHECK_RUN_ON(signaling_thread_); - if (!connected_to_transport_ || controller_detached_) + if (!connected_to_transport_ || !controller_) return; controller_->DisconnectDataChannel(this); @@ -614,7 +610,7 @@ bool SctpDataChannel::SendDataMessage(const DataBuffer& buffer, bool queue_if_blocked) { RTC_DCHECK_RUN_ON(signaling_thread_); SendDataParams send_params; - if (controller_detached_) { + if (!controller_) { return false; } @@ -696,7 +692,7 @@ bool SctpDataChannel::SendControlMessage(const rtc::CopyOnWriteBuffer& buffer) { RTC_DCHECK(writable_); RTC_DCHECK_GE(config_.id, 0); - if (controller_detached_) { + if (!controller_) { return false; } bool is_open_message = handshake_state_ == kHandshakeShouldSendOpen; diff --git a/pc/sctp_data_channel.h b/pc/sctp_data_channel.h index 909f6bc757..f42818f8cc 100644 --- a/pc/sctp_data_channel.h +++ b/pc/sctp_data_channel.h @@ -30,6 +30,7 @@ #include "rtc_base/third_party/sigslot/sigslot.h" #include "rtc_base/thread.h" #include "rtc_base/thread_annotations.h" +#include "rtc_base/weak_ptr.h" namespace webrtc { @@ -123,7 +124,7 @@ class SctpDataChannel : public DataChannelInterface, public sigslot::has_slots<> { public: static rtc::scoped_refptr Create( - SctpDataChannelControllerInterface* controller, + rtc::WeakPtr controller, const std::string& label, const InternalDataChannelInit& config, rtc::Thread* signaling_thread, @@ -134,9 +135,6 @@ 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; @@ -223,7 +221,7 @@ class SctpDataChannel : public DataChannelInterface, protected: SctpDataChannel(const InternalDataChannelInit& config, - SctpDataChannelControllerInterface* client, + rtc::WeakPtr controller, const std::string& label, rtc::Thread* signaling_thread, rtc::Thread* network_thread); @@ -266,9 +264,8 @@ 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; - SctpDataChannelControllerInterface* const controller_ + rtc::WeakPtr 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_transport_ RTC_GUARDED_BY(signaling_thread_) = false; diff --git a/pc/test/fake_data_channel_controller.h b/pc/test/fake_data_channel_controller.h index 158362dd87..f7a2afe1df 100644 --- a/pc/test/fake_data_channel_controller.h +++ b/pc/test/fake_data_channel_controller.h @@ -15,6 +15,7 @@ #include "pc/sctp_data_channel.h" #include "rtc_base/checks.h" +#include "rtc_base/weak_ptr.h" class FakeDataChannelController : public webrtc::SctpDataChannelControllerInterface { @@ -26,6 +27,10 @@ class FakeDataChannelController transport_error_(false) {} virtual ~FakeDataChannelController() {} + rtc::WeakPtr weak_ptr() { + return weak_factory_.GetWeakPtr(); + } + bool SendData(int sid, const webrtc::SendDataParams& params, const rtc::CopyOnWriteBuffer& payload, @@ -171,5 +176,6 @@ class FakeDataChannelController std::set connected_channels_; std::set send_ssrcs_; std::set recv_ssrcs_; + rtc::WeakPtrFactory weak_factory_{this}; }; #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 d1914fd708..49f9dd64ad 100644 --- a/pc/test/fake_peer_connection_for_stats.h +++ b/pc/test/fake_peer_connection_for_stats.h @@ -303,9 +303,9 @@ 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_controller_, label, - init, rtc::Thread::Current(), - rtc::Thread::Current())); + AddSctpDataChannel(SctpDataChannel::Create( + data_channel_controller_.weak_ptr(), label, init, + rtc::Thread::Current(), rtc::Thread::Current())); } void AddSctpDataChannel(rtc::scoped_refptr data_channel) { diff --git a/pc/test/mock_data_channel.h b/pc/test/mock_data_channel.h index f1c5374d28..eb63d3b9e4 100644 --- a/pc/test/mock_data_channel.h +++ b/pc/test/mock_data_channel.h @@ -42,7 +42,7 @@ class MockSctpDataChannel : public SctpDataChannel { rtc::Thread* signaling_thread = rtc::Thread::Current(), rtc::Thread* network_thread = rtc::Thread::Current()) : SctpDataChannel(config, - nullptr, + rtc::WeakPtr(), label, signaling_thread, network_thread) {