Revert "Add thread guards to JsepTransport"
This reverts commit 7e1db52c93c57a180073906eda6a58919a9fd537.
Reason for revert: Breaks downstream.
Original change's description:
> 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}
TBR=steveanton@webrtc.org,solenberg@webrtc.org,kwiberg@webrtc.org,hbos@webrtc.org,hta@webrtc.org
Change-Id: I30c65d2161de9376ccd1172e2b261f2280fb1d75
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:10300
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/130519
Reviewed-by: Gustaf Ullberg <gustaf@webrtc.org>
Commit-Queue: Gustaf Ullberg <gustaf@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27429}diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc
index 284a722..aacee66 100644
--- a/pc/jsep_transport.cc
+++ b/pc/jsep_transport.cc
@@ -99,12 +99,8 @@
std::unique_ptr<DtlsTransportInternal> rtp_dtls_transport,
std::unique_ptr<DtlsTransportInternal> rtcp_dtls_transport,
std::unique_ptr<webrtc::MediaTransportInterface> 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<webrtc::DtlsTransport>(
std::move(rtp_dtls_transport))
@@ -116,17 +112,19 @@
: 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 @@
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 @@
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 @@
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 @@
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::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 @@
}
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::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 @@
}
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 @@
}
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;
@@ -340,8 +330,6 @@
}
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 @@
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 @@
}
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::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::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 @@
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.
@@ -436,7 +418,6 @@
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 @@
}
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 @@
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 @@
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) {
@@ -544,7 +514,6 @@
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 @@
// 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 @@
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 @@
// 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 0f31403..21747a6 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 @@
// 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_;
}
@@ -133,10 +130,7 @@
// 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 @@
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 @@
}
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 @@
}
DtlsTransportInternal* rtp_dtls_transport() {
- rtc::CritScope scope(&accessor_lock_);
if (rtp_dtls_transport_) {
return rtp_dtls_transport_->internal();
} else {
@@ -187,7 +174,6 @@
}
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 @@
}
DtlsTransportInternal* rtcp_dtls_transport() {
- rtc::CritScope scope(&accessor_lock_);
if (rtcp_dtls_transport_) {
return rtcp_dtls_transport_->internal();
} else {
@@ -205,7 +190,6 @@
}
rtc::scoped_refptr<webrtc::DtlsTransport> RtpDtlsTransport() {
- rtc::CritScope scope(&accessor_lock_);
return rtp_dtls_transport_;
}
@@ -213,13 +197,11 @@
// 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 @@
// 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<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_);
+ bool needs_ice_restart_ = false;
+ rtc::scoped_refptr<rtc::RTCCertificate> local_certificate_;
+ std::unique_ptr<JsepTransportDescription> local_description_;
+ std::unique_ptr<JsepTransportDescription> 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<webrtc::RtpTransport> unencrypted_rtp_transport_;
- const std::unique_ptr<webrtc::SrtpTransport> sdes_transport_;
- const std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_transport_;
+ std::unique_ptr<webrtc::RtpTransport> unencrypted_rtp_transport_;
+ std::unique_ptr<webrtc::SrtpTransport> sdes_transport_;
+ std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_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_);
+ rtc::scoped_refptr<webrtc::DtlsTransport> rtp_dtls_transport_;
+ rtc::scoped_refptr<webrtc::DtlsTransport> 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<std::vector<int>> send_extension_ids_
- RTC_GUARDED_BY(network_thread_);
- absl::optional<std::vector<int>> recv_extension_ids_
- RTC_GUARDED_BY(network_thread_);
+ absl::optional<std::vector<int>> send_extension_ids_;
+ absl::optional<std::vector<int>> recv_extension_ids_;
// Optional media transport (experimental).
- std::unique_ptr<webrtc::MediaTransportInterface> media_transport_
- RTC_GUARDED_BY(accessor_lock_);
+ std::unique_ptr<webrtc::MediaTransportInterface> 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);
};
