From 2697ac1a1bf38a6988d57b4e7d18c70395d9fbd0 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Mon, 16 Dec 2019 10:37:04 +0100 Subject: [PATCH] Stop an SCTP connection when the DTLS transport closes. This CL propagates a "closed" signal from DTLS up to the SCTP section of the data channel controller, where it causes closing of all open datachannels. Bug: chromium:1030631, webrtc:10360 Change-Id: I88bb9e1aff5c25f330edfd092ef609d4fcc3a9f8 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/162206 Reviewed-by: Steve Anton Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#30099} --- .../data_channel_transport_interface.h | 5 +++ media/sctp/sctp_transport.cc | 6 +++ media/sctp/sctp_transport.h | 1 + media/sctp/sctp_transport_internal.h | 3 ++ p2p/base/dtls_transport.cc | 2 + p2p/base/packet_transport_internal.h | 3 ++ pc/data_channel_controller.cc | 9 +++++ pc/data_channel_controller.h | 1 + pc/peer_connection_integrationtest.cc | 38 +++++++++++++++++++ pc/sctp_data_channel_transport.cc | 8 ++++ pc/sctp_data_channel_transport.h | 1 + 11 files changed, 77 insertions(+) diff --git a/api/transport/data_channel_transport_interface.h b/api/transport/data_channel_transport_interface.h index db53a5ed1d..671deffc6e 100644 --- a/api/transport/data_channel_transport_interface.h +++ b/api/transport/data_channel_transport_interface.h @@ -84,6 +84,11 @@ class DataChannelSink { // invoked again following send errors (eg. due to the transport being // temporarily blocked or unavailable). virtual void OnReadyToSend() = 0; + + // Callback issued when the data channel becomes unusable (closed). + // TODO(https://crbug.com/webrtc/10360): Make pure virtual when all + // consumers updated. + virtual void OnTransportClosed() {} }; // Transport for data channels. diff --git a/media/sctp/sctp_transport.cc b/media/sctp/sctp_transport.cc index 9f1e862650..31489eba26 100644 --- a/media/sctp/sctp_transport.cc +++ b/media/sctp/sctp_transport.cc @@ -662,6 +662,7 @@ void SctpTransport::ConnectTransportSignals() { transport_->SignalWritableState.connect(this, &SctpTransport::OnWritableState); transport_->SignalReadPacket.connect(this, &SctpTransport::OnPacketRead); + transport_->SignalClosed.connect(this, &SctpTransport::OnClosed); } void SctpTransport::DisconnectTransportSignals() { @@ -671,6 +672,7 @@ void SctpTransport::DisconnectTransportSignals() { } transport_->SignalWritableState.disconnect(this); transport_->SignalReadPacket.disconnect(this); + transport_->SignalClosed.disconnect(this); } bool SctpTransport::Connect() { @@ -990,6 +992,10 @@ void SctpTransport::OnPacketRead(rtc::PacketTransportInternal* transport, } } +void SctpTransport::OnClosed(rtc::PacketTransportInternal* transport) { + SignalClosedAbruptly(); +} + void SctpTransport::OnSendThresholdCallback() { RTC_DCHECK_RUN_ON(network_thread_); if (partial_outgoing_message_.has_value()) { diff --git a/media/sctp/sctp_transport.h b/media/sctp/sctp_transport.h index 7337f01033..d346cfc71f 100644 --- a/media/sctp/sctp_transport.h +++ b/media/sctp/sctp_transport.h @@ -164,6 +164,7 @@ class SctpTransport : public SctpTransportInternal, size_t len, const int64_t& packet_time_us, int flags); + void OnClosed(rtc::PacketTransportInternal* transport); // Methods related to usrsctp callbacks. void OnSendThresholdCallback(); diff --git a/media/sctp/sctp_transport_internal.h b/media/sctp/sctp_transport_internal.h index 378453a5cb..b0e0e0f7e6 100644 --- a/media/sctp/sctp_transport_internal.h +++ b/media/sctp/sctp_transport_internal.h @@ -134,6 +134,9 @@ class SctpTransportInternal { // Parameter is SID; fired when closing procedure is complete (both incoming // and outgoing streams reset). sigslot::signal1 SignalClosingProcedureComplete; + // Fired when the underlying DTLS transport has closed due to an error + // or an incoming DTLS disconnect. + sigslot::signal0<> SignalClosedAbruptly; // Helper for debugging. virtual void set_debug_name_for_testing(const char* debug_name) = 0; diff --git a/p2p/base/dtls_transport.cc b/p2p/base/dtls_transport.cc index acd5765f59..538aa86f2c 100644 --- a/p2p/base/dtls_transport.cc +++ b/p2p/base/dtls_transport.cc @@ -656,6 +656,7 @@ void DtlsTransport::OnDtlsEvent(rtc::StreamInterface* dtls, int sig, int err) { RTC_LOG(LS_INFO) << ToString() << ": DTLS transport closed by remote"; set_writable(false); set_dtls_state(DTLS_TRANSPORT_CLOSED); + SignalClosed(this); } else if (ret == rtc::SR_ERROR) { // Remote peer shut down the association with an error. RTC_LOG(LS_INFO) @@ -664,6 +665,7 @@ void DtlsTransport::OnDtlsEvent(rtc::StreamInterface* dtls, int sig, int err) { << read_error; set_writable(false); set_dtls_state(DTLS_TRANSPORT_FAILED); + SignalClosed(this); } } while (ret == rtc::SR_SUCCESS); } diff --git a/p2p/base/packet_transport_internal.h b/p2p/base/packet_transport_internal.h index a5321835a9..f65d7f4981 100644 --- a/p2p/base/packet_transport_internal.h +++ b/p2p/base/packet_transport_internal.h @@ -95,6 +95,9 @@ class RTC_EXPORT PacketTransportInternal : public sigslot::has_slots<> { // Signalled when the current network route has changed. sigslot::signal1> SignalNetworkRouteChanged; + // Signalled when the transport is closed. + sigslot::signal1 SignalClosed; + protected: PacketTransportInternal(); ~PacketTransportInternal() override; diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index cb933843d8..2800992ab6 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -181,6 +181,15 @@ void DataChannelController::OnReadyToSend() { }); } +void DataChannelController::OnTransportClosed() { + RTC_DCHECK_RUN_ON(network_thread()); + data_channel_transport_invoker_->AsyncInvoke( + RTC_FROM_HERE, signaling_thread(), [this] { + RTC_DCHECK_RUN_ON(signaling_thread()); + OnTransportChannelClosed(); + }); +} + void DataChannelController::SetupDataChannelTransport_n() { RTC_DCHECK_RUN_ON(network_thread()); data_channel_transport_invoker_ = std::make_unique(); diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h index 91bba66066..5e00259efe 100644 --- a/pc/data_channel_controller.h +++ b/pc/data_channel_controller.h @@ -46,6 +46,7 @@ class DataChannelController : public DataChannelProviderInterface, void OnChannelClosing(int channel_id) override; void OnChannelClosed(int channel_id) override; void OnReadyToSend() override; + void OnTransportClosed() override; // Called from PeerConnection::SetupDataChannelTransport_n void SetupDataChannelTransport_n(); diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 7a8d152abc..58f5aa63fe 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -5989,6 +5989,44 @@ TEST_F(PeerConnectionIntegrationTestUnifiedPlan, ASSERT_TRUE(caller()->data_observer()->IsOpen()); } +TEST_F(PeerConnectionIntegrationTestUnifiedPlan, DataChannelClosesWhenClosed) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + caller()->CreateDataChannel(); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + ASSERT_TRUE_WAIT(callee()->data_observer(), kDefaultTimeout); + ASSERT_TRUE_WAIT(callee()->data_observer()->IsOpen(), kDefaultTimeout); + caller()->data_channel()->Close(); + ASSERT_TRUE_WAIT(!callee()->data_observer()->IsOpen(), kDefaultTimeout); +} + +TEST_F(PeerConnectionIntegrationTestUnifiedPlan, + DataChannelClosesWhenClosedReverse) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + caller()->CreateDataChannel(); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + ASSERT_TRUE_WAIT(callee()->data_observer(), kDefaultTimeout); + ASSERT_TRUE_WAIT(callee()->data_observer()->IsOpen(), kDefaultTimeout); + callee()->data_channel()->Close(); + ASSERT_TRUE_WAIT(!caller()->data_observer()->IsOpen(), kDefaultTimeout); +} + +TEST_F(PeerConnectionIntegrationTestUnifiedPlan, + DataChannelClosesWhenPeerConnectionClosed) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + caller()->CreateDataChannel(); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + ASSERT_TRUE_WAIT(callee()->data_observer(), kDefaultTimeout); + ASSERT_TRUE_WAIT(callee()->data_observer()->IsOpen(), kDefaultTimeout); + caller()->pc()->Close(); + ASSERT_TRUE_WAIT(!callee()->data_observer()->IsOpen(), kDefaultTimeout); +} + #endif // HAVE_SCTP } // namespace diff --git a/pc/sctp_data_channel_transport.cc b/pc/sctp_data_channel_transport.cc index d1505f3945..497e11fcc9 100644 --- a/pc/sctp_data_channel_transport.cc +++ b/pc/sctp_data_channel_transport.cc @@ -24,6 +24,8 @@ SctpDataChannelTransport::SctpDataChannelTransport( this, &SctpDataChannelTransport::OnClosingProcedureStartedRemotely); sctp_transport_->SignalClosingProcedureComplete.connect( this, &SctpDataChannelTransport::OnClosingProcedureComplete); + sctp_transport_->SignalClosedAbruptly.connect( + this, &SctpDataChannelTransport::OnClosedAbruptly); } RTCError SctpDataChannelTransport::OpenChannel(int channel_id) { @@ -109,4 +111,10 @@ void SctpDataChannelTransport::OnClosingProcedureComplete(int channel_id) { } } +void SctpDataChannelTransport::OnClosedAbruptly() { + if (sink_) { + sink_->OnTransportClosed(); + } +} + } // namespace webrtc diff --git a/pc/sctp_data_channel_transport.h b/pc/sctp_data_channel_transport.h index 281c30edf4..623a490053 100644 --- a/pc/sctp_data_channel_transport.h +++ b/pc/sctp_data_channel_transport.h @@ -38,6 +38,7 @@ class SctpDataChannelTransport : public DataChannelTransportInterface, const rtc::CopyOnWriteBuffer& buffer); void OnClosingProcedureStartedRemotely(int channel_id); void OnClosingProcedureComplete(int channel_id); + void OnClosedAbruptly(); cricket::SctpTransportInternal* const sctp_transport_;