From 78f87ab106a12728daa3dc8c1f7ae8f6673a7484 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Mon, 1 Feb 2021 12:19:19 +0100 Subject: [PATCH] Delete use of RecursiveCriticalSection in JsepTransport Mark corresponding webrtc::Mutex as mutable, to allow use in const methods. Bug: webrtc:11567 Change-Id: Ia8c731a91c719a531799abf24fd30a15b54428af Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/204801 Reviewed-by: Markus Handell Reviewed-by: Tommi Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#33126} --- pc/jsep_transport.cc | 18 +++++++++--------- pc/jsep_transport.h | 32 ++++++++++++++++++-------------- 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index 9d700211d2..787e9b68df 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -176,7 +176,7 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( // If doing SDES, setup the SDES crypto parameters. { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); if (sdes_transport_) { RTC_DCHECK(!unencrypted_rtp_transport_); RTC_DCHECK(!dtls_srtp_transport_); @@ -213,7 +213,7 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( } } { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); RTC_DCHECK(rtp_dtls_transport_->internal()); rtp_dtls_transport_->internal()->ice_transport()->SetIceParameters( ice_parameters); @@ -233,7 +233,7 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( return error; } { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); if (needs_ice_restart_ && ice_restarting) { needs_ice_restart_ = false; RTC_LOG(LS_VERBOSE) << "needs-ice-restart flag cleared for transport " @@ -270,7 +270,7 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription( // If doing SDES, setup the SDES crypto parameters. { - rtc::CritScope lock(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); if (sdes_transport_) { RTC_DCHECK(!unencrypted_rtp_transport_); RTC_DCHECK(!dtls_srtp_transport_); @@ -341,7 +341,7 @@ webrtc::RTCError JsepTransport::AddRemoteCandidates( } void JsepTransport::SetNeedsIceRestartFlag() { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); if (!needs_ice_restart_) { needs_ice_restart_ = true; RTC_LOG(LS_VERBOSE) << "needs-ice-restart flag set for transport " << mid(); @@ -350,7 +350,7 @@ void JsepTransport::SetNeedsIceRestartFlag() { absl::optional JsepTransport::GetDtlsRole() const { RTC_DCHECK_RUN_ON(network_thread_); - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); RTC_DCHECK(rtp_dtls_transport_); RTC_DCHECK(rtp_dtls_transport_->internal()); rtc::SSLRole dtls_role; @@ -363,7 +363,7 @@ absl::optional JsepTransport::GetDtlsRole() const { bool JsepTransport::GetStats(TransportStats* stats) { RTC_DCHECK_RUN_ON(network_thread_); - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); stats->transport_name = mid(); stats->channel_stats.clear(); RTC_DCHECK(rtp_dtls_transport_->internal()); @@ -405,7 +405,7 @@ webrtc::RTCError JsepTransport::VerifyCertificateFingerprint( void JsepTransport::SetActiveResetSrtpParams(bool active_reset_srtp_params) { RTC_DCHECK_RUN_ON(network_thread_); - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); if (dtls_srtp_transport_) { RTC_LOG(INFO) << "Setting active_reset_srtp_params of DtlsSrtpTransport to: " @@ -487,7 +487,7 @@ void JsepTransport::ActivateRtcpMux() { RTC_DCHECK_RUN_ON(network_thread_); } { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); if (unencrypted_rtp_transport_) { RTC_DCHECK(!sdes_transport_); RTC_DCHECK(!dtls_srtp_transport_); diff --git a/pc/jsep_transport.h b/pc/jsep_transport.h index 0ade20e4e8..20d0c926e0 100644 --- a/pc/jsep_transport.h +++ b/pc/jsep_transport.h @@ -44,10 +44,10 @@ #include "pc/transport_stats.h" #include "rtc_base/checks.h" #include "rtc_base/constructor_magic.h" -#include "rtc_base/deprecated/recursive_critical_section.h" #include "rtc_base/rtc_certificate.h" #include "rtc_base/ssl_fingerprint.h" #include "rtc_base/ssl_stream_adapter.h" +#include "rtc_base/synchronization/mutex.h" #include "rtc_base/synchronization/sequence_checker.h" #include "rtc_base/third_party/sigslot/sigslot.h" #include "rtc_base/thread.h" @@ -144,12 +144,14 @@ class JsepTransport : public sigslot::has_slots<> { // // This and the below method can be called safely from any thread as long as // SetXTransportDescription is not in progress. + // TODO(tommi): Investigate on which threads (network or signal?) we really + // need to access the needs_ice_restart flag. void SetNeedsIceRestartFlag() RTC_LOCKS_EXCLUDED(accessor_lock_); // Returns true if the ICE restart flag above was set, and no ICE restart has // occurred yet for this transport (by applying a local description with // changed ufrag/password). bool needs_ice_restart() const RTC_LOCKS_EXCLUDED(accessor_lock_) { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); return needs_ice_restart_; } @@ -173,7 +175,7 @@ class JsepTransport : public sigslot::has_slots<> { webrtc::RtpTransportInternal* rtp_transport() const RTC_LOCKS_EXCLUDED(accessor_lock_) { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); if (composite_rtp_transport_) { return composite_rtp_transport_.get(); } else if (datagram_rtp_transport_) { @@ -185,7 +187,7 @@ class JsepTransport : public sigslot::has_slots<> { const DtlsTransportInternal* rtp_dtls_transport() const RTC_LOCKS_EXCLUDED(accessor_lock_) { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); if (rtp_dtls_transport_) { return rtp_dtls_transport_->internal(); } else { @@ -195,13 +197,13 @@ class JsepTransport : public sigslot::has_slots<> { DtlsTransportInternal* rtp_dtls_transport() RTC_LOCKS_EXCLUDED(accessor_lock_) { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); return rtp_dtls_transport_locked(); } const DtlsTransportInternal* rtcp_dtls_transport() const RTC_LOCKS_EXCLUDED(accessor_lock_) { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); if (rtcp_dtls_transport_) { return rtcp_dtls_transport_->internal(); } else { @@ -211,7 +213,7 @@ class JsepTransport : public sigslot::has_slots<> { DtlsTransportInternal* rtcp_dtls_transport() RTC_LOCKS_EXCLUDED(accessor_lock_) { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); if (rtcp_dtls_transport_) { return rtcp_dtls_transport_->internal(); } else { @@ -221,13 +223,13 @@ class JsepTransport : public sigslot::has_slots<> { rtc::scoped_refptr RtpDtlsTransport() RTC_LOCKS_EXCLUDED(accessor_lock_) { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); return rtp_dtls_transport_; } rtc::scoped_refptr SctpTransport() const RTC_LOCKS_EXCLUDED(accessor_lock_) { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); return sctp_transport_; } @@ -235,7 +237,7 @@ class JsepTransport : public sigslot::has_slots<> { // SctpTransport() instead. webrtc::DataChannelTransportInterface* data_channel_transport() const RTC_LOCKS_EXCLUDED(accessor_lock_) { - rtc::CritScope scope(&accessor_lock_); + webrtc::MutexLock lock(&accessor_lock_); if (sctp_data_channel_transport_) { return sctp_data_channel_transport_.get(); } @@ -257,7 +259,8 @@ class JsepTransport : public sigslot::has_slots<> { const rtc::RTCCertificate* certificate, const rtc::SSLFingerprint* fingerprint) const; - void SetActiveResetSrtpParams(bool active_reset_srtp_params); + void SetActiveResetSrtpParams(bool active_reset_srtp_params) + RTC_LOCKS_EXCLUDED(accessor_lock_); private: DtlsTransportInternal* rtp_dtls_transport_locked() @@ -271,7 +274,7 @@ class JsepTransport : public sigslot::has_slots<> { bool SetRtcpMux(bool enable, webrtc::SdpType type, ContentSource source); - void ActivateRtcpMux(); + void ActivateRtcpMux() RTC_LOCKS_EXCLUDED(accessor_lock_); bool SetSdes(const std::vector& cryptos, const std::vector& encrypted_extension_ids, @@ -327,9 +330,10 @@ class JsepTransport : public sigslot::has_slots<> { // Owning thread, for safety checks const rtc::Thread* const network_thread_; - // Critical scope for fields accessed off-thread + // Critical scope for fields accessed off-thread. Mutable, since it is used by + // getter methods. // TODO(https://bugs.webrtc.org/10300): Stop doing this. - rtc::RecursiveCriticalSection accessor_lock_; + mutable webrtc::Mutex accessor_lock_; const std::string mid_; // needs-ice-restart bit as described in JSEP. bool needs_ice_restart_ RTC_GUARDED_BY(accessor_lock_) = false;