Rewrite StaleHostResolver around CreateRequest() API

Motivation is that HostResolver::Resolve() is soon to be deleted and
all usage converted to CreateRequest (or to network service APIs). Need
to ensure cronet/StaleHostResolver continue working during transition.

Fully implemented StaleHostResolver::CreateRequest().
StaleHostResolver::Resolve() now implemented using CreateRequest() and
HostResolver::ResolveLegacy().

As the new API is more focused on the ResolveHostRequest object, it
made more sense to convert the StaleHostResolver::RequestImpl to be a
direct implementation, rather than using a separate Handle. Main
difference is lifetime semantics. The object is now owned by the client
instead of self-destroying on completion.  Extra orphaned requests
(allowed to complete to backfill the cache) are then detached to be
owned by StaleHostResolver instance itself as the client will likely
destroy the request after getting results.

With added weak ptr back to StaleHostResolver, it became simpler to
have RequestImpl directly read options_ and implement the stale usable
checks rather than pass in a callback.

Bug: 922699
Change-Id: Iadb48c0d574cc47ff6f79a6d313164b126c1d76d
Reviewed-on: https://chromium-review.googlesource.com/c/1452699Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Commit-Queue: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629601}

components/cronet/stale_host_resolver.h

@@ -6,8 +6,9 @@
 #define COMPONENTS_CRONET_STALE_HOST_RESOLVER_H_
 
 #include <memory>
-#include <unordered_set>
+#include <unordered_map>
 
+#include "base/memory/weak_ptr.h"
 #include "base/time/default_tick_clock.h"
 #include "net/base/completion_once_callback.h"
 #include "net/dns/host_resolver.h"
@@ -106,8 +107,16 @@ class StaleHostResolver : public net::HostResolver {
   class RequestImpl;
   friend class StaleHostResolverTest;
 
-  // Called from |Request| when a request is complete and can be destroyed.
-  void OnRequestComplete(Request* request);
+  // Called on completion of |network_request| when completed asynchronously (a
+  // "network" request). Determines if the request is owned by a RequestImpl or
+  // if it is a detached request and handles appropriately.
+  void OnNetworkRequestComplete(ResolveHostRequest* network_request,
+                                base::WeakPtr<RequestImpl> stale_request,
+                                int error);
+
+  // Detach an inner request from a RequestImpl, letting it finish (and populate
+  // the host cache) as long as |this| is not destroyed.
+  void DetachRequest(std::unique_ptr<ResolveHostRequest> request);
 
   // Set |tick_clock_| for testing. Must be set before issuing any requests.
   void SetTickClockForTesting(const base::TickClock* tick_clock);
@@ -120,7 +129,14 @@ class StaleHostResolver : public net::HostResolver {
   const base::TickClock* tick_clock_ = base::DefaultTickClock::GetInstance();
 
   // Options that govern when a stale response can or can't be returned.
-  StaleOptions options_;
+  const StaleOptions options_;
+
+  // Requests not used for returned results but allowed to continue (unless
+  // |this| is destroyed) to backfill the cache.
+  std::unordered_map<ResolveHostRequest*, std::unique_ptr<ResolveHostRequest>>
+      detached_requests_;
+
+  base::WeakPtrFactory<StaleHostResolver> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(StaleHostResolver);
 };

components/cronet/stale_host_resolver_unittest.cc

@@ -32,6 +32,7 @@
 #include "net/dns/dns_test_util.h"
 #include "net/dns/host_resolver_impl.h"
 #include "net/dns/host_resolver_proc.h"
+#include "net/dns/public/dns_protocol.h"
 #include "net/http/http_network_session.h"
 #include "net/log/net_log.h"
 #include "net/log/net_log_with_source.h"
@@ -82,6 +83,25 @@ std::unique_ptr<net::DnsClient> CreateMockDnsClientForHosts() {
                                               net::MockDnsClientRuleList());
 }
 
+// Create a net::DnsClient where address requests for |kHostname| will hang
+// until unblocked via CompleteDelayedTransactions() and then fail.
+std::unique_ptr<net::MockDnsClient> CreateHangingMockDnsClient() {
+  net::DnsConfig config;
+  config.nameservers.push_back(net::IPEndPoint());
+
+  net::MockDnsClientRuleList rules;
+  rules.emplace_back(
+      kHostname, net::dns_protocol::kTypeA,
+      net::MockDnsClientRule::Result(net::MockDnsClientRule::FAIL),
+      true /* delay */);
+  rules.emplace_back(
+      kHostname, net::dns_protocol::kTypeAAAA,
+      net::MockDnsClientRule::Result(net::MockDnsClientRule::FAIL),
+      true /* delay */);
+
+  return std::make_unique<net::MockDnsClient>(config, std::move(rules));
+}
+
 class MockHostResolverProc : public net::HostResolverProc {
  public:
   // |result| is the net error code to return from resolution attempts.
@@ -397,6 +417,20 @@ TEST_F(StaleHostResolverTest, MAYBE_StaleCache) {
   EXPECT_EQ(kCacheAddress, resolve_addresses()[0].ToStringWithoutPort());
 }
 
+// If the resolver is destroyed before a stale cache entry is returned, the
+// resolve should not complete.
+TEST_F(StaleHostResolverTest, StaleCache_DestroyedResolver) {
+  SetStaleDelay(kNoStaleDelaySec);
+  CreateResolverWithDnsClient(CreateHangingMockDnsClient());
+  CreateCacheEntry(kAgeExpiredSec, net::OK);
+
+  Resolve();
+  DestroyResolver();
+  WaitForResolve();
+
+  EXPECT_FALSE(resolve_complete());
+}
+
 // Ensure that |use_stale_on_name_not_resolved| causes stale results to be
 // returned when ERR_NAME_NOT_RESOLVED is returned from network resolution.
 TEST_F(StaleHostResolverTest, StaleCacheNameNotResolvedEnabled) {