Google Git
Sign in
webrtc/src/27be378fa6^!/.
commit
27be378fa6eee87a69c1cfc3c9abc22082ea6c11
[log]
author
Tommi <tommi@webrtc.org>
Tue Apr 14 10:16:54 2026
committer
webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com>
Tue Apr 14 12:12:21 2026
tree
feab7fe97a2b9a818515f561c93271bda5e43025
parent
a4caab31c93ef2926326a3880cc407016d2b6903 [diff]
Update how the DTLS role is queried and reported.

Update DataChannelController to query the DtlsTransport transport
directly for the SSL role instead of sending the request through
PeerConnection, JsepTransportController, JsepTransport and eventually
DtlsTransport. This reduces unnecessary coupling and threading checks
for several of the classes (most importantly PeerConnection and
JsepTransportController) and simplifies the PeerConnection interface by
removing the redundant method, associated mocks and fakes.

While making this change, also update SctpTransport to query its
internal DTLS transport directly for the role.

Lastly, DtlsTransport is adjusted to ensure role information is
correctly propagated during state changes, not only for the
`DtlsTransportState::kConnected` state so that Information() returns up
to date information.

The change also clarifies threading requirements in the
JsepTransportController.

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

diff --git a/p2p/dtls/dtls_transport_unittest.cc b/p2p/dtls/dtls_transport_unittest.cc
index 450781a..70632f3 100644
--- a/p2p/dtls/dtls_transport_unittest.cc
+++ b/p2p/dtls/dtls_transport_unittest.cc

@@ -1576,6 +1576,31 @@
             certificate1->GetSSLCertificate().ToPEMString());
 }
 
+TEST_F(DtlsTransportInternalImplTest, TestImplicitRoleDetection) {
+  PrepareDtls(KT_DEFAULT);
+
+  client1_.SetupTransports(env_, ICEROLE_CONTROLLING);
+  client2_.SetupTransports(env_, ICEROLE_CONTROLLED);
+
+  client2_.dtls_transport()->SetDtlsRole(SSL_CLIENT);
+
+  SetRemoteFingerprintFromCert(client2_.dtls_transport(),
+                               client1_.certificate());
+
+  client2_.Connect(&client1_, false);
+
+  EXPECT_THAT(
+      webrtc::WaitUntil(
+          [&] {
+            SSLRole role;
+            return client1_.dtls_transport()->GetDtlsRole(&role) &&
+                   role == SSL_SERVER;
+          },
+          IsTrue(),
+          {.timeout = TimeDelta::Millis(kTimeout), .clock = &time_controller_}),
+      IsRtcOk());
+}
+
 // Test that packets are retransmitted according to the expected schedule.
 // Each time a timeout occurs, the retransmission timer should be doubled up to
 // 60 seconds. The timer defaults to 1 second, but for WebRTC we should be

diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc
index 34bce61..09b62cd 100644
--- a/pc/data_channel_controller.cc
+++ b/pc/data_channel_controller.cc

@@ -367,9 +367,13 @@
   }
 
   // Attempt to allocate an ID based on the negotiated role.
-  std::optional<SSLRole> role = pc_->GetSctpSslRole_n();
-  if (!role)
-    role = fallback_ssl_role;
+  std::optional<SSLRole> role;
+  if (data_channel_transport_) {
+    role = data_channel_transport_->DtlsRole();
+    if (!role) {
+      role = fallback_ssl_role;
+    }
+  }
   if (role) {
     sid = sid_allocator_.AllocateSid(*role);
     if (!sid.has_value())

diff --git a/pc/data_channel_controller_unittest.cc b/pc/data_channel_controller_unittest.cc
index 8dbd358..2c12d3b 100644
--- a/pc/data_channel_controller_unittest.cc
+++ b/pc/data_channel_controller_unittest.cc

@@ -195,7 +195,7 @@
   NiceMock<MockDataChannelTransport> transport;
   int channel_id = 0;
 
-  ON_CALL(*pc_, GetSctpSslRole_n).WillByDefault([&]() {
+  EXPECT_CALL(transport, DtlsRole()).WillRepeatedly([&]() {
     return std::optional<SSLRole>((channel_id & 1) ? SSL_SERVER : SSL_CLIENT);
   });
 
@@ -220,7 +220,7 @@
   NiceMock<MockDataChannelTransport> transport;
   int channel_id = 0;
 
-  ON_CALL(*pc_, GetSctpSslRole_n).WillByDefault([&]() {
+  ON_CALL(transport, DtlsRole).WillByDefault([&]() {
     return std::optional<SSLRole>((channel_id & 1) ? SSL_SERVER : SSL_CLIENT);
   });
   EXPECT_CALL(transport, OpenChannel(_, _))
@@ -263,7 +263,7 @@
 TEST_F(DataChannelControllerTest, BufferedAmountIncludesFromTransport) {
   NiceMock<MockDataChannelTransport> transport;
   EXPECT_CALL(transport, buffered_amount(0)).WillOnce(Return(4711));
-  ON_CALL(*pc_, GetSctpSslRole_n).WillByDefault([&]() { return SSL_CLIENT; });
+  ON_CALL(transport, DtlsRole).WillByDefault([&]() { return SSL_CLIENT; });
 
   DataChannelControllerForTest dcc(pc_.get(), &transport);
   auto dc = dcc.InternalCreateDataChannelWithProxy(
@@ -276,9 +276,8 @@
 // not get re-used for new channels. Only once the state reaches `kClosed`
 // should a StreamId be available again for allocation.
 TEST_F(DataChannelControllerTest, NoStreamIdReuseWhileClosing) {
-  ON_CALL(*pc_, GetSctpSslRole_n).WillByDefault([&]() { return SSL_CLIENT; });
-
   NiceMock<MockDataChannelTransport> transport;  // Wider scope than `dcc`.
+  ON_CALL(transport, DtlsRole).WillByDefault([&]() { return SSL_CLIENT; });
   DataChannelControllerForTest dcc(pc_.get(), &transport);
 
   // Create the first channel and check that we got the expected, first sid.

diff --git a/pc/data_channel_integrationtest.cc b/pc/data_channel_integrationtest.cc
index b1ce96b..be2f2e1 100644
--- a/pc/data_channel_integrationtest.cc
+++ b/pc/data_channel_integrationtest.cc

@@ -2020,7 +2020,13 @@
       IsRtcOk());
 
   VerifyDtlsRoles(caller(), callee());
-  ASSERT_THAT(callee2->dtls_transport_role(), Eq(std::nullopt));
+  if (callee_active) {
+    ASSERT_THAT(callee2->dtls_transport_role(),
+                Eq(DtlsTransportTlsRole::kClient));
+  } else {
+    ASSERT_THAT(callee2->dtls_transport_role(),
+                Eq(DtlsTransportTlsRole::kServer));
+  }
 
   std::atomic<int> caller_sent_on_dc(0);
   std::atomic<int> callee2_sent_on_dc(0);

diff --git a/pc/dtls_transport.cc b/pc/dtls_transport.cc
index ed060580..a5c01c8 100644
--- a/pc/dtls_transport.cc
+++ b/pc/dtls_transport.cc

@@ -84,24 +84,24 @@
 void DtlsTransport::UpdateInformation(DtlsTransportInternal* internal) {
   RTC_DCHECK_RUN_ON(owner_thread_);
   if (internal) {
+    SSLRole internal_role;
+    std::optional<DtlsTransportTlsRole> role;
+    if (internal->GetDtlsRole(&internal_role)) {
+      switch (internal_role) {
+        case SSL_CLIENT:
+          role = DtlsTransportTlsRole::kClient;
+          break;
+        case SSL_SERVER:
+          role = DtlsTransportTlsRole::kServer;
+          break;
+      }
+    }
+
     if (internal->dtls_state() == DtlsTransportState::kConnected) {
       bool success = true;
-      SSLRole internal_role;
-      std::optional<DtlsTransportTlsRole> role;
       int ssl_cipher_suite;
       int tls_version;
       int srtp_cipher;
-      success &= internal->GetDtlsRole(&internal_role);
-      if (success) {
-        switch (internal_role) {
-          case SSL_CLIENT:
-            role = DtlsTransportTlsRole::kClient;
-            break;
-          case SSL_SERVER:
-            role = DtlsTransportTlsRole::kServer;
-            break;
-        }
-      }
       success &= internal->GetSslVersionBytes(&tls_version);
       success &= internal->GetSslCipherSuite(&ssl_cipher_suite);
       success &= internal->GetSrtpCryptoSuite(&srtp_cipher);
@@ -119,7 +119,10 @@
             /* ssl_group_id= */ std::nullopt));
       }
     } else {
-      set_info(DtlsTransportInformation(internal->dtls_state()));
+      set_info(DtlsTransportInformation(
+          internal->dtls_state(), role, std::nullopt, std::nullopt,
+          std::nullopt, internal->GetRemoteSSLCertChain(),
+          /* ssl_group_id= */ std::nullopt));
     }
   } else {
     set_info(DtlsTransportInformation(DtlsTransportState::kClosed));

diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc
index 4d2126e..e1ae0db 100644
--- a/pc/jsep_transport_controller.cc
+++ b/pc/jsep_transport_controller.cc

@@ -265,6 +265,7 @@
   // thread during negotiations, potentially multiple times.
   // WebRtcSessionDescriptionFactory::InternalCreateAnswer is one example.
   if (!network_thread_->IsCurrent()) {
+    RTC_DCHECK_RUN_ON(signaling_thread_);
     return network_thread_->BlockingCall([&] { return GetDtlsRole(mid); });
   }
 

diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 87cfb43..bdd6ba8 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc

@@ -2414,12 +2414,6 @@
   env_.event_log().StopLogging();
 }
 
-std::optional<SSLRole> PeerConnection::GetSctpSslRole_n() {
-  RTC_DCHECK_RUN_ON(network_thread());
-  return sctp_mid_n_ ? transport_controller_->GetDtlsRole(*sctp_mid_n_)
-                     : std::nullopt;
-}
-
 bool PeerConnection::GetSslRole(const std::string& content_name,
                                 SSLRole* role) {
   RTC_DCHECK_RUN_ON(signaling_thread());

diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index b48c82b..2b68531 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h

@@ -335,8 +335,6 @@
     return !sdp_handler_ ||
            sdp_handler_->signaling_state() == PeerConnectionInterface::kClosed;
   }
-  // Get current SSL role used by SCTP's underlying transport.
-  std::optional<SSLRole> GetSctpSslRole_n() override;
 
   void OnSctpDataChannelStateChanged(
       int channel_id,

diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h
index f31f999..cdffa21 100644
--- a/pc/peer_connection_internal.h
+++ b/pc/peer_connection_internal.h

@@ -101,7 +101,6 @@
   // observer is removed.
   virtual void RunWithObserver(
       absl::AnyInvocable<void(webrtc::PeerConnectionObserver*) &&>) = 0;
-  virtual std::optional<SSLRole> GetSctpSslRole_n() = 0;
   virtual PeerConnectionInterface::IceConnectionState
   ice_connection_state_internal() = 0;
   virtual void SetIceConnectionState(

diff --git a/pc/sctp_transport.cc b/pc/sctp_transport.cc
index abca8a1..243de86 100644
--- a/pc/sctp_transport.cc
+++ b/pc/sctp_transport.cc

@@ -143,20 +143,15 @@
 
 std::optional<SSLRole> SctpTransport::DtlsRole() {
   RTC_DCHECK_RUN_ON(owner_thread_);
-  if (!dtls_transport_) {
+  if (!internal_sctp_transport_ ||
+      !internal_sctp_transport_->dtls_transport()) {
     return std::nullopt;
   }
-  std::optional<DtlsTransportTlsRole> role =
-      dtls_transport_->Information().role();
-  if (!role.has_value()) {
-    return std::nullopt;
+  SSLRole role;
+  if (internal_sctp_transport_->dtls_transport()->GetDtlsRole(&role)) {
+    return role;
   }
-  switch (*role) {
-    case DtlsTransportTlsRole::kServer:
-      return SSL_SERVER;
-    case DtlsTransportTlsRole::kClient:
-      return SSL_CLIENT;
-  }
+  return std::nullopt;
 }
 
 scoped_refptr<DtlsTransportInterface> SctpTransport::dtls_transport() const {

diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h
index bcea410..d395e83 100644
--- a/pc/test/fake_peer_connection_base.h
+++ b/pc/test/fake_peer_connection_base.h

@@ -381,7 +381,6 @@
       absl::AnyInvocable<void(webrtc::PeerConnectionObserver*) &&>) override {
     RTC_DCHECK_NOTREACHED();
   }
-  std::optional<SSLRole> GetSctpSslRole_n() override { return std::nullopt; }
   PeerConnectionInterface::IceConnectionState ice_connection_state_internal()
       override {
     return PeerConnectionInterface::IceConnectionState::kIceConnectionNew;

diff --git a/pc/test/mock_peer_connection_internal.h b/pc/test/mock_peer_connection_internal.h
index dddc254..37bd49b 100644
--- a/pc/test/mock_peer_connection_internal.h
+++ b/pc/test/mock_peer_connection_internal.h

@@ -296,7 +296,6 @@
               RunWithObserver,
               (absl::AnyInvocable<void(webrtc::PeerConnectionObserver*) &&>),
               (override));
-  MOCK_METHOD(std::optional<SSLRole>, GetSctpSslRole_n, (), (override));
   MOCK_METHOD(PeerConnectionInterface::IceConnectionState,
               ice_connection_state_internal,
               (),