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 <hta@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36852}
diff --git a/pc/data_channel_unittest.cc b/pc/data_channel_unittest.cc
index 5797d1d..55f9c34 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 @@
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 @@
}
webrtc::InternalDataChannelInit init_;
- std::unique_ptr<FakeDataChannelProvider> provider_;
+ std::unique_ptr<FakeDataChannelController> controller_;
std::unique_ptr<FakeDataChannelObserver> observer_;
rtc::scoped_refptr<SctpDataChannel> webrtc_data_channel_;
};
@@ -122,29 +122,29 @@
// 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<SctpDataChannel> 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
webrtc::InternalDataChannelInit init;
init.id = 1;
rtc::scoped_refptr<SctpDataChannel> 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 @@
init.id = 1;
init.ordered = false;
rtc::scoped_refptr<SctpDataChannel> 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 @@
// 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 @@
// 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 @@
init.id = 1;
init.ordered = false;
rtc::scoped_refptr<SctpDataChannel> 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 @@
// 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, 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 @@
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 @@
SetChannelReady();
rtc::scoped_refptr<SctpDataChannel> 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 @@
SetChannelReady();
rtc::scoped_refptr<SctpDataChannel> 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 @@
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, 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 @@
// 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 @@
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 @@
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 @@
error.set_sctp_cause_code(
static_cast<uint16_t>(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());
