commit 3ba809d6a6bc20931adde485bb04586a6556db11
author Tommi <tommi@webrtc.org> Thu Dec 14 18:49:49 2023
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Dec 14 21:01:04 2023
tree 0c29a29fb9417245029e21efa9dec8eb9cae06c6
parent 6f0f158af0290f928a4597cdacdc065ccd39d1fe

Reduce locking in DtlsTransport

Access to `internal_dtls_transport_` only occurs on the network thread
and doesn't require locking. Access to `info_` still requires a lock
but writing to it only occurs on the network thread. If reading from
the network thread is needed, that could be done without requiring
the lock.

The scope of holding the lock is much smaller now.

Bug: none
Change-Id: Ic284df04196dfcf8b77c66a48e484ca6893de050
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/325283
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41387}

pc/dtls_transport.cc

diff --git a/pc/dtls_transport.cc b/pc/dtls_transport.cc
index 15eed9e..4888d9f 100644
--- a/pc/dtls_transport.cc
+++ b/pc/dtls_transport.cc
@@ -41,8 +41,18 @@
 }
 
 DtlsTransport::~DtlsTransport() {
+  // TODO(tommi): Due to a reference being held by the RtpSenderBase
+  // implementation, the last reference to the `DtlsTransport` instance can
+  // be released on the signaling thread.
+  // RTC_DCHECK_RUN_ON(owner_thread_);
+
   // We depend on the signaling thread to call Clear() before dropping
   // its last reference to this object.
+
+  // If there are non `owner_thread_` references outstanding, and those
+  // references are the last ones released, we depend on Clear() having been
+  // called from the owner_thread before the last reference is deleted.
+  // `Clear()` is currently called from `JsepTransport::~JsepTransport`.
   RTC_DCHECK(owner_thread_->IsCurrent() || !internal_dtls_transport_);
 }
 
@@ -72,14 +82,8 @@
   RTC_DCHECK(internal());
   bool must_send_event =
       (internal()->dtls_state() != DtlsTransportState::kClosed);
-  // The destructor of cricket::DtlsTransportInternal calls back
-  // into DtlsTransport, so we can't hold the lock while releasing.
-  std::unique_ptr<cricket::DtlsTransportInternal> transport_to_release;
-  {
-    MutexLock lock(&lock_);
-    transport_to_release = std::move(internal_dtls_transport_);
-    ice_transport_->Clear();
-  }
+  internal_dtls_transport_.reset();
+  ice_transport_->Clear();
   UpdateInformation();
   if (observer_ && must_send_event) {
     observer_->OnStateChange(Information());
@@ -100,7 +104,6 @@
 
 void DtlsTransport::UpdateInformation() {
   RTC_DCHECK_RUN_ON(owner_thread_);
-  MutexLock lock(&lock_);
   if (internal_dtls_transport_) {
     if (internal_dtls_transport_->dtls_state() ==
         DtlsTransportState::kConnected) {
@@ -125,23 +128,24 @@
       success &= internal_dtls_transport_->GetSslCipherSuite(&ssl_cipher_suite);
       success &= internal_dtls_transport_->GetSrtpCryptoSuite(&srtp_cipher);
       if (success) {
-        info_ = DtlsTransportInformation(
+        set_info(DtlsTransportInformation(
             internal_dtls_transport_->dtls_state(), role, tls_version,
             ssl_cipher_suite, srtp_cipher,
-            internal_dtls_transport_->GetRemoteSSLCertChain());
+            internal_dtls_transport_->GetRemoteSSLCertChain()));
       } else {
         RTC_LOG(LS_ERROR) << "DtlsTransport in connected state has incomplete "
                              "TLS information";
-        info_ = DtlsTransportInformation(
+        set_info(DtlsTransportInformation(
             internal_dtls_transport_->dtls_state(), role, absl::nullopt,
             absl::nullopt, absl::nullopt,
-            internal_dtls_transport_->GetRemoteSSLCertChain());
+            internal_dtls_transport_->GetRemoteSSLCertChain()));
       }
     } else {
-      info_ = DtlsTransportInformation(internal_dtls_transport_->dtls_state());
+      set_info(
+          DtlsTransportInformation(internal_dtls_transport_->dtls_state()));
     }
   } else {
-    info_ = DtlsTransportInformation(DtlsTransportState::kClosed);
+    set_info(DtlsTransportInformation(DtlsTransportState::kClosed));
   }
 }
 

pc/dtls_transport.h

diff --git a/pc/dtls_transport.h b/pc/dtls_transport.h
index cca4cc9..a189329 100644
--- a/pc/dtls_transport.h
+++ b/pc/dtls_transport.h
@@ -12,6 +12,7 @@
 #define PC_DTLS_TRANSPORT_H_
 
 #include <memory>
+#include <utility>
 
 #include "api/dtls_transport_interface.h"
 #include "api/ice_transport_interface.h"
@@ -40,18 +41,22 @@
       std::unique_ptr<cricket::DtlsTransportInternal> internal);
 
   rtc::scoped_refptr<IceTransportInterface> ice_transport() override;
+
+  // Currently called from the signaling thread and potentially Chromium's
+  // JS thread.
   DtlsTransportInformation Information() override;
+
   void RegisterObserver(DtlsTransportObserverInterface* observer) override;
   void UnregisterObserver() override;
   void Clear();
 
   cricket::DtlsTransportInternal* internal() {
-    MutexLock lock(&lock_);
+    RTC_DCHECK_RUN_ON(owner_thread_);
     return internal_dtls_transport_.get();
   }
 
   const cricket::DtlsTransportInternal* internal() const {
-    MutexLock lock(&lock_);
+    RTC_DCHECK_RUN_ON(owner_thread_);
     return internal_dtls_transport_.get();
   }
 
@@ -63,12 +68,19 @@
                            DtlsTransportState state);
   void UpdateInformation();
 
+  // Called when changing `info_`. We only change the values from the
+  // `owner_thread_` (a.k.a. the network thread).
+  void set_info(DtlsTransportInformation&& info) RTC_RUN_ON(owner_thread_) {
+    MutexLock lock(&lock_);
+    info_ = std::move(info);
+  }
+
   DtlsTransportObserverInterface* observer_ = nullptr;
   rtc::Thread* owner_thread_;
   mutable Mutex lock_;
   DtlsTransportInformation info_ RTC_GUARDED_BY(lock_);
   std::unique_ptr<cricket::DtlsTransportInternal> internal_dtls_transport_
-      RTC_GUARDED_BY(lock_);
+      RTC_GUARDED_BY(owner_thread_);
   const rtc::scoped_refptr<IceTransportWithPointer> ice_transport_;
 };