Make SCTPtransport enter "closed" state when DTLStransport does. Bug: webrtc:11090 Change-Id: I30e0b70387746d6c544ed1818f276569d4258cf4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/159888 Reviewed-by: Emad Omara <emadomara@webrtc.org> Commit-Queue: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/master@{#29810}
diff --git a/p2p/base/fake_dtls_transport.h b/p2p/base/fake_dtls_transport.h index cf6fd78..7061ea4 100644 --- a/p2p/base/fake_dtls_transport.h +++ b/p2p/base/fake_dtls_transport.h
@@ -83,7 +83,10 @@ ice_transport_->SetReceiving(receiving); set_receiving(receiving); } - void SetDtlsState(DtlsTransportState state) { dtls_state_ = state; } + void SetDtlsState(DtlsTransportState state) { + dtls_state_ = state; + SignalDtlsState(this, dtls_state_); + } // Simulates the two DTLS transports connecting to each other. // If |asymmetric| is true this method only affects this FakeDtlsTransport. @@ -108,12 +111,11 @@ if (!asymmetric) { dest->SetDestination(this, true); } - dtls_state_ = DTLS_TRANSPORT_CONNECTED; // If the |dtls_role_| is unset, set it to SSL_CLIENT by default. if (!dtls_role_) { dtls_role_ = std::move(rtc::SSL_CLIENT); } - SignalDtlsState(this, dtls_state_); + SetDtlsState(DTLS_TRANSPORT_CONNECTED); ice_transport_->SetDestination( static_cast<FakeIceTransport*>(dest->ice_transport()), asymmetric); } else {
diff --git a/pc/data_channel.cc b/pc/data_channel.cc index c5a8aeb..e87bb85 100644 --- a/pc/data_channel.cc +++ b/pc/data_channel.cc
@@ -359,9 +359,10 @@ } } -void DataChannel::OnTransportChannelDestroyed() { - // The SctpTransport is going away (for example, because the SCTP m= section - // was rejected), so we need to close abruptly. +void DataChannel::OnTransportChannelClosed() { + // The SctpTransport is unusable (for example, because the SCTP m= section + // was rejected, or because the DTLS transport closed), so we need to close + // abruptly. CloseAbruptly(); }
diff --git a/pc/data_channel.h b/pc/data_channel.h index 728226c..c7dc6ea 100644 --- a/pc/data_channel.h +++ b/pc/data_channel.h
@@ -185,10 +185,10 @@ // Called when the transport channel is created. // Only needs to be called for SCTP data channels. void OnTransportChannelCreated(); - // Called when the transport channel is destroyed. + // Called when the transport channel is unusable. // This method makes sure the DataChannel is disconnected and changes state // to kClosed. - void OnTransportChannelDestroyed(); + void OnTransportChannelClosed(); /******************************************* * The following methods are for RTP only. *
diff --git a/pc/data_channel_unittest.cc b/pc/data_channel_unittest.cc index ad0e9b6..46e1df2 100644 --- a/pc/data_channel_unittest.cc +++ b/pc/data_channel_unittest.cc
@@ -610,10 +610,10 @@ provider_->set_send_blocked(true); EXPECT_TRUE(webrtc_data_channel_->Send(packet)); - // Tell the data channel that its tranpsort is being destroyed. + // Tell the data channel that its transport is being destroyed. // It should then stop using the transport (allowing us to delete it) and // transition to the "closed" state. - webrtc_data_channel_->OnTransportChannelDestroyed(); + webrtc_data_channel_->OnTransportChannelClosed(); provider_.reset(nullptr); EXPECT_EQ_WAIT(webrtc::DataChannelInterface::kClosed, webrtc_data_channel_->state(), kDefaultTimeout);
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 1d7b4ea..9fac748 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc
@@ -1106,7 +1106,7 @@ DestroyTransceiverChannel(transceiver); } } - DestroyDataChannel(); + DestroyDataChannelTransport(); } bool PeerConnection::Initialize( @@ -3597,7 +3597,7 @@ } if (content.rejected) { RTC_LOG(LS_INFO) << "Rejected data channel, mid=" << content.mid(); - DestroyDataChannel(); + DestroyDataChannelTransport(); } else { if (!rtp_data_channel_ && !data_channel_transport_) { RTC_LOG(LS_INFO) << "Creating data channel, mid=" << content.mid(); @@ -5907,19 +5907,19 @@ } } -void PeerConnection::OnDataChannelDestroyed() { +void PeerConnection::OnTransportChannelClosed() { // Use a temporary copy of the RTP/SCTP DataChannel list because the // DataChannel may callback to us and try to modify the list. std::map<std::string, rtc::scoped_refptr<DataChannel>> temp_rtp_dcs; temp_rtp_dcs.swap(rtp_data_channels_); for (const auto& kv : temp_rtp_dcs) { - kv.second->OnTransportChannelDestroyed(); + kv.second->OnTransportChannelClosed(); } std::vector<rtc::scoped_refptr<DataChannel>> temp_sctp_dcs; temp_sctp_dcs.swap(sctp_data_channels_); for (const auto& channel : temp_sctp_dcs) { - channel->OnTransportChannelDestroyed(); + channel->OnTransportChannelClosed(); } } @@ -6978,7 +6978,7 @@ const cricket::ContentInfo* data_info = cricket::GetFirstDataContent(desc); if (!data_info || data_info->rejected) { - DestroyDataChannel(); + DestroyDataChannelTransport(); } } @@ -7709,9 +7709,9 @@ } } -void PeerConnection::DestroyDataChannel() { +void PeerConnection::DestroyDataChannelTransport() { if (rtp_data_channel_) { - OnDataChannelDestroyed(); + OnTransportChannelClosed(); DestroyChannelInterface(rtp_data_channel_); rtp_data_channel_ = nullptr; } @@ -7723,7 +7723,7 @@ // rtc::Bind will cause "Pure virtual function called" error to appear. if (sctp_mid_) { - OnDataChannelDestroyed(); + OnTransportChannelClosed(); network_thread()->Invoke<void>(RTC_FROM_HERE, [this] { RTC_DCHECK_RUN_ON(network_thread()); TeardownDataChannelTransport_n();
diff --git a/pc/peer_connection.h b/pc/peer_connection.h index cbff7e7..19af912 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h
@@ -854,7 +854,8 @@ void OnSctpDataChannelClosed(DataChannel* channel) RTC_RUN_ON(signaling_thread()); - void OnDataChannelDestroyed() RTC_RUN_ON(signaling_thread()); + // Called when the transport for the data channels is closed or destroyed. + void OnTransportChannelClosed() RTC_RUN_ON(signaling_thread()); // Called when a valid data channel OPEN message is received. void OnDataChannelOpenMessage(const std::string& label, const InternalDataChannelInit& config) @@ -1169,14 +1170,19 @@ const std::string GetTransportName(const std::string& content_name) RTC_RUN_ON(signaling_thread()); + // Functions for dealing with transports. + // Note that cricket code uses the term "channel" for what other code + // refers to as "transport". + // Destroys and clears the BaseChannel associated with the given transceiver, // if such channel is set. void DestroyTransceiverChannel( rtc::scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>> transceiver); - // Destroys the RTP data channel and/or the SCTP data channel and clears it. - void DestroyDataChannel() RTC_RUN_ON(signaling_thread()); + // Destroys the RTP data channel transport and/or the SCTP data channel + // transport and clears it. + void DestroyDataChannelTransport() RTC_RUN_ON(signaling_thread()); // Destroys the given ChannelInterface. // The channel cannot be accessed after this method is called.
diff --git a/pc/sctp_transport.cc b/pc/sctp_transport.cc index 6c4a8be..532e91c 100644 --- a/pc/sctp_transport.cc +++ b/pc/sctp_transport.cc
@@ -86,6 +86,8 @@ if (internal_sctp_transport_) { if (transport) { internal_sctp_transport_->SetDtlsTransport(transport->internal()); + transport->internal()->SignalDtlsState.connect( + this, &SctpTransport::OnDtlsStateChange); if (info_.state() == SctpTransportState::kNew) { next_state = SctpTransportState::kConnecting; } @@ -162,4 +164,15 @@ UpdateInformation(SctpTransportState::kConnected); } +void SctpTransport::OnDtlsStateChange(cricket::DtlsTransportInternal* transport, + cricket::DtlsTransportState state) { + RTC_DCHECK_RUN_ON(owner_thread_); + RTC_CHECK(transport == dtls_transport_->internal()); + if (state == cricket::DTLS_TRANSPORT_CLOSED || + state == cricket::DTLS_TRANSPORT_FAILED) { + UpdateInformation(SctpTransportState::kClosed); + // TODO(http://bugs.webrtc.org/11090): Close all the data channels + } +} + } // namespace webrtc
diff --git a/pc/sctp_transport.h b/pc/sctp_transport.h index c7727df..a13a58c 100644 --- a/pc/sctp_transport.h +++ b/pc/sctp_transport.h
@@ -65,6 +65,8 @@ void OnAssociationChangeCommunicationUp(); void OnInternalClosingProcedureStartedRemotely(int sid); void OnInternalClosingProcedureComplete(int sid); + void OnDtlsStateChange(cricket::DtlsTransportInternal* transport, + cricket::DtlsTransportState state); // Note - owner_thread never changes, but can't be const if we do // Invoke() on it.
diff --git a/pc/sctp_transport_unittest.cc b/pc/sctp_transport_unittest.cc index 8566ef3..f3070cd 100644 --- a/pc/sctp_transport_unittest.cc +++ b/pc/sctp_transport_unittest.cc
@@ -195,4 +195,17 @@ *(observer_.LastReceivedInformation().MaxChannels())); } +TEST_F(SctpTransportTest, CloseWhenTransportCloses) { + CreateTransport(); + transport()->RegisterObserver(observer()); + AddDtlsTransport(); + CompleteSctpHandshake(); + ASSERT_EQ_WAIT(SctpTransportState::kConnected, observer_.State(), + kDefaultTimeout); + static_cast<cricket::FakeDtlsTransport*>(dtls_transport_->internal()) + ->SetDtlsState(cricket::DTLS_TRANSPORT_CLOSED); + ASSERT_EQ_WAIT(SctpTransportState::kClosed, observer_.State(), + kDefaultTimeout); +} + } // namespace webrtc