admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove GetSctpSslRole, only use GetSctpSslRole_n

Browse Source 
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}
This commit is contained in:
Tommi 2023-03-25 10:56:18 +01:00 committed by WebRTC LUCI CQ
parent cbe5d81498
commit 335d084b3b
11 changed files with 106 additions and 104 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
pc/data_channel_controller.cc
View File

@ -227,7 +227,7 @@ void DataChannelController::OnDataChannelOpenMessage(
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 @@ void DataChannelController::OnDataChannelOpenMessage(
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 @@ DataChannelController::InternalCreateDataChannelWithProxy(
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;
}

6
pc/data_channel_controller.h
View File

@ -82,8 +82,7 @@ class DataChannelController : public SctpDataChannelControllerInterface,
// 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 @@ class DataChannelController : public SctpDataChannelControllerInterface,
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.

24
pc/data_channel_controller_unittest.cc
View File

@ -65,8 +65,7 @@ TEST_F(DataChannelControllerTest, CreateAndDestroy) {
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 @@ TEST_F(DataChannelControllerTest, CreateDataChannelEarlyClose) {
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, CreateDataChannelEarlyClose) {
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, CreateDataChannelLateRelease) {
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 @@ TEST_F(DataChannelControllerTest, AsyncChannelCloseTeardown) {
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 @@ TEST_F(DataChannelControllerTest, MaxChannels) {
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 @@ TEST_F(DataChannelControllerTest, MaxChannels) {
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.

55
pc/peer_connection.cc
View File

@ -1394,14 +1394,17 @@ PeerConnection::CreateDataChannelOrError(const std::string& label,
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 @@ void PeerConnection::StopRtcEventLog_w() {
}
}
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,

4
pc/peer_connection.h
View File

@ -314,9 +314,7 @@ class PeerConnection : public PeerConnectionInternal,
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,

8
pc/peer_connection_internal.h
View File

@ -76,13 +76,7 @@ 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 absl::optional<rtc::SSLRole> GetSctpSslRole_n() = 0;
virtual PeerConnectionInterface::IceConnectionState
ice_connection_state_internal() = 0;
virtual void SetIceConnectionState(

5
pc/sctp_data_channel.h
View File

@ -70,6 +70,11 @@ struct InternalDataChannelInit : public DataChannelInit {
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.

58
pc/sdp_offer_answer.cc
View File

@ -3134,7 +3134,7 @@ void SdpOfferAnswerHandler::OnOperationsChainEmpty() {
}
}
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 @@ void SdpOfferAnswerHandler::AllocateSctpSids() {
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 @@ void SdpOfferAnswerHandler::AllocateSctpSids() {
}
}
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

7
pc/sdp_offer_answer.h
View File

@ -156,10 +156,15 @@ class SdpOfferAnswerHandler : public SdpStateProvider {
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();

4
pc/test/fake_peer_connection_base.h
View File

@ -320,9 +320,7 @@ class FakePeerConnectionBase : public PeerConnectionInternal {
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()

6
pc/test/mock_peer_connection_internal.h
View File

@ -231,11 +231,7 @@ class MockPeerConnectionInternal : public PeerConnectionInternal {
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,
(),