From aa653c0d76645db0f0d534818a9ce6e17ecba26e Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Tue, 17 Oct 2023 03:34:39 +0000 Subject: [PATCH] Reland "Deprecate all classes related to AsyncResolver" This reverts commit 08d431ec34a1c7ab52557702f2cebd9fdfacae9e. Reason for revert: Last (hopefully) Chrome blocker removed Original change's description: > Revert "Deprecate all classes related to AsyncResolver" > > This reverts commit 61a442809cc06de93a613186084d2dfa9c934d81. > > Reason for revert: Breaks roll into Chromium > > Original change's description: > > Deprecate all classes related to AsyncResolver > > > > and remove internal usage. > > > > Bug: webrtc:12598 > > Change-Id: Ie208682bfa0163f6c7a8e805151cfbda76324496 > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/322860 > > Reviewed-by: Tomas Gunnarsson > > Auto-Submit: Harald Alvestrand > > Commit-Queue: Harald Alvestrand > > Cr-Commit-Position: refs/heads/main@{#40919} > > Bug: webrtc:12598 > Change-Id: I8aef5e062e19a51baec75873eddfca2a10467d3c > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/322901 > Bot-Commit: rubber-stamper@appspot.gserviceaccount.com > Auto-Submit: Harald Alvestrand > Commit-Queue: Harald Alvestrand > Cr-Commit-Position: refs/heads/main@{#40927} Bug: webrtc:12598 Change-Id: I3c7b07c831eb9ff808368433d9b9ae8ec4b2afb6 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/323720 Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#40944} --- api/async_resolver_factory.h | 5 +- api/ice_transport_interface.h | 3 + api/packet_socket_factory.h | 5 +- api/peer_connection_interface.h | 10 +-- api/wrapping_async_dns_resolver.h | 8 ++- p2p/BUILD.gn | 1 + p2p/base/basic_async_resolver_factory.h | 12 +++- .../basic_async_resolver_factory_unittest.cc | 5 ++ p2p/base/basic_packet_socket_factory.cc | 7 +-- p2p/base/mock_async_resolver.h | 11 +++- p2p/base/turn_port.h | 2 +- p2p/stunprober/stun_prober.cc | 26 ++++---- p2p/stunprober/stun_prober.h | 5 +- p2p/stunprober/stun_prober_unittest.cc | 62 +++++++++++++++---- rtc_base/async_resolver.h | 7 ++- rtc_base/async_resolver_interface.h | 2 +- 16 files changed, 122 insertions(+), 49 deletions(-) diff --git a/api/async_resolver_factory.h b/api/async_resolver_factory.h index ffa958268d..997fe5ce57 100644 --- a/api/async_resolver_factory.h +++ b/api/async_resolver_factory.h @@ -19,13 +19,16 @@ namespace webrtc { // client applications to provide WebRTC with their own mechanism for // performing DNS resolution. // TODO(bugs.webrtc.org/12598): Deprecate and remove. -class AsyncResolverFactory { +class [[deprecated("Use AsyncDnsResolverFactory")]] AsyncResolverFactory { public: AsyncResolverFactory() = default; virtual ~AsyncResolverFactory() = default; +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" // The caller should call Destroy on the returned object to delete it. virtual rtc::AsyncResolverInterface* Create() = 0; +#pragma clang diagnostic pop }; } // namespace webrtc diff --git a/api/ice_transport_interface.h b/api/ice_transport_interface.h index 431f3330a5..001395c215 100644 --- a/api/ice_transport_interface.h +++ b/api/ice_transport_interface.h @@ -115,8 +115,11 @@ struct IceTransportInit final { private: cricket::PortAllocator* port_allocator_ = nullptr; AsyncDnsResolverFactoryInterface* async_dns_resolver_factory_ = nullptr; +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" // For backwards compatibility. Only one resolver factory can be set. AsyncResolverFactory* async_resolver_factory_ = nullptr; +#pragma clang diagnostic pop RtcEventLog* event_log_ = nullptr; cricket::IceControllerFactoryInterface* ice_controller_factory_ = nullptr; cricket::ActiveIceControllerFactoryInterface* active_ice_controller_factory_ = diff --git a/api/packet_socket_factory.h b/api/packet_socket_factory.h index 29d2606b9b..e389ccb2ff 100644 --- a/api/packet_socket_factory.h +++ b/api/packet_socket_factory.h @@ -76,7 +76,9 @@ class RTC_EXPORT PacketSocketFactory { // to switch to the AsyncDnsResolverInterface. // TODO(bugs.webrtc.org/12598): Remove once all downstream users // are converted. - virtual AsyncResolverInterface* CreateAsyncResolver() { +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" + [[deprecated]] virtual AsyncResolverInterface* CreateAsyncResolver() { // Default implementation, so that downstream users can remove this // immediately after changing to CreateAsyncDnsResolver RTC_DCHECK_NOTREACHED(); @@ -89,6 +91,7 @@ class RTC_EXPORT PacketSocketFactory { return std::make_unique( CreateAsyncResolver()); } +#pragma clang diagnostic pop private: PacketSocketFactory(const PacketSocketFactory&) = delete; diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index e2f284e25e..08586f8d6c 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -1386,10 +1386,12 @@ struct RTC_EXPORT PeerConnectionDependencies final { std::unique_ptr async_dns_resolver_factory; // Deprecated - use async_dns_resolver_factory - // Deprecation is in abeyance until Chromium is updated. - // TODO(crbug.com/1475925): Deprecate once Chromium is updated - // [[deprecated("Use async_dns_resolver_factory")]] - std::unique_ptr async_resolver_factory; +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" + [[deprecated("Use async_dns_resolver_factory")]] std::unique_ptr< + webrtc::AsyncResolverFactory> + async_resolver_factory; +#pragma clang diagnostic pop std::unique_ptr ice_transport_factory; std::unique_ptr cert_generator; std::unique_ptr tls_cert_verifier; diff --git a/api/wrapping_async_dns_resolver.h b/api/wrapping_async_dns_resolver.h index d07f1464c5..b384f97652 100644 --- a/api/wrapping_async_dns_resolver.h +++ b/api/wrapping_async_dns_resolver.h @@ -33,9 +33,10 @@ namespace webrtc { -class WrappingAsyncDnsResolver; +class [[deprecated("Use AsyncDnsResolver directly")]] WrappingAsyncDnsResolver; -class RTC_EXPORT WrappingAsyncDnsResolverResult +class [[deprecated( + "Use AsyncDnsResolver directly")]] RTC_EXPORT WrappingAsyncDnsResolverResult : public AsyncDnsResolverResult { public: explicit WrappingAsyncDnsResolverResult(WrappingAsyncDnsResolver* owner) @@ -54,6 +55,8 @@ class RTC_EXPORT WrappingAsyncDnsResolverResult class RTC_EXPORT WrappingAsyncDnsResolver : public AsyncDnsResolverInterface, public sigslot::has_slots<> { public: +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" explicit WrappingAsyncDnsResolver(rtc::AsyncResolverInterface* wrapped) : wrapped_(absl::WrapUnique(wrapped)), result_(this) {} @@ -124,6 +127,7 @@ class RTC_EXPORT WrappingAsyncDnsResolver : public AsyncDnsResolverInterface, 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; +#pragma clang diagnostic pop }; } // namespace webrtc diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index b8dc0b03ad..4e4924a841 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn @@ -393,6 +393,7 @@ rtc_library("libstunprober") { deps = [ ":rtc_p2p", + "../api:async_dns_resolver", "../api:packet_socket_factory", "../api:sequence_checker", "../api/task_queue:pending_task_safety_flag", diff --git a/p2p/base/basic_async_resolver_factory.h b/p2p/base/basic_async_resolver_factory.h index 9c1af6a1e1..1a94fb9679 100644 --- a/p2p/base/basic_async_resolver_factory.h +++ b/p2p/base/basic_async_resolver_factory.h @@ -21,10 +21,15 @@ namespace webrtc { -class BasicAsyncResolverFactory final : public AsyncResolverFactory { +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" +class [[deprecated( + "Use BasicAsyncDnsResolverFactory")]] BasicAsyncResolverFactory final + : public AsyncResolverFactory { public: rtc::AsyncResolverInterface* Create() override; }; +#pragma clang diagnostic pop // A factory that vends AsyncDnsResolver instances. class BasicAsyncDnsResolverFactory final @@ -47,9 +52,11 @@ class BasicAsyncDnsResolverFactory final // This class wraps a factory using the older webrtc::AsyncResolverFactory API, // and produces webrtc::AsyncDnsResolver objects that contain an // rtc::AsyncResolver object. -class WrappingAsyncDnsResolverFactory final +class [[deprecated]] WrappingAsyncDnsResolverFactory final : public AsyncDnsResolverFactoryInterface { public: +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" explicit WrappingAsyncDnsResolverFactory( std::unique_ptr wrapped_factory) : owned_factory_(std::move(wrapped_factory)), @@ -58,6 +65,7 @@ class WrappingAsyncDnsResolverFactory final explicit WrappingAsyncDnsResolverFactory( AsyncResolverFactory* non_owned_factory) : wrapped_factory_(non_owned_factory) {} +#pragma clang diagnostic pop std::unique_ptr CreateAndResolve( const rtc::SocketAddress& addr, diff --git a/p2p/base/basic_async_resolver_factory_unittest.cc b/p2p/base/basic_async_resolver_factory_unittest.cc index 77b97e75e6..313907abb9 100644 --- a/p2p/base/basic_async_resolver_factory_unittest.cc +++ b/p2p/base/basic_async_resolver_factory_unittest.cc @@ -22,6 +22,9 @@ namespace webrtc { +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" + class BasicAsyncResolverFactoryTest : public ::testing::Test, public sigslot::has_slots<> { public: @@ -108,4 +111,6 @@ TEST(WrappingAsyncDnsResolverFactoryDeathTest, DestroyResolverInCallback) { } #endif +#pragma clang diagnostic pop + } // namespace webrtc diff --git a/p2p/base/basic_packet_socket_factory.cc b/p2p/base/basic_packet_socket_factory.cc index b07a407164..7d87e12859 100644 --- a/p2p/base/basic_packet_socket_factory.cc +++ b/p2p/base/basic_packet_socket_factory.cc @@ -18,6 +18,7 @@ #include "api/async_dns_resolver.h" #include "api/wrapping_async_dns_resolver.h" #include "p2p/base/async_stun_tcp_socket.h" +#include "rtc_base/async_dns_resolver.h" #include "rtc_base/async_tcp_socket.h" #include "rtc_base/async_udp_socket.h" #include "rtc_base/checks.h" @@ -184,14 +185,10 @@ AsyncResolverInterface* BasicPacketSocketFactory::CreateAsyncResolver() { return new AsyncResolver(); } -#pragma clang diagnostic push -#pragma clang diagnostic ignored "-Wdeprecated-declarations" std::unique_ptr BasicPacketSocketFactory::CreateAsyncDnsResolver() { - return std::make_unique( - new AsyncResolver()); + return std::make_unique(); } -#pragma clang diagnostic pop int BasicPacketSocketFactory::BindSocket(Socket* socket, const SocketAddress& local_address, diff --git a/p2p/base/mock_async_resolver.h b/p2p/base/mock_async_resolver.h index 44164716b2..73ad1a4c34 100644 --- a/p2p/base/mock_async_resolver.h +++ b/p2p/base/mock_async_resolver.h @@ -20,7 +20,10 @@ namespace rtc { using ::testing::_; using ::testing::InvokeWithoutArgs; -class MockAsyncResolver : public AsyncResolverInterface { +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" +class [[deprecated]] MockAsyncResolver : public AsyncResolverInterface { +#pragma clang diagnostic pop public: MockAsyncResolver() { ON_CALL(*this, Start(_)).WillByDefault(InvokeWithoutArgs([this] { @@ -47,11 +50,13 @@ class MockAsyncResolver : public AsyncResolverInterface { namespace webrtc { -class MockAsyncResolverFactory : public AsyncResolverFactory { +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" +class [[deprecated]] MockAsyncResolverFactory : public AsyncResolverFactory { public: MOCK_METHOD(rtc::AsyncResolverInterface*, Create, (), (override)); }; - +#pragma clang diagnostic pop } // namespace webrtc #endif // P2P_BASE_MOCK_ASYNC_RESOLVER_H_ diff --git a/p2p/base/turn_port.h b/p2p/base/turn_port.h index ac660d6599..8fa4607e51 100644 --- a/p2p/base/turn_port.h +++ b/p2p/base/turn_port.h @@ -267,7 +267,7 @@ class TurnPort : public Port { void HandleRefreshError(); bool SetAlternateServer(const rtc::SocketAddress& address); void ResolveTurnAddress(const rtc::SocketAddress& address); - void OnResolveResult(rtc::AsyncResolverInterface* resolver); + void OnResolveResult(const webrtc::AsyncDnsResolverResult& result); void AddRequestAuthInfo(StunMessage* msg); void OnSendStunPacket(const void* data, size_t size, StunRequest* request); diff --git a/p2p/stunprober/stun_prober.cc b/p2p/stunprober/stun_prober.cc index 977ead4d72..f5abf43beb 100644 --- a/p2p/stunprober/stun_prober.cc +++ b/p2p/stunprober/stun_prober.cc @@ -329,13 +329,12 @@ bool StunProber::Start(StunProber::Observer* observer) { } bool StunProber::ResolveServerName(const rtc::SocketAddress& addr) { - rtc::AsyncResolverInterface* resolver = - socket_factory_->CreateAsyncResolver(); - if (!resolver) { + RTC_DCHECK(!resolver_); + resolver_ = socket_factory_->CreateAsyncDnsResolver(); + if (!resolver_) { return false; } - resolver->SignalDone.connect(this, &StunProber::OnServerResolved); - resolver->Start(addr); + resolver_->Start(addr, [this] { OnServerResolved(resolver_->result()); }); return true; } @@ -347,20 +346,17 @@ void StunProber::OnSocketReady(rtc::AsyncPacketSocket* socket, } } -void StunProber::OnServerResolved(rtc::AsyncResolverInterface* resolver) { +void StunProber::OnServerResolved( + const webrtc::AsyncDnsResolverResult& result) { RTC_DCHECK(thread_checker_.IsCurrent()); - - if (resolver->GetError() == 0) { - rtc::SocketAddress addr(resolver->address().ipaddr(), - resolver->address().port()); + rtc::SocketAddress received_address; + if (result.GetResolvedAddress(AF_INET, &received_address)) { + // Construct an address without the name in it. + rtc::SocketAddress addr(received_address.ipaddr(), received_address.port()); all_servers_addrs_.push_back(addr); } - - // Deletion of AsyncResolverInterface can't be done in OnResolveResult which - // handles SignalDone. - thread_->PostTask([resolver] { resolver->Destroy(false); }); + resolver_.reset(); servers_.pop_back(); - if (servers_.size()) { if (!ResolveServerName(servers_.back())) { ReportOnPrepared(RESOLVE_FAILED); diff --git a/p2p/stunprober/stun_prober.h b/p2p/stunprober/stun_prober.h index 7d5094a3b9..3f0f4a2476 100644 --- a/p2p/stunprober/stun_prober.h +++ b/p2p/stunprober/stun_prober.h @@ -11,10 +11,12 @@ #ifndef P2P_STUNPROBER_STUN_PROBER_H_ #define P2P_STUNPROBER_STUN_PROBER_H_ +#include #include #include #include +#include "api/async_dns_resolver.h" #include "api/sequence_checker.h" #include "api/task_queue/pending_task_safety_flag.h" #include "rtc_base/byte_buffer.h" @@ -166,7 +168,7 @@ class RTC_EXPORT StunProber : public sigslot::has_slots<> { }; bool ResolveServerName(const rtc::SocketAddress& addr); - void OnServerResolved(rtc::AsyncResolverInterface* resolver); + void OnServerResolved(const webrtc::AsyncDnsResolverResult& resolver); void OnSocketReady(rtc::AsyncPacketSocket* socket, const rtc::SocketAddress& addr); @@ -241,6 +243,7 @@ class RTC_EXPORT StunProber : public sigslot::has_slots<> { ObserverAdapter observer_adapter_; const std::vector networks_; + std::unique_ptr resolver_; webrtc::ScopedTaskSafety task_safety_; }; diff --git a/p2p/stunprober/stun_prober_unittest.cc b/p2p/stunprober/stun_prober_unittest.cc index b57f93b634..ca20fccb6b 100644 --- a/p2p/stunprober/stun_prober_unittest.cc +++ b/p2p/stunprober/stun_prober_unittest.cc @@ -51,16 +51,23 @@ class StunProberTest : public ::testing::Test { rtc::InitializeSSL(); } + static constexpr int pings_per_ip = 3; + void set_expected_result(int result) { result_ = result; } + void CreateProber(rtc::PacketSocketFactory* socket_factory, + std::vector networks) { + prober_ = std::make_unique( + socket_factory, rtc::Thread::Current(), std::move(networks)); + } + void StartProbing(rtc::PacketSocketFactory* socket_factory, const std::vector& addrs, std::vector networks, bool shared_socket, uint16_t interval, uint16_t pings_per_ip) { - prober_ = std::make_unique( - socket_factory, rtc::Thread::Current(), std::move(networks)); + CreateProber(socket_factory, networks); prober_->Start(addrs, shared_socket, interval, pings_per_ip, 100 /* timeout_ms */, [this](StunProber* prober, int result) { @@ -69,13 +76,17 @@ class StunProberTest : public ::testing::Test { } void RunProber(bool shared_mode) { - const int pings_per_ip = 3; std::vector addrs; addrs.push_back(kStunAddr1); addrs.push_back(kStunAddr2); // Add a non-existing server. This shouldn't pollute the result. addrs.push_back(kFailedStunAddr); + RunProber(shared_mode, addrs, /* check_results= */ true); + } + void RunProber(bool shared_mode, + const std::vector& addrs, + bool check_results) { rtc::Network ipv4_network1("test_eth0", "Test Network Adapter 1", rtc::IPAddress(0x12345600U), 24); ipv4_network1.AddIP(rtc::IPAddress(0x12345678)); @@ -100,17 +111,21 @@ class StunProberTest : public ::testing::Test { WAIT(stopped_, 1000); - StunProber::Stats stats; - EXPECT_TRUE(prober_->GetStats(&stats)); - EXPECT_EQ(stats.success_percent, 100); - EXPECT_TRUE(stats.nat_type > stunprober::NATTYPE_NONE); - EXPECT_EQ(stats.srflx_addrs, srflx_addresses); - EXPECT_EQ(static_cast(stats.num_request_sent), - total_pings_reported); - EXPECT_EQ(static_cast(stats.num_response_received), - total_pings_reported); + EXPECT_TRUE(prober_->GetStats(&stats_)); + if (check_results) { + EXPECT_EQ(stats_.success_percent, 100); + EXPECT_TRUE(stats_.nat_type > stunprober::NATTYPE_NONE); + EXPECT_EQ(stats_.srflx_addrs, srflx_addresses); + EXPECT_EQ(static_cast(stats_.num_request_sent), + total_pings_reported); + EXPECT_EQ(static_cast(stats_.num_response_received), + total_pings_reported); + } } + StunProber* prober() { return prober_.get(); } + StunProber::Stats& stats() { return stats_; } + private: void StopCallback(StunProber* prober, int result) { EXPECT_EQ(result, result_); @@ -124,6 +139,7 @@ class StunProberTest : public ::testing::Test { bool stopped_ = false; std::unique_ptr stun_server_1_; std::unique_ptr stun_server_2_; + StunProber::Stats stats_; }; TEST_F(StunProberTest, NonSharedMode) { @@ -134,4 +150,26 @@ TEST_F(StunProberTest, SharedMode) { RunProber(true); } +TEST_F(StunProberTest, ResolveNonexistentHostname) { + std::vector addrs; + addrs.push_back(kStunAddr1); + // Add a non-existing server by name. This should cause a failed lookup. + addrs.push_back(rtc::SocketAddress("nonexistent.test", 3478)); + RunProber(false, addrs, false); + // One server is pinged + EXPECT_EQ(stats().raw_num_request_sent, pings_per_ip); +} + +TEST_F(StunProberTest, ResolveExistingHostname) { + std::vector addrs; + addrs.push_back(kStunAddr1); + // Add a non-existing server by name. This should cause a failed lookup. + addrs.push_back(rtc::SocketAddress("localhost", 3478)); + RunProber(false, addrs, false); + // Two servers are pinged, only one responds. + // TODO(bugs.webrtc.org/15559): Figure out why this doesn't always work + // EXPECT_EQ(stats().raw_num_request_sent, pings_per_ip * 2); + EXPECT_EQ(stats().num_request_sent, pings_per_ip); +} + } // namespace stunprober diff --git a/rtc_base/async_resolver.h b/rtc_base/async_resolver.h index 46be43860e..9de4d12fed 100644 --- a/rtc_base/async_resolver.h +++ b/rtc_base/async_resolver.h @@ -39,7 +39,12 @@ namespace rtc { // happen from the same rtc::Thread, except for Destroy which is allowed to // happen on another context provided it's not happening concurrently to another // public API call, and is the last access to the object. -class RTC_EXPORT AsyncResolver : public AsyncResolverInterface { +// TODO(bugs.webrtc.org/12598): Deprecate and remove +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wstrict-aliasing" +class [[deprecated("Use AsyncDnsResolver")]] RTC_EXPORT AsyncResolver + : public AsyncResolverInterface { +#pragma clang diagnostic pop public: AsyncResolver(); ~AsyncResolver() override; diff --git a/rtc_base/async_resolver_interface.h b/rtc_base/async_resolver_interface.h index 851fa38ce1..a0bda2774a 100644 --- a/rtc_base/async_resolver_interface.h +++ b/rtc_base/async_resolver_interface.h @@ -20,7 +20,7 @@ namespace rtc { // This interface defines the methods to resolve the address asynchronously. // TODO(bugs.webrtc.org/12598): Deprecate and remove. -class RTC_EXPORT AsyncResolverInterface { +class [[deprecated("Use AsyncDnsResolver")]] RTC_EXPORT AsyncResolverInterface { public: AsyncResolverInterface(); virtual ~AsyncResolverInterface();