diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index 284a72238e..aacee66190 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -99,12 +99,8 @@ JsepTransport::JsepTransport( std::unique_ptr rtp_dtls_transport, std::unique_ptr rtcp_dtls_transport, std::unique_ptr media_transport) - : network_thread_(rtc::Thread::Current()), - mid_(mid), + : mid_(mid), local_certificate_(local_certificate), - unencrypted_rtp_transport_(std::move(unencrypted_rtp_transport)), - sdes_transport_(std::move(sdes_transport)), - dtls_srtp_transport_(std::move(dtls_srtp_transport)), rtp_dtls_transport_( rtp_dtls_transport ? new rtc::RefCountedObject( std::move(rtp_dtls_transport)) @@ -116,17 +112,19 @@ JsepTransport::JsepTransport( : nullptr), media_transport_(std::move(media_transport)) { RTC_DCHECK(rtp_dtls_transport_); - // Verify the "only one out of these three can be set" invariant. - if (unencrypted_rtp_transport_) { + if (unencrypted_rtp_transport) { RTC_DCHECK(!sdes_transport); RTC_DCHECK(!dtls_srtp_transport); - } else if (sdes_transport_) { + unencrypted_rtp_transport_ = std::move(unencrypted_rtp_transport); + } else if (sdes_transport) { RTC_DCHECK(!unencrypted_rtp_transport); RTC_DCHECK(!dtls_srtp_transport); + sdes_transport_ = std::move(sdes_transport); } else { - RTC_DCHECK(dtls_srtp_transport_); + RTC_DCHECK(dtls_srtp_transport); RTC_DCHECK(!unencrypted_rtp_transport); RTC_DCHECK(!sdes_transport); + dtls_srtp_transport_ = std::move(dtls_srtp_transport); } if (media_transport_) { @@ -154,7 +152,6 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( SdpType type) { webrtc::RTCError error; - RTC_DCHECK_RUN_ON(network_thread_); if (!VerifyIceParams(jsep_description)) { return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, "Invalid ice-ufrag or ice-pwd length."); @@ -182,6 +179,7 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( dtls_srtp_transport_->UpdateRecvEncryptedHeaderExtensionIds( jsep_description.encrypted_header_extension_ids); } + bool ice_restarting = local_description_ != nullptr && IceCredentialsChanged(local_description_->transport_desc.ice_ufrag, @@ -196,22 +194,21 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( if (!local_fp) { local_certificate_ = nullptr; } else { - error = VerifyCertificateFingerprint(local_certificate_, local_fp); + error = VerifyCertificateFingerprint(local_certificate_.get(), local_fp); if (!error.ok()) { local_description_.reset(); return error; } } - { - rtc::CritScope scope(&accessor_lock_); - RTC_DCHECK(rtp_dtls_transport_->internal()); - SetLocalIceParameters(rtp_dtls_transport_->internal()->ice_transport()); - if (rtcp_dtls_transport_) { - RTC_DCHECK(rtcp_dtls_transport_->internal()); - SetLocalIceParameters(rtcp_dtls_transport_->internal()->ice_transport()); - } + RTC_DCHECK(rtp_dtls_transport_->internal()); + SetLocalIceParameters(rtp_dtls_transport_->internal()->ice_transport()); + + if (rtcp_dtls_transport_) { + RTC_DCHECK(rtcp_dtls_transport_->internal()); + SetLocalIceParameters(rtcp_dtls_transport_->internal()->ice_transport()); } + // If PRANSWER/ANSWER is set, we should decide transport protocol type. if (type == SdpType::kPrAnswer || type == SdpType::kAnswer) { error = NegotiateAndSetDtlsParameters(type); @@ -220,13 +217,11 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( local_description_.reset(); return error; } - { - rtc::CritScope scope(&accessor_lock_); - if (needs_ice_restart_ && ice_restarting) { - needs_ice_restart_ = false; - RTC_LOG(LS_VERBOSE) << "needs-ice-restart flag cleared for transport " - << mid(); - } + + if (needs_ice_restart_ && ice_restarting) { + needs_ice_restart_ = false; + RTC_LOG(LS_VERBOSE) << "needs-ice-restart flag cleared for transport " + << mid(); } return webrtc::RTCError::OK(); @@ -237,7 +232,6 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription( webrtc::SdpType type) { webrtc::RTCError error; - RTC_DCHECK_RUN_ON(network_thread_); if (!VerifyIceParams(jsep_description)) { remote_description_.reset(); return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, @@ -272,11 +266,12 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription( } remote_description_.reset(new JsepTransportDescription(jsep_description)); - RTC_DCHECK(rtp_dtls_transport()); - SetRemoteIceParameters(rtp_dtls_transport()->ice_transport()); + RTC_DCHECK(rtp_dtls_transport_->internal()); + SetRemoteIceParameters(rtp_dtls_transport_->internal()->ice_transport()); - if (rtcp_dtls_transport()) { - SetRemoteIceParameters(rtcp_dtls_transport()->ice_transport()); + if (rtcp_dtls_transport_) { + RTC_DCHECK(rtcp_dtls_transport_->internal()); + SetRemoteIceParameters(rtcp_dtls_transport_->internal()->ice_transport()); } // If PRANSWER/ANSWER is set, we should decide transport protocol type. @@ -292,7 +287,6 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription( webrtc::RTCError JsepTransport::AddRemoteCandidates( const Candidates& candidates) { - RTC_DCHECK_RUN_ON(network_thread_); if (!local_description_ || !remote_description_) { return webrtc::RTCError(webrtc::RTCErrorType::INVALID_STATE, mid() + @@ -318,8 +312,6 @@ webrtc::RTCError JsepTransport::AddRemoteCandidates( } void JsepTransport::SetNeedsIceRestartFlag() { - RTC_DCHECK_RUN_ON(network_thread_); - rtc::CritScope scope(&accessor_lock_); if (!needs_ice_restart_) { needs_ice_restart_ = true; RTC_LOG(LS_VERBOSE) << "needs-ice-restart flag set for transport " << mid(); @@ -327,8 +319,6 @@ void JsepTransport::SetNeedsIceRestartFlag() { } absl::optional JsepTransport::GetDtlsRole() const { - RTC_DCHECK_RUN_ON(network_thread_); - rtc::CritScope scope(&accessor_lock_); RTC_DCHECK(rtp_dtls_transport_); RTC_DCHECK(rtp_dtls_transport_->internal()); rtc::SSLRole dtls_role; @@ -340,8 +330,6 @@ absl::optional JsepTransport::GetDtlsRole() const { } bool JsepTransport::GetStats(TransportStats* stats) { - RTC_DCHECK_RUN_ON(network_thread_); - rtc::CritScope scope(&accessor_lock_); stats->transport_name = mid(); stats->channel_stats.clear(); RTC_DCHECK(rtp_dtls_transport_->internal()); @@ -356,7 +344,6 @@ bool JsepTransport::GetStats(TransportStats* stats) { webrtc::RTCError JsepTransport::VerifyCertificateFingerprint( const rtc::RTCCertificate* certificate, const rtc::SSLFingerprint* fingerprint) const { - RTC_DCHECK_RUN_ON(network_thread_); if (!fingerprint) { return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, "No fingerprint"); @@ -382,8 +369,6 @@ webrtc::RTCError JsepTransport::VerifyCertificateFingerprint( } void JsepTransport::SetActiveResetSrtpParams(bool active_reset_srtp_params) { - RTC_DCHECK_RUN_ON(network_thread_); - rtc::CritScope scope(&accessor_lock_); if (dtls_srtp_transport_) { RTC_LOG(INFO) << "Setting active_reset_srtp_params of DtlsSrtpTransport to: " @@ -393,7 +378,6 @@ void JsepTransport::SetActiveResetSrtpParams(bool active_reset_srtp_params) { } void JsepTransport::SetLocalIceParameters(IceTransportInternal* ice_transport) { - RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK(ice_transport); RTC_DCHECK(local_description_); ice_transport->SetIceParameters( @@ -402,7 +386,6 @@ void JsepTransport::SetLocalIceParameters(IceTransportInternal* ice_transport) { void JsepTransport::SetRemoteIceParameters( IceTransportInternal* ice_transport) { - RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK(ice_transport); RTC_DCHECK(remote_description_); ice_transport->SetRemoteIceParameters( @@ -414,7 +397,6 @@ webrtc::RTCError JsepTransport::SetNegotiatedDtlsParameters( DtlsTransportInternal* dtls_transport, absl::optional dtls_role, rtc::SSLFingerprint* remote_fingerprint) { - RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK(dtls_transport); // Set SSL role. Role must be set before fingerprint is applied, which // initiates DTLS setup. @@ -436,7 +418,6 @@ webrtc::RTCError JsepTransport::SetNegotiatedDtlsParameters( bool JsepTransport::SetRtcpMux(bool enable, webrtc::SdpType type, ContentSource source) { - RTC_DCHECK_RUN_ON(network_thread_); bool ret = false; switch (type) { case SdpType::kOffer: @@ -467,12 +448,6 @@ bool JsepTransport::SetRtcpMux(bool enable, } void JsepTransport::ActivateRtcpMux() { - { - // Don't hold the network_thread_ lock while calling other functions, - // since they might call other functions that call RTC_DCHECK_RUN_ON. - // TODO(https://crbug.com/webrtc/10318): Simplify when possible. - RTC_DCHECK_RUN_ON(network_thread_); - } if (unencrypted_rtp_transport_) { RTC_DCHECK(!sdes_transport_); RTC_DCHECK(!dtls_srtp_transport_); @@ -488,10 +463,7 @@ void JsepTransport::ActivateRtcpMux() { dtls_srtp_transport_->SetDtlsTransports(rtp_dtls_transport(), /*rtcp_dtls_transport=*/nullptr); } - { - rtc::CritScope scope(&accessor_lock_); - rtcp_dtls_transport_ = nullptr; // Destroy this reference. - } + rtcp_dtls_transport_ = nullptr; // Destroy this reference. // Notify the JsepTransportController to update the aggregate states. SignalRtcpMuxActive(); } @@ -500,8 +472,6 @@ bool JsepTransport::SetSdes(const std::vector& cryptos, const std::vector& encrypted_extension_ids, webrtc::SdpType type, ContentSource source) { - RTC_DCHECK_RUN_ON(network_thread_); - rtc::CritScope scope(&accessor_lock_); bool ret = false; ret = sdes_negotiator_.Process(cryptos, type, source); if (!ret) { @@ -544,7 +514,6 @@ bool JsepTransport::SetSdes(const std::vector& cryptos, webrtc::RTCError JsepTransport::NegotiateAndSetDtlsParameters( SdpType local_description_type) { - RTC_DCHECK_RUN_ON(network_thread_); if (!local_description_ || !remote_description_) { return webrtc::RTCError(webrtc::RTCErrorType::INVALID_STATE, "Applying an answer transport description " @@ -581,16 +550,19 @@ webrtc::RTCError JsepTransport::NegotiateAndSetDtlsParameters( // between future SetRemote/SetLocal invocations and new transport // creation, we have the negotiation state saved until a new // negotiation happens. - RTC_DCHECK(rtp_dtls_transport()); + RTC_DCHECK(rtp_dtls_transport_->internal()); webrtc::RTCError error = SetNegotiatedDtlsParameters( - rtp_dtls_transport(), negotiated_dtls_role, remote_fingerprint.get()); + rtp_dtls_transport_->internal(), negotiated_dtls_role, + remote_fingerprint.get()); if (!error.ok()) { return error; } - if (rtcp_dtls_transport()) { - error = SetNegotiatedDtlsParameters( - rtcp_dtls_transport(), negotiated_dtls_role, remote_fingerprint.get()); + if (rtcp_dtls_transport_) { + RTC_DCHECK(rtcp_dtls_transport_->internal()); + error = SetNegotiatedDtlsParameters(rtcp_dtls_transport_->internal(), + negotiated_dtls_role, + remote_fingerprint.get()); } return error; } @@ -685,8 +657,6 @@ webrtc::RTCError JsepTransport::NegotiateDtlsRole( bool JsepTransport::GetTransportStats(DtlsTransportInternal* dtls_transport, TransportStats* stats) { - RTC_DCHECK_RUN_ON(network_thread_); - rtc::CritScope scope(&accessor_lock_); RTC_DCHECK(dtls_transport); TransportChannelStats substats; if (rtcp_dtls_transport_) { @@ -711,11 +681,7 @@ void JsepTransport::OnStateChanged(webrtc::MediaTransportState state) { // TODO(bugs.webrtc.org/9719) This method currently fires on the network // thread, but media transport does not make such guarantees. We need to make // sure this callback is guaranteed to be executed on the network thread. - RTC_DCHECK_RUN_ON(network_thread_); - { - rtc::CritScope scope(&accessor_lock_); - media_transport_state_ = state; - } + media_transport_state_ = state; SignalMediaTransportStateChanged(); } diff --git a/pc/jsep_transport.h b/pc/jsep_transport.h index 0f314039d5..21747a6dca 100644 --- a/pc/jsep_transport.h +++ b/pc/jsep_transport.h @@ -36,7 +36,6 @@ #include "rtc_base/rtc_certificate.h" #include "rtc_base/ssl_stream_adapter.h" #include "rtc_base/third_party/sigslot/sigslot.h" -#include "rtc_base/thread_checker.h" namespace cricket { @@ -102,13 +101,11 @@ class JsepTransport : public sigslot::has_slots<>, // Needed in order to verify the local fingerprint. void SetLocalCertificate( const rtc::scoped_refptr& local_certificate) { - RTC_DCHECK_RUN_ON(network_thread_); local_certificate_ = local_certificate; } // Return the local certificate provided by SetLocalCertificate. rtc::scoped_refptr GetLocalCertificate() const { - RTC_DCHECK_RUN_ON(network_thread_); return local_certificate_; } @@ -133,10 +130,7 @@ class JsepTransport : public sigslot::has_slots<>, // 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::CritScope scope(&accessor_lock_); - return needs_ice_restart_; - } + bool needs_ice_restart() const { return needs_ice_restart_; } // Returns role if negotiated, or empty absl::optional if it hasn't been // negotiated yet. @@ -146,19 +140,14 @@ class JsepTransport : public sigslot::has_slots<>, bool GetStats(TransportStats* stats); const JsepTransportDescription* local_description() const { - RTC_DCHECK_RUN_ON(network_thread_); return local_description_.get(); } const JsepTransportDescription* remote_description() const { - RTC_DCHECK_RUN_ON(network_thread_); return remote_description_.get(); } webrtc::RtpTransportInternal* rtp_transport() const { - // This method is called from the signaling thread, which means - // that a race is possible, making safety analysis complex. - // After fixing, this method should be marked "network thread only". if (dtls_srtp_transport_) { return dtls_srtp_transport_.get(); } else if (sdes_transport_) { @@ -169,7 +158,6 @@ class JsepTransport : public sigslot::has_slots<>, } const DtlsTransportInternal* rtp_dtls_transport() const { - rtc::CritScope scope(&accessor_lock_); if (rtp_dtls_transport_) { return rtp_dtls_transport_->internal(); } else { @@ -178,7 +166,6 @@ class JsepTransport : public sigslot::has_slots<>, } DtlsTransportInternal* rtp_dtls_transport() { - rtc::CritScope scope(&accessor_lock_); if (rtp_dtls_transport_) { return rtp_dtls_transport_->internal(); } else { @@ -187,7 +174,6 @@ class JsepTransport : public sigslot::has_slots<>, } const DtlsTransportInternal* rtcp_dtls_transport() const { - rtc::CritScope scope(&accessor_lock_); if (rtcp_dtls_transport_) { return rtcp_dtls_transport_->internal(); } else { @@ -196,7 +182,6 @@ class JsepTransport : public sigslot::has_slots<>, } DtlsTransportInternal* rtcp_dtls_transport() { - rtc::CritScope scope(&accessor_lock_); if (rtcp_dtls_transport_) { return rtcp_dtls_transport_->internal(); } else { @@ -205,7 +190,6 @@ class JsepTransport : public sigslot::has_slots<>, } rtc::scoped_refptr RtpDtlsTransport() { - rtc::CritScope scope(&accessor_lock_); return rtp_dtls_transport_; } @@ -213,13 +197,11 @@ class JsepTransport : public sigslot::has_slots<>, // Note that media transport is owned by jseptransport and the pointer // to media transport will becomes invalid after destruction of jseptransport. webrtc::MediaTransportInterface* media_transport() const { - rtc::CritScope scope(&accessor_lock_); return media_transport_.get(); } // Returns the latest media transport state. webrtc::MediaTransportState media_transport_state() const { - rtc::CritScope scope(&accessor_lock_); return media_transport_state_; } @@ -289,51 +271,36 @@ class JsepTransport : public sigslot::has_slots<>, // Invoked whenever the state of the media transport changes. void OnStateChanged(webrtc::MediaTransportState state) override; - // Owning thread, for safety checks - const rtc::Thread* const network_thread_; - // Critical scope for fields accessed off-thread - // TODO(https://bugs.webrtc.org/10300): Stop doing this. - rtc::CriticalSection accessor_lock_; const std::string mid_; // needs-ice-restart bit as described in JSEP. - bool needs_ice_restart_ RTC_GUARDED_BY(accessor_lock_) = false; - rtc::scoped_refptr local_certificate_ - RTC_GUARDED_BY(network_thread_); - std::unique_ptr local_description_ - RTC_GUARDED_BY(network_thread_); - std::unique_ptr remote_description_ - RTC_GUARDED_BY(network_thread_); + bool needs_ice_restart_ = false; + rtc::scoped_refptr local_certificate_; + std::unique_ptr local_description_; + std::unique_ptr remote_description_; // To avoid downcasting and make it type safe, keep three unique pointers for // different SRTP mode and only one of these is non-nullptr. - // Since these are const, the variables don't need locks; - // accessing the objects depends on the objects' thread safety contract. - const std::unique_ptr unencrypted_rtp_transport_; - const std::unique_ptr sdes_transport_; - const std::unique_ptr dtls_srtp_transport_; + std::unique_ptr unencrypted_rtp_transport_; + std::unique_ptr sdes_transport_; + std::unique_ptr dtls_srtp_transport_; - rtc::scoped_refptr rtp_dtls_transport_ - RTC_GUARDED_BY(accessor_lock_); - rtc::scoped_refptr rtcp_dtls_transport_ - RTC_GUARDED_BY(accessor_lock_); + rtc::scoped_refptr rtp_dtls_transport_; + rtc::scoped_refptr rtcp_dtls_transport_; - SrtpFilter sdes_negotiator_ RTC_GUARDED_BY(network_thread_); - RtcpMuxFilter rtcp_mux_negotiator_ RTC_GUARDED_BY(network_thread_); + SrtpFilter sdes_negotiator_; + RtcpMuxFilter rtcp_mux_negotiator_; // Cache the encrypted header extension IDs for SDES negoitation. - absl::optional> send_extension_ids_ - RTC_GUARDED_BY(network_thread_); - absl::optional> recv_extension_ids_ - RTC_GUARDED_BY(network_thread_); + absl::optional> send_extension_ids_; + absl::optional> recv_extension_ids_; // Optional media transport (experimental). - std::unique_ptr media_transport_ - RTC_GUARDED_BY(accessor_lock_); + std::unique_ptr media_transport_; // If |media_transport_| is provided, this variable represents the state of // media transport. - webrtc::MediaTransportState media_transport_state_ - RTC_GUARDED_BY(accessor_lock_) = webrtc::MediaTransportState::kPending; + webrtc::MediaTransportState media_transport_state_ = + webrtc::MediaTransportState::kPending; RTC_DISALLOW_COPY_AND_ASSIGN(JsepTransport); };