From c61eee28f3c3353433e26344357f58f599334123 Mon Sep 17 00:00:00 2001 From: Tommi Date: Wed, 22 Mar 2023 08:25:38 +0100 Subject: [PATCH] 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 Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#39642} --- pc/data_channel_controller.cc | 1 + pc/peer_connection.cc | 65 ++++++++++++++----------- pc/peer_connection.h | 2 + pc/peer_connection_internal.h | 6 +++ pc/sdp_offer_answer.cc | 32 +++++++++--- pc/sdp_offer_answer.h | 1 + pc/test/fake_peer_connection_base.h | 4 ++ pc/test/mock_peer_connection_internal.h | 4 ++ 8 files changed, 78 insertions(+), 37 deletions(-) diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index 7eb1501a59..e1475e0ded 100644 --- a/pc/data_channel_controller.cc +++ b/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()) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 16c803d246..38e52b6a13 100644 --- a/pc/peer_connection.cc +++ b/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 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 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 PeerConnection::GetSctpSslRole_n( + absl::optional is_caller) { + RTC_DCHECK_RUN_ON(network_thread()); + if (!sctp_mid_n_) + return absl::nullopt; + + absl::optional 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, diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 3298203b39..1e50ec891b 100644 --- a/pc/peer_connection.h +++ b/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 GetSctpSslRole_n( + absl::optional is_caller) override; void OnSctpDataChannelStateChanged( DataChannelInterface* channel, diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h index 262d2d5e19..debe830126 100644 --- a/pc/peer_connection_internal.h +++ b/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 GetSctpSslRole_n( + absl::optional is_caller) = 0; virtual PeerConnectionInterface::IceConnectionState ice_connection_state_internal() = 0; virtual void SetIceConnectionState( diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index a80f297c85..953dc86c40 100644 --- a/pc/sdp_offer_answer.cc +++ b/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 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 diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index 2124ed8697..ff075bcb5d 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -159,6 +159,7 @@ class SdpOfferAnswerHandler : public SdpStateProvider { absl::optional is_caller(); bool HasNewIceCredentials(); void UpdateNegotiationNeeded(); + void AllocateSctpSids(); // Destroys all BaseChannels and destroys the SCTP data channel, if present. void DestroyAllChannels(); diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h index 5a1859ef5d..69b0106d1f 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/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 GetSctpSslRole_n( + absl::optional is_caller) override { + return absl::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 a47d0b7eeb..a3ee7d8933 100644 --- a/pc/test/mock_peer_connection_internal.h +++ b/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, + GetSctpSslRole_n, + (absl::optional), + (override)); MOCK_METHOD(PeerConnectionInterface::IceConnectionState, ice_connection_state_internal, (),