zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Unset sinks when deleting CompositeDataChannelTransport.

Browse Source 
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}
This commit is contained in:
Bjorn A Mellem
2019-09-24 14:59:21 -07:00
committed by Commit Bot
parent 9d91174d39
commit fc604aa990
5 changed files with 63 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
pc/composite_data_channel_transport.cc
View File

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

1
pc/composite_data_channel_transport.h
View File

@ -26,6 +26,7 @@ class CompositeDataChannelTransport : public DataChannelTransportInterface,
public:
explicit CompositeDataChannelTransport(
std::vector<DataChannelTransportInterface*> transports);
~CompositeDataChannelTransport() override;
// Specifies which transport to be used for sending. Must be called before
// sending data.

10
pc/jsep_transport.cc
View File

@ -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<webrtc::DtlsTransport>(
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.
}

5
pc/jsep_transport.h
View File

@ -378,8 +378,6 @@ class JsepTransport : public sigslot::has_slots<>,
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 @@ class JsepTransport : public sigslot::has_slots<>,
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.

54
pc/peer_connection_integrationtest.cc
View File

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