admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Use WeakPtr in SctpDataChannel.

Browse Source 
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 <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39464}
This commit is contained in:
Tommi 2023-03-02 15:42:06 +01:00 committed by WebRTC LUCI CQ
parent 24c78f0983
commit 1c0d91f047
9 changed files with 46 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
pc/BUILD.gn
View File

@ -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",

14
pc/data_channel_controller.cc
View File

@ -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<SctpDataChannel> channel(SctpDataChannel::Create(
this, label, new_config, signaling_thread(), network_thread()));
rtc::scoped_refptr<SctpDataChannel> channel(
SctpDataChannel::Create(weak_factory_.GetWeakPtr(), label, new_config,
signaling_thread(), network_thread()));
if (!channel) {
sid_allocator_.ReleaseSid(new_config.id);
return nullptr;

14
pc/data_channel_unittest.cc
View File

@ -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<SctpDataChannel> 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<SctpDataChannel> 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<SctpDataChannel> 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<SctpDataChannel> 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<SctpDataChannel> 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<SctpDataChannel> 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);

4
pc/rtc_stats_collector_unittest.cc
View File

@ -2109,10 +2109,10 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) {
// TODO(bugs.webrtc.org/11547): Supply a separate network thread.
FakeDataChannelController controller;
rtc::scoped_refptr<SctpDataChannel> dummy_channel_a = SctpDataChannel::Create(
&controller, "DummyChannelA", InternalDataChannelInit(),
controller.weak_ptr(), "DummyChannelA", InternalDataChannelInit(),
rtc::Thread::Current(), rtc::Thread::Current());
rtc::scoped_refptr<SctpDataChannel> dummy_channel_b = SctpDataChannel::Create(
&controller, "DummyChannelB", InternalDataChannelInit(),
controller.weak_ptr(), "DummyChannelB", InternalDataChannelInit(),
rtc::Thread::Current(), rtc::Thread::Current());
stats_->stats_collector()->OnSctpDataChannelStateChanged(

38
pc/sctp_data_channel.cc
View File

@ -138,14 +138,15 @@ bool SctpSidAllocator::IsSidAvailable(int sid) const {
return used_sids_.find(sid) == used_sids_.end();
}
// static
rtc::scoped_refptr<SctpDataChannel> SctpDataChannel::Create(
SctpDataChannelControllerInterface* controller,
rtc::WeakPtr<SctpDataChannelControllerInterface> controller,
const std::string& label,
const InternalDataChannelInit& config,
rtc::Thread* signaling_thread,
rtc::Thread* network_thread) {
auto channel = rtc::make_ref_counted<SctpDataChannel>(
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<DataChannelInterface> 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<SctpDataChannelControllerInterface> 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<InternalDataChannelInit&>(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;

11
pc/sctp_data_channel.h
View File

@ -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<SctpDataChannel> Create(
SctpDataChannelControllerInterface* controller,
rtc::WeakPtr<SctpDataChannelControllerInterface> controller,
const std::string& label,
const InternalDataChannelInit& config,
rtc::Thread* signaling_thread,
@ -134,9 +135,6 @@ class SctpDataChannel : public DataChannelInterface,
static rtc::scoped_refptr<DataChannelInterface> CreateProxy(
rtc::scoped_refptr<SctpDataChannel> 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<SctpDataChannelControllerInterface> 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<SctpDataChannelControllerInterface> 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;

6
pc/test/fake_data_channel_controller.h
View File

@ -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<FakeDataChannelController> 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<webrtc::SctpDataChannel*> connected_channels_;
std::set<uint32_t> send_ssrcs_;
std::set<uint32_t> recv_ssrcs_;
rtc::WeakPtrFactory<FakeDataChannelController> weak_factory_{this};
};
#endif // PC_TEST_FAKE_DATA_CHANNEL_CONTROLLER_H_

6
pc/test/fake_peer_connection_for_stats.h
View File

@ -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<SctpDataChannel> data_channel) {

2
pc/test/mock_data_channel.h
View File

@ -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<SctpDataChannelControllerInterface>(),
label,
signaling_thread,
network_thread) {