diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index aacee66190..f82cf2a11a 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -99,8 +99,12 @@ JsepTransport::JsepTransport( std::unique_ptr rtp_dtls_transport, std::unique_ptr rtcp_dtls_transport, std::unique_ptr media_transport) - : mid_(mid), + : network_thread_(rtc::Thread::Current()), + 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)) @@ -112,19 +116,17 @@ JsepTransport::JsepTransport( : nullptr), media_transport_(std::move(media_transport)) { RTC_DCHECK(rtp_dtls_transport_); - if (unencrypted_rtp_transport) { + // Verify the "only one out of these three can be set" invariant. + if (unencrypted_rtp_transport_) { RTC_DCHECK(!sdes_transport); RTC_DCHECK(!dtls_srtp_transport); - unencrypted_rtp_transport_ = std::move(unencrypted_rtp_transport); - } else if (sdes_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_) { @@ -152,6 +154,7 @@ 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."); @@ -179,7 +182,6 @@ 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, @@ -194,21 +196,22 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( if (!local_fp) { local_certificate_ = nullptr; } else { - error = VerifyCertificateFingerprint(local_certificate_.get(), local_fp); + error = VerifyCertificateFingerprint(local_certificate_, 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()); - 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 (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); @@ -217,11 +220,13 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( local_description_.reset(); return error; } - - if (needs_ice_restart_ && ice_restarting) { - needs_ice_restart_ = false; - RTC_LOG(LS_VERBOSE) << "needs-ice-restart flag cleared for transport " - << mid(); + { + 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(); + } } return webrtc::RTCError::OK(); @@ -232,6 +237,7 @@ 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, @@ -266,12 +272,11 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription( } remote_description_.reset(new JsepTransportDescription(jsep_description)); - RTC_DCHECK(rtp_dtls_transport_->internal()); - SetRemoteIceParameters(rtp_dtls_transport_->internal()->ice_transport()); + RTC_DCHECK(rtp_dtls_transport()); + SetRemoteIceParameters(rtp_dtls_transport()->ice_transport()); - if (rtcp_dtls_transport_) { - RTC_DCHECK(rtcp_dtls_transport_->internal()); - SetRemoteIceParameters(rtcp_dtls_transport_->internal()->ice_transport()); + if (rtcp_dtls_transport()) { + SetRemoteIceParameters(rtcp_dtls_transport()->ice_transport()); } // If PRANSWER/ANSWER is set, we should decide transport protocol type. @@ -287,6 +292,7 @@ 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() + @@ -312,6 +318,7 @@ webrtc::RTCError JsepTransport::AddRemoteCandidates( } void JsepTransport::SetNeedsIceRestartFlag() { + 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(); @@ -319,6 +326,8 @@ 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; @@ -330,6 +339,8 @@ 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()); @@ -344,6 +355,7 @@ 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"); @@ -369,6 +381,8 @@ 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: " @@ -378,6 +392,7 @@ 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( @@ -386,6 +401,7 @@ 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( @@ -397,6 +413,7 @@ 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. @@ -418,6 +435,7 @@ 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: @@ -448,6 +466,12 @@ 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_); @@ -463,7 +487,10 @@ void JsepTransport::ActivateRtcpMux() { dtls_srtp_transport_->SetDtlsTransports(rtp_dtls_transport(), /*rtcp_dtls_transport=*/nullptr); } - rtcp_dtls_transport_ = nullptr; // Destroy this reference. + { + rtc::CritScope scope(&accessor_lock_); + rtcp_dtls_transport_ = nullptr; // Destroy this reference. + } // Notify the JsepTransportController to update the aggregate states. SignalRtcpMuxActive(); } @@ -472,6 +499,8 @@ 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) { @@ -514,6 +543,7 @@ 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 " @@ -550,19 +580,16 @@ 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_->internal()); + RTC_DCHECK(rtp_dtls_transport()); webrtc::RTCError error = SetNegotiatedDtlsParameters( - rtp_dtls_transport_->internal(), negotiated_dtls_role, - remote_fingerprint.get()); + rtp_dtls_transport(), negotiated_dtls_role, remote_fingerprint.get()); if (!error.ok()) { return error; } - if (rtcp_dtls_transport_) { - RTC_DCHECK(rtcp_dtls_transport_->internal()); - error = SetNegotiatedDtlsParameters(rtcp_dtls_transport_->internal(), - negotiated_dtls_role, - remote_fingerprint.get()); + if (rtcp_dtls_transport()) { + error = SetNegotiatedDtlsParameters( + rtcp_dtls_transport(), negotiated_dtls_role, remote_fingerprint.get()); } return error; } @@ -657,6 +684,8 @@ 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_) { @@ -681,7 +710,11 @@ 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. - media_transport_state_ = state; + RTC_DCHECK_RUN_ON(network_thread_); + { + rtc::CritScope scope(&accessor_lock_); + media_transport_state_ = state; + } SignalMediaTransportStateChanged(); } diff --git a/pc/jsep_transport.h b/pc/jsep_transport.h index 21747a6dca..0f314039d5 100644 --- a/pc/jsep_transport.h +++ b/pc/jsep_transport.h @@ -36,6 +36,7 @@ #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 { @@ -101,11 +102,13 @@ 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_; } @@ -130,7 +133,10 @@ 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 { return needs_ice_restart_; } + bool needs_ice_restart() const { + rtc::CritScope scope(&accessor_lock_); + return needs_ice_restart_; + } // Returns role if negotiated, or empty absl::optional if it hasn't been // negotiated yet. @@ -140,14 +146,19 @@ 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_) { @@ -158,6 +169,7 @@ 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 { @@ -166,6 +178,7 @@ 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 { @@ -174,6 +187,7 @@ 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 { @@ -182,6 +196,7 @@ 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 { @@ -190,6 +205,7 @@ class JsepTransport : public sigslot::has_slots<>, } rtc::scoped_refptr RtpDtlsTransport() { + rtc::CritScope scope(&accessor_lock_); return rtp_dtls_transport_; } @@ -197,11 +213,13 @@ 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_; } @@ -271,36 +289,51 @@ 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_ = false; - rtc::scoped_refptr local_certificate_; - std::unique_ptr local_description_; - std::unique_ptr remote_description_; + 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_); // To avoid downcasting and make it type safe, keep three unique pointers for // different SRTP mode and only one of these is non-nullptr. - std::unique_ptr unencrypted_rtp_transport_; - std::unique_ptr sdes_transport_; - std::unique_ptr dtls_srtp_transport_; + // 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_; - rtc::scoped_refptr rtp_dtls_transport_; - rtc::scoped_refptr rtcp_dtls_transport_; + rtc::scoped_refptr rtp_dtls_transport_ + RTC_GUARDED_BY(accessor_lock_); + rtc::scoped_refptr rtcp_dtls_transport_ + RTC_GUARDED_BY(accessor_lock_); - SrtpFilter sdes_negotiator_; - RtcpMuxFilter rtcp_mux_negotiator_; + SrtpFilter sdes_negotiator_ RTC_GUARDED_BY(network_thread_); + RtcpMuxFilter rtcp_mux_negotiator_ RTC_GUARDED_BY(network_thread_); // Cache the encrypted header extension IDs for SDES negoitation. - absl::optional> send_extension_ids_; - absl::optional> recv_extension_ids_; + absl::optional> send_extension_ids_ + RTC_GUARDED_BY(network_thread_); + absl::optional> recv_extension_ids_ + RTC_GUARDED_BY(network_thread_); // Optional media transport (experimental). - std::unique_ptr media_transport_; + std::unique_ptr media_transport_ + RTC_GUARDED_BY(accessor_lock_); // If |media_transport_| is provided, this variable represents the state of // media transport. - webrtc::MediaTransportState media_transport_state_ = - webrtc::MediaTransportState::kPending; + webrtc::MediaTransportState media_transport_state_ + RTC_GUARDED_BY(accessor_lock_) = webrtc::MediaTransportState::kPending; RTC_DISALLOW_COPY_AND_ASSIGN(JsepTransport); }; diff --git a/pc/peer_connection_end_to_end_unittest.cc b/pc/peer_connection_end_to_end_unittest.cc index b37d00a6e8..a30319992c 100644 --- a/pc/peer_connection_end_to_end_unittest.cc +++ b/pc/peer_connection_end_to_end_unittest.cc @@ -747,6 +747,24 @@ TEST_P(PeerConnectionEndToEndTest, TooManyDataChannelsOpenedBeforeConnecting) { #endif // HAVE_SCTP +TEST_P(PeerConnectionEndToEndTest, CanRestartIce) { + rtc::scoped_refptr real_decoder_factory = + webrtc::CreateBuiltinAudioDecoderFactory(); + CreatePcs(webrtc::CreateBuiltinAudioEncoderFactory(), + CreateForwardingMockDecoderFactory(real_decoder_factory.get())); + GetAndAddUserMedia(); + Negotiate(); + WaitForCallEstablished(); + // Cause ICE restart to be requested. + auto config = caller_->pc()->GetConfiguration(); + ASSERT_NE(PeerConnectionInterface::kRelay, config.type); + config.type = PeerConnectionInterface::kRelay; + webrtc::RTCError error; + ASSERT_TRUE(caller_->pc()->SetConfiguration(config, &error)); + // When solving https://crbug.com/webrtc/10504, all we need to check + // is that we do not crash. We should also be testing that restart happens. +} + INSTANTIATE_TEST_SUITE_P(PeerConnectionEndToEndTest, PeerConnectionEndToEndTest, Values(SdpSemantics::kPlanB,