Split up GetSctpSslRole to include an _n variant.

This allows the SslRole to be queried from the network thread which
will simplify some code paths and avoid thread hopping.

The next steps will be to remove GetSctpSslRole and only query the
DTLS role on the network thread and start combining other operations.

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

pc/data_channel_controller.cc

@@ -275,6 +275,7 @@ DataChannelController::InternalCreateSctpDataChannel(
     // the network thread. (unless there's no transport). Change this so that
     // the role is checked on the network thread and any network thread related
     // initialization is done at the same time (to avoid additional hops).
+    // Use `GetSctpSslRole_n` on the network thread.
     if (pc_->GetSctpSslRole(&role)) {
       sid = sid_allocator_.AllocateSid(role);
       if (!sid.HasValue())

pc/peer_connection.cc

@@ -2231,41 +2231,48 @@ void PeerConnection::StopRtcEventLog_w() {
 
 bool PeerConnection::GetSctpSslRole(rtc::SSLRole* role) {
   RTC_DCHECK_RUN_ON(signaling_thread());
-  if (!local_description() || !remote_description()) {
-    RTC_LOG(LS_VERBOSE)
-        << "Local and Remote descriptions must be applied to get the "
-           "SSL Role of the SCTP transport.";
-    return false;
-  }
-  if (!data_channel_controller_.data_channel_transport()) {
+  if (!sctp_mid_s_ || !data_channel_controller_.data_channel_transport()) {
     RTC_LOG(LS_INFO) << "Non-rejected SCTP m= section is needed to get the "
                         "SSL Role of the SCTP transport.";
     return false;
   }
 
-  absl::optional<rtc::SSLRole> dtls_role;
-  if (sctp_mid_s_) {
-    dtls_role = network_thread()->BlockingCall([this] {
-      RTC_DCHECK_RUN_ON(network_thread());
-      return transport_controller_->GetDtlsRole(*sctp_mid_n_);
-    });
-    if (!dtls_role && sdp_handler_->is_caller().has_value()) {
-      // This works fine if we are the offerer, but can be a mistake if
-      // we are the answerer and the remote offer is ACTIVE. In that
-      // case, we will guess the role wrong.
-      // TODO(bugs.webrtc.org/13668): Check if this actually happens.
-      RTC_LOG(LS_ERROR)
-          << "Possible risk: DTLS role guesser is active, is_caller is "
-          << *sdp_handler_->is_caller();
-      dtls_role =
-          *sdp_handler_->is_caller() ? rtc::SSL_SERVER : rtc::SSL_CLIENT;
-    }
-    if (dtls_role) {
-      *role = *dtls_role;
-      return true;
-    }
+  absl::optional<rtc::SSLRole> dtls_role = network_thread()->BlockingCall(
+      [this, is_caller = sdp_handler_->is_caller()] {
+        RTC_DCHECK_RUN_ON(network_thread());
+        return GetSctpSslRole_n(is_caller);
+      });
+  if (!dtls_role) {
+    return false;
   }
-  return false;
+
+  *role = *dtls_role;
+  return true;
+}
+
+absl::optional<rtc::SSLRole> PeerConnection::GetSctpSslRole_n(
+    absl::optional<bool> is_caller) {
+  RTC_DCHECK_RUN_ON(network_thread());
+  if (!sctp_mid_n_)
+    return absl::nullopt;
+
+  absl::optional<rtc::SSLRole> dtls_role =
+      transport_controller_->GetDtlsRole(*sctp_mid_n_);
+  if (!dtls_role) {
+    if (!is_caller.has_value())
+      return absl::nullopt;
+
+    // This works fine if we are the offerer, but can be a mistake if
+    // we are the answerer and the remote offer is ACTIVE. In that
+    // case, we will guess the role wrong.
+    // TODO(bugs.webrtc.org/13668): Check if this actually happens.
+    RTC_LOG(LS_ERROR)
+        << "Possible risk: DTLS role guesser is active, is_caller is "
+        << *is_caller;
+    dtls_role = *is_caller ? rtc::SSL_SERVER : rtc::SSL_CLIENT;
+  }
+
+  return dtls_role;
 }
 
 bool PeerConnection::GetSslRole(const std::string& content_name,

pc/peer_connection.h

@@ -315,6 +315,8 @@ class PeerConnection : public PeerConnectionInternal,
   }
   // Get current SSL role used by SCTP's underlying transport.
   bool GetSctpSslRole(rtc::SSLRole* role) override;
+  absl::optional<rtc::SSLRole> GetSctpSslRole_n(
+      absl::optional<bool> is_caller) override;
 
   void OnSctpDataChannelStateChanged(
       DataChannelInterface* channel,

pc/peer_connection_internal.h

@@ -76,7 +76,13 @@ class PeerConnectionSdpMethods {
   virtual LegacyStatsCollector* legacy_stats() = 0;
   // Returns the observer. Will crash on CHECK if the observer is removed.
   virtual PeerConnectionObserver* Observer() const = 0;
+  // TODO(webrtc:11547): Remove `GetSctpSslRole` and require `GetSctpSslRole_n`
+  // instead. Currently `GetSctpSslRole` relied upon by `DataChannelController`.
+  // Once that path has been updated to use `GetSctpSslRole_n`, this method
+  // can be removed.
   virtual bool GetSctpSslRole(rtc::SSLRole* role) = 0;
+  virtual absl::optional<rtc::SSLRole> GetSctpSslRole_n(
+      absl::optional<bool> is_caller) = 0;
   virtual PeerConnectionInterface::IceConnectionState
   ice_connection_state_internal() = 0;
   virtual void SetIceConnectionState(

pc/sdp_offer_answer.cc

@@ -1739,10 +1739,7 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription(
 
   // If setting the description decided our SSL role, allocate any necessary
   // SCTP sids.
-  rtc::SSLRole role;
-  if (pc_->GetSctpSslRole(&role)) {
-    data_channel_controller()->AllocateSctpSids(role);
-  }
+  AllocateSctpSids();
 
   if (IsUnifiedPlan()) {
     if (ConfiguredForMedia()) {
@@ -1990,10 +1987,7 @@ void SdpOfferAnswerHandler::ApplyRemoteDescription(
 
   // If setting the description decided our SSL role, allocate any necessary
   // SCTP sids.
-  rtc::SSLRole role;
-  if (pc_->GetSctpSslRole(&role)) {
-    data_channel_controller()->AllocateSctpSids(role);
-  }
+  AllocateSctpSids();
 
   if (operation->unified_plan()) {
     ApplyRemoteDescriptionUpdateTransceiverState(operation->type());
@@ -3231,6 +3225,28 @@ void SdpOfferAnswerHandler::UpdateNegotiationNeeded() {
   GenerateNegotiationNeededEvent();
 }
 
+void SdpOfferAnswerHandler::AllocateSctpSids() {
+  RTC_DCHECK_RUN_ON(signaling_thread());
+  if (!local_description() || !remote_description()) {
+    RTC_DLOG(LS_VERBOSE)
+        << "Local and Remote descriptions must be applied to get the "
+           "SSL Role of the SCTP transport.";
+    return;
+  }
+
+  absl::optional<rtc::SSLRole> role =
+      network_thread()->BlockingCall([this, is_caller = is_caller()] {
+        RTC_DCHECK_RUN_ON(network_thread());
+        return pc_->GetSctpSslRole_n(is_caller);
+      });
+
+  if (role) {
+    // TODO(webrtc:11547): Make this call on the network thread too once
+    // `AllocateSctpSids` has been updated.
+    data_channel_controller()->AllocateSctpSids(*role);
+  }
+}
+
 bool SdpOfferAnswerHandler::CheckIfNegotiationIsNeeded() {
   RTC_DCHECK_RUN_ON(signaling_thread());
   // 1. If any implementation-specific negotiation is required, as described

pc/sdp_offer_answer.h

@@ -159,6 +159,7 @@ class SdpOfferAnswerHandler : public SdpStateProvider {
   absl::optional<bool> is_caller();
   bool HasNewIceCredentials();
   void UpdateNegotiationNeeded();
+  void AllocateSctpSids();
 
   // Destroys all BaseChannels and destroys the SCTP data channel, if present.
   void DestroyAllChannels();

pc/test/fake_peer_connection_base.h

@@ -321,6 +321,10 @@ class FakePeerConnectionBase : public PeerConnectionInternal {
   LegacyStatsCollector* legacy_stats() override { return nullptr; }
   PeerConnectionObserver* Observer() const override { return nullptr; }
   bool GetSctpSslRole(rtc::SSLRole* role) override { return false; }
+  absl::optional<rtc::SSLRole> GetSctpSslRole_n(
+      absl::optional<bool> is_caller) override {
+    return absl::nullopt;
+  }
   PeerConnectionInterface::IceConnectionState ice_connection_state_internal()
       override {
     return PeerConnectionInterface::IceConnectionState::kIceConnectionNew;

pc/test/mock_peer_connection_internal.h

@@ -232,6 +232,10 @@ class MockPeerConnectionInternal : public PeerConnectionInternal {
   MOCK_METHOD(LegacyStatsCollector*, legacy_stats, (), (override));
   MOCK_METHOD(PeerConnectionObserver*, Observer, (), (const, override));
   MOCK_METHOD(bool, GetSctpSslRole, (rtc::SSLRole*), (override));
+  MOCK_METHOD(absl::optional<rtc::SSLRole>,
+              GetSctpSslRole_n,
+              (absl::optional<bool>),
+              (override));
   MOCK_METHOD(PeerConnectionInterface::IceConnectionState,
               ice_connection_state_internal,
               (),