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}

2 files changed, 34 insertions, 18 deletions

diff --git a/pc/dtls_transport.cc b/pc/dtls_transport.cc
index 15eed9e47b..4888d9f9e7 100644
--- a/pc/dtls_transport.cc
+++ b/pc/dtls_transport.cc
@@ -41,8 +41,18 @@ DtlsTransport::DtlsTransport(
 }
 
 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 @@ void DtlsTransport::Clear() {
   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::OnInternalDtlsState(
 
 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 @@ void DtlsTransport::UpdateInformation() {
       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));
   }
 }
 
diff --git a/pc/dtls_transport.h b/pc/dtls_transport.h
index cca4cc980a..a1893297e6 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 @@ class DtlsTransport : public DtlsTransportInterface {
       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 @@ class DtlsTransport : public DtlsTransportInterface {
                            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_;
 };