Add thread guards to JsepTransport
This ensures that JsepTransport's methods are either only accessed on the thread
that creates it, or using methods that are marked for off-thread use
(using a lock to prevent simultaneous access).
The intent is to document the existing contract, and to make it easy to find the
actions needed to convert the class to a pure single-threaded class.
Bug: webrtc:10300
Change-Id: Ib5cdc027632c36baec55179937d6eb664bbaf6f5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/121946
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27427}diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc
index aacee66..284a722 100644
--- a/pc/jsep_transport.cc
+++ b/pc/jsep_transport.cc
@@ -99,8 +99,12 @@
std::unique_ptr<DtlsTransportInternal> rtp_dtls_transport,
std::unique_ptr<DtlsTransportInternal> rtcp_dtls_transport,
std::unique_ptr<webrtc::MediaTransportInterface> 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<webrtc::DtlsTransport>(
std::move(rtp_dtls_transport))
@@ -112,19 +116,17 @@
: 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 @@
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 @@
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 @@
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 @@
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::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 @@
}
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::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,8 @@
}
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();
@@ -319,6 +327,8 @@
}
absl::optional<rtc::SSLRole> 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 +340,8 @@
}
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 +356,7 @@
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 +382,8 @@
}
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 +393,7 @@
}
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 +402,7 @@
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 +414,7 @@
DtlsTransportInternal* dtls_transport,
absl::optional<rtc::SSLRole> 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 +436,7 @@
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 +467,12 @@
}
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 +488,10 @@
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 +500,8 @@
const std::vector<int>& 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 +544,7 @@
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 +581,16 @@
// 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 +685,8 @@
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 +711,11 @@
// 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 21747a6..0f31403 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 @@
// Needed in order to verify the local fingerprint.
void SetLocalCertificate(
const rtc::scoped_refptr<rtc::RTCCertificate>& local_certificate) {
+ RTC_DCHECK_RUN_ON(network_thread_);
local_certificate_ = local_certificate;
}
// Return the local certificate provided by SetLocalCertificate.
rtc::scoped_refptr<rtc::RTCCertificate> GetLocalCertificate() const {
+ RTC_DCHECK_RUN_ON(network_thread_);
return local_certificate_;
}
@@ -130,7 +133,10 @@
// 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 @@
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 @@
}
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 @@
}
DtlsTransportInternal* rtp_dtls_transport() {
+ rtc::CritScope scope(&accessor_lock_);
if (rtp_dtls_transport_) {
return rtp_dtls_transport_->internal();
} else {
@@ -174,6 +187,7 @@
}
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 @@
}
DtlsTransportInternal* rtcp_dtls_transport() {
+ rtc::CritScope scope(&accessor_lock_);
if (rtcp_dtls_transport_) {
return rtcp_dtls_transport_->internal();
} else {
@@ -190,6 +205,7 @@
}
rtc::scoped_refptr<webrtc::DtlsTransport> RtpDtlsTransport() {
+ rtc::CritScope scope(&accessor_lock_);
return rtp_dtls_transport_;
}
@@ -197,11 +213,13 @@
// 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 @@
// 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<rtc::RTCCertificate> local_certificate_;
- std::unique_ptr<JsepTransportDescription> local_description_;
- std::unique_ptr<JsepTransportDescription> remote_description_;
+ bool needs_ice_restart_ RTC_GUARDED_BY(accessor_lock_) = false;
+ rtc::scoped_refptr<rtc::RTCCertificate> local_certificate_
+ RTC_GUARDED_BY(network_thread_);
+ std::unique_ptr<JsepTransportDescription> local_description_
+ RTC_GUARDED_BY(network_thread_);
+ std::unique_ptr<JsepTransportDescription> 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<webrtc::RtpTransport> unencrypted_rtp_transport_;
- std::unique_ptr<webrtc::SrtpTransport> sdes_transport_;
- std::unique_ptr<webrtc::DtlsSrtpTransport> 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<webrtc::RtpTransport> unencrypted_rtp_transport_;
+ const std::unique_ptr<webrtc::SrtpTransport> sdes_transport_;
+ const std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_transport_;
- rtc::scoped_refptr<webrtc::DtlsTransport> rtp_dtls_transport_;
- rtc::scoped_refptr<webrtc::DtlsTransport> rtcp_dtls_transport_;
+ rtc::scoped_refptr<webrtc::DtlsTransport> rtp_dtls_transport_
+ RTC_GUARDED_BY(accessor_lock_);
+ rtc::scoped_refptr<webrtc::DtlsTransport> 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<std::vector<int>> send_extension_ids_;
- absl::optional<std::vector<int>> recv_extension_ids_;
+ absl::optional<std::vector<int>> send_extension_ids_
+ RTC_GUARDED_BY(network_thread_);
+ absl::optional<std::vector<int>> recv_extension_ids_
+ RTC_GUARDED_BY(network_thread_);
// Optional media transport (experimental).
- std::unique_ptr<webrtc::MediaTransportInterface> media_transport_;
+ std::unique_ptr<webrtc::MediaTransportInterface> 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);
};
