Remove GetSctpSslRole, only use GetSctpSslRole_n
This updates DataChannelController and test classes to use
GetSctpSslRole_n instead and query the role on the network thread.
Along the way this CL makes the init config struct for when constructing
data channels, mandatory. It's now passed via const& instead of by pointer. In practice a valid pointer was always being passed.
Bug: webrtc:11547
Change-Id: I0f4bbf364969cc2dec07871c297ddbef0c175f86
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/298307
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39676}
diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc
index e1475e0..275c82c 100644
--- a/pc/data_channel_controller.cc
+++ b/pc/data_channel_controller.cc
@@ -227,7 +227,7 @@
const std::string& label,
const InternalDataChannelInit& config) {
rtc::scoped_refptr<DataChannelInterface> channel(
- InternalCreateDataChannelWithProxy(label, &config));
+ InternalCreateDataChannelWithProxy(label, config));
if (!channel.get()) {
RTC_LOG(LS_ERROR) << "Failed to create DataChannel from the OPEN message.";
return;
@@ -240,7 +240,7 @@
rtc::scoped_refptr<DataChannelInterface>
DataChannelController::InternalCreateDataChannelWithProxy(
const std::string& label,
- const InternalDataChannelInit* config) {
+ const InternalDataChannelInit& config) {
RTC_DCHECK_RUN_ON(signaling_thread());
if (pc_->IsClosed()) {
return nullptr;
@@ -258,26 +258,31 @@
rtc::scoped_refptr<SctpDataChannel>
DataChannelController::InternalCreateSctpDataChannel(
const std::string& label,
- const InternalDataChannelInit* config) {
+ const InternalDataChannelInit& config) {
RTC_DCHECK_RUN_ON(signaling_thread());
- if (config && !config->IsValid()) {
+ if (!config.IsValid()) {
RTC_LOG(LS_ERROR) << "Failed to initialize the SCTP data channel due to "
"invalid DataChannelInit.";
return nullptr;
}
- InternalDataChannelInit new_config =
- config ? (*config) : InternalDataChannelInit();
+ InternalDataChannelInit new_config = config;
StreamId sid(new_config.id);
if (!sid.HasValue()) {
- rtc::SSLRole role;
- // TODO(bugs.webrtc.org/11547): `GetSctpSslRole` likely involves a hop to
- // 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);
+ // TODO(bugs.webrtc.org/11547): Use this call to the network thread more
+ // broadly to initialize the channel on the network thread, assign
+ // an id and/or other things that belong on the network thread.
+ // Move `sid_allocator_` to the network thread.
+ absl::optional<rtc::SSLRole> role = network_thread()->BlockingCall([this] {
+ RTC_DCHECK_RUN_ON(network_thread());
+ return pc_->GetSctpSslRole_n();
+ });
+
+ if (!role)
+ role = new_config.fallback_ssl_role;
+
+ if (role) {
+ sid = sid_allocator_.AllocateSid(*role);
if (!sid.HasValue())
return nullptr;
}
diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h
index 6275d62..8291670 100644
--- a/pc/data_channel_controller.h
+++ b/pc/data_channel_controller.h
@@ -82,8 +82,7 @@
// be offered in a SessionDescription, and wraps it in a proxy object.
rtc::scoped_refptr<DataChannelInterface> InternalCreateDataChannelWithProxy(
const std::string& label,
- const InternalDataChannelInit*
- config) /* RTC_RUN_ON(signaling_thread()) */;
+ const InternalDataChannelInit& config);
void AllocateSctpSids(rtc::SSLRole role);
// Checks if any data channel has been added.
@@ -104,8 +103,7 @@
private:
rtc::scoped_refptr<SctpDataChannel> InternalCreateSctpDataChannel(
const std::string& label,
- const InternalDataChannelInit*
- config) /* RTC_RUN_ON(signaling_thread()) */;
+ const InternalDataChannelInit& config);
// Parses and handles open messages. Returns true if the message is an open
// message and should be considered to be handled, false otherwise.
diff --git a/pc/data_channel_controller_unittest.cc b/pc/data_channel_controller_unittest.cc
index 1e575e6..f5575b7 100644
--- a/pc/data_channel_controller_unittest.cc
+++ b/pc/data_channel_controller_unittest.cc
@@ -65,8 +65,7 @@
TEST_F(DataChannelControllerTest, CreateDataChannelEarlyRelease) {
DataChannelController dcc(pc_.get());
auto channel = dcc.InternalCreateDataChannelWithProxy(
- "label",
- std::make_unique<InternalDataChannelInit>(DataChannelInit()).get());
+ "label", InternalDataChannelInit(DataChannelInit()));
channel = nullptr; // dcc holds a reference to channel, so not destroyed yet
}
@@ -75,8 +74,7 @@
EXPECT_FALSE(dcc.HasDataChannels());
EXPECT_FALSE(dcc.HasUsedDataChannels());
auto channel = dcc.InternalCreateDataChannelWithProxy(
- "label",
- std::make_unique<InternalDataChannelInit>(DataChannelInit()).get());
+ "label", InternalDataChannelInit(DataChannelInit()));
EXPECT_TRUE(dcc.HasDataChannels());
EXPECT_TRUE(dcc.HasUsedDataChannels());
channel->Close();
@@ -87,8 +85,7 @@
TEST_F(DataChannelControllerTest, CreateDataChannelLateRelease) {
auto dcc = std::make_unique<DataChannelController>(pc_.get());
auto channel = dcc->InternalCreateDataChannelWithProxy(
- "label",
- std::make_unique<InternalDataChannelInit>(DataChannelInit()).get());
+ "label", InternalDataChannelInit(DataChannelInit()));
dcc.reset();
channel = nullptr;
}
@@ -96,8 +93,7 @@
TEST_F(DataChannelControllerTest, CloseAfterControllerDestroyed) {
auto dcc = std::make_unique<DataChannelController>(pc_.get());
auto channel = dcc->InternalCreateDataChannelWithProxy(
- "label",
- std::make_unique<InternalDataChannelInit>(DataChannelInit()).get());
+ "label", InternalDataChannelInit(DataChannelInit()));
dcc.reset();
channel->Close();
}
@@ -106,8 +102,7 @@
DataChannelController dcc(pc_.get());
rtc::scoped_refptr<DataChannelInterface> channel =
dcc.InternalCreateDataChannelWithProxy(
- "label",
- std::make_unique<InternalDataChannelInit>(DataChannelInit()).get());
+ "label", InternalDataChannelInit(DataChannelInit()));
SctpDataChannel* inner_channel =
DowncastProxiedDataChannelInterfaceToSctpDataChannelForTesting(
channel.get());
@@ -146,9 +141,9 @@
NiceMock<MockDataChannelTransport> transport;
int channel_id = 0;
- ON_CALL(*pc_, GetSctpSslRole).WillByDefault([&](rtc::SSLRole* role) {
- *role = (channel_id & 1) ? rtc::SSL_SERVER : rtc::SSL_CLIENT;
- return true;
+ ON_CALL(*pc_, GetSctpSslRole_n).WillByDefault([&]() {
+ return absl::optional<rtc::SSLRole>((channel_id & 1) ? rtc::SSL_SERVER
+ : rtc::SSL_CLIENT);
});
DataChannelController dcc(pc_.get());
@@ -160,8 +155,7 @@
for (channel_id = 0; channel_id <= cricket::kMaxSctpStreams; ++channel_id) {
rtc::scoped_refptr<DataChannelInterface> channel =
dcc.InternalCreateDataChannelWithProxy(
- "label",
- std::make_unique<InternalDataChannelInit>(DataChannelInit()).get());
+ "label", InternalDataChannelInit(DataChannelInit()));
if (channel_id == cricket::kMaxSctpStreams) {
// We've reached the maximum and the previous call should have failed.
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 38e52b6..4ae7642 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -1394,14 +1394,17 @@
bool first_datachannel = !data_channel_controller_.HasUsedDataChannels();
- std::unique_ptr<InternalDataChannelInit> internal_config;
+ InternalDataChannelInit internal_config;
if (config) {
- internal_config.reset(new InternalDataChannelInit(*config));
+ internal_config = InternalDataChannelInit(*config);
}
+
+ internal_config.fallback_ssl_role = sdp_handler_->GuessSslRole();
+
// TODO(bugs.webrtc.org/12796): Return a more specific error.
rtc::scoped_refptr<DataChannelInterface> channel(
data_channel_controller_.InternalCreateDataChannelWithProxy(
- label, internal_config.get()));
+ label, internal_config));
if (!channel.get()) {
return RTCError(RTCErrorType::INTERNAL_ERROR,
"Data channel creation failed");
@@ -2229,50 +2232,10 @@
}
}
-bool PeerConnection::GetSctpSslRole(rtc::SSLRole* role) {
- RTC_DCHECK_RUN_ON(signaling_thread());
- 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 = 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;
- }
-
- *role = *dtls_role;
- return true;
-}
-
-absl::optional<rtc::SSLRole> PeerConnection::GetSctpSslRole_n(
- absl::optional<bool> is_caller) {
+absl::optional<rtc::SSLRole> PeerConnection::GetSctpSslRole_n() {
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;
+ return sctp_mid_n_ ? transport_controller_->GetDtlsRole(*sctp_mid_n_)
+ : absl::nullopt;
}
bool PeerConnection::GetSslRole(const std::string& content_name,
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 1e50ec8..d1b197c 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -314,9 +314,7 @@
sdp_handler_->signaling_state() == PeerConnectionInterface::kClosed;
}
// 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;
+ absl::optional<rtc::SSLRole> GetSctpSslRole_n() override;
void OnSctpDataChannelStateChanged(
DataChannelInterface* channel,
diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h
index debe830..2ddb57b 100644
--- a/pc/peer_connection_internal.h
+++ b/pc/peer_connection_internal.h
@@ -76,13 +76,7 @@
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 absl::optional<rtc::SSLRole> GetSctpSslRole_n() = 0;
virtual PeerConnectionInterface::IceConnectionState
ice_connection_state_internal() = 0;
virtual void SetIceConnectionState(
diff --git a/pc/sctp_data_channel.h b/pc/sctp_data_channel.h
index e0f8bb2..5217505 100644
--- a/pc/sctp_data_channel.h
+++ b/pc/sctp_data_channel.h
@@ -70,6 +70,11 @@
bool IsValid() const;
OpenHandshakeRole open_handshake_role;
+ // Optional fallback or backup flag from PC that's used for non-prenegotiated
+ // stream ids in situations where we cannot determine the SSL role from the
+ // transport for purposes of generating a stream ID.
+ // See: https://www.rfc-editor.org/rfc/rfc8832.html#name-protocol-overview
+ absl::optional<rtc::SSLRole> fallback_ssl_role;
};
// Helper class to allocate unique IDs for SCTP DataChannels.
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index 8dc6d84..f19cd13 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -3134,7 +3134,7 @@
}
}
-absl::optional<bool> SdpOfferAnswerHandler::is_caller() {
+absl::optional<bool> SdpOfferAnswerHandler::is_caller() const {
RTC_DCHECK_RUN_ON(signaling_thread());
return is_caller_;
}
@@ -3234,11 +3234,14 @@
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);
- });
+ absl::optional<rtc::SSLRole> role = network_thread()->BlockingCall([this] {
+ RTC_DCHECK_RUN_ON(network_thread());
+ return pc_->GetSctpSslRole_n();
+ });
+
+ if (!role) {
+ role = GuessSslRole();
+ }
if (role) {
// TODO(webrtc:11547): Make this call on the network thread too once
@@ -3247,6 +3250,49 @@
}
}
+absl::optional<rtc::SSLRole> SdpOfferAnswerHandler::GuessSslRole() const {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ if (!pc_->sctp_mid())
+ return absl::nullopt;
+
+ // TODO(bugs.webrtc.org/13668): This guesswork is guessing wrong (returning
+ // SSL_CLIENT = ACTIVE) if remote offer has role ACTIVE, but we'll be able
+ // to detect that by looking at the SDP.
+ //
+ // The phases of establishing an SCTP session are:
+ //
+ // Offerer:
+ //
+ // * Before negotiation: Neither is_caller nor sctp_mid exists.
+ // * After setting an offer as local description: is_caller is known (true),
+ // sctp_mid is known, but we don't know the SSL role for sure (or if we'll
+ // eventually get an SCTP session).
+ // * After setting an answer as the remote description: We know is_caller,
+ // sctp_mid and that we'll get the SCTP channel established (m-section
+ // wasn't rejected).
+ // * Special case: The SCTP m-section was rejected: Close datachannels.
+ // * We MAY know the SSL role if we offered actpass and got back active or
+ // passive; if the other end is a webrtc implementation, it will be active.
+ // * After the TLS handshake: We have a definitive answer on the SSL role.
+ //
+ // Answerer:
+ //
+ // * After setting an offer as remote description: We know is_caller (false).
+ // * If there was an SCTP session, we know the SCTP mid. We also know the
+ // SSL role, since if the remote offer was actpass or passive, we'll answer
+ // active, and if the remote offer was active, we're passive.
+ // * Special case: No SCTP m= line. We don't know for sure if the remote
+ // doesn't support it or just didn't offer it. Not sure what we do in this
+ // case (logic would suggest fire a `negotiationneeded` event and generate a
+ // subsequent offer, but this needs to be tested).
+ // * After the TLS handshake: We know that TLS obeyed the protocol. There
+ // should be an error surfaced somewhere if it didn't.
+ // * "Guessing" should always be correct if we get an SCTP session and are not
+ // the offerer.
+
+ return is_caller() ? rtc::SSL_SERVER : rtc::SSL_CLIENT;
+}
+
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 ff075bc..80a2139 100644
--- a/pc/sdp_offer_answer.h
+++ b/pc/sdp_offer_answer.h
@@ -156,10 +156,15 @@
bool AddStream(MediaStreamInterface* local_stream);
void RemoveStream(MediaStreamInterface* local_stream);
- absl::optional<bool> is_caller();
+ absl::optional<bool> is_caller() const;
bool HasNewIceCredentials();
void UpdateNegotiationNeeded();
void AllocateSctpSids();
+ // Based on the negotiation state, guess what the SSLRole might be without
+ // directly getting the information from the transport.
+ // This is used for allocating stream ids for data channels.
+ // See also `InternalDataChannelInit::fallback_ssl_role`.
+ absl::optional<rtc::SSLRole> GuessSslRole() const;
// 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 69b0106..0508673 100644
--- a/pc/test/fake_peer_connection_base.h
+++ b/pc/test/fake_peer_connection_base.h
@@ -320,9 +320,7 @@
cricket::PortAllocator* port_allocator() override { return nullptr; }
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 {
+ absl::optional<rtc::SSLRole> GetSctpSslRole_n() override {
return absl::nullopt;
}
PeerConnectionInterface::IceConnectionState ice_connection_state_internal()
diff --git a/pc/test/mock_peer_connection_internal.h b/pc/test/mock_peer_connection_internal.h
index a3ee7d8..81f8903 100644
--- a/pc/test/mock_peer_connection_internal.h
+++ b/pc/test/mock_peer_connection_internal.h
@@ -231,11 +231,7 @@
MOCK_METHOD(cricket::PortAllocator*, port_allocator, (), (override));
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(absl::optional<rtc::SSLRole>, GetSctpSslRole_n, (), (override));
MOCK_METHOD(PeerConnectionInterface::IceConnectionState,
ice_connection_state_internal,
(),
