admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

JsepTransport: remove lock recursions.

Browse Source 
This change removes lock recursions and adds thread annotations.

Bug: webrtc:11567
Change-Id: Iefe1875182b7f8f8df3e9bd02e964530389b0b3d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175123
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Commit-Queue: Markus Handell <handellm@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31296}
This commit is contained in:
Markus Handell 2020-05-15 13:03:27 +02:00 committed by Commit Bot
parent 35fc1537af
commit c18b7bfeb6
2 changed files with 50 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
pc/jsep_transport.cc
View File

@ -386,12 +386,12 @@ absl::optional<rtc::SSLRole> JsepTransport::GetDtlsRole() const {
absl::optional<OpaqueTransportParameters> absl::optional<OpaqueTransportParameters>
JsepTransport::GetTransportParameters() const { JsepTransport::GetTransportParameters() const {
rtc::CritScope scope(&accessor_lock_); rtc::CritScope scope(&accessor_lock_);
if (!datagram_transport()) { if (!datagram_transport_) {
return absl::nullopt; return absl::nullopt;
} }
OpaqueTransportParameters params; OpaqueTransportParameters params;
params.parameters = datagram_transport()->GetTransportParameters(); params.parameters = datagram_transport_->GetTransportParameters();
return params; return params;
} }
@ -462,7 +462,6 @@ webrtc::RTCError JsepTransport::SetNegotiatedDtlsParameters(
DtlsTransportInternal* dtls_transport, DtlsTransportInternal* dtls_transport,
absl::optional<rtc::SSLRole> dtls_role, absl::optional<rtc::SSLRole> dtls_role,
rtc::SSLFingerprint* remote_fingerprint) { rtc::SSLFingerprint* remote_fingerprint) {
RTC_DCHECK_RUN_ON(network_thread_);
RTC_DCHECK(dtls_transport); RTC_DCHECK(dtls_transport);
// Set SSL role. Role must be set before fingerprint is applied, which // Set SSL role. Role must be set before fingerprint is applied, which
// initiates DTLS setup. // initiates DTLS setup.
@ -535,7 +534,7 @@ void JsepTransport::ActivateRtcpMux() {
RTC_DCHECK(dtls_srtp_transport_); RTC_DCHECK(dtls_srtp_transport_);
RTC_DCHECK(!unencrypted_rtp_transport_); RTC_DCHECK(!unencrypted_rtp_transport_);
RTC_DCHECK(!sdes_transport_); RTC_DCHECK(!sdes_transport_);
dtls_srtp_transport_->SetDtlsTransports(rtp_dtls_transport(), dtls_srtp_transport_->SetDtlsTransports(rtp_dtls_transport_locked(),
/*rtcp_dtls_transport=*/nullptr); /*rtcp_dtls_transport=*/nullptr);
} }
rtcp_dtls_transport_ = nullptr; // Destroy this reference. rtcp_dtls_transport_ = nullptr; // Destroy this reference.
@ -549,7 +548,6 @@ bool JsepTransport::SetSdes(const std::vector<CryptoParams>& cryptos,
webrtc::SdpType type, webrtc::SdpType type,
ContentSource source) { ContentSource source) {
RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK_RUN_ON(network_thread_);
rtc::CritScope scope(&accessor_lock_);
bool ret = false; bool ret = false;
ret = sdes_negotiator_.Process(cryptos, type, source); ret = sdes_negotiator_.Process(cryptos, type, source);
if (!ret) { if (!ret) {
@ -734,7 +732,6 @@ webrtc::RTCError JsepTransport::NegotiateDtlsRole(
bool JsepTransport::GetTransportStats(DtlsTransportInternal* dtls_transport, bool JsepTransport::GetTransportStats(DtlsTransportInternal* dtls_transport,
TransportStats* stats) { TransportStats* stats) {
RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK_RUN_ON(network_thread_);
rtc::CritScope scope(&accessor_lock_);
RTC_DCHECK(dtls_transport); RTC_DCHECK(dtls_transport);
TransportChannelStats substats; TransportChannelStats substats;
if (rtcp_dtls_transport_) { if (rtcp_dtls_transport_) {

74
pc/jsep_transport.h
View File

@ -128,14 +128,15 @@ class JsepTransport : public sigslot::has_slots<> {
webrtc::RTCError SetLocalJsepTransportDescription( webrtc::RTCError SetLocalJsepTransportDescription(
const JsepTransportDescription& jsep_description, const JsepTransportDescription& jsep_description,
webrtc::SdpType type); webrtc::SdpType type) RTC_LOCKS_EXCLUDED(accessor_lock_);
// Set the remote TransportDescription to be used by DTLS and ICE channels // Set the remote TransportDescription to be used by DTLS and ICE channels
// that are part of this Transport. // that are part of this Transport.
webrtc::RTCError SetRemoteJsepTransportDescription( webrtc::RTCError SetRemoteJsepTransportDescription(
const JsepTransportDescription& jsep_description, const JsepTransportDescription& jsep_description,
webrtc::SdpType type); webrtc::SdpType type) RTC_LOCKS_EXCLUDED(accessor_lock_);
webrtc::RTCError AddRemoteCandidates(const Candidates& candidates); webrtc::RTCError AddRemoteCandidates(const Candidates& candidates)
RTC_LOCKS_EXCLUDED(accessor_lock_);
// Set the "needs-ice-restart" flag as described in JSEP. After the flag is // Set the "needs-ice-restart" flag as described in JSEP. After the flag is
// set, offers should generate new ufrags/passwords until an ICE restart // set, offers should generate new ufrags/passwords until an ICE restart
@ -143,23 +144,25 @@ class JsepTransport : public sigslot::has_slots<> {
// //
// This and the below method can be called safely from any thread as long as // This and the below method can be called safely from any thread as long as
// SetXTransportDescription is not in progress. // SetXTransportDescription is not in progress.
void SetNeedsIceRestartFlag(); void SetNeedsIceRestartFlag() RTC_LOCKS_EXCLUDED(accessor_lock_);
// Returns true if the ICE restart flag above was set, and no ICE restart has // 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 // occurred yet for this transport (by applying a local description with
// changed ufrag/password). // changed ufrag/password).
bool needs_ice_restart() const { bool needs_ice_restart() const RTC_LOCKS_EXCLUDED(accessor_lock_) {
rtc::CritScope scope(&accessor_lock_); rtc::CritScope scope(&accessor_lock_);
return needs_ice_restart_; return needs_ice_restart_;
} }
// Returns role if negotiated, or empty absl::optional if it hasn't been // Returns role if negotiated, or empty absl::optional if it hasn't been
// negotiated yet. // negotiated yet.
absl::optional<rtc::SSLRole> GetDtlsRole() const; absl::optional<rtc::SSLRole> GetDtlsRole() const
RTC_LOCKS_EXCLUDED(accessor_lock_);
absl::optional<OpaqueTransportParameters> GetTransportParameters() const; absl::optional<OpaqueTransportParameters> GetTransportParameters() const
RTC_LOCKS_EXCLUDED(accessor_lock_);
// TODO(deadbeef): Make this const. See comment in transportcontroller.h. // TODO(deadbeef): Make this const. See comment in transportcontroller.h.
bool GetStats(TransportStats* stats); bool GetStats(TransportStats* stats) RTC_LOCKS_EXCLUDED(accessor_lock_);
const JsepTransportDescription* local_description() const { const JsepTransportDescription* local_description() const {
RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK_RUN_ON(network_thread_);
@ -171,7 +174,8 @@ class JsepTransport : public sigslot::has_slots<> {
return remote_description_.get(); return remote_description_.get();
} }
webrtc::RtpTransportInternal* rtp_transport() const { webrtc::RtpTransportInternal* rtp_transport() const
RTC_LOCKS_EXCLUDED(accessor_lock_) {
rtc::CritScope scope(&accessor_lock_); rtc::CritScope scope(&accessor_lock_);
if (composite_rtp_transport_) { if (composite_rtp_transport_) {
return composite_rtp_transport_.get(); return composite_rtp_transport_.get();
@ -182,7 +186,8 @@ class JsepTransport : public sigslot::has_slots<> {
} }
} }
const DtlsTransportInternal* rtp_dtls_transport() const { const DtlsTransportInternal* rtp_dtls_transport() const
RTC_LOCKS_EXCLUDED(accessor_lock_) {
rtc::CritScope scope(&accessor_lock_); rtc::CritScope scope(&accessor_lock_);
if (rtp_dtls_transport_) { if (rtp_dtls_transport_) {
return rtp_dtls_transport_->internal(); return rtp_dtls_transport_->internal();
@ -191,16 +196,14 @@ class JsepTransport : public sigslot::has_slots<> {
} }
} }
DtlsTransportInternal* rtp_dtls_transport() { DtlsTransportInternal* rtp_dtls_transport()
RTC_LOCKS_EXCLUDED(accessor_lock_) {
rtc::CritScope scope(&accessor_lock_); rtc::CritScope scope(&accessor_lock_);
if (rtp_dtls_transport_) { return rtp_dtls_transport_locked();
return rtp_dtls_transport_->internal();
} else {
return nullptr;
}
} }
const DtlsTransportInternal* rtcp_dtls_transport() const { const DtlsTransportInternal* rtcp_dtls_transport() const
RTC_LOCKS_EXCLUDED(accessor_lock_) {
rtc::CritScope scope(&accessor_lock_); rtc::CritScope scope(&accessor_lock_);
if (rtcp_dtls_transport_) { if (rtcp_dtls_transport_) {
return rtcp_dtls_transport_->internal(); return rtcp_dtls_transport_->internal();
@ -209,7 +212,8 @@ class JsepTransport : public sigslot::has_slots<> {
} }
} }
DtlsTransportInternal* rtcp_dtls_transport() { DtlsTransportInternal* rtcp_dtls_transport()
RTC_LOCKS_EXCLUDED(accessor_lock_) {
rtc::CritScope scope(&accessor_lock_); rtc::CritScope scope(&accessor_lock_);
if (rtcp_dtls_transport_) { if (rtcp_dtls_transport_) {
return rtcp_dtls_transport_->internal(); return rtcp_dtls_transport_->internal();
@ -218,17 +222,20 @@ class JsepTransport : public sigslot::has_slots<> {
} }
} }
rtc::scoped_refptr<webrtc::DtlsTransport> RtpDtlsTransport() { rtc::scoped_refptr<webrtc::DtlsTransport> RtpDtlsTransport()
RTC_LOCKS_EXCLUDED(accessor_lock_) {
rtc::CritScope scope(&accessor_lock_); rtc::CritScope scope(&accessor_lock_);
return rtp_dtls_transport_; return rtp_dtls_transport_;
} }
rtc::scoped_refptr<webrtc::SctpTransport> SctpTransport() const { rtc::scoped_refptr<webrtc::SctpTransport> SctpTransport() const
RTC_LOCKS_EXCLUDED(accessor_lock_) {
rtc::CritScope scope(&accessor_lock_); rtc::CritScope scope(&accessor_lock_);
return sctp_transport_; return sctp_transport_;
} }
webrtc::DataChannelTransportInterface* data_channel_transport() const { webrtc::DataChannelTransportInterface* data_channel_transport() const
RTC_LOCKS_EXCLUDED(accessor_lock_) {
rtc::CritScope scope(&accessor_lock_); rtc::CritScope scope(&accessor_lock_);
if (composite_data_channel_transport_) { if (composite_data_channel_transport_) {
return composite_data_channel_transport_.get(); return composite_data_channel_transport_.get();
@ -239,7 +246,8 @@ class JsepTransport : public sigslot::has_slots<> {
} }
// Returns datagram transport, if available. // Returns datagram transport, if available.
webrtc::DatagramTransportInterface* datagram_transport() const { webrtc::DatagramTransportInterface* datagram_transport() const
RTC_LOCKS_EXCLUDED(accessor_lock_) {
rtc::CritScope scope(&accessor_lock_); rtc::CritScope scope(&accessor_lock_);
return datagram_transport_.get(); return datagram_transport_.get();
} }
@ -271,6 +279,15 @@ class JsepTransport : public sigslot::has_slots<> {
void SetActiveResetSrtpParams(bool active_reset_srtp_params); void SetActiveResetSrtpParams(bool active_reset_srtp_params);
private: private:
DtlsTransportInternal* rtp_dtls_transport_locked()
RTC_EXCLUSIVE_LOCKS_REQUIRED(accessor_lock_) {
if (rtp_dtls_transport_) {
return rtp_dtls_transport_->internal();
} else {
return nullptr;
}
}
bool SetRtcpMux(bool enable, webrtc::SdpType type, ContentSource source); bool SetRtcpMux(bool enable, webrtc::SdpType type, ContentSource source);
void ActivateRtcpMux(); void ActivateRtcpMux();
@ -278,7 +295,8 @@ class JsepTransport : public sigslot::has_slots<> {
bool SetSdes(const std::vector<CryptoParams>& cryptos, bool SetSdes(const std::vector<CryptoParams>& cryptos,
const std::vector<int>& encrypted_extension_ids, const std::vector<int>& encrypted_extension_ids,
webrtc::SdpType type, webrtc::SdpType type,
ContentSource source); ContentSource source)
RTC_EXCLUSIVE_LOCKS_REQUIRED(accessor_lock_);
// Negotiates and sets the DTLS parameters based on the current local and // Negotiates and sets the DTLS parameters based on the current local and
// remote transport description, such as the DTLS role to use, and whether // remote transport description, such as the DTLS role to use, and whether
@ -295,26 +313,28 @@ class JsepTransport : public sigslot::has_slots<> {
webrtc::SdpType local_description_type, webrtc::SdpType local_description_type,
ConnectionRole local_connection_role, ConnectionRole local_connection_role,
ConnectionRole remote_connection_role, ConnectionRole remote_connection_role,
absl::optional<rtc::SSLRole>* negotiated_dtls_role); absl::optional<rtc::SSLRole>* negotiated_dtls_role)
RTC_LOCKS_EXCLUDED(accessor_lock_);
// Pushes down the ICE parameters from the remote description. // Pushes down the ICE parameters from the remote description.
void SetRemoteIceParameters(const IceParameters& ice_parameters, void SetRemoteIceParameters(const IceParameters& ice_parameters,
IceTransportInternal* ice); IceTransportInternal* ice);
// Pushes down the DTLS parameters obtained via negotiation. // Pushes down the DTLS parameters obtained via negotiation.
webrtc::RTCError SetNegotiatedDtlsParameters( static webrtc::RTCError SetNegotiatedDtlsParameters(
DtlsTransportInternal* dtls_transport, DtlsTransportInternal* dtls_transport,
absl::optional<rtc::SSLRole> dtls_role, absl::optional<rtc::SSLRole> dtls_role,
rtc::SSLFingerprint* remote_fingerprint); rtc::SSLFingerprint* remote_fingerprint);
bool GetTransportStats(DtlsTransportInternal* dtls_transport, bool GetTransportStats(DtlsTransportInternal* dtls_transport,
TransportStats* stats); TransportStats* stats)
RTC_EXCLUSIVE_LOCKS_REQUIRED(accessor_lock_);
// Deactivates, signals removal, and deletes |composite_rtp_transport_| if the // Deactivates, signals removal, and deletes |composite_rtp_transport_| if the
// current state of negotiation is sufficient to determine which rtp_transport // current state of negotiation is sufficient to determine which rtp_transport
// and data channel transport to use. // and data channel transport to use.
void NegotiateDatagramTransport(webrtc::SdpType type) void NegotiateDatagramTransport(webrtc::SdpType type)
RTC_RUN_ON(network_thread_); RTC_RUN_ON(network_thread_) RTC_LOCKS_EXCLUDED(accessor_lock_);
// Returns the default (non-datagram) rtp transport, if any. // Returns the default (non-datagram) rtp transport, if any.
webrtc::RtpTransportInternal* default_rtp_transport() const webrtc::RtpTransportInternal* default_rtp_transport() const