Modernize host resolution in NetworkQualityEstimatorUtil
Also improved MockHostResolver's handling of LOCAL_ONLY requests.
Bug: 922699
Change-Id: Ib5c94c1212ff06ddcbe97132993702b668e3ece7
Reviewed-on: https://chromium-review.googlesource.com/c/1457464
Commit-Queue: Eric Orth <ericorth@chromium.org>
Auto-Submit: Eric Orth <ericorth@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#631841}diff --git a/net/dns/mock_host_resolver.cc b/net/dns/mock_host_resolver.cc
index 87a4692..dbb22b3 100644
--- a/net/dns/mock_host_resolver.cc
+++ b/net/dns/mock_host_resolver.cc
@@ -453,6 +453,7 @@
next_request_id_(1),
num_resolve_(0),
num_resolve_from_cache_(0),
+ num_non_local_resolves_(0),
tick_clock_(base::DefaultTickClock::GetInstance()) {
rules_map_[HostResolverSource::ANY] = CreateCatchAllHostResolverProc();
rules_map_[HostResolverSource::SYSTEM] = CreateCatchAllHostResolverProc();
@@ -478,8 +479,10 @@
request->parameters().cache_usage, &addresses, &stale_info);
if (rv == OK && !request->parameters().is_speculative)
request->set_address_results(addresses, std::move(stale_info));
- if (rv != ERR_DNS_CACHE_MISS)
+ if (rv != ERR_DNS_CACHE_MISS ||
+ request->parameters().source == HostResolverSource::LOCAL_ONLY) {
return rv;
+ }
// Just like the real resolver, refuse to do anything with invalid
// hostnames.
@@ -543,7 +546,11 @@
cache_usage ==
HostResolver::ResolveHostParameters::CacheUsage::STALE_ALLOWED;
if (cache_.get() && cache_allowed) {
- HostCache::Key key(host.host(), dns_query_type, flags, source);
+ // Local-only requests search the cache for non-local-only results.
+ HostResolverSource effective_source =
+ source == HostResolverSource::LOCAL_ONLY ? HostResolverSource::ANY
+ : source;
+ HostCache::Key key(host.host(), dns_query_type, flags, effective_source);
const HostCache::Entry* entry;
HostCache::EntryStaleness stale_info = HostCache::kNotStale;
if (cache_usage ==
@@ -570,6 +577,7 @@
HostResolverSource source,
AddressList* addresses) {
DCHECK(rules_map_.find(source) != rules_map_.end());
+ ++num_non_local_resolves_;
AddressList addr;
int rv = rules_map_[source]->Resolve(host.host(), requested_address_family,
diff --git a/net/dns/mock_host_resolver.h b/net/dns/mock_host_resolver.h
index 9baa3f7..79c4dcf 100644
--- a/net/dns/mock_host_resolver.h
+++ b/net/dns/mock_host_resolver.h
@@ -177,6 +177,9 @@
return num_resolve_from_cache_;
}
+ // The number of times resolve was attempted non-locally.
+ size_t num_non_local_resolves() const { return num_non_local_resolves_; }
+
// Returns the RequestPriority of the last call to Resolve() (or
// DEFAULT_PRIORITY if Resolve() hasn't been called yet).
RequestPriority last_request_priority() const {
@@ -250,6 +253,7 @@
size_t num_resolve_;
size_t num_resolve_from_cache_;
+ size_t num_non_local_resolves_;
const base::TickClock* tick_clock_;
diff --git a/net/nqe/network_quality_estimator_util.cc b/net/nqe/network_quality_estimator_util.cc
index 91f5fd1b..d12934f 100644
--- a/net/nqe/network_quality_estimator_util.cc
+++ b/net/nqe/network_quality_estimator_util.cc
@@ -4,12 +4,16 @@
#include "net/nqe/network_quality_estimator_util.h"
-#include "net/base/address_list.h"
+#include <memory>
+
+#include "base/bind.h"
+#include "base/logging.h"
#include "net/base/host_port_pair.h"
#include "net/base/ip_address.h"
#include "net/base/ip_endpoint.h"
#include "net/base/net_errors.h"
#include "net/dns/host_resolver.h"
+#include "net/dns/host_resolver_source.h"
#include "net/log/net_log_with_source.h"
namespace net {
@@ -21,18 +25,20 @@
bool IsPrivateHost(HostResolver* host_resolver,
const HostPortPair& host_port_pair) {
// Try resolving |host_port_pair.host()| synchronously.
- HostResolver::RequestInfo resolve_info(host_port_pair);
- resolve_info.set_allow_cached_response(true);
- AddressList addresses;
- // Resolve synchronously using the resolver's cache.
- int rv = host_resolver->ResolveFromCache(resolve_info, &addresses,
- NetLogWithSource());
+ HostResolver::ResolveHostParameters parameters;
+ parameters.source = HostResolverSource::LOCAL_ONLY;
+ std::unique_ptr<HostResolver::ResolveHostRequest> request =
+ host_resolver->CreateRequest(host_port_pair, NetLogWithSource(),
+ parameters);
+ int rv = request->Start(base::BindOnce([](int error) { NOTREACHED(); }));
DCHECK_NE(rv, ERR_IO_PENDING);
- if (rv == OK && !addresses.empty()) {
+
+ if (rv == OK && request->GetAddressResults() &&
+ !request->GetAddressResults().value().empty()) {
// Checking only the first address should be sufficient.
- IPEndPoint ip_end_point = addresses.front();
- net::IPAddress ip_address = ip_end_point.address();
+ IPEndPoint ip_endpoint = request->GetAddressResults().value().front();
+ IPAddress ip_address = ip_endpoint.address();
if (!ip_address.IsPubliclyRoutable())
return true;
}
diff --git a/net/nqe/network_quality_estimator_util_unittest.cc b/net/nqe/network_quality_estimator_util_unittest.cc
index 0cc8daa..0f464f9 100644
--- a/net/nqe/network_quality_estimator_util_unittest.cc
+++ b/net/nqe/network_quality_estimator_util_unittest.cc
@@ -72,30 +72,25 @@
EXPECT_EQ(OK, callback.WaitForResult());
}
- EXPECT_EQ(2u, mock_host_resolver.num_resolve());
+ EXPECT_EQ(2u, mock_host_resolver.num_non_local_resolves());
EXPECT_FALSE(IsPrivateHost(&mock_host_resolver,
HostPortPair("2607:f8b0:4006:819::200e", 80)));
- EXPECT_EQ(1u, mock_host_resolver.num_resolve_from_cache());
EXPECT_TRUE(
IsPrivateHost(&mock_host_resolver, HostPortPair("192.168.0.1", 443)));
- EXPECT_EQ(2u, mock_host_resolver.num_resolve_from_cache());
EXPECT_FALSE(
IsPrivateHost(&mock_host_resolver, HostPortPair("92.168.0.1", 443)));
- EXPECT_EQ(3u, mock_host_resolver.num_resolve_from_cache());
EXPECT_TRUE(
IsPrivateHost(&mock_host_resolver, HostPortPair("example1.com", 443)));
- EXPECT_EQ(4u, mock_host_resolver.num_resolve_from_cache());
EXPECT_FALSE(
IsPrivateHost(&mock_host_resolver, HostPortPair("example2.com", 443)));
- EXPECT_EQ(5u, mock_host_resolver.num_resolve_from_cache());
// IsPrivateHost() should have queried only the resolver's cache.
- EXPECT_EQ(2u, mock_host_resolver.num_resolve());
+ EXPECT_EQ(2u, mock_host_resolver.num_non_local_resolves());
}
// Verify that IsPrivateHost() returns false for a hostname whose DNS
@@ -118,8 +113,7 @@
// Not in DNS host cache, so should not be marked as private.
EXPECT_FALSE(
IsPrivateHost(&mock_host_resolver, HostPortPair("example3.com", 443)));
- EXPECT_EQ(0u, mock_host_resolver.num_resolve());
- EXPECT_EQ(1u, mock_host_resolver.num_resolve_from_cache());
+ EXPECT_EQ(0u, mock_host_resolver.num_non_local_resolves());
{
// Resolve example3.com so that the resolution entry is cached.
@@ -132,14 +126,13 @@
NetLogWithSource());
EXPECT_EQ(ERR_IO_PENDING, rv);
EXPECT_EQ(OK, callback.WaitForResult());
- EXPECT_EQ(1u, mock_host_resolver.num_resolve());
+ EXPECT_EQ(1u, mock_host_resolver.num_non_local_resolves());
}
EXPECT_TRUE(
IsPrivateHost(&mock_host_resolver, HostPortPair("example3.com", 443)));
// IsPrivateHost() should have queried only the resolver's cache.
- EXPECT_EQ(1u, mock_host_resolver.num_resolve());
- EXPECT_EQ(2u, mock_host_resolver.num_resolve_from_cache());
+ EXPECT_EQ(1u, mock_host_resolver.num_non_local_resolves());
}
// Verify that IsPrivateHost() returns correct results for local hosts.
