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

Move ownership of ICE from DtlsTransport to JsepTransport.

Browse Source 
It does not make sense for DtlsTransport to own ICE, and this arrangement will
not work when negotiating datagram or DTLS transport.  During negotiation, both
a DTLS transport and a datagram transport need to be ready to receive from the
same ICE transport, depending on which protocol is chosen by the answerer.  Once
the answerer chooses a protocol, the transport that is not chosen must be
deleted, but ICE must be left intact for use by the remaining transport.

Bug: webrtc:9719
Change-Id: Ibab969b574c981e3834ced71f8ff88008cb26a6c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/139340
Reviewed-by: Anton Sukhanov <sukhanov@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Commit-Queue: Bjorn Mellem <mellem@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28113}
This commit is contained in:
Bjorn A Mellem 2019-05-29 17:34:13 -07:00 committed by Commit Bot
parent a913c12462
commit 0c1c1b4014
14 changed files with 80 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
p2p/base/datagram_dtls_adaptor.cc
View File

@ -44,12 +44,12 @@ constexpr bool kBypassDatagramDtlsTestOnly = false;
namespace cricket {
DatagramDtlsAdaptor::DatagramDtlsAdaptor(
std::unique_ptr<IceTransportInternal> ice_transport,
IceTransportInternal* ice_transport,
std::unique_ptr<webrtc::DatagramTransportInterface> datagram_transport,
const webrtc::CryptoOptions& crypto_options,
webrtc::RtcEventLog* event_log)
: crypto_options_(crypto_options),
ice_transport_(std::move(ice_transport)),
ice_transport_(ice_transport),
datagram_transport_(std::move(datagram_transport)),
event_log_(event_log) {
RTC_DCHECK(ice_transport_);
@ -91,7 +91,6 @@ DatagramDtlsAdaptor::~DatagramDtlsAdaptor() {
// Make sure datagram transport is destroyed before ICE.
datagram_transport_.reset();
ice_transport_.reset();
}
const webrtc::CryptoOptions& DatagramDtlsAdaptor::crypto_options() const {
@ -127,7 +126,7 @@ void DatagramDtlsAdaptor::OnReadPacket(rtc::PacketTransportInternal* transport,
RTC_DCHECK(kBypassDatagramDtlsTestOnly);
RTC_DCHECK_RUN_ON(&thread_checker_);
RTC_DCHECK_EQ(transport, ice_transport_.get());
RTC_DCHECK_EQ(transport, ice_transport_);
RTC_DCHECK(flags == 0);
PropagateReadPacket(
@ -245,7 +244,7 @@ bool DatagramDtlsAdaptor::SetSslMaxProtocolVersion(
}
IceTransportInternal* DatagramDtlsAdaptor::ice_transport() {
return ice_transport_.get();
return ice_transport_;
}
webrtc::DatagramTransportInterface* DatagramDtlsAdaptor::datagram_transport() {
@ -264,7 +263,7 @@ void DatagramDtlsAdaptor::OnReadyToSend(
void DatagramDtlsAdaptor::OnWritableState(
rtc::PacketTransportInternal* transport) {
RTC_DCHECK_RUN_ON(&thread_checker_);
RTC_DCHECK(transport == ice_transport_.get());
RTC_DCHECK(transport == ice_transport_);
RTC_LOG(LS_VERBOSE) << ": ice_transport writable state changed to "
<< ice_transport_->writable();
@ -352,7 +351,7 @@ void DatagramDtlsAdaptor::OnNetworkRouteChanged(
void DatagramDtlsAdaptor::OnReceivingState(
rtc::PacketTransportInternal* transport) {
RTC_DCHECK_RUN_ON(&thread_checker_);
RTC_DCHECK(transport == ice_transport_.get());
RTC_DCHECK(transport == ice_transport_);
RTC_LOG(LS_VERBOSE) << "ice_transport receiving state changed to "
<< ice_transport_->receiving();

4
p2p/base/datagram_dtls_adaptor.h
View File

@ -43,7 +43,7 @@ class DatagramDtlsAdaptor : public DtlsTransportInternal,
// has a virtual getter crypto_options(). Consider removing getter and
// removing crypto_options from DatagramDtlsAdaptor.
DatagramDtlsAdaptor(
std::unique_ptr<IceTransportInternal> ice_transport,
IceTransportInternal* ice_transport,
std::unique_ptr<webrtc::DatagramTransportInterface> datagram_transport,
const webrtc::CryptoOptions& crypto_options,
webrtc::RtcEventLog* event_log);
@ -130,7 +130,7 @@ class DatagramDtlsAdaptor : public DtlsTransportInternal,
rtc::ThreadChecker thread_checker_;
webrtc::CryptoOptions crypto_options_;
std::unique_ptr<IceTransportInternal> ice_transport_;
IceTransportInternal* ice_transport_;
std::unique_ptr<webrtc::DatagramTransportInterface> datagram_transport_;

20
p2p/base/dtls_transport.cc
View File

@ -118,13 +118,12 @@ void StreamInterfaceChannel::Close() {
state_ = rtc::SS_CLOSED;
}
DtlsTransport::DtlsTransport(
std::unique_ptr<IceTransportInternal> ice_transport,
const webrtc::CryptoOptions& crypto_options,
webrtc::RtcEventLog* event_log)
DtlsTransport::DtlsTransport(IceTransportInternal* ice_transport,
const webrtc::CryptoOptions& crypto_options,
webrtc::RtcEventLog* event_log)
: transport_name_(ice_transport->transport_name()),
component_(ice_transport->component()),
ice_transport_(std::move(ice_transport)),
ice_transport_(ice_transport),
downward_(NULL),
srtp_ciphers_(crypto_options.GetSupportedDtlsSrtpCryptoSuites()),
ssl_max_version_(rtc::SSL_PROTOCOL_DTLS_12),
@ -328,8 +327,7 @@ bool DtlsTransport::ExportKeyingMaterial(const std::string& label,
bool DtlsTransport::SetupDtls() {
RTC_DCHECK(dtls_role_);
StreamInterfaceChannel* downward =
new StreamInterfaceChannel(ice_transport_.get());
StreamInterfaceChannel* downward = new StreamInterfaceChannel(ice_transport_);
dtls_.reset(rtc::SSLStreamAdapter::Create(downward));
if (!dtls_) {
@ -425,7 +423,7 @@ int DtlsTransport::SendPacket(const char* data,
}
IceTransportInternal* DtlsTransport::ice_transport() {
return ice_transport_.get();
return ice_transport_;
}
bool DtlsTransport::IsDtlsConnected() {
@ -482,7 +480,7 @@ void DtlsTransport::ConnectToIceTransport() {
// impl again
void DtlsTransport::OnWritableState(rtc::PacketTransportInternal* transport) {
RTC_DCHECK_RUN_ON(&thread_checker_);
RTC_DCHECK(transport == ice_transport_.get());
RTC_DCHECK(transport == ice_transport_);
RTC_LOG(LS_VERBOSE) << ToString()
<< ": ice_transport writable state changed to "
<< ice_transport_->writable();
@ -514,7 +512,7 @@ void DtlsTransport::OnWritableState(rtc::PacketTransportInternal* transport) {
void DtlsTransport::OnReceivingState(rtc::PacketTransportInternal* transport) {
RTC_DCHECK_RUN_ON(&thread_checker_);
RTC_DCHECK(transport == ice_transport_.get());
RTC_DCHECK(transport == ice_transport_);
RTC_LOG(LS_VERBOSE) << ToString()
<< ": ice_transport "
"receiving state changed to "
@ -531,7 +529,7 @@ void DtlsTransport::OnReadPacket(rtc::PacketTransportInternal* transport,
const int64_t& packet_time_us,
int flags) {
RTC_DCHECK_RUN_ON(&thread_checker_);
RTC_DCHECK(transport == ice_transport_.get());
RTC_DCHECK(transport == ice_transport_);
RTC_DCHECK(flags == 0);
if (!dtls_active_) {

9
p2p/base/dtls_transport.h
View File

@ -91,14 +91,15 @@ class StreamInterfaceChannel : public rtc::StreamInterface {
// as the constructor.
class DtlsTransport : public DtlsTransportInternal {
public:
// |ice_transport| is the ICE transport this DTLS transport is wrapping.
// |ice_transport| is the ICE transport this DTLS transport is wrapping. It
// must outlive this DTLS transport.
//
// |crypto_options| are the options used for the DTLS handshake. This affects
// whether GCM crypto suites are negotiated.
//
// |event_log| is an optional RtcEventLog for logging state changes. It should
// outlive the DtlsTransport.
explicit DtlsTransport(std::unique_ptr<IceTransportInternal> ice_transport,
explicit DtlsTransport(IceTransportInternal* ice_transport,
const webrtc::CryptoOptions& crypto_options,
webrtc::RtcEventLog* event_log);
@ -222,8 +223,8 @@ class DtlsTransport : public DtlsTransportInternal {
std::string transport_name_;
int component_;
DtlsTransportState dtls_state_ = DTLS_TRANSPORT_NEW;
// Underlying ice_transport, owned by this class.
std::unique_ptr<IceTransportInternal> ice_transport_;
// Underlying ice_transport, not owned by this class.
IceTransportInternal* ice_transport_;
std::unique_ptr<rtc::SSLStreamAdapter> dtls_; // The DTLS stream
StreamInterfaceChannel*
downward_; // Wrapper for ice_transport_, owned by dtls_.

16
p2p/base/dtls_transport_unittest.cc
View File

@ -77,18 +77,18 @@ class DtlsTestClient : public sigslot::has_slots<> {
}
// Set up fake ICE transport and real DTLS transport under test.
void SetupTransports(IceRole role, int async_delay_ms = 0) {
std::unique_ptr<FakeIceTransport> fake_ice_transport;
fake_ice_transport.reset(new FakeIceTransport("fake", 0));
fake_ice_transport->SetAsync(true);
fake_ice_transport->SetAsyncDelay(async_delay_ms);
fake_ice_transport->SetIceRole(role);
fake_ice_transport->SetIceTiebreaker((role == ICEROLE_CONTROLLING) ? 1 : 2);
fake_ice_transport_.reset(new FakeIceTransport("fake", 0));
fake_ice_transport_->SetAsync(true);
fake_ice_transport_->SetAsyncDelay(async_delay_ms);
fake_ice_transport_->SetIceRole(role);
fake_ice_transport_->SetIceTiebreaker((role == ICEROLE_CONTROLLING) ? 1
: 2);
// Hook the raw packets so that we can verify they are encrypted.
fake_ice_transport->SignalReadPacket.connect(
fake_ice_transport_->SignalReadPacket.connect(
this, &DtlsTestClient::OnFakeIceTransportReadPacket);
dtls_transport_ = absl::make_unique<DtlsTransport>(
std::move(fake_ice_transport), webrtc::CryptoOptions(),
fake_ice_transport_.get(), webrtc::CryptoOptions(),
/*event_log=*/nullptr);
dtls_transport_->SetSslMaxProtocolVersion(ssl_max_version_);
// Note: Certificate may be null here if testing passthrough.

6
p2p/base/no_op_dtls_transport.cc
View File

@ -32,10 +32,10 @@
namespace cricket {
NoOpDtlsTransport::NoOpDtlsTransport(
std::unique_ptr<IceTransportInternal> ice_transport,
IceTransportInternal* ice_transport,
const webrtc::CryptoOptions& crypto_options)
: crypto_options_(webrtc::CryptoOptions::NoGcm()),
ice_transport_(std::move(ice_transport)) {
ice_transport_(ice_transport) {
RTC_DCHECK(ice_transport_);
ice_transport_->SignalWritableState.connect(
this, &NoOpDtlsTransport::OnWritableState);
@ -102,7 +102,7 @@ bool NoOpDtlsTransport::SetSslMaxProtocolVersion(
return true;
}
IceTransportInternal* NoOpDtlsTransport::ice_transport() {
return ice_transport_.get();
return ice_transport_;
}
void NoOpDtlsTransport::OnReadyToSend(rtc::PacketTransportInternal* transport) {

4
p2p/base/no_op_dtls_transport.h
View File

@ -54,7 +54,7 @@ constexpr int kNoOpDtlsTransportComponent = -1;
// unprotect RTCP packet".
class NoOpDtlsTransport : public DtlsTransportInternal {
public:
NoOpDtlsTransport(std::unique_ptr<IceTransportInternal> ice_transport,
NoOpDtlsTransport(IceTransportInternal* ice_transport,
const webrtc::CryptoOptions& crypto_options);
~NoOpDtlsTransport() override;
@ -102,7 +102,7 @@ class NoOpDtlsTransport : public DtlsTransportInternal {
rtc::ThreadChecker thread_checker_;
webrtc::CryptoOptions crypto_options_;
std::unique_ptr<IceTransportInternal> ice_transport_;
IceTransportInternal* ice_transport_;
bool is_writable_ = false;
};

2
p2p/base/transport_factory_interface.h
View File

@ -33,7 +33,7 @@ class TransportFactoryInterface {
int component) = 0;
virtual std::unique_ptr<DtlsTransportInternal> CreateDtlsTransport(
std::unique_ptr<IceTransportInternal> ice,
IceTransportInternal* ice,
const webrtc::CryptoOptions& crypto_options) = 0;
};

9
pc/jsep_transport.cc
View File

@ -93,6 +93,8 @@ JsepTransportDescription& JsepTransportDescription::operator=(
JsepTransport::JsepTransport(
const std::string& mid,
const rtc::scoped_refptr<rtc::RTCCertificate>& local_certificate,
std::unique_ptr<cricket::IceTransportInternal> ice_transport,
std::unique_ptr<cricket::IceTransportInternal> rtcp_ice_transport,
std::unique_ptr<webrtc::RtpTransport> unencrypted_rtp_transport,
std::unique_ptr<webrtc::SrtpTransport> sdes_transport,
std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_transport,
@ -102,6 +104,8 @@ JsepTransport::JsepTransport(
: network_thread_(rtc::Thread::Current()),
mid_(mid),
local_certificate_(local_certificate),
ice_transport_(std::move(ice_transport)),
rtcp_ice_transport_(std::move(rtcp_ice_transport)),
unencrypted_rtp_transport_(std::move(unencrypted_rtp_transport)),
sdes_transport_(std::move(sdes_transport)),
dtls_srtp_transport_(std::move(dtls_srtp_transport)),
@ -115,7 +119,12 @@ JsepTransport::JsepTransport(
std::move(rtcp_dtls_transport))
: nullptr),
media_transport_(std::move(media_transport)) {
RTC_DCHECK(ice_transport_);
RTC_DCHECK(rtp_dtls_transport_);
// |rtcp_ice_transport_| must be present iff |rtcp_dtls_transport_| is
// present.
RTC_DCHECK_EQ((rtcp_ice_transport_ != nullptr),
(rtcp_dtls_transport_ != nullptr));
RTC_DCHECK(!datagram_transport() || !media_transport_);
// Verify the "only one out of these three can be set" invariant.
if (unencrypted_rtp_transport_) {

7
pc/jsep_transport.h
View File

@ -86,6 +86,8 @@ class JsepTransport : public sigslot::has_slots<>,
JsepTransport(
const std::string& mid,
const rtc::scoped_refptr<rtc::RTCCertificate>& local_certificate,
std::unique_ptr<cricket::IceTransportInternal> ice_transport,
std::unique_ptr<cricket::IceTransportInternal> rtcp_ice_transport,
std::unique_ptr<webrtc::RtpTransport> unencrypted_rtp_transport,
std::unique_ptr<webrtc::SrtpTransport> sdes_transport,
std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_transport,
@ -310,6 +312,11 @@ class JsepTransport : public sigslot::has_slots<>,
std::unique_ptr<JsepTransportDescription> remote_description_
RTC_GUARDED_BY(network_thread_);
// Ice transport which may be used by any of upper-layer transports (below).
// Owned by JsepTransport and guaranteed to outlive the transports below.
const std::unique_ptr<cricket::IceTransportInternal> ice_transport_;
const std::unique_ptr<cricket::IceTransportInternal> rtcp_ice_transport_;
// To avoid downcasting and make it type safe, keep three unique pointers for
// different SRTP mode and only one of these is non-nullptr.
// Since these are const, the variables don't need locks;

27
pc/jsep_transport_controller.cc
View File

@ -474,7 +474,7 @@ JsepTransportController::CreateIceTransport(const std::string transport_name,
std::unique_ptr<cricket::DtlsTransportInternal>
JsepTransportController::CreateDtlsTransport(
std::unique_ptr<cricket::IceTransportInternal> ice,
cricket::IceTransportInternal* ice,
std::unique_ptr<DatagramTransportInterface> datagram_transport) {
RTC_DCHECK(network_thread_->IsCurrent());
@ -485,7 +485,7 @@ JsepTransportController::CreateDtlsTransport(
// Create DTLS wrapper around DatagramTransportInterface.
dtls = absl::make_unique<cricket::DatagramDtlsAdaptor>(
std::move(ice), std::move(datagram_transport), config_.crypto_options,
ice, std::move(datagram_transport), config_.crypto_options,
config_.event_log);
} else if (config_.media_transport_factory &&
config_.use_media_transport_for_media &&
@ -494,13 +494,13 @@ JsepTransportController::CreateDtlsTransport(
// then we don't need to create DTLS.
// Otherwise, DTLS is still created.
dtls = absl::make_unique<cricket::NoOpDtlsTransport>(
std::move(ice), config_.crypto_options);
ice, config_.crypto_options);
} else if (config_.external_transport_factory) {
dtls = config_.external_transport_factory->CreateDtlsTransport(
std::move(ice), config_.crypto_options);
ice, config_.crypto_options);
} else {
dtls = absl::make_unique<cricket::DtlsTransport>(
std::move(ice), config_.crypto_options, config_.event_log);
ice, config_.crypto_options, config_.event_log);
}
RTC_DCHECK(dtls);
@ -1170,21 +1170,22 @@ RTCError JsepTransportController::MaybeCreateJsepTransport(
}
std::unique_ptr<cricket::DtlsTransportInternal> rtp_dtls_transport =
CreateDtlsTransport(std::move(ice), std::move(datagram_transport));
CreateDtlsTransport(ice.get(), std::move(datagram_transport));
std::unique_ptr<cricket::DtlsTransportInternal> rtcp_dtls_transport;
std::unique_ptr<RtpTransport> unencrypted_rtp_transport;
std::unique_ptr<SrtpTransport> sdes_transport;
std::unique_ptr<DtlsSrtpTransport> dtls_srtp_transport;
std::unique_ptr<cricket::IceTransportInternal> rtcp_ice;
if (config_.rtcp_mux_policy !=
PeerConnectionInterface::kRtcpMuxPolicyRequire &&
content_info.type == cricket::MediaProtocolType::kRtp) {
RTC_DCHECK(media_transport == nullptr);
RTC_DCHECK(datagram_transport == nullptr);
rtcp_dtls_transport = CreateDtlsTransport(
CreateIceTransport(content_info.name, /*rtcp=*/true),
/*datagram_transport=*/nullptr);
rtcp_ice = CreateIceTransport(content_info.name, /*rtcp=*/true);
rtcp_dtls_transport = CreateDtlsTransport(rtcp_ice.get(),
/*datagram_transport=*/nullptr);
}
if (datagram_transport) {
@ -1217,10 +1218,10 @@ RTCError JsepTransportController::MaybeCreateJsepTransport(
std::unique_ptr<cricket::JsepTransport> jsep_transport =
absl::make_unique<cricket::JsepTransport>(
content_info.name, certificate_, std::move(unencrypted_rtp_transport),
std::move(sdes_transport), std::move(dtls_srtp_transport),
std::move(rtp_dtls_transport), std::move(rtcp_dtls_transport),
std::move(media_transport));
content_info.name, certificate_, std::move(ice), std::move(rtcp_ice),
std::move(unencrypted_rtp_transport), std::move(sdes_transport),
std::move(dtls_srtp_transport), std::move(rtp_dtls_transport),
std::move(rtcp_dtls_transport), std::move(media_transport));
jsep_transport->SignalRtcpMuxActive.connect(
this, &JsepTransportController::UpdateAggregateStates_n);

2
pc/jsep_transport_controller.h
View File

@ -345,7 +345,7 @@ class JsepTransportController : public sigslot::has_slots<> {
bool local);
std::unique_ptr<cricket::DtlsTransportInternal> CreateDtlsTransport(
std::unique_ptr<cricket::IceTransportInternal> ice,
cricket::IceTransportInternal* ice,
std::unique_ptr<DatagramTransportInterface> datagram_transport);
std::unique_ptr<cricket::IceTransportInternal> CreateIceTransport(
const std::string transport_name,

7
pc/jsep_transport_controller_unittest.cc
View File

@ -69,11 +69,10 @@ class FakeTransportFactory : public cricket::TransportFactoryInterface {
}
std::unique_ptr<cricket::DtlsTransportInternal> CreateDtlsTransport(
std::unique_ptr<cricket::IceTransportInternal> ice,
cricket::IceTransportInternal* ice,
const webrtc::CryptoOptions& crypto_options) override {
std::unique_ptr<cricket::FakeIceTransport> fake_ice(
static_cast<cricket::FakeIceTransport*>(ice.release()));
return absl::make_unique<FakeDtlsTransport>(std::move(fake_ice));
return absl::make_unique<FakeDtlsTransport>(
static_cast<cricket::FakeIceTransport*>(ice));
}
};

19
pc/jsep_transport_unittest.cc
View File

@ -71,15 +71,15 @@ class JsepTransport2Test : public ::testing::Test, public sigslot::has_slots<> {
SrtpMode srtp_mode) {
auto ice = absl::make_unique<FakeIceTransport>(kTransportName,
ICE_CANDIDATE_COMPONENT_RTP);
auto rtp_dtls_transport =
absl::make_unique<FakeDtlsTransport>(std::move(ice));
auto rtp_dtls_transport = absl::make_unique<FakeDtlsTransport>(ice.get());
std::unique_ptr<FakeIceTransport> rtcp_ice;
std::unique_ptr<FakeDtlsTransport> rtcp_dtls_transport;
if (!rtcp_mux_enabled) {
ice = absl::make_unique<FakeIceTransport>(kTransportName,
ICE_CANDIDATE_COMPONENT_RTCP);
rtcp_ice = absl::make_unique<FakeIceTransport>(
kTransportName, ICE_CANDIDATE_COMPONENT_RTCP);
rtcp_dtls_transport =
absl::make_unique<FakeDtlsTransport>(std::move(ice));
absl::make_unique<FakeDtlsTransport>(rtcp_ice.get());
}
std::unique_ptr<webrtc::RtpTransport> unencrypted_rtp_transport;
@ -105,10 +105,11 @@ class JsepTransport2Test : public ::testing::Test, public sigslot::has_slots<> {
// more logic that require unit tests. Note that creation of media_transport
// is covered in jseptransportcontroller_unittest.
auto jsep_transport = absl::make_unique<JsepTransport>(
kTransportName, /*local_certificate=*/nullptr,
std::move(unencrypted_rtp_transport), std::move(sdes_transport),
std::move(dtls_srtp_transport), std::move(rtp_dtls_transport),
std::move(rtcp_dtls_transport), /*media_transport=*/nullptr);
kTransportName, /*local_certificate=*/nullptr, std::move(ice),
std::move(rtcp_ice), std::move(unencrypted_rtp_transport),
std::move(sdes_transport), std::move(dtls_srtp_transport),
std::move(rtp_dtls_transport), std::move(rtcp_dtls_transport),
/*media_transport=*/nullptr);
signal_rtcp_mux_active_received_ = false;
jsep_transport->SignalRtcpMuxActive.connect(