Modernize host resolution in SpdySessionPool

Includes fix for an affected test with brittle logic for determining
request IDs in MockHostResolver.  The new API has different effects on
the logic for generating such IDs.

Also fixed HangingHostResolver to always return ERR_DNS_CACHE_MISS when
source is LOCAL_ONLY to match HangingHostResolver::ResolveFromCache()
behavior.

Bug: 922699
Change-Id: I388dfc2a42feb49ca055160eb1bbcee05bb05de9
Reviewed-on: https://chromium-review.googlesource.com/c/1457215
Auto-Submit: Eric Orth <ericorth@chromium.org>
Commit-Queue: Asanka Herath <asanka@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#632726}

net/dns/mock_host_resolver.cc

@@ -362,6 +362,12 @@ void MockHostResolverBase::ResolveAllPending() {
   }
 }
 
+size_t MockHostResolverBase::last_id() {
+  if (requests_.empty())
+    return 0;
+  return requests_.rbegin()->first;
+}
+
 void MockHostResolverBase::ResolveNow(size_t id) {
   auto it = requests_.find(id);
   if (it == requests_.end())
@@ -841,8 +847,12 @@ class HangingHostResolver::RequestImpl
     : public HostResolver::Request,
       public HostResolver::ResolveHostRequest {
  public:
-  RequestImpl(base::WeakPtr<HangingHostResolver> resolver, bool is_running)
-      : resolver_(resolver), is_running_(is_running) {}
+  RequestImpl(base::WeakPtr<HangingHostResolver> resolver,
+              bool is_running,
+              bool is_local_only)
+      : resolver_(resolver),
+        is_running_(is_running),
+        is_local_only_(is_local_only) {}
 
   ~RequestImpl() override {
     if (is_running_ && resolver_)
@@ -851,27 +861,41 @@ class HangingHostResolver::RequestImpl
 
   int Start(CompletionOnceCallback callback) override {
     DCHECK(resolver_);
+    if (is_local_only_)
+      return ERR_DNS_CACHE_MISS;
+
     is_running_ = true;
     return ERR_IO_PENDING;
   }
 
   const base::Optional<AddressList>& GetAddressResults() const override {
-    IMMEDIATE_CRASH();
+    DCHECK(is_local_only_);
+    static const base::NoDestructor<base::Optional<AddressList>> nullopt_result;
+    return *nullopt_result;
   }
 
   const base::Optional<std::vector<std::string>>& GetTextResults()
       const override {
-    IMMEDIATE_CRASH();
+    DCHECK(is_local_only_);
+    static const base::NoDestructor<base::Optional<std::vector<std::string>>>
+        nullopt_result;
+    return *nullopt_result;
   }
 
   const base::Optional<std::vector<HostPortPair>>& GetHostnameResults()
       const override {
-    IMMEDIATE_CRASH();
+    DCHECK(is_local_only_);
+    static const base::NoDestructor<base::Optional<std::vector<HostPortPair>>>
+        nullopt_result;
+    return *nullopt_result;
   }
 
   const base::Optional<HostCache::EntryStaleness>& GetStaleInfo()
       const override {
-    IMMEDIATE_CRASH();
+    DCHECK(is_local_only_);
+    static const base::NoDestructor<base::Optional<HostCache::EntryStaleness>>
+        nullopt_result;
+    return *nullopt_result;
   }
 
   void ChangeRequestPriority(RequestPriority priority) override {}
@@ -881,6 +905,7 @@ class HangingHostResolver::RequestImpl
   // outstanding request objects.
   base::WeakPtr<HangingHostResolver> resolver_;
   bool is_running_;
+  bool is_local_only_;
 
   DISALLOW_COPY_AND_ASSIGN(RequestImpl);
 };
@@ -894,8 +919,12 @@ HangingHostResolver::CreateRequest(
     const HostPortPair& host,
     const NetLogWithSource& source_net_log,
     const base::Optional<ResolveHostParameters>& optional_parameters) {
+  bool is_local_only =
+      optional_parameters
+          ? optional_parameters.value().source == HostResolverSource::LOCAL_ONLY
+          : false;
   return std::make_unique<RequestImpl>(weak_ptr_factory_.GetWeakPtr(),
-                                       false /* started */);
+                                       false /* started */, is_local_only);
 }
 
 int HangingHostResolver::Resolve(const RequestInfo& info,
@@ -905,7 +934,8 @@ int HangingHostResolver::Resolve(const RequestInfo& info,
                                  std::unique_ptr<Request>* request,
                                  const NetLogWithSource& net_log) {
   *request = std::make_unique<RequestImpl>(weak_ptr_factory_.GetWeakPtr(),
-                                           true /* started */);
+                                           true /* started */,
+                                           false /* is_local_only */);
   return ERR_IO_PENDING;
 }
 

net/dns/mock_host_resolver.h

@@ -151,6 +151,10 @@ class MockHostResolverBase
   // for async resolution, starting with 1. IDs are not reused. Once a request
   // completes, it is destroyed, and can no longer be accessed.
 
+  // Returns the ID of the most recently started still-active request. Zero if
+  // no requests are currently active.
+  size_t last_id();
+
   // Resolve request stored in |requests_|. Pass rv to callback.
   void ResolveNow(size_t id);
 
@@ -389,7 +393,9 @@ class RuleBasedHostResolverProc : public HostResolverProc {
 // Create rules that map all requests to localhost.
 RuleBasedHostResolverProc* CreateCatchAllHostResolverProc();
 
-// HangingHostResolver never completes its |Resolve| request.
+// HangingHostResolver never completes its |Resolve| request. As LOCAL_ONLY
+// requests are not allowed to complete asynchronously, they will always result
+// in |ERR_DNS_CACHE_MISS|.
 class HangingHostResolver : public HostResolver {
  public:
   HangingHostResolver();

net/http/http_proxy_connect_job_unittest.cc

@@ -524,7 +524,7 @@ TEST_P(HttpProxyConnectJobTest, RequestPriority) {
       EXPECT_FALSE(test_delegate.has_result());
 
       MockHostResolverBase* host_resolver = session_deps_.host_resolver.get();
-      int request_id = host_resolver->num_resolve();
+      size_t request_id = host_resolver->last_id();
       EXPECT_EQ(initial_priority, host_resolver->request_priority(request_id));
 
       connect_job->ChangePriority(static_cast<RequestPriority>(new_priority));

net/spdy/spdy_session_pool.cc

@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <utility>
 
+#include "base/bind.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
@@ -19,6 +20,8 @@
 #include "build/build_config.h"
 #include "net/base/address_list.h"
 #include "net/base/trace_constants.h"
+#include "net/dns/host_resolver.h"
+#include "net/dns/host_resolver_source.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_server_properties.h"
 #include "net/http/http_stream_request.h"
@@ -173,18 +176,19 @@ base::WeakPtr<SpdySession> SpdySessionPool::FindAvailableSession(
     return base::WeakPtr<SpdySession>();
 
   // Look up IP addresses from resolver cache.
-  HostResolver::RequestInfo resolve_info(key.host_port_pair());
-  AddressList addresses;
-  int rv = resolver_->ResolveFromCache(resolve_info, &addresses, net_log);
+  HostResolver::ResolveHostParameters parameters;
+  parameters.source = HostResolverSource::LOCAL_ONLY;
+  std::unique_ptr<HostResolver::ResolveHostRequest> request =
+      resolver_->CreateRequest(key.host_port_pair(), net_log, parameters);
+
+  int rv = request->Start(base::BindOnce([](int error) { NOTREACHED(); }));
   DCHECK_NE(rv, ERR_IO_PENDING);
   if (rv != OK)
     return base::WeakPtr<SpdySession>();
 
   // Check if we have a session through a domain alias.
-  for (AddressList::const_iterator address_it = addresses.begin();
-       address_it != addresses.end();
-       ++address_it) {
-    auto range = aliases_.equal_range(*address_it);
+  for (const auto& address : request->GetAddressResults().value().endpoints()) {
+    auto range = aliases_.equal_range(address);
     for (auto alias_it = range.first; alias_it != range.second; ++alias_it) {
       // We found an alias.
       const SpdySessionKey& alias_key = alias_it->second;