Add is_speculative flag to DNS API.

Slight behavior change to never return any address results when the
flag is set (to prevent misuse). Affects even the old API usage, but
should be safe since a quick check showed that all current usage of the
flag never reads the resulting addresses.

Bug: 821021
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I2579521d181022a8b0c425f959bcc0808f7ed7fa
Reviewed-on: https://chromium-review.googlesource.com/1177913
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584187}

net/dns/host_resolver.cc

@@ -203,6 +203,7 @@ HostResolver::RequestInfoToResolveHostParameters(
   parameters.dns_query_type =
       AddressFamilyToDnsQueryType(request_info.address_family());
   parameters.initial_priority = priority;
+  parameters.is_speculative = request_info.is_speculative();
 
   return parameters;
 }

net/dns/host_resolver.h

@@ -196,6 +196,12 @@ class NET_EXPORT HostResolver {
 
     // The initial net priority for the host resolution request.
     RequestPriority initial_priority = RequestPriority::DEFAULT_PRIORITY;
+
+    // Set |true| iff the host resolve request is only being made speculatively
+    // to fill the cache and the result addresses will not be used. The request
+    // will receive special logging/observer treatment, and the result addresses
+    // will always be |base::nullopt|.
+    bool is_speculative = false;
   };
 
   // Set Options.max_concurrent_resolves to this to select a default level

net/dns/host_resolver_impl.cc

@@ -618,14 +618,12 @@ class HostResolverImpl::RequestImpl
   RequestImpl(const NetLogWithSource& source_net_log,
               const HostPortPair& request_host,
               const base::Optional<ResolveHostParameters>& optional_parameters,
-              bool is_speculative,
               base::WeakPtr<HostResolverImpl> resolver)
       : RequestImpl(source_net_log,
                     request_host,
                     optional_parameters,
                     0 /* host_resolver_flags */,
                     true /* allow_cached_response */,
-                    is_speculative,
                     resolver) {}
 
   // Overload for use by the legacy Resolve() API. Has more advanced parameters
@@ -635,13 +633,11 @@ class HostResolverImpl::RequestImpl
               const base::Optional<ResolveHostParameters>& optional_parameters,
               HostResolverFlags host_resolver_flags,
               bool allow_cached_response,
-              bool is_speculative,
               base::WeakPtr<HostResolverImpl> resolver)
       : source_net_log_(source_net_log),
         request_host_(request_host),
         host_resolver_flags_(host_resolver_flags),
         allow_cached_response_(allow_cached_response),
-        is_speculative_(is_speculative),
         parameters_(optional_parameters ? optional_parameters.value()
                                         : ResolveHostParameters()),
         priority_(parameters_.initial_priority),
@@ -684,6 +680,7 @@ class HostResolverImpl::RequestImpl
     // completed.
     DCHECK(!complete_);
     DCHECK(!address_results_);
+    DCHECK(!parameters_.is_speculative);
 
     address_results_ = address_results;
   }
@@ -733,8 +730,6 @@ class HostResolverImpl::RequestImpl
 
   bool allow_cached_response() const { return allow_cached_response_; }
 
-  bool is_speculative() const { return is_speculative_; }
-
   const ResolveHostParameters& parameters() const { return parameters_; }
 
   RequestPriority priority() const { return priority_; }
@@ -758,7 +753,6 @@ class HostResolverImpl::RequestImpl
   const HostPortPair request_host_;
   const HostResolverFlags host_resolver_flags_;
   const bool allow_cached_response_;
-  const bool is_speculative_;
   const ResolveHostParameters parameters_;
 
   RequestPriority priority_;
@@ -827,7 +821,7 @@ class HostResolverImpl::LegacyRequestImpl : public HostResolver::Request {
     // Must call AssignCallback() before async results.
     DCHECK(callback_);
 
-    if (error == OK) {
+    if (error == OK && !inner_request_->parameters().is_speculative) {
       // Legacy API does not allow non-address results (eg TXT), so AddressList
       // is always expected to be present on OK.
       DCHECK(inner_request_->GetAddressResults());
@@ -1456,7 +1450,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
         base::Bind(&NetLogJobAttachCallback, request->source_net_log().source(),
                    priority()));
 
-    if (!request->is_speculative())
+    if (!request->parameters().is_speculative)
       had_non_speculative_request_ = true;
 
     requests_.Append(request);
@@ -1954,10 +1948,11 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
       LogFinishRequest(req->source_net_log(), entry.error());
       if (did_complete) {
         // Record effective total time from creation to completion.
-        RecordTotalTime(req->is_speculative(), false /* from_cache */,
+        RecordTotalTime(req->parameters().is_speculative,
+                        false /* from_cache */,
                         tick_clock_->NowTicks() - req->request_time());
       }
-      if (entry.error() == OK) {
+      if (entry.error() == OK && !req->parameters().is_speculative) {
         req->set_address_results(EnsurePortOnAddressList(
             entry.addresses(), req->request_host().port()));
       }
@@ -2145,7 +2140,6 @@ HostResolverImpl::CreateRequest(
     const NetLogWithSource& net_log,
     const base::Optional<ResolveHostParameters>& optional_parameters) {
   return std::make_unique<RequestImpl>(net_log, host, optional_parameters,
-                                       false /* is_speculative */,
                                        weak_ptr_factory_.GetWeakPtr());
 }
 
@@ -2164,13 +2158,13 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
       source_net_log, info.host_port_pair(),
       RequestInfoToResolveHostParameters(info, priority),
       info.host_resolver_flags(), info.allow_cached_response(),
-      info.is_speculative(), weak_ptr_factory_.GetWeakPtr());
+      weak_ptr_factory_.GetWeakPtr());
   auto wrapped_request =
       std::make_unique<LegacyRequestImpl>(std::move(request));
 
   int rv = wrapped_request->Start();
 
-  if (rv == OK) {
+  if (rv == OK && !info.is_speculative()) {
     DCHECK(wrapped_request->inner_request().GetAddressResults());
     *addresses = wrapped_request->inner_request().GetAddressResults().value();
   } else if (rv == ERR_IO_PENDING) {
@@ -2357,13 +2351,13 @@ int HostResolverImpl::Resolve(RequestImpl* request) {
       request->host_resolver_flags(), request->allow_cached_response(),
       false /* allow_stale */, nullptr /* stale_info */,
       request->source_net_log(), &addresses, &key);
-  if (rv == OK) {
+  if (rv == OK && !request->parameters().is_speculative) {
     request->set_address_results(
         EnsurePortOnAddressList(addresses, request->request_host().port()));
   }
   if (rv != ERR_DNS_CACHE_MISS) {
     LogFinishRequest(request->source_net_log(), rv);
-    RecordTotalTime(request->is_speculative(), true /* from_cache */,
+    RecordTotalTime(request->parameters().is_speculative, true /* from_cache */,
                     base::TimeDelta());
     return rv;
   }

net/dns/host_resolver_impl_unittest.cc

@@ -2891,6 +2891,35 @@ TEST_F(HostResolverImplTest, InputObjectsBoundToCallback_Async) {
   EXPECT_TRUE(result_request);
 }
 
+TEST_F(HostResolverImplTest, IsSpeculative_ResolveHost) {
+  proc_->AddRuleForAllFamilies("just.testing", "192.168.1.42");
+  proc_->SignalMultiple(1u);
+
+  HostResolver::ResolveHostParameters parameters;
+  parameters.is_speculative = true;
+
+  ResolveHostResponseHelper response(resolver_->CreateRequest(
+      HostPortPair("just.testing", 80), NetLogWithSource(), parameters));
+
+  EXPECT_THAT(response.result_error(), IsOk());
+  EXPECT_FALSE(response.request()->GetAddressResults());
+
+  ASSERT_EQ(1u, proc_->GetCaptureList().size());
+  EXPECT_EQ("just.testing", proc_->GetCaptureList()[0].hostname);
+
+  // Reresolve without the |is_speculative| flag should immediately return from
+  // cache.
+  ResolveHostResponseHelper response2(resolver_->CreateRequest(
+      HostPortPair("just.testing", 80), NetLogWithSource(), base::nullopt));
+
+  EXPECT_THAT(response2.result_error(), IsOk());
+  EXPECT_THAT(response2.request()->GetAddressResults().value().endpoints(),
+              testing::ElementsAre(CreateExpected("192.168.1.42", 80)));
+
+  EXPECT_EQ("just.testing", proc_->GetCaptureList()[0].hostname);
+  EXPECT_EQ(1u, proc_->GetCaptureList().size());  // No increase.
+}
+
 DnsConfig CreateValidDnsConfig() {
   IPAddress dns_ip(192, 168, 1, 0);
   DnsConfig config;

net/dns/mock_host_resolver.cc

@@ -67,19 +67,16 @@ class MockHostResolverBase::RequestImpl
                     optional_parameters,
                     0 /* host_resolver_flags */,
                     true /* allow_cached_response */,
-                    false /* is_speculative */,
                     resolver) {}
 
   RequestImpl(const HostPortPair& request_host,
               const base::Optional<ResolveHostParameters>& optional_parameters,
               HostResolverFlags host_resolver_flags,
               bool allow_cached_response,
-              bool is_speculative,
               base::WeakPtr<MockHostResolverBase> resolver)
       : request_host_(request_host),
         host_resolver_flags_(host_resolver_flags),
         allow_cached_response_(allow_cached_response),
-        is_speculative_(is_speculative),
         parameters_(optional_parameters ? optional_parameters.value()
                                         : ResolveHostParameters()),
         id_(0),
@@ -127,6 +124,7 @@ class MockHostResolverBase::RequestImpl
     // completed.
     DCHECK(!complete_);
     DCHECK(!address_results_);
+    DCHECK(!parameters_.is_speculative);
 
     address_results_ = address_results;
   }
@@ -148,8 +146,6 @@ class MockHostResolverBase::RequestImpl
 
   bool allow_cached_response() const { return allow_cached_response_; }
 
-  bool is_speculative() const { return is_speculative_; }
-
   const ResolveHostParameters& parameters() const { return parameters_; }
 
   size_t id() { return id_; }
@@ -167,7 +163,6 @@ class MockHostResolverBase::RequestImpl
   const HostPortPair request_host_;
   int host_resolver_flags_;
   bool allow_cached_response_;
-  bool is_speculative_;
   const ResolveHostParameters parameters_;
 
   base::Optional<AddressList> address_results_;
@@ -219,7 +214,7 @@ class MockHostResolverBase::LegacyRequestImpl : public HostResolver::Request {
     // Must call AssignCallback() before async results.
     DCHECK(callback_);
 
-    if (error == OK) {
+    if (error == OK && !inner_request_->parameters().is_speculative) {
       // Legacy API does not allow non-address results (eg TXT), so AddressList
       // is always expected to be present on OK.
       DCHECK(inner_request_->GetAddressResults());
@@ -263,8 +258,7 @@ int MockHostResolverBase::Resolve(const RequestInfo& info,
 
   auto request = std::make_unique<RequestImpl>(
       info.host_port_pair(), RequestInfoToResolveHostParameters(info, priority),
-      info.host_resolver_flags(), info.allow_cached_response(),
-      info.is_speculative(), AsWeakPtr());
+      info.host_resolver_flags(), info.allow_cached_response(), AsWeakPtr());
   auto wrapped_request =
       std::make_unique<LegacyRequestImpl>(std::move(request));
 
@@ -363,7 +357,7 @@ int MockHostResolverBase::Resolve(RequestImpl* request) {
       DnsQueryTypeToAddressFamily(request->parameters().dns_query_type),
       request->host_resolver_flags(), request->allow_cached_response(),
       &addresses);
-  if (rv == OK)
+  if (rv == OK && !request->parameters().is_speculative)
     request->set_address_results(addresses);
   if (rv != ERR_DNS_CACHE_MISS)
     return rv;
@@ -378,7 +372,7 @@ int MockHostResolverBase::Resolve(RequestImpl* request) {
         request->request_host(),
         DnsQueryTypeToAddressFamily(request->parameters().dns_query_type),
         request->host_resolver_flags(), &addresses);
-    if (rv == OK)
+    if (rv == OK && !request->parameters().is_speculative)
       request->set_address_results(addresses);
     return rv;
   }
@@ -469,7 +463,7 @@ void MockHostResolverBase::ResolveNow(size_t id) {
       ResolveProc(req->request_host(),
                   DnsQueryTypeToAddressFamily(req->parameters().dns_query_type),
                   req->host_resolver_flags(), &addresses);
-  if (error == OK)
+  if (error == OK && !req->parameters().is_speculative)
     req->set_address_results(addresses);
   req->OnAsyncCompleted(id, error);
 }

services/network/host_resolver.cc

@@ -31,6 +31,7 @@ ConvertOptionalParameters(
   net::HostResolver::ResolveHostParameters parameters;
   parameters.dns_query_type = mojo_parameters->dns_query_type;
   parameters.initial_priority = mojo_parameters->initial_priority;
+  parameters.is_speculative = mojo_parameters->is_speculative;
   return parameters;
 }
 }  // namespace

services/network/host_resolver_unittest.cc

@@ -671,5 +671,30 @@ TEST_F(HostResolverTest, CloseBinding_SubsequentRequest) {
   EXPECT_EQ(0u, resolver.GetNumOutstandingRequestsForTesting());
 }
 
+TEST_F(HostResolverTest, IsSpeculative) {
+  net::NetLog net_log;
+  std::unique_ptr<net::HostResolver> inner_resolver =
+      net::HostResolver::CreateDefaultResolver(&net_log);
+
+  HostResolver resolver(inner_resolver.get(), &net_log);
+
+  base::RunLoop run_loop;
+  mojom::ResolveHostClientPtr response_client_ptr;
+  TestResolveHostClient response_client(&response_client_ptr, &run_loop);
+  mojom::ResolveHostParametersPtr parameters =
+      mojom::ResolveHostParameters::New();
+  parameters->is_speculative = true;
+
+  // Resolve "localhost" because it should always resolve fast and locally, even
+  // when using a real HostResolver.
+  resolver.ResolveHost(net::HostPortPair("localhost", 80),
+                       std::move(parameters), std::move(response_client_ptr));
+  run_loop.Run();
+
+  EXPECT_EQ(net::OK, response_client.result_error());
+  EXPECT_FALSE(response_client.result_addresses());
+  EXPECT_EQ(0u, resolver.GetNumOutstandingRequestsForTesting());
+}
+
 }  // namespace
 }  // namespace network

services/network/public/mojom/host_resolver.mojom

@@ -55,6 +55,12 @@ struct ResolveHostParameters {
   // If set, the outstanding request can be controlled, eg cancelled, via the
   // handle.
   ResolveHostHandle&? control_handle;
+
+  // Set |true| iff the host resolve request is only being made speculatively
+  // to fill the cache and the result addresses will not be used. The request
+  // will receive special logging/observer treatment, and
+  // ResolveHostClient::OnComplete will not receive any addresses.
+  bool is_speculative = false;
 };
 
 // Interface that can be passed to code/processes without direct access to