diff --git a/api/BUILD.gn b/api/BUILD.gn index 4274014e96..4f729d5c77 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -160,6 +160,7 @@ rtc_library("libjingle_peerconnection_api") { ] deps = [ ":array_view", + ":async_dns_resolver", ":audio_options_api", ":callfactory_api", ":fec_controller_api", @@ -249,11 +250,11 @@ rtc_library("rtc_error") { rtc_source_set("packet_socket_factory") { visibility = [ "*" ] sources = [ - "async_dns_resolver.h", "async_resolver_factory.h", "packet_socket_factory.h", ] deps = [ + ":async_dns_resolver", "../rtc_base:async_resolver_interface", "../rtc_base:rtc_base", "../rtc_base:socket_address", @@ -261,6 +262,14 @@ rtc_source_set("packet_socket_factory") { ] } +rtc_source_set("async_dns_resolver") { + sources = [ "async_dns_resolver.h" ] + deps = [ + "../rtc_base:socket_address", + "../rtc_base/system:rtc_export", + ] +} + rtc_source_set("scoped_refptr") { visibility = [ "*" ] sources = [ "scoped_refptr.h" ] @@ -940,6 +949,15 @@ if (rtc_include_tests) { ] } + rtc_source_set("mock_async_dns_resolver") { + testonly = true + sources = [ "test/mock_async_dns_resolver.h" ] + deps = [ + ":async_dns_resolver", + "../test:test_support", + ] + } + rtc_source_set("mock_rtp") { visibility = [ "*" ] testonly = true @@ -1092,6 +1110,7 @@ if (rtc_include_tests) { ":dummy_peer_connection", ":fake_frame_decryptor", ":fake_frame_encryptor", + ":mock_async_dns_resolver", ":mock_audio_mixer", ":mock_data_channel", ":mock_frame_decryptor", diff --git a/api/async_dns_resolver.h b/api/async_dns_resolver.h index 800ffa613d..eabb41c11f 100644 --- a/api/async_dns_resolver.h +++ b/api/async_dns_resolver.h @@ -11,6 +11,7 @@ #ifndef API_ASYNC_DNS_RESOLVER_H_ #define API_ASYNC_DNS_RESOLVER_H_ +#include #include #include "rtc_base/socket_address.h" diff --git a/api/ice_transport_factory.cc b/api/ice_transport_factory.cc index c9a0a6a4d4..d507812ab7 100644 --- a/api/ice_transport_factory.cc +++ b/api/ice_transport_factory.cc @@ -58,10 +58,18 @@ rtc::scoped_refptr CreateIceTransport( rtc::scoped_refptr CreateIceTransport( IceTransportInit init) { - return new rtc::RefCountedObject( - std::make_unique( - "", cricket::ICE_CANDIDATE_COMPONENT_RTP, init.port_allocator(), - init.async_resolver_factory(), init.event_log())); + if (init.async_resolver_factory()) { + // Backwards compatibility mode + return new rtc::RefCountedObject( + std::make_unique( + "", cricket::ICE_CANDIDATE_COMPONENT_RTP, init.port_allocator(), + init.async_resolver_factory(), init.event_log())); + } else { + return new rtc::RefCountedObject( + cricket::P2PTransportChannel::Create( + "", cricket::ICE_CANDIDATE_COMPONENT_RTP, init.port_allocator(), + init.async_dns_resolver_factory(), init.event_log())); + } } } // namespace webrtc diff --git a/api/ice_transport_interface.h b/api/ice_transport_interface.h index d2f1edc012..a3b364c87a 100644 --- a/api/ice_transport_interface.h +++ b/api/ice_transport_interface.h @@ -13,6 +13,7 @@ #include +#include "api/async_dns_resolver.h" #include "api/async_resolver_factory.h" #include "api/rtc_error.h" #include "api/rtc_event_log/rtc_event_log.h" @@ -52,11 +53,21 @@ struct IceTransportInit final { port_allocator_ = port_allocator; } + AsyncDnsResolverFactoryInterface* async_dns_resolver_factory() { + return async_dns_resolver_factory_; + } + void set_async_dns_resolver_factory( + AsyncDnsResolverFactoryInterface* async_dns_resolver_factory) { + RTC_DCHECK(!async_resolver_factory_); + async_dns_resolver_factory_ = async_dns_resolver_factory; + } AsyncResolverFactory* async_resolver_factory() { return async_resolver_factory_; } + ABSL_DEPRECATED("bugs.webrtc.org/12598") void set_async_resolver_factory( AsyncResolverFactory* async_resolver_factory) { + RTC_DCHECK(!async_dns_resolver_factory_); async_resolver_factory_ = async_resolver_factory; } @@ -65,8 +76,11 @@ struct IceTransportInit final { private: cricket::PortAllocator* port_allocator_ = nullptr; + AsyncDnsResolverFactoryInterface* async_dns_resolver_factory_ = nullptr; + // For backwards compatibility. Only one resolver factory can be set. AsyncResolverFactory* async_resolver_factory_ = nullptr; RtcEventLog* event_log_ = nullptr; + // TODO(https://crbug.com/webrtc/12657): Redesign to have const members. }; // TODO(qingsi): The factory interface is defined in this file instead of its diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index cc655926f3..17d9004eb2 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -74,6 +74,7 @@ #include #include "api/adaptation/resource.h" +#include "api/async_dns_resolver.h" #include "api/async_resolver_factory.h" #include "api/audio/audio_mixer.h" #include "api/audio_codecs/audio_decoder_factory.h" @@ -1322,6 +1323,10 @@ struct RTC_EXPORT PeerConnectionDependencies final { // packet_socket_factory, not both. std::unique_ptr allocator; std::unique_ptr packet_socket_factory; + // Factory for creating resolvers that look up hostnames in DNS + std::unique_ptr + async_dns_resolver_factory; + // Deprecated - use async_dns_resolver_factory std::unique_ptr async_resolver_factory; std::unique_ptr ice_transport_factory; std::unique_ptr cert_generator; diff --git a/api/test/compile_all_headers.cc b/api/test/compile_all_headers.cc index 6f06742995..5ecdcc1eb8 100644 --- a/api/test/compile_all_headers.cc +++ b/api/test/compile_all_headers.cc @@ -30,6 +30,7 @@ #include "api/test/dummy_peer_connection.h" #include "api/test/fake_frame_decryptor.h" #include "api/test/fake_frame_encryptor.h" +#include "api/test/mock_async_dns_resolver.h" #include "api/test/mock_audio_mixer.h" #include "api/test/mock_data_channel.h" #include "api/test/mock_frame_decryptor.h" diff --git a/api/test/mock_async_dns_resolver.h b/api/test/mock_async_dns_resolver.h new file mode 100644 index 0000000000..e863cac6e6 --- /dev/null +++ b/api/test/mock_async_dns_resolver.h @@ -0,0 +1,54 @@ +/* + * Copyright 2021 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#ifndef API_TEST_MOCK_ASYNC_DNS_RESOLVER_H_ +#define API_TEST_MOCK_ASYNC_DNS_RESOLVER_H_ + +#include +#include + +#include "api/async_dns_resolver.h" +#include "test/gmock.h" + +namespace webrtc { + +class MockAsyncDnsResolverResult : public AsyncDnsResolverResult { + public: + MOCK_METHOD(bool, + GetResolvedAddress, + (int, rtc::SocketAddress*), + (const override)); + MOCK_METHOD(int, GetError, (), (const override)); +}; + +class MockAsyncDnsResolver : public AsyncDnsResolverInterface { + public: + MOCK_METHOD(void, + Start, + (const rtc::SocketAddress&, std::function), + (override)); + MOCK_METHOD(AsyncDnsResolverResult&, result, (), (const override)); +}; + +class MockAsyncDnsResolverFactory : public AsyncDnsResolverFactoryInterface { + public: + MOCK_METHOD(std::unique_ptr, + CreateAndResolve, + (const rtc::SocketAddress&, std::function), + (override)); + MOCK_METHOD(std::unique_ptr, + Create, + (), + (override)); +}; + +} // namespace webrtc + +#endif // API_TEST_MOCK_ASYNC_DNS_RESOLVER_H_ diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index e2ba8cafa9..84832a3bbf 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn @@ -86,6 +86,8 @@ rtc_library("rtc_p2p") { ] deps = [ + "../api:array_view", + "../api:async_dns_resolver", "../api:libjingle_peerconnection_api", "../api:packet_socket_factory", "../api:rtc_error", @@ -93,6 +95,7 @@ rtc_library("rtc_p2p") { "../api:sequence_checker", "../api/crypto:options", "../api/rtc_event_log", + "../api/task_queue", "../api/transport:enums", "../api/transport:stun_types", "../logging:ice_log", @@ -235,6 +238,7 @@ if (rtc_include_tests) { ":p2p_test_utils", ":rtc_p2p", "../api:libjingle_peerconnection_api", + "../api:mock_async_dns_resolver", "../api:packet_socket_factory", "../api:scoped_refptr", "../api/transport:stun_types", @@ -256,6 +260,7 @@ if (rtc_include_tests) { "../rtc_base/third_party/sigslot", "../system_wrappers:metrics", "../test:field_trial", + "../test:rtc_expect_death", "../test:test_support", "//testing/gtest", ] diff --git a/p2p/base/basic_async_resolver_factory.cc b/p2p/base/basic_async_resolver_factory.cc index 67dd18f5f2..7f26a981ee 100644 --- a/p2p/base/basic_async_resolver_factory.cc +++ b/p2p/base/basic_async_resolver_factory.cc @@ -16,6 +16,7 @@ #include "absl/memory/memory.h" #include "api/async_dns_resolver.h" #include "rtc_base/async_resolver.h" +#include "rtc_base/logging.h" namespace webrtc { @@ -46,11 +47,19 @@ class WrappingAsyncDnsResolver : public AsyncDnsResolverInterface, explicit WrappingAsyncDnsResolver(rtc::AsyncResolverInterface* wrapped) : wrapped_(absl::WrapUnique(wrapped)), result_(this) {} - ~WrappingAsyncDnsResolver() override { wrapped_.release()->Destroy(false); } + ~WrappingAsyncDnsResolver() override { + // Workaround to get around the fact that sigslot-using objects can't be + // destroyed from within their callback: Alert class users early. + // TODO(bugs.webrtc.org/12651): Delete this class once the sigslot users are + // gone. + RTC_CHECK(!within_resolve_result_); + wrapped_.release()->Destroy(false); + } void Start(const rtc::SocketAddress& addr, std::function callback) override { - RTC_DCHECK(state_ == State::kNotStarted); + RTC_DCHECK_RUN_ON(&sequence_checker_); + RTC_DCHECK_EQ(State::kNotStarted, state_); state_ = State::kStarted; callback_ = callback; wrapped_->SignalDone.connect(this, @@ -59,27 +68,39 @@ class WrappingAsyncDnsResolver : public AsyncDnsResolverInterface, } const AsyncDnsResolverResult& result() const override { - RTC_DCHECK(state_ == State::kResolved); + RTC_DCHECK_RUN_ON(&sequence_checker_); + RTC_DCHECK_EQ(State::kResolved, state_); return result_; } - // For use by WrappingAsyncDnsResolverResult - rtc::AsyncResolverInterface* wrapped() const { return wrapped_.get(); } - private: enum class State { kNotStarted, kStarted, kResolved }; + friend class WrappingAsyncDnsResolverResult; + // For use by WrappingAsyncDnsResolverResult + rtc::AsyncResolverInterface* wrapped() const { + RTC_DCHECK_RUN_ON(&sequence_checker_); + return wrapped_.get(); + } + void OnResolveResult(rtc::AsyncResolverInterface* ref) { + RTC_DCHECK_RUN_ON(&sequence_checker_); RTC_DCHECK(state_ == State::kStarted); RTC_DCHECK_EQ(ref, wrapped_.get()); state_ = State::kResolved; + within_resolve_result_ = true; callback_(); + within_resolve_result_ = false; } - std::function callback_; - std::unique_ptr wrapped_; - State state_ = State::kNotStarted; - WrappingAsyncDnsResolverResult result_; + // The class variables need to be accessed on a single thread. + SequenceChecker sequence_checker_; + std::function callback_ RTC_GUARDED_BY(sequence_checker_); + std::unique_ptr wrapped_ + RTC_GUARDED_BY(sequence_checker_); + State state_ RTC_GUARDED_BY(sequence_checker_) = State::kNotStarted; + WrappingAsyncDnsResolverResult result_ RTC_GUARDED_BY(sequence_checker_); + bool within_resolve_result_ RTC_GUARDED_BY(sequence_checker_) = false; }; bool WrappingAsyncDnsResolverResult::GetResolvedAddress( diff --git a/p2p/base/basic_async_resolver_factory.h b/p2p/base/basic_async_resolver_factory.h index 90427444be..c988913068 100644 --- a/p2p/base/basic_async_resolver_factory.h +++ b/p2p/base/basic_async_resolver_factory.h @@ -32,9 +32,15 @@ class BasicAsyncResolverFactory final : public AsyncResolverFactory { class WrappingAsyncDnsResolverFactory final : public AsyncDnsResolverFactoryInterface { public: - WrappingAsyncDnsResolverFactory( + explicit WrappingAsyncDnsResolverFactory( std::unique_ptr wrapped_factory) - : wrapped_factory_(std::move(wrapped_factory)) {} + : owned_factory_(std::move(wrapped_factory)), + wrapped_factory_(owned_factory_.get()) {} + + explicit WrappingAsyncDnsResolverFactory( + AsyncResolverFactory* non_owned_factory) + : wrapped_factory_(non_owned_factory) {} + std::unique_ptr CreateAndResolve( const rtc::SocketAddress& addr, std::function callback) override; @@ -42,7 +48,8 @@ class WrappingAsyncDnsResolverFactory final std::unique_ptr Create() override; private: - const std::unique_ptr wrapped_factory_; + const std::unique_ptr owned_factory_; + AsyncResolverFactory* const wrapped_factory_; }; } // namespace webrtc diff --git a/p2p/base/basic_async_resolver_factory_unittest.cc b/p2p/base/basic_async_resolver_factory_unittest.cc index 03fb2df8b0..ec13601643 100644 --- a/p2p/base/basic_async_resolver_factory_unittest.cc +++ b/p2p/base/basic_async_resolver_factory_unittest.cc @@ -10,10 +10,15 @@ #include "p2p/base/basic_async_resolver_factory.h" +#include "api/test/mock_async_dns_resolver.h" +#include "p2p/base/mock_async_resolver.h" +#include "rtc_base/async_resolver.h" #include "rtc_base/gunit.h" #include "rtc_base/socket_address.h" #include "rtc_base/third_party/sigslot/sigslot.h" +#include "test/gmock.h" #include "test/gtest.h" +#include "test/testsupport/rtc_expect_death.h" namespace webrtc { @@ -47,7 +52,7 @@ TEST_F(BasicAsyncResolverFactoryTest, TestCreate) { TestCreate(); } -TEST(WrappingAsyncDnsResolverFactoryTest, TestCreate) { +TEST(WrappingAsyncDnsResolverFactoryTest, TestCreateAndResolve) { WrappingAsyncDnsResolverFactory factory( std::make_unique()); @@ -61,4 +66,24 @@ TEST(WrappingAsyncDnsResolverFactoryTest, TestCreate) { resolver.reset(); } +TEST(WrappingAsyncDnsResolverFactoryTest, WrapOtherResolver) { + BasicAsyncResolverFactory non_owned_factory; + WrappingAsyncDnsResolverFactory factory(&non_owned_factory); + std::unique_ptr resolver(factory.Create()); + ASSERT_TRUE(resolver); + + bool address_resolved = false; + rtc::SocketAddress address("", 0); + resolver->Start(address, [&address_resolved]() { address_resolved = true; }); + ASSERT_TRUE_WAIT(address_resolved, 10000 /*ms*/); + resolver.reset(); +} + +void CallResolver(WrappingAsyncDnsResolverFactory& factory) { + rtc::SocketAddress address("", 0); + std::unique_ptr resolver(factory.Create()); + resolver->Start(address, [&resolver]() { resolver.reset(); }); + WAIT(!resolver.get(), 10000 /*ms*/); +} + } // namespace webrtc diff --git a/p2p/base/default_ice_transport_factory.cc b/p2p/base/default_ice_transport_factory.cc index f4b182efdf..7d2fdb8fb4 100644 --- a/p2p/base/default_ice_transport_factory.cc +++ b/p2p/base/default_ice_transport_factory.cc @@ -45,9 +45,9 @@ DefaultIceTransportFactory::CreateIceTransport( IceTransportInit init) { BasicIceControllerFactory factory; return new rtc::RefCountedObject( - std::make_unique( + cricket::P2PTransportChannel::Create( transport_name, component, init.port_allocator(), - init.async_resolver_factory(), init.event_log(), &factory)); + init.async_dns_resolver_factory(), init.event_log(), &factory)); } } // namespace webrtc diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 19e172a8e3..eff79ab9be 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -10,27 +10,38 @@ #include "p2p/base/p2p_transport_channel.h" -#include +#include +#include + +#include +#include #include #include #include #include "absl/algorithm/container.h" +#include "absl/memory/memory.h" #include "absl/strings/match.h" +#include "api/async_dns_resolver.h" #include "api/candidate.h" +#include "api/task_queue/queued_task.h" #include "logging/rtc_event_log/ice_logger.h" +#include "p2p/base/basic_async_resolver_factory.h" #include "p2p/base/basic_ice_controller.h" -#include "p2p/base/candidate_pair_interface.h" #include "p2p/base/connection.h" +#include "p2p/base/connection_info.h" #include "p2p/base/port.h" #include "rtc_base/checks.h" #include "rtc_base/crc32.h" #include "rtc_base/experiments/struct_parameters_parser.h" +#include "rtc_base/ip_address.h" #include "rtc_base/logging.h" #include "rtc_base/net_helper.h" -#include "rtc_base/net_helpers.h" +#include "rtc_base/network.h" +#include "rtc_base/network_constants.h" #include "rtc_base/string_encode.h" #include "rtc_base/task_utils/to_queued_task.h" +#include "rtc_base/third_party/sigslot/sigslot.h" #include "rtc_base/time_utils.h" #include "system_wrappers/include/field_trial.h" #include "system_wrappers/include/metrics.h" @@ -122,26 +133,50 @@ bool IceCredentialsChanged(const std::string& old_ufrag, return (old_ufrag != new_ufrag) || (old_pwd != new_pwd); } +// static +std::unique_ptr P2PTransportChannel::Create( + const std::string& transport_name, + int component, + PortAllocator* allocator, + webrtc::AsyncDnsResolverFactoryInterface* async_dns_resolver_factory, + webrtc::RtcEventLog* event_log, + IceControllerFactoryInterface* ice_controller_factory) { + return absl::WrapUnique(new P2PTransportChannel( + transport_name, component, allocator, async_dns_resolver_factory, + /* owned_dns_resolver_factory= */ nullptr, event_log, + ice_controller_factory)); +} + P2PTransportChannel::P2PTransportChannel(const std::string& transport_name, int component, PortAllocator* allocator) : P2PTransportChannel(transport_name, component, allocator, - nullptr, - nullptr) {} + /* async_dns_resolver_factory= */ nullptr, + /* owned_dns_resolver_factory= */ nullptr, + /* event_log= */ nullptr, + /* ice_controller_factory= */ nullptr) {} +// Private constructor, called from Create() P2PTransportChannel::P2PTransportChannel( const std::string& transport_name, int component, PortAllocator* allocator, - webrtc::AsyncResolverFactory* async_resolver_factory, + webrtc::AsyncDnsResolverFactoryInterface* async_dns_resolver_factory, + std::unique_ptr + owned_dns_resolver_factory, webrtc::RtcEventLog* event_log, IceControllerFactoryInterface* ice_controller_factory) : transport_name_(transport_name), component_(component), allocator_(allocator), - async_resolver_factory_(async_resolver_factory), + // If owned_dns_resolver_factory is given, async_dns_resolver_factory is + // ignored. + async_dns_resolver_factory_(owned_dns_resolver_factory + ? owned_dns_resolver_factory.get() + : async_dns_resolver_factory), + owned_dns_resolver_factory_(std::move(owned_dns_resolver_factory)), network_thread_(rtc::Thread::Current()), incoming_only_(false), error_(0), @@ -192,15 +227,31 @@ P2PTransportChannel::P2PTransportChannel( } } +// Public constructor, exposed for backwards compatibility. +// Deprecated. +P2PTransportChannel::P2PTransportChannel( + const std::string& transport_name, + int component, + PortAllocator* allocator, + webrtc::AsyncResolverFactory* async_resolver_factory, + webrtc::RtcEventLog* event_log, + IceControllerFactoryInterface* ice_controller_factory) + : P2PTransportChannel( + transport_name, + component, + allocator, + nullptr, + std::make_unique( + async_resolver_factory), + event_log, + ice_controller_factory) {} + P2PTransportChannel::~P2PTransportChannel() { RTC_DCHECK_RUN_ON(network_thread_); std::vector copy(connections().begin(), connections().end()); for (Connection* con : copy) { con->Destroy(); } - for (auto& p : resolvers_) { - p.resolver_->Destroy(false); - } resolvers_.clear(); } @@ -1164,16 +1215,17 @@ void P2PTransportChannel::OnNominated(Connection* conn) { void P2PTransportChannel::ResolveHostnameCandidate(const Candidate& candidate) { RTC_DCHECK_RUN_ON(network_thread_); - if (!async_resolver_factory_) { + if (!async_dns_resolver_factory_) { RTC_LOG(LS_WARNING) << "Dropping ICE candidate with hostname address " "(no AsyncResolverFactory)"; return; } - rtc::AsyncResolverInterface* resolver = async_resolver_factory_->Create(); - resolvers_.emplace_back(candidate, resolver); - resolver->SignalDone.connect(this, &P2PTransportChannel::OnCandidateResolved); - resolver->Start(candidate.address()); + auto resolver = async_dns_resolver_factory_->Create(); + auto resptr = resolver.get(); + resolvers_.emplace_back(candidate, std::move(resolver)); + resptr->Start(candidate.address(), + [this, resptr]() { OnCandidateResolved(resptr); }); RTC_LOG(LS_INFO) << "Asynchronously resolving ICE candidate hostname " << candidate.address().HostAsSensitiveURIString(); } @@ -1228,38 +1280,44 @@ void P2PTransportChannel::AddRemoteCandidate(const Candidate& candidate) { P2PTransportChannel::CandidateAndResolver::CandidateAndResolver( const Candidate& candidate, - rtc::AsyncResolverInterface* resolver) - : candidate_(candidate), resolver_(resolver) {} + std::unique_ptr&& resolver) + : candidate_(candidate), resolver_(std::move(resolver)) {} P2PTransportChannel::CandidateAndResolver::~CandidateAndResolver() {} void P2PTransportChannel::OnCandidateResolved( - rtc::AsyncResolverInterface* resolver) { + webrtc::AsyncDnsResolverInterface* resolver) { RTC_DCHECK_RUN_ON(network_thread_); auto p = absl::c_find_if(resolvers_, [resolver](const CandidateAndResolver& cr) { - return cr.resolver_ == resolver; + return cr.resolver_.get() == resolver; }); if (p == resolvers_.end()) { - RTC_LOG(LS_ERROR) << "Unexpected AsyncResolver signal"; + RTC_LOG(LS_ERROR) << "Unexpected AsyncDnsResolver return"; RTC_NOTREACHED(); return; } Candidate candidate = p->candidate_; - resolvers_.erase(p); - AddRemoteCandidateWithResolver(candidate, resolver); + AddRemoteCandidateWithResult(candidate, resolver->result()); + // Now we can delete the resolver. + // TODO(bugs.webrtc.org/12651): Replace the stuff below with + // resolvers_.erase(p); + std::unique_ptr to_delete = + std::move(p->resolver_); + // Delay the actual deletion of the resolver until the lambda executes. network_thread_->PostTask( - ToQueuedTask([resolver]() { resolver->Destroy(false); })); + ToQueuedTask([delete_this = std::move(to_delete)] {})); + resolvers_.erase(p); } -void P2PTransportChannel::AddRemoteCandidateWithResolver( +void P2PTransportChannel::AddRemoteCandidateWithResult( Candidate candidate, - rtc::AsyncResolverInterface* resolver) { + const webrtc::AsyncDnsResolverResult& result) { RTC_DCHECK_RUN_ON(network_thread_); - if (resolver->GetError()) { + if (result.GetError()) { RTC_LOG(LS_WARNING) << "Failed to resolve ICE candidate hostname " << candidate.address().HostAsSensitiveURIString() - << " with error " << resolver->GetError(); + << " with error " << result.GetError(); return; } @@ -1267,9 +1325,8 @@ void P2PTransportChannel::AddRemoteCandidateWithResolver( // Prefer IPv6 to IPv4 if we have it (see RFC 5245 Section 15.1). // TODO(zstein): This won't work if we only have IPv4 locally but receive an // AAAA DNS record. - bool have_address = - resolver->GetResolvedAddress(AF_INET6, &resolved_address) || - resolver->GetResolvedAddress(AF_INET, &resolved_address); + bool have_address = result.GetResolvedAddress(AF_INET6, &resolved_address) || + result.GetResolvedAddress(AF_INET, &resolved_address); if (!have_address) { RTC_LOG(LS_INFO) << "ICE candidate hostname " << candidate.address().HostAsSensitiveURIString() diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h index 1e93942fe9..462aa105b1 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -20,6 +20,9 @@ #ifndef P2P_BASE_P2P_TRANSPORT_CHANNEL_H_ #define P2P_BASE_P2P_TRANSPORT_CHANNEL_H_ +#include +#include + #include #include #include @@ -27,26 +30,43 @@ #include #include +#include "absl/base/attributes.h" +#include "absl/types/optional.h" +#include "api/array_view.h" +#include "api/async_dns_resolver.h" #include "api/async_resolver_factory.h" #include "api/candidate.h" #include "api/rtc_error.h" +#include "api/sequence_checker.h" +#include "api/transport/enums.h" +#include "api/transport/stun.h" #include "logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.h" #include "logging/rtc_event_log/ice_logger.h" #include "p2p/base/candidate_pair_interface.h" +#include "p2p/base/connection.h" #include "p2p/base/ice_controller_factory_interface.h" #include "p2p/base/ice_controller_interface.h" #include "p2p/base/ice_transport_internal.h" #include "p2p/base/p2p_constants.h" #include "p2p/base/p2p_transport_channel_ice_field_trials.h" +#include "p2p/base/port.h" #include "p2p/base/port_allocator.h" #include "p2p/base/port_interface.h" #include "p2p/base/regathering_controller.h" +#include "p2p/base/transport_description.h" #include "rtc_base/async_packet_socket.h" +#include "rtc_base/checks.h" #include "rtc_base/constructor_magic.h" +#include "rtc_base/dscp.h" +#include "rtc_base/network/sent_packet.h" +#include "rtc_base/network_route.h" +#include "rtc_base/socket.h" +#include "rtc_base/socket_address.h" #include "rtc_base/strings/string_builder.h" #include "rtc_base/system/rtc_export.h" #include "rtc_base/task_utils/pending_task_safety_flag.h" #include "rtc_base/third_party/sigslot/sigslot.h" +#include "rtc_base/thread.h" #include "rtc_base/thread_annotations.h" namespace webrtc { @@ -82,11 +102,19 @@ class RemoteCandidate : public Candidate { // two P2P clients connected to each other. class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { public: + static std::unique_ptr Create( + const std::string& transport_name, + int component, + PortAllocator* allocator, + webrtc::AsyncDnsResolverFactoryInterface* async_dns_resolver_factory, + webrtc::RtcEventLog* event_log = nullptr, + IceControllerFactoryInterface* ice_controller_factory = nullptr); // For testing only. - // TODO(zstein): Remove once AsyncResolverFactory is required. + // TODO(zstein): Remove once AsyncDnsResolverFactory is required. P2PTransportChannel(const std::string& transport_name, int component, PortAllocator* allocator); + ABSL_DEPRECATED("bugs.webrtc.org/12598") P2PTransportChannel( const std::string& transport_name, int component, @@ -209,6 +237,18 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { } private: + P2PTransportChannel( + const std::string& transport_name, + int component, + PortAllocator* allocator, + // DNS resolver factory + webrtc::AsyncDnsResolverFactoryInterface* async_dns_resolver_factory, + // If the P2PTransportChannel has to delete the DNS resolver factory + // on release, this pointer is set. + std::unique_ptr + owned_dns_resolver_factory, + webrtc::RtcEventLog* event_log = nullptr, + IceControllerFactoryInterface* ice_controller_factory = nullptr); bool IsGettingPorts() { RTC_DCHECK_RUN_ON(network_thread_); return allocator_session()->IsGettingPorts(); @@ -363,8 +403,10 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { std::string transport_name_ RTC_GUARDED_BY(network_thread_); int component_ RTC_GUARDED_BY(network_thread_); PortAllocator* allocator_ RTC_GUARDED_BY(network_thread_); - webrtc::AsyncResolverFactory* async_resolver_factory_ + webrtc::AsyncDnsResolverFactoryInterface* const async_dns_resolver_factory_ RTC_GUARDED_BY(network_thread_); + const std::unique_ptr + owned_dns_resolver_factory_; rtc::Thread* const network_thread_; bool incoming_only_ RTC_GUARDED_BY(network_thread_); int error_ RTC_GUARDED_BY(network_thread_); @@ -426,17 +468,23 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { RTC_GUARDED_BY(network_thread_); struct CandidateAndResolver final { - CandidateAndResolver(const Candidate& candidate, - rtc::AsyncResolverInterface* resolver); + CandidateAndResolver( + const Candidate& candidate, + std::unique_ptr&& resolver); ~CandidateAndResolver(); + // Moveable, but not copyable. + CandidateAndResolver(CandidateAndResolver&&) = default; + CandidateAndResolver& operator=(CandidateAndResolver&&) = default; + Candidate candidate_; - rtc::AsyncResolverInterface* resolver_; + std::unique_ptr resolver_; }; std::vector resolvers_ RTC_GUARDED_BY(network_thread_); void FinishAddingRemoteCandidate(const Candidate& new_remote_candidate); - void OnCandidateResolved(rtc::AsyncResolverInterface* resolver); - void AddRemoteCandidateWithResolver(Candidate candidate, - rtc::AsyncResolverInterface* resolver); + void OnCandidateResolved(webrtc::AsyncDnsResolverInterface* resolver); + void AddRemoteCandidateWithResult( + Candidate candidate, + const webrtc::AsyncDnsResolverResult& result); // Number of times the selected_connection_ has been modified. uint32_t selected_candidate_pair_changes_ = 0; diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index 3ea9ca72ae..19ba3702ad 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -14,6 +14,7 @@ #include #include +#include "api/test/mock_async_dns_resolver.h" #include "p2p/base/basic_ice_controller.h" #include "p2p/base/connection.h" #include "p2p/base/fake_port_allocator.h" @@ -51,9 +52,12 @@ using ::testing::Assign; using ::testing::Contains; using ::testing::DoAll; using ::testing::InSequence; +using ::testing::InvokeArgument; using ::testing::InvokeWithoutArgs; using ::testing::NiceMock; using ::testing::Return; +using ::testing::ReturnRef; +using ::testing::SaveArg; using ::testing::SetArgPointee; using ::testing::SizeIs; @@ -187,6 +191,51 @@ class MockIceControllerFactory : public cricket::IceControllerFactoryInterface { MOCK_METHOD(void, RecordIceControllerCreated, ()); }; +// An one-shot resolver factory with default return arguments. +// Resolution is immediate, always succeeds, and returns nonsense. +class ResolverFactoryFixture : public webrtc::MockAsyncDnsResolverFactory { + public: + ResolverFactoryFixture() { + mock_async_dns_resolver_ = std::make_unique(); + ON_CALL(*mock_async_dns_resolver_, Start(_, _)) + .WillByDefault(InvokeArgument<1>()); + EXPECT_CALL(*mock_async_dns_resolver_, result()) + .WillOnce(ReturnRef(mock_async_dns_resolver_result_)); + + // A default action for GetResolvedAddress. Will be overruled + // by SetAddressToReturn. + ON_CALL(mock_async_dns_resolver_result_, GetResolvedAddress(_, _)) + .WillByDefault(Return(true)); + + EXPECT_CALL(mock_async_dns_resolver_result_, GetError()) + .WillOnce(Return(0)); + EXPECT_CALL(*this, Create()).WillOnce([this]() { + return std::move(mock_async_dns_resolver_); + }); + } + + void SetAddressToReturn(rtc::SocketAddress address_to_return) { + EXPECT_CALL(mock_async_dns_resolver_result_, GetResolvedAddress(_, _)) + .WillOnce(DoAll(SetArgPointee<1>(address_to_return), Return(true))); + } + void DelayResolution() { + // This function must be called before Create(). + ASSERT_TRUE(!!mock_async_dns_resolver_); + EXPECT_CALL(*mock_async_dns_resolver_, Start(_, _)) + .WillOnce(SaveArg<1>(&saved_callback_)); + } + void FireDelayedResolution() { + // This function must be called after Create(). + ASSERT_TRUE(saved_callback_); + saved_callback_(); + } + + private: + std::unique_ptr mock_async_dns_resolver_; + webrtc::MockAsyncDnsResolverResult mock_async_dns_resolver_result_; + std::function saved_callback_; +}; + } // namespace namespace cricket { @@ -345,7 +394,7 @@ class P2PTransportChannelTestBase : public ::testing::Test, rtc::FakeNetworkManager network_manager_; std::unique_ptr allocator_; - webrtc::AsyncResolverFactory* async_resolver_factory_; + webrtc::AsyncDnsResolverFactoryInterface* async_dns_resolver_factory_; ChannelData cd1_; ChannelData cd2_; IceRole role_; @@ -378,10 +427,10 @@ class P2PTransportChannelTestBase : public ::testing::Test, IceParamsWithRenomination(kIceParams[0], renomination); IceParameters ice_ep2_cd1_ch = IceParamsWithRenomination(kIceParams[1], renomination); - ep1_.cd1_.ch_.reset(CreateChannel(0, ICE_CANDIDATE_COMPONENT_DEFAULT, - ice_ep1_cd1_ch, ice_ep2_cd1_ch)); - ep2_.cd1_.ch_.reset(CreateChannel(1, ICE_CANDIDATE_COMPONENT_DEFAULT, - ice_ep2_cd1_ch, ice_ep1_cd1_ch)); + ep1_.cd1_.ch_ = CreateChannel(0, ICE_CANDIDATE_COMPONENT_DEFAULT, + ice_ep1_cd1_ch, ice_ep2_cd1_ch); + ep2_.cd1_.ch_ = CreateChannel(1, ICE_CANDIDATE_COMPONENT_DEFAULT, + ice_ep2_cd1_ch, ice_ep1_cd1_ch); ep1_.cd1_.ch_->SetIceConfig(ep1_config); ep2_.cd1_.ch_->SetIceConfig(ep2_config); ep1_.cd1_.ch_->MaybeStartGathering(); @@ -397,13 +446,14 @@ class P2PTransportChannelTestBase : public ::testing::Test, CreateChannels(default_config, default_config, false); } - P2PTransportChannel* CreateChannel(int endpoint, - int component, - const IceParameters& local_ice, - const IceParameters& remote_ice) { - P2PTransportChannel* channel = new P2PTransportChannel( + std::unique_ptr CreateChannel( + int endpoint, + int component, + const IceParameters& local_ice, + const IceParameters& remote_ice) { + auto channel = P2PTransportChannel::Create( "test content name", component, GetAllocator(endpoint), - GetEndpoint(endpoint)->async_resolver_factory_); + GetEndpoint(endpoint)->async_dns_resolver_factory_); channel->SignalReadyToSend.connect( this, &P2PTransportChannelTestBase::OnReadyToSend); channel->SignalCandidateGathered.connect( @@ -2079,8 +2129,8 @@ TEST_F(P2PTransportChannelTest, TurnToTurnPresumedWritable) { kDefaultPortAllocatorFlags); // Only configure one channel so we can control when the remote candidate // is added. - GetEndpoint(0)->cd1_.ch_.reset(CreateChannel( - 0, ICE_CANDIDATE_COMPONENT_DEFAULT, kIceParams[0], kIceParams[1])); + GetEndpoint(0)->cd1_.ch_ = CreateChannel(0, ICE_CANDIDATE_COMPONENT_DEFAULT, + kIceParams[0], kIceParams[1]); IceConfig config; config.presume_writable_when_fully_relayed = true; ep1_ch1()->SetIceConfig(config); @@ -2128,10 +2178,10 @@ TEST_F(P2PTransportChannelTest, TurnToPrflxPresumedWritable) { test_turn_server()->set_enable_permission_checks(false); IceConfig config; config.presume_writable_when_fully_relayed = true; - GetEndpoint(0)->cd1_.ch_.reset(CreateChannel( - 0, ICE_CANDIDATE_COMPONENT_DEFAULT, kIceParams[0], kIceParams[1])); - GetEndpoint(1)->cd1_.ch_.reset(CreateChannel( - 1, ICE_CANDIDATE_COMPONENT_DEFAULT, kIceParams[1], kIceParams[0])); + GetEndpoint(0)->cd1_.ch_ = CreateChannel(0, ICE_CANDIDATE_COMPONENT_DEFAULT, + kIceParams[0], kIceParams[1]); + GetEndpoint(1)->cd1_.ch_ = CreateChannel(1, ICE_CANDIDATE_COMPONENT_DEFAULT, + kIceParams[1], kIceParams[0]); ep1_ch1()->SetIceConfig(config); ep2_ch1()->SetIceConfig(config); // Don't signal candidates from channel 2, so that channel 1 sees the TURN @@ -2167,10 +2217,10 @@ TEST_F(P2PTransportChannelTest, PresumedWritablePreferredOverUnreliable) { kDefaultPortAllocatorFlags); IceConfig config; config.presume_writable_when_fully_relayed = true; - GetEndpoint(0)->cd1_.ch_.reset(CreateChannel( - 0, ICE_CANDIDATE_COMPONENT_DEFAULT, kIceParams[0], kIceParams[1])); - GetEndpoint(1)->cd1_.ch_.reset(CreateChannel( - 1, ICE_CANDIDATE_COMPONENT_DEFAULT, kIceParams[1], kIceParams[0])); + GetEndpoint(0)->cd1_.ch_ = CreateChannel(0, ICE_CANDIDATE_COMPONENT_DEFAULT, + kIceParams[0], kIceParams[1]); + GetEndpoint(1)->cd1_.ch_ = CreateChannel(1, ICE_CANDIDATE_COMPONENT_DEFAULT, + kIceParams[1], kIceParams[0]); ep1_ch1()->SetIceConfig(config); ep2_ch1()->SetIceConfig(config); ep1_ch1()->MaybeStartGathering(); @@ -2205,8 +2255,8 @@ TEST_F(P2PTransportChannelTest, SignalReadyToSendWithPresumedWritable) { kDefaultPortAllocatorFlags); // Only test one endpoint, so we can ensure the connection doesn't receive a // binding response and advance beyond being "presumed" writable. - GetEndpoint(0)->cd1_.ch_.reset(CreateChannel( - 0, ICE_CANDIDATE_COMPONENT_DEFAULT, kIceParams[0], kIceParams[1])); + GetEndpoint(0)->cd1_.ch_ = CreateChannel(0, ICE_CANDIDATE_COMPONENT_DEFAULT, + kIceParams[0], kIceParams[1]); IceConfig config; config.presume_writable_when_fully_relayed = true; ep1_ch1()->SetIceConfig(config); @@ -2258,10 +2308,10 @@ TEST_F(P2PTransportChannelTest, // to configure the server to accept packets from an address we haven't // explicitly installed permission for. test_turn_server()->set_enable_permission_checks(false); - GetEndpoint(0)->cd1_.ch_.reset(CreateChannel( - 0, ICE_CANDIDATE_COMPONENT_DEFAULT, kIceParams[0], kIceParams[1])); - GetEndpoint(1)->cd1_.ch_.reset(CreateChannel( - 1, ICE_CANDIDATE_COMPONENT_DEFAULT, kIceParams[1], kIceParams[0])); + GetEndpoint(0)->cd1_.ch_ = CreateChannel(0, ICE_CANDIDATE_COMPONENT_DEFAULT, + kIceParams[0], kIceParams[1]); + GetEndpoint(1)->cd1_.ch_ = CreateChannel(1, ICE_CANDIDATE_COMPONENT_DEFAULT, + kIceParams[1], kIceParams[0]); // Don't signal candidates from channel 2, so that channel 1 sees the TURN // candidate as peer reflexive. PauseCandidates(1); @@ -4834,31 +4884,18 @@ TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, TestTcpTurn) { // when the address is a hostname. The destruction should happen even // if the channel is not destroyed. TEST(P2PTransportChannelResolverTest, HostnameCandidateIsResolved) { - rtc::MockAsyncResolver mock_async_resolver; - EXPECT_CALL(mock_async_resolver, GetError()).WillOnce(Return(0)); - EXPECT_CALL(mock_async_resolver, GetResolvedAddress(_, _)) - .WillOnce(Return(true)); - // Destroy is called asynchronously after the address is resolved, - // so we need a variable to wait on. - bool destroy_called = false; - EXPECT_CALL(mock_async_resolver, Destroy(_)) - .WillOnce(Assign(&destroy_called, true)); - webrtc::MockAsyncResolverFactory mock_async_resolver_factory; - EXPECT_CALL(mock_async_resolver_factory, Create()) - .WillOnce(Return(&mock_async_resolver)); - + ResolverFactoryFixture resolver_fixture; FakePortAllocator allocator(rtc::Thread::Current(), nullptr); - P2PTransportChannel channel("tn", 0, &allocator, - &mock_async_resolver_factory); + auto channel = + P2PTransportChannel::Create("tn", 0, &allocator, &resolver_fixture); Candidate hostname_candidate; SocketAddress hostname_address("fake.test", 1000); hostname_candidate.set_address(hostname_address); - channel.AddRemoteCandidate(hostname_candidate); + channel->AddRemoteCandidate(hostname_candidate); - ASSERT_EQ_WAIT(1u, channel.remote_candidates().size(), kDefaultTimeout); - const RemoteCandidate& candidate = channel.remote_candidates()[0]; + ASSERT_EQ_WAIT(1u, channel->remote_candidates().size(), kDefaultTimeout); + const RemoteCandidate& candidate = channel->remote_candidates()[0]; EXPECT_FALSE(candidate.address().IsUnresolvedIP()); - WAIT(destroy_called, kShortTimeout); } // Test that if we signal a hostname candidate after the remote endpoint @@ -4867,11 +4904,6 @@ TEST(P2PTransportChannelResolverTest, HostnameCandidateIsResolved) { // done. TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignalingWithMdnsName) { - rtc::MockAsyncResolver mock_async_resolver; - webrtc::MockAsyncResolverFactory mock_async_resolver_factory; - EXPECT_CALL(mock_async_resolver_factory, Create()) - .WillOnce(Return(&mock_async_resolver)); - // ep1 and ep2 will only gather host candidates with addresses // kPublicAddrs[0] and kPublicAddrs[1], respectively. ConfigureEndpoints(OPEN, OPEN, kOnlyLocalPorts, kOnlyLocalPorts); @@ -4879,7 +4911,9 @@ TEST_F(P2PTransportChannelTest, set_remote_ice_parameter_source(FROM_SETICEPARAMETERS); GetEndpoint(0)->network_manager_.set_mdns_responder( std::make_unique(rtc::Thread::Current())); - GetEndpoint(1)->async_resolver_factory_ = &mock_async_resolver_factory; + + ResolverFactoryFixture resolver_fixture; + GetEndpoint(1)->async_dns_resolver_factory_ = &resolver_fixture; CreateChannels(); // Pause sending candidates from both endpoints until we find out what port // number is assgined to ep1's host candidate. @@ -4894,6 +4928,7 @@ TEST_F(P2PTransportChannelTest, // This is the underlying private IP address of the same candidate at ep1. const auto local_address = rtc::SocketAddress( kPublicAddrs[0].ipaddr(), local_candidate.address().port()); + // Let ep2 signal its candidate to ep1. ep1 should form a candidate // pair and start to ping. After receiving the ping, ep2 discovers a prflx // remote candidate and form a candidate pair as well. @@ -4909,19 +4944,7 @@ TEST_F(P2PTransportChannelTest, EXPECT_EQ(kIceUfrag[0], selected_connection->remote_candidate().username()); EXPECT_EQ(kIcePwd[0], selected_connection->remote_candidate().password()); // Set expectation before ep1 signals a hostname candidate. - { - InSequence sequencer; - EXPECT_CALL(mock_async_resolver, Start(_)); - EXPECT_CALL(mock_async_resolver, GetError()).WillOnce(Return(0)); - // Let the mock resolver of ep2 receives the correct resolution. - EXPECT_CALL(mock_async_resolver, GetResolvedAddress(_, _)) - .WillOnce(DoAll(SetArgPointee<1>(local_address), Return(true))); - } - // Destroy is called asynchronously after the address is resolved, - // so we need a variable to wait on. - bool destroy_called = false; - EXPECT_CALL(mock_async_resolver, Destroy(_)) - .WillOnce(Assign(&destroy_called, true)); + resolver_fixture.SetAddressToReturn(local_address); ResumeCandidates(0); // Verify ep2's selected connection is updated to use the 'local' candidate. EXPECT_EQ_WAIT(LOCAL_PORT_TYPE, @@ -4929,7 +4952,6 @@ TEST_F(P2PTransportChannelTest, kMediumTimeout); EXPECT_EQ(selected_connection, ep2_ch1()->selected_connection()); - WAIT(destroy_called, kShortTimeout); DestroyChannels(); } @@ -4939,13 +4961,9 @@ TEST_F(P2PTransportChannelTest, // address after the resolution completes. TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateDuringResolvingHostCandidateWithMdnsName) { - auto mock_async_resolver = new NiceMock(); - ON_CALL(*mock_async_resolver, Destroy).WillByDefault([mock_async_resolver] { - delete mock_async_resolver; - }); - webrtc::MockAsyncResolverFactory mock_async_resolver_factory; - EXPECT_CALL(mock_async_resolver_factory, Create()) - .WillOnce(Return(mock_async_resolver)); + ResolverFactoryFixture resolver_fixture; + // Prevent resolution until triggered by FireDelayedResolution. + resolver_fixture.DelayResolution(); // ep1 and ep2 will only gather host candidates with addresses // kPublicAddrs[0] and kPublicAddrs[1], respectively. @@ -4954,12 +4972,13 @@ TEST_F(P2PTransportChannelTest, set_remote_ice_parameter_source(FROM_SETICEPARAMETERS); GetEndpoint(0)->network_manager_.set_mdns_responder( std::make_unique(rtc::Thread::Current())); - GetEndpoint(1)->async_resolver_factory_ = &mock_async_resolver_factory; + GetEndpoint(1)->async_dns_resolver_factory_ = &resolver_fixture; CreateChannels(); // Pause sending candidates from both endpoints until we find out what port // number is assgined to ep1's host candidate. PauseCandidates(0); PauseCandidates(1); + ASSERT_EQ_WAIT(1u, GetEndpoint(0)->saved_candidates_.size(), kMediumTimeout); ASSERT_EQ(1u, GetEndpoint(0)->saved_candidates_[0]->candidates.size()); const auto& local_candidate = @@ -4969,24 +4988,16 @@ TEST_F(P2PTransportChannelTest, // This is the underlying private IP address of the same candidate at ep1. const auto local_address = rtc::SocketAddress( kPublicAddrs[0].ipaddr(), local_candidate.address().port()); - bool mock_async_resolver_started = false; - // Not signaling done yet, and only make sure we are in the process of - // resolution. - EXPECT_CALL(*mock_async_resolver, Start(_)) - .WillOnce(InvokeWithoutArgs([&mock_async_resolver_started]() { - mock_async_resolver_started = true; - })); // Let ep1 signal its hostname candidate to ep2. ResumeCandidates(0); - ASSERT_TRUE_WAIT(mock_async_resolver_started, kMediumTimeout); // Now that ep2 is in the process of resolving the hostname candidate signaled // by ep1. Let ep2 signal its host candidate with an IP address to ep1, so // that ep1 can form a candidate pair, select it and start to ping ep2. ResumeCandidates(1); ASSERT_TRUE_WAIT(ep1_ch1()->selected_connection() != nullptr, kMediumTimeout); // Let the mock resolver of ep2 receives the correct resolution. - EXPECT_CALL(*mock_async_resolver, GetResolvedAddress(_, _)) - .WillOnce(DoAll(SetArgPointee<1>(local_address), Return(true))); + resolver_fixture.SetAddressToReturn(local_address); + // Upon receiving a ping from ep1, ep2 adds a prflx candidate from the // unknown address and establishes a connection. // @@ -4997,7 +5008,9 @@ TEST_F(P2PTransportChannelTest, ep2_ch1()->selected_connection()->remote_candidate().type()); // ep2 should also be able resolve the hostname candidate. The resolved remote // host candidate should be merged with the prflx remote candidate. - mock_async_resolver->SignalDone(mock_async_resolver); + + resolver_fixture.FireDelayedResolution(); + EXPECT_EQ_WAIT(LOCAL_PORT_TYPE, ep2_ch1()->selected_connection()->remote_candidate().type(), kMediumTimeout); @@ -5010,10 +5023,7 @@ TEST_F(P2PTransportChannelTest, // which is obfuscated by an mDNS name, and if the peer can complete the name // resolution with the correct IP address, we can have a p2p connection. TEST_F(P2PTransportChannelTest, CanConnectWithHostCandidateWithMdnsName) { - NiceMock mock_async_resolver; - webrtc::MockAsyncResolverFactory mock_async_resolver_factory; - EXPECT_CALL(mock_async_resolver_factory, Create()) - .WillOnce(Return(&mock_async_resolver)); + ResolverFactoryFixture resolver_fixture; // ep1 and ep2 will only gather host candidates with addresses // kPublicAddrs[0] and kPublicAddrs[1], respectively. @@ -5022,7 +5032,7 @@ TEST_F(P2PTransportChannelTest, CanConnectWithHostCandidateWithMdnsName) { set_remote_ice_parameter_source(FROM_SETICEPARAMETERS); GetEndpoint(0)->network_manager_.set_mdns_responder( std::make_unique(rtc::Thread::Current())); - GetEndpoint(1)->async_resolver_factory_ = &mock_async_resolver_factory; + GetEndpoint(1)->async_dns_resolver_factory_ = &resolver_fixture; CreateChannels(); // Pause sending candidates from both endpoints until we find out what port // number is assgined to ep1's host candidate. @@ -5039,8 +5049,7 @@ TEST_F(P2PTransportChannelTest, CanConnectWithHostCandidateWithMdnsName) { rtc::SocketAddress resolved_address_ep1(local_candidate_ep1.address()); resolved_address_ep1.SetResolvedIP(kPublicAddrs[0].ipaddr()); - EXPECT_CALL(mock_async_resolver, GetResolvedAddress(_, _)) - .WillOnce(DoAll(SetArgPointee<1>(resolved_address_ep1), Return(true))); + resolver_fixture.SetAddressToReturn(resolved_address_ep1); // Let ep1 signal its hostname candidate to ep2. ResumeCandidates(0); @@ -5064,10 +5073,7 @@ TEST_F(P2PTransportChannelTest, CanConnectWithHostCandidateWithMdnsName) { // this remote host candidate in stats. TEST_F(P2PTransportChannelTest, CandidatesSanitizedInStatsWhenMdnsObfuscationEnabled) { - NiceMock mock_async_resolver; - webrtc::MockAsyncResolverFactory mock_async_resolver_factory; - EXPECT_CALL(mock_async_resolver_factory, Create()) - .WillOnce(Return(&mock_async_resolver)); + ResolverFactoryFixture resolver_fixture; // ep1 and ep2 will gather host candidates with addresses // kPublicAddrs[0] and kPublicAddrs[1], respectively. ep1 also gathers a srflx @@ -5079,7 +5085,7 @@ TEST_F(P2PTransportChannelTest, set_remote_ice_parameter_source(FROM_SETICEPARAMETERS); GetEndpoint(0)->network_manager_.set_mdns_responder( std::make_unique(rtc::Thread::Current())); - GetEndpoint(1)->async_resolver_factory_ = &mock_async_resolver_factory; + GetEndpoint(1)->async_dns_resolver_factory_ = &resolver_fixture; CreateChannels(); // Pause sending candidates from both endpoints until we find out what port // number is assigned to ep1's host candidate. @@ -5097,9 +5103,7 @@ TEST_F(P2PTransportChannelTest, // and let the mock resolver of ep2 receive the correct resolution. rtc::SocketAddress resolved_address_ep1(local_candidate_ep1.address()); resolved_address_ep1.SetResolvedIP(kPublicAddrs[0].ipaddr()); - EXPECT_CALL(mock_async_resolver, GetResolvedAddress(_, _)) - .WillOnce( - DoAll(SetArgPointee<1>(resolved_address_ep1), Return(true))); + resolver_fixture.SetAddressToReturn(resolved_address_ep1); break; } } @@ -5248,10 +5252,7 @@ TEST_F(P2PTransportChannelTest, // when it is queried via GetSelectedCandidatePair. TEST_F(P2PTransportChannelTest, SelectedCandidatePairSanitizedWhenMdnsObfuscationEnabled) { - NiceMock mock_async_resolver; - webrtc::MockAsyncResolverFactory mock_async_resolver_factory; - EXPECT_CALL(mock_async_resolver_factory, Create()) - .WillOnce(Return(&mock_async_resolver)); + ResolverFactoryFixture resolver_fixture; // ep1 and ep2 will gather host candidates with addresses // kPublicAddrs[0] and kPublicAddrs[1], respectively. @@ -5260,7 +5261,7 @@ TEST_F(P2PTransportChannelTest, set_remote_ice_parameter_source(FROM_SETICEPARAMETERS); GetEndpoint(0)->network_manager_.set_mdns_responder( std::make_unique(rtc::Thread::Current())); - GetEndpoint(1)->async_resolver_factory_ = &mock_async_resolver_factory; + GetEndpoint(1)->async_dns_resolver_factory_ = &resolver_fixture; CreateChannels(); // Pause sending candidates from both endpoints until we find out what port // number is assigned to ep1's host candidate. @@ -5275,8 +5276,8 @@ TEST_F(P2PTransportChannelTest, // and let the mock resolver of ep2 receive the correct resolution. rtc::SocketAddress resolved_address_ep1(local_candidate_ep1.address()); resolved_address_ep1.SetResolvedIP(kPublicAddrs[0].ipaddr()); - EXPECT_CALL(mock_async_resolver, GetResolvedAddress(_, _)) - .WillOnce(DoAll(SetArgPointee<1>(resolved_address_ep1), Return(true))); + resolver_fixture.SetAddressToReturn(resolved_address_ep1); + ResumeCandidates(0); ResumeCandidates(1); @@ -5305,8 +5306,8 @@ TEST_F(P2PTransportChannelTest, // We use one endpoint to test the behavior of adding remote candidates, and // this endpoint only gathers relay candidates. ConfigureEndpoints(OPEN, OPEN, kOnlyRelayPorts, kDefaultPortAllocatorFlags); - GetEndpoint(0)->cd1_.ch_.reset(CreateChannel( - 0, ICE_CANDIDATE_COMPONENT_DEFAULT, kIceParams[0], kIceParams[1])); + GetEndpoint(0)->cd1_.ch_ = CreateChannel(0, ICE_CANDIDATE_COMPONENT_DEFAULT, + kIceParams[0], kIceParams[1]); IceConfig config; // Start gathering and we should have only a single relay port. ep1_ch1()->SetIceConfig(config); @@ -5869,21 +5870,21 @@ class ForgetLearnedStateControllerFactory TEST_F(P2PTransportChannelPingTest, TestForgetLearnedState) { ForgetLearnedStateControllerFactory factory; FakePortAllocator pa(rtc::Thread::Current(), nullptr); - P2PTransportChannel ch("ping sufficiently", 1, &pa, nullptr, nullptr, - &factory); - PrepareChannel(&ch); - ch.MaybeStartGathering(); - ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); - ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 2)); + auto ch = P2PTransportChannel::Create("ping sufficiently", 1, &pa, nullptr, + nullptr, &factory); + PrepareChannel(ch.get()); + ch->MaybeStartGathering(); + ch->AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); + ch->AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 2)); - Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); - Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2); + Connection* conn1 = WaitForConnectionTo(ch.get(), "1.1.1.1", 1); + Connection* conn2 = WaitForConnectionTo(ch.get(), "2.2.2.2", 2); ASSERT_TRUE(conn1 != nullptr); ASSERT_TRUE(conn2 != nullptr); // Wait for conn1 to be selected. conn1->ReceivedPingResponse(LOW_RTT, "id"); - EXPECT_EQ_WAIT(conn1, ch.selected_connection(), kMediumTimeout); + EXPECT_EQ_WAIT(conn1, ch->selected_connection(), kMediumTimeout); conn2->ReceivedPingResponse(LOW_RTT, "id"); EXPECT_TRUE(conn2->writable()); @@ -5904,23 +5905,23 @@ TEST_F(P2PTransportChannelTest, DisableDnsLookupsWithTransportPolicyRelay) { auto* ep1 = GetEndpoint(0); ep1->allocator_->SetCandidateFilter(CF_RELAY); - rtc::MockAsyncResolver mock_async_resolver; - webrtc::MockAsyncResolverFactory mock_async_resolver_factory; - ON_CALL(mock_async_resolver_factory, Create()) - .WillByDefault(Return(&mock_async_resolver)); - ep1->async_resolver_factory_ = &mock_async_resolver_factory; + std::unique_ptr mock_async_resolver = + std::make_unique(); + // This test expects resolution to not be started. + EXPECT_CALL(*mock_async_resolver, Start(_, _)).Times(0); - bool lookup_started = false; - ON_CALL(mock_async_resolver, Start(_)) - .WillByDefault(Assign(&lookup_started, true)); + webrtc::MockAsyncDnsResolverFactory mock_async_resolver_factory; + ON_CALL(mock_async_resolver_factory, Create()) + .WillByDefault( + [&mock_async_resolver]() { return std::move(mock_async_resolver); }); + + ep1->async_dns_resolver_factory_ = &mock_async_resolver_factory; CreateChannels(); ep1_ch1()->AddRemoteCandidate( CreateUdpCandidate(LOCAL_PORT_TYPE, "hostname.test", 1, 100)); - EXPECT_FALSE(lookup_started); - DestroyChannels(); } @@ -5930,23 +5931,23 @@ TEST_F(P2PTransportChannelTest, DisableDnsLookupsWithTransportPolicyNone) { auto* ep1 = GetEndpoint(0); ep1->allocator_->SetCandidateFilter(CF_NONE); - rtc::MockAsyncResolver mock_async_resolver; - webrtc::MockAsyncResolverFactory mock_async_resolver_factory; - ON_CALL(mock_async_resolver_factory, Create()) - .WillByDefault(Return(&mock_async_resolver)); - ep1->async_resolver_factory_ = &mock_async_resolver_factory; + std::unique_ptr mock_async_resolver = + std::make_unique(); + // This test expects resolution to not be started. + EXPECT_CALL(*mock_async_resolver, Start(_, _)).Times(0); - bool lookup_started = false; - ON_CALL(mock_async_resolver, Start(_)) - .WillByDefault(Assign(&lookup_started, true)); + webrtc::MockAsyncDnsResolverFactory mock_async_resolver_factory; + ON_CALL(mock_async_resolver_factory, Create()) + .WillByDefault( + [&mock_async_resolver]() { return std::move(mock_async_resolver); }); + + ep1->async_dns_resolver_factory_ = &mock_async_resolver_factory; CreateChannels(); ep1_ch1()->AddRemoteCandidate( CreateUdpCandidate(LOCAL_PORT_TYPE, "hostname.test", 1, 100)); - EXPECT_FALSE(lookup_started); - DestroyChannels(); } @@ -5956,18 +5957,19 @@ TEST_F(P2PTransportChannelTest, EnableDnsLookupsWithTransportPolicyNoHost) { auto* ep1 = GetEndpoint(0); ep1->allocator_->SetCandidateFilter(CF_ALL & ~CF_HOST); - rtc::MockAsyncResolver mock_async_resolver; - webrtc::MockAsyncResolverFactory mock_async_resolver_factory; - EXPECT_CALL(mock_async_resolver_factory, Create()) - .WillOnce(Return(&mock_async_resolver)); - EXPECT_CALL(mock_async_resolver, Destroy(_)); - - ep1->async_resolver_factory_ = &mock_async_resolver_factory; - + std::unique_ptr mock_async_resolver = + std::make_unique(); bool lookup_started = false; - EXPECT_CALL(mock_async_resolver, Start(_)) + EXPECT_CALL(*mock_async_resolver, Start(_, _)) .WillOnce(Assign(&lookup_started, true)); + webrtc::MockAsyncDnsResolverFactory mock_async_resolver_factory; + EXPECT_CALL(mock_async_resolver_factory, Create()) + .WillOnce( + [&mock_async_resolver]() { return std::move(mock_async_resolver); }); + + ep1->async_dns_resolver_factory_ = &mock_async_resolver_factory; + CreateChannels(); ep1_ch1()->AddRemoteCandidate( diff --git a/pc/BUILD.gn b/pc/BUILD.gn index f0bdafa14b..a61e04d8a1 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -77,6 +77,7 @@ rtc_library("rtc_pc_base") { deps = [ ":media_protocol_names", "../api:array_view", + "../api:async_dns_resolver", "../api:audio_options_api", "../api:call_api", "../api:function_view", @@ -236,6 +237,7 @@ rtc_library("peerconnection") { ":video_track", ":video_track_source", "../api:array_view", + "../api:async_dns_resolver", "../api:audio_options_api", "../api:call_api", "../api:callfactory_api", diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 2784e80786..61288b2b1f 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -39,11 +39,11 @@ namespace webrtc { JsepTransportController::JsepTransportController( rtc::Thread* network_thread, cricket::PortAllocator* port_allocator, - AsyncResolverFactory* async_resolver_factory, + AsyncDnsResolverFactoryInterface* async_dns_resolver_factory, Config config) : network_thread_(network_thread), port_allocator_(port_allocator), - async_resolver_factory_(async_resolver_factory), + async_dns_resolver_factory_(async_dns_resolver_factory), config_(config), active_reset_srtp_params_(config.active_reset_srtp_params) { // The |transport_observer| is assumed to be non-null. @@ -398,7 +398,7 @@ JsepTransportController::CreateIceTransport(const std::string& transport_name, IceTransportInit init; init.set_port_allocator(port_allocator_); - init.set_async_resolver_factory(async_resolver_factory_); + init.set_async_dns_resolver_factory(async_dns_resolver_factory_); init.set_event_log(config_.event_log); return config_.ice_transport_factory->CreateIceTransport( transport_name, component, std::move(init)); diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index 949c9ad1dc..568058571f 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -21,7 +21,7 @@ #include #include "absl/types/optional.h" -#include "api/async_resolver_factory.h" +#include "api/async_dns_resolver.h" #include "api/candidate.h" #include "api/crypto/crypto_options.h" #include "api/ice_transport_factory.h" @@ -140,10 +140,11 @@ class JsepTransportController : public sigslot::has_slots<> { // All the transport related methods are called on the |network_thread| // and destruction of the JsepTransportController must occur on the // |network_thread|. - JsepTransportController(rtc::Thread* network_thread, - cricket::PortAllocator* port_allocator, - AsyncResolverFactory* async_resolver_factory, - Config config); + JsepTransportController( + rtc::Thread* network_thread, + cricket::PortAllocator* port_allocator, + AsyncDnsResolverFactoryInterface* async_dns_resolver_factory, + Config config); virtual ~JsepTransportController(); // The main method to be called; applies a description at the transport @@ -461,7 +462,7 @@ class JsepTransportController : public sigslot::has_slots<> { rtc::Thread* const network_thread_ = nullptr; cricket::PortAllocator* const port_allocator_ = nullptr; - AsyncResolverFactory* const async_resolver_factory_ = nullptr; + AsyncDnsResolverFactoryInterface* const async_dns_resolver_factory_ = nullptr; std::map> jsep_transports_by_name_ RTC_GUARDED_BY(network_thread_); diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index aaf72324c7..97dca34f75 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -12,6 +12,7 @@ #include #include + #include #include #include @@ -33,6 +34,7 @@ #include "media/base/rid_description.h" #include "media/base/stream_params.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" +#include "p2p/base/basic_async_resolver_factory.h" #include "p2p/base/connection.h" #include "p2p/base/connection_info.h" #include "p2p/base/dtls_transport_internal.h" @@ -435,6 +437,30 @@ RTCErrorOr> PeerConnection::Create( bool is_unified_plan = configuration.sdp_semantics == SdpSemantics::kUnifiedPlan; bool dtls_enabled = DtlsEnabled(configuration, options, dependencies); + + // Interim code: If an AsyncResolverFactory is given, but not an + // AsyncDnsResolverFactory, wrap it in a WrappingAsyncDnsResolverFactory + // If neither is given, create a WrappingAsyncDnsResolverFactory wrapping + // a BasicAsyncResolver. + // TODO(bugs.webrtc.org/12598): Remove code once all callers pass a + // AsyncDnsResolverFactory. + if (dependencies.async_dns_resolver_factory && + dependencies.async_resolver_factory) { + RTC_LOG(LS_ERROR) + << "Attempt to set both old and new type of DNS resolver factory"; + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Both old and new type of DNS resolver given"); + } + if (dependencies.async_resolver_factory) { + dependencies.async_dns_resolver_factory = + std::make_unique( + std::move(dependencies.async_resolver_factory)); + } else { + dependencies.async_dns_resolver_factory = + std::make_unique( + std::make_unique()); + } + // The PeerConnection constructor consumes some, but not all, dependencies. rtc::scoped_refptr pc( new rtc::RefCountedObject( @@ -462,7 +488,8 @@ PeerConnection::PeerConnection( is_unified_plan_(is_unified_plan), event_log_(std::move(event_log)), event_log_ptr_(event_log_.get()), - async_resolver_factory_(std::move(dependencies.async_resolver_factory)), + async_dns_resolver_factory_( + std::move(dependencies.async_dns_resolver_factory)), port_allocator_(std::move(dependencies.allocator)), ice_transport_factory_(std::move(dependencies.ice_transport_factory)), tls_cert_verifier_(std::move(dependencies.tls_cert_verifier)), @@ -672,7 +699,7 @@ void PeerConnection::InitializeTransportController_n( transport_controller_.reset( new JsepTransportController(network_thread(), port_allocator_.get(), - async_resolver_factory_.get(), config)); + async_dns_resolver_factory_.get(), config)); transport_controller_->SubscribeIceConnectionState( [this](cricket::IceConnectionState s) { diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 7818e7bf4c..8722ab18ee 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -23,6 +23,7 @@ #include "absl/types/optional.h" #include "api/adaptation/resource.h" +#include "api/async_dns_resolver.h" #include "api/async_resolver_factory.h" #include "api/audio_options.h" #include "api/candidate.h" @@ -637,11 +638,8 @@ class PeerConnection : public PeerConnectionInternal, PeerConnectionInterface::RTCConfiguration configuration_ RTC_GUARDED_BY(signaling_thread()); - // TODO(zstein): |async_resolver_factory_| can currently be nullptr if it - // is not injected. It should be required once chromium supplies it. - // This member variable is only used by JsepTransportController so we should - // consider moving ownership to there. - const std::unique_ptr async_resolver_factory_; + const std::unique_ptr + async_dns_resolver_factory_; std::unique_ptr port_allocator_; // TODO(bugs.webrtc.org/9987): Accessed on both // signaling and network thread.