From fc604aa990d5313307d2950b1e68cfa61e99948d Mon Sep 17 00:00:00 2001 From: Bjorn A Mellem Date: Tue, 24 Sep 2019 14:59:21 -0700 Subject: [PATCH] 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 Reviewed-by: Seth Hampson Cr-Commit-Position: refs/heads/master@{#29292} --- pc/composite_data_channel_transport.cc | 6 +++ pc/composite_data_channel_transport.h | 1 + pc/jsep_transport.cc | 10 +---- pc/jsep_transport.h | 5 ++- pc/peer_connection_integrationtest.cc | 54 ++++++++++++++++++++++++-- 5 files changed, 63 insertions(+), 13 deletions(-) diff --git a/pc/composite_data_channel_transport.cc b/pc/composite_data_channel_transport.cc index 185dd1e23a..e66febc12b 100644 --- a/pc/composite_data_channel_transport.cc +++ b/pc/composite_data_channel_transport.cc @@ -24,6 +24,12 @@ CompositeDataChannelTransport::CompositeDataChannelTransport( } } +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 ccff4fe7ab..b2a40fdb7a 100644 --- a/pc/composite_data_channel_transport.h +++ b/pc/composite_data_channel_transport.h @@ -26,6 +26,7 @@ class CompositeDataChannelTransport : public DataChannelTransportInterface, public: explicit CompositeDataChannelTransport( std::vector 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 b95dc22cfb..7c83e85e87 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -114,7 +114,6 @@ JsepTransport::JsepTransport( 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( std::move(rtp_dtls_transport)) @@ -134,6 +133,7 @@ JsepTransport::JsepTransport( : 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( } 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 @@ JsepTransport::~JsepTransport() { 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 868f7b92c6..b6199f8d0f 100644 --- a/pc/jsep_transport.h +++ b/pc/jsep_transport.h @@ -378,8 +378,6 @@ class JsepTransport : public sigslot::has_slots<>, RTC_GUARDED_BY(accessor_lock_); std::unique_ptr dtls_srtp_transport_ RTC_GUARDED_BY(accessor_lock_); - std::unique_ptr 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 @@ class JsepTransport : public sigslot::has_slots<>, std::unique_ptr datagram_transport_ RTC_GUARDED_BY(accessor_lock_); + std::unique_ptr 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 33fc9b9b9d..465dca12bc 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -3599,6 +3599,54 @@ TEST_P(PeerConnectionIntegrationTest, 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 @@ TEST_P(PeerConnectionIntegrationTest, DatagramTransportDataChannelEndToEnd) { 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 @@ TEST_P(PeerConnectionIntegrationTest, 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 @@ TEST_P(PeerConnectionIntegrationTest, 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();