Move ownership of ICE from DtlsTransport to JsepTransport.
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}
diff --git a/p2p/base/datagram_dtls_adaptor.cc b/p2p/base/datagram_dtls_adaptor.cc
index 344f1c1..f62cfbf 100644
--- a/p2p/base/datagram_dtls_adaptor.cc
+++ b/p2p/base/datagram_dtls_adaptor.cc
@@ -44,12 +44,12 @@
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 @@
// 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 @@
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 @@
}
IceTransportInternal* DatagramDtlsAdaptor::ice_transport() {
- return ice_transport_.get();
+ return ice_transport_;
}
webrtc::DatagramTransportInterface* DatagramDtlsAdaptor::datagram_transport() {
@@ -264,7 +263,7 @@
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::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();
diff --git a/p2p/base/datagram_dtls_adaptor.h b/p2p/base/datagram_dtls_adaptor.h
index 2f6fdc1..c1014cf 100644
--- a/p2p/base/datagram_dtls_adaptor.h
+++ b/p2p/base/datagram_dtls_adaptor.h
@@ -43,7 +43,7 @@
// 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 @@
rtc::ThreadChecker thread_checker_;
webrtc::CryptoOptions crypto_options_;
- std::unique_ptr<IceTransportInternal> ice_transport_;
+ IceTransportInternal* ice_transport_;
std::unique_ptr<webrtc::DatagramTransportInterface> datagram_transport_;
diff --git a/p2p/base/dtls_transport.cc b/p2p/base/dtls_transport.cc
index 46f0f99..8a8fca2 100644
--- a/p2p/base/dtls_transport.cc
+++ b/p2p/base/dtls_transport.cc
@@ -118,13 +118,12 @@
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::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 @@
}
IceTransportInternal* DtlsTransport::ice_transport() {
- return ice_transport_.get();
+ return ice_transport_;
}
bool DtlsTransport::IsDtlsConnected() {
@@ -482,7 +480,7 @@
// 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::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 @@
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_) {
diff --git a/p2p/base/dtls_transport.h b/p2p/base/dtls_transport.h
index ceef1dc..bf3e056 100644
--- a/p2p/base/dtls_transport.h
+++ b/p2p/base/dtls_transport.h
@@ -91,14 +91,15 @@
// 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 @@
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_.
diff --git a/p2p/base/dtls_transport_unittest.cc b/p2p/base/dtls_transport_unittest.cc
index b71d65e..3c1cd2f 100644
--- a/p2p/base/dtls_transport_unittest.cc
+++ b/p2p/base/dtls_transport_unittest.cc
@@ -77,18 +77,18 @@
}
// 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.
diff --git a/p2p/base/no_op_dtls_transport.cc b/p2p/base/no_op_dtls_transport.cc
index aef3fae..dda668b 100644
--- a/p2p/base/no_op_dtls_transport.cc
+++ b/p2p/base/no_op_dtls_transport.cc
@@ -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 @@
return true;
}
IceTransportInternal* NoOpDtlsTransport::ice_transport() {
- return ice_transport_.get();
+ return ice_transport_;
}
void NoOpDtlsTransport::OnReadyToSend(rtc::PacketTransportInternal* transport) {
diff --git a/p2p/base/no_op_dtls_transport.h b/p2p/base/no_op_dtls_transport.h
index 3b6252b..7111b29 100644
--- a/p2p/base/no_op_dtls_transport.h
+++ b/p2p/base/no_op_dtls_transport.h
@@ -54,7 +54,7 @@
// 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 @@
rtc::ThreadChecker thread_checker_;
webrtc::CryptoOptions crypto_options_;
- std::unique_ptr<IceTransportInternal> ice_transport_;
+ IceTransportInternal* ice_transport_;
bool is_writable_ = false;
};
diff --git a/p2p/base/transport_factory_interface.h b/p2p/base/transport_factory_interface.h
index 6da5534..e7eead7 100644
--- a/p2p/base/transport_factory_interface.h
+++ b/p2p/base/transport_factory_interface.h
@@ -33,7 +33,7 @@
int component) = 0;
virtual std::unique_ptr<DtlsTransportInternal> CreateDtlsTransport(
- std::unique_ptr<IceTransportInternal> ice,
+ IceTransportInternal* ice,
const webrtc::CryptoOptions& crypto_options) = 0;
};
diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc
index 26311d1..3098681 100644
--- a/pc/jsep_transport.cc
+++ b/pc/jsep_transport.cc
@@ -93,6 +93,8 @@
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 @@
: 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 @@
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_) {
diff --git a/pc/jsep_transport.h b/pc/jsep_transport.h
index fce21be..9add2e5 100644
--- a/pc/jsep_transport.h
+++ b/pc/jsep_transport.h
@@ -86,6 +86,8 @@
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 @@
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;
diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc
index 55f1d1c..4f14e00 100644
--- a/pc/jsep_transport_controller.cc
+++ b/pc/jsep_transport_controller.cc
@@ -474,7 +474,7 @@
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 @@
// 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 @@
// 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 @@
}
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 @@
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);
diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h
index a79817c..ffc9515 100644
--- a/pc/jsep_transport_controller.h
+++ b/pc/jsep_transport_controller.h
@@ -345,7 +345,7 @@
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,
diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc
index 8081df9..d796b23 100644
--- a/pc/jsep_transport_controller_unittest.cc
+++ b/pc/jsep_transport_controller_unittest.cc
@@ -69,11 +69,10 @@
}
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));
}
};
diff --git a/pc/jsep_transport_unittest.cc b/pc/jsep_transport_unittest.cc
index ac17d6f..ec64379 100644
--- a/pc/jsep_transport_unittest.cc
+++ b/pc/jsep_transport_unittest.cc
@@ -71,15 +71,15 @@
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 @@
// 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(
