Unset sinks when deleting CompositeDataChannelTransport.
This fixes a DCHECK during teardown in the case when the primary
DataChannelTranspot (eg. DatagramTransport) is successfully negotiated.
DatagramTransport expects the DataSink to be unset before it's deleted.
This was not caught by existing tests because the fallback transport
(SctpDataChannelTransport) does not have the same DCHECK.
Also adds a regression test for the issue, in which SCTP is available
as a fallback but DataChannelTransport is negotiated successfully.
Bug: webrtc:9719
Change-Id: I414d964d3c85d3d01cdb5e34d6b248659a613c39
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/154365
Commit-Queue: Bjorn Mellem <mellem@webrtc.org>
Reviewed-by: Seth Hampson <shampson@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29292}
diff --git a/pc/composite_data_channel_transport.cc b/pc/composite_data_channel_transport.cc
index 185dd1e..e66febc 100644
--- a/pc/composite_data_channel_transport.cc
+++ b/pc/composite_data_channel_transport.cc
@@ -24,6 +24,12 @@
}
}
+CompositeDataChannelTransport::~CompositeDataChannelTransport() {
+ for (auto transport : transports_) {
+ transport->SetDataSink(nullptr);
+ }
+}
+
void CompositeDataChannelTransport::SetSendTransport(
DataChannelTransportInterface* send_transport) {
if (!absl::c_linear_search(transports_, send_transport)) {
diff --git a/pc/composite_data_channel_transport.h b/pc/composite_data_channel_transport.h
index ccff4fe..b2a40fd 100644
--- a/pc/composite_data_channel_transport.h
+++ b/pc/composite_data_channel_transport.h
@@ -26,6 +26,7 @@
public:
explicit CompositeDataChannelTransport(
std::vector<DataChannelTransportInterface*> transports);
+ ~CompositeDataChannelTransport() override;
// Specifies which transport to be used for sending. Must be called before
// sending data.
diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc
index b95dc22..7c83e85 100644
--- a/pc/jsep_transport.cc
+++ b/pc/jsep_transport.cc
@@ -114,7 +114,6 @@
unencrypted_rtp_transport_(std::move(unencrypted_rtp_transport)),
sdes_transport_(std::move(sdes_transport)),
dtls_srtp_transport_(std::move(dtls_srtp_transport)),
- datagram_rtp_transport_(std::move(datagram_rtp_transport)),
rtp_dtls_transport_(
rtp_dtls_transport ? new rtc::RefCountedObject<webrtc::DtlsTransport>(
std::move(rtp_dtls_transport))
@@ -134,6 +133,7 @@
: nullptr),
media_transport_(std::move(media_transport)),
datagram_transport_(std::move(datagram_transport)),
+ datagram_rtp_transport_(std::move(datagram_rtp_transport)),
data_channel_transport_(data_channel_transport) {
RTC_DCHECK(ice_transport_);
RTC_DCHECK(rtp_dtls_transport_);
@@ -178,11 +178,9 @@
}
JsepTransport::~JsepTransport() {
- // Disconnect media transport state callbacks and make sure we delete media
- // transport before ICE.
+ // Disconnect media transport state callbacks.
if (media_transport_) {
media_transport_->SetMediaTransportStateCallback(nullptr);
- media_transport_.reset();
}
if (sctp_transport_) {
@@ -196,10 +194,6 @@
rtcp_dtls_transport_->Clear();
}
- // Delete datagram transport before ICE, but after its RTP transport.
- datagram_rtp_transport_.reset();
- datagram_transport_.reset();
-
// ICE will be the last transport to be deleted.
}
diff --git a/pc/jsep_transport.h b/pc/jsep_transport.h
index 868f7b9..b6199f8 100644
--- a/pc/jsep_transport.h
+++ b/pc/jsep_transport.h
@@ -378,8 +378,6 @@
RTC_GUARDED_BY(accessor_lock_);
std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_transport_
RTC_GUARDED_BY(accessor_lock_);
- std::unique_ptr<webrtc::RtpTransportInternal> datagram_rtp_transport_
- RTC_GUARDED_BY(accessor_lock_);
// If multiple RTP transports are in use, |composite_rtp_transport_| will be
// passed to callers. This is only valid for offer-only, receive-only
@@ -417,6 +415,9 @@
std::unique_ptr<webrtc::DatagramTransportInterface> datagram_transport_
RTC_GUARDED_BY(accessor_lock_);
+ std::unique_ptr<webrtc::RtpTransportInternal> datagram_rtp_transport_
+ RTC_GUARDED_BY(accessor_lock_);
+
// Non-SCTP data channel transport. Set to one of |media_transport_| or
// |datagram_transport_| if that transport should be used for data chanels.
// Unset if neither should be used for data channels.
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc
index 33fc9b9..465dca1 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -3599,6 +3599,54 @@
ASSERT_TRUE(ExpectNewFrames(media_expectations));
}
+// Tests that the data channel transport works correctly when datagram transport
+// negotiation succeeds and does not fall back to SCTP.
+TEST_P(PeerConnectionIntegrationTest,
+ DatagramTransportDataChannelDoesNotFallbackToSctp) {
+ PeerConnectionInterface::RTCConfiguration rtc_config;
+ rtc_config.rtcp_mux_policy = PeerConnectionInterface::kRtcpMuxPolicyRequire;
+ rtc_config.bundle_policy = PeerConnectionInterface::kBundlePolicyMaxBundle;
+ rtc_config.use_datagram_transport_for_data_channels = true;
+
+ // Configure one endpoint to use datagram transport for data channels while
+ // the other does not.
+ ASSERT_TRUE(CreatePeerConnectionWrappersWithConfigAndMediaTransportFactory(
+ rtc_config, rtc_config, loopback_media_transports()->first_factory(),
+ loopback_media_transports()->second_factory()));
+ ConnectFakeSignaling();
+
+ // The caller offers a data channel using either datagram transport or SCTP.
+ caller()->CreateDataChannel();
+ caller()->AddAudioVideoTracks();
+ callee()->AddAudioVideoTracks();
+ caller()->CreateAndSetAndSignalOffer();
+ ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout);
+
+ // Ensure that the data channel transport is ready.
+ loopback_media_transports()->SetState(webrtc::MediaTransportState::kWritable);
+ loopback_media_transports()->FlushAsyncInvokes();
+
+ // Negotiation should succeed, allowing the data channel to be established.
+ ASSERT_NE(nullptr, caller()->data_channel());
+ ASSERT_TRUE_WAIT(callee()->data_channel() != nullptr, kDefaultTimeout);
+ EXPECT_TRUE_WAIT(caller()->data_observer()->IsOpen(), kDefaultTimeout);
+ EXPECT_TRUE_WAIT(callee()->data_observer()->IsOpen(), kDefaultTimeout);
+
+ // Ensure data can be sent in both directions.
+ std::string data = "hello world";
+ caller()->data_channel()->Send(DataBuffer(data));
+ EXPECT_EQ_WAIT(data, callee()->data_observer()->last_message(),
+ kDefaultTimeout);
+ callee()->data_channel()->Send(DataBuffer(data));
+ EXPECT_EQ_WAIT(data, caller()->data_observer()->last_message(),
+ kDefaultTimeout);
+
+ // Ensure that failure of the datagram negotiation doesn't impede media flow.
+ MediaExpectations media_expectations;
+ media_expectations.ExpectBidirectionalAudioAndVideo();
+ ASSERT_TRUE(ExpectNewFrames(media_expectations));
+}
+
#endif // HAVE_SCTP
// This test sets up a call between two parties with a datagram transport data
@@ -3620,7 +3668,7 @@
caller()->CreateAndSetAndSignalOffer();
ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout);
- // Ensure that the media transport is ready.
+ // Ensure that the data channel transport is ready.
loopback_media_transports()->SetState(webrtc::MediaTransportState::kWritable);
loopback_media_transports()->FlushAsyncInvokes();
@@ -3706,7 +3754,7 @@
caller()->CreateAndSetAndSignalOffer();
ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout);
- // Ensure that the media transport is ready.
+ // Ensure that the data channel transport is ready.
loopback_media_transports()->SetState(webrtc::MediaTransportState::kWritable);
loopback_media_transports()->FlushAsyncInvokes();
@@ -3743,7 +3791,7 @@
caller()->CreateAndSetAndSignalOffer();
ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout);
- // Ensure that the media transport is ready.
+ // Ensure that the data channel transport is ready.
loopback_media_transports()->SetState(webrtc::MediaTransportState::kWritable);
loopback_media_transports()->FlushAsyncInvokes();
