Revert "Add thread guards to JsepTransport"

This reverts commit 7e1db52c93.

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}

pc/jsep_transport.cc

@@ -99,12 +99,8 @@ JsepTransport::JsepTransport(
     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 @@ JsepTransport::JsepTransport(
               : 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 @@ 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.");
@@ -182,6 +179,7 @@ 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,
@@ -196,14 +194,13 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
   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());
 
@@ -211,7 +208,7 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
     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,14 +217,12 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
     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();
   }
-  }
 
   return webrtc::RTCError::OK();
 }
@@ -237,7 +232,6 @@ 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,
@@ -272,11 +266,12 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription(
   }
 
   remote_description_.reset(new JsepTransportDescription(jsep_description));
-  RTC_DCHECK(rtp_dtls_transport());
+  RTC_DCHECK(rtp_dtls_transport_->internal());
-  SetRemoteIceParameters(rtp_dtls_transport()->ice_transport());
+  SetRemoteIceParameters(rtp_dtls_transport_->internal()->ice_transport());
 
-  if (rtcp_dtls_transport()) {
+  if (rtcp_dtls_transport_) {
-    SetRemoteIceParameters(rtcp_dtls_transport()->ice_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::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() +
@@ -318,8 +312,6 @@ webrtc::RTCError JsepTransport::AddRemoteCandidates(
 }
 
 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 @@ void JsepTransport::SetNeedsIceRestartFlag() {
 }
 
 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 @@ absl::optional<rtc::SSLRole> 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());
@@ -356,7 +344,6 @@ 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");
@@ -382,8 +369,6 @@ 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: "
@@ -393,7 +378,6 @@ 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(
@@ -402,7 +386,6 @@ 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(
@@ -414,7 +397,6 @@ webrtc::RTCError JsepTransport::SetNegotiatedDtlsParameters(
     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 @@ 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:
@@ -467,12 +448,6 @@ 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_);
@@ -488,10 +463,7 @@ void JsepTransport::ActivateRtcpMux() {
     dtls_srtp_transport_->SetDtlsTransports(rtp_dtls_transport(),
                                             /*rtcp_dtls_transport=*/nullptr);
   }
-  {
-    rtc::CritScope scope(&accessor_lock_);
   rtcp_dtls_transport_ = nullptr;  // Destroy this reference.
-  }
   // Notify the JsepTransportController to update the aggregate states.
   SignalRtcpMuxActive();
 }
@@ -500,8 +472,6 @@ bool JsepTransport::SetSdes(const std::vector<CryptoParams>& cryptos,
                             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 @@ bool JsepTransport::SetSdes(const std::vector<CryptoParams>& 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 "
@@ -581,16 +550,19 @@ 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());
+  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()) {
+  if (rtcp_dtls_transport_) {
-    error = SetNegotiatedDtlsParameters(
+    RTC_DCHECK(rtcp_dtls_transport_->internal());
-        rtcp_dtls_transport(), negotiated_dtls_role, remote_fingerprint.get());
+    error = SetNegotiatedDtlsParameters(rtcp_dtls_transport_->internal(),
+                                        negotiated_dtls_role,
+                                        remote_fingerprint.get());
   }
   return error;
 }
@@ -685,8 +657,6 @@ 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_) {
@@ -711,11 +681,7 @@ 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.
-  RTC_DCHECK_RUN_ON(network_thread_);
-  {
-    rtc::CritScope scope(&accessor_lock_);
   media_transport_state_ = state;
-  }
   SignalMediaTransportStateChanged();
 }
 

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 @@ class JsepTransport : public sigslot::has_slots<>,
   // 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 @@ 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 {
+  bool needs_ice_restart() const { return needs_ice_restart_; }
-    rtc::CritScope scope(&accessor_lock_);
-    return needs_ice_restart_;
-  }
 
   // Returns role if negotiated, or empty absl::optional if it hasn't been
   // negotiated yet.
@@ -146,19 +140,14 @@ 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_) {
@@ -169,7 +158,6 @@ 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 {
@@ -178,7 +166,6 @@ 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 {
@@ -187,7 +174,6 @@ 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 {
@@ -196,7 +182,6 @@ 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 {
@@ -205,7 +190,6 @@ class JsepTransport : public sigslot::has_slots<>,
   }
 
   rtc::scoped_refptr<webrtc::DtlsTransport> RtpDtlsTransport() {
-    rtc::CritScope scope(&accessor_lock_);
     return rtp_dtls_transport_;
   }
 
@@ -213,13 +197,11 @@ 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_;
   }
 
@@ -289,51 +271,36 @@ 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_ RTC_GUARDED_BY(accessor_lock_) = false;
+  bool needs_ice_restart_ = false;
-  rtc::scoped_refptr<rtc::RTCCertificate> local_certificate_
+  rtc::scoped_refptr<rtc::RTCCertificate> local_certificate_;
-      RTC_GUARDED_BY(network_thread_);
+  std::unique_ptr<JsepTransportDescription> local_description_;
-  std::unique_ptr<JsepTransportDescription> local_description_
+  std::unique_ptr<JsepTransportDescription> remote_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.
-  // Since these are const, the variables don't need locks;
+  std::unique_ptr<webrtc::RtpTransport> unencrypted_rtp_transport_;
-  // accessing the objects depends on the objects' thread safety contract.
+  std::unique_ptr<webrtc::SrtpTransport> sdes_transport_;
-  const std::unique_ptr<webrtc::RtpTransport> unencrypted_rtp_transport_;
+  std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_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> rtp_dtls_transport_;
-      RTC_GUARDED_BY(accessor_lock_);
+  rtc::scoped_refptr<webrtc::DtlsTransport> rtcp_dtls_transport_;
-  rtc::scoped_refptr<webrtc::DtlsTransport> rtcp_dtls_transport_
-      RTC_GUARDED_BY(accessor_lock_);
 
-  SrtpFilter sdes_negotiator_ RTC_GUARDED_BY(network_thread_);
+  SrtpFilter sdes_negotiator_;
-  RtcpMuxFilter rtcp_mux_negotiator_ RTC_GUARDED_BY(network_thread_);
+  RtcpMuxFilter rtcp_mux_negotiator_;
 
   // Cache the encrypted header extension IDs for SDES negoitation.
-  absl::optional<std::vector<int>> send_extension_ids_
+  absl::optional<std::vector<int>> send_extension_ids_;
-      RTC_GUARDED_BY(network_thread_);
+  absl::optional<std::vector<int>> recv_extension_ids_;
-  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 media_transport_state_ =
-      RTC_GUARDED_BY(accessor_lock_) = webrtc::MediaTransportState::kPending;
+      webrtc::MediaTransportState::kPending;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(JsepTransport);
 };