Reland "Add thread guards to JsepTransport"
This reverts commit caedb5db82b2bc8273910f4a0d1afb1d0e2994f3.
Reason for revert: Fixed issue (allow SetNeedsIceRestart from off-thread).
Original change's description:
> 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}
Change-Id: Ic32bfc04d96e657fc67c3d3999f77969e55ed994
Bug: webrtc:10300
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/130962
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27434}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);
};
