Cancel requests on destruction of ContextHostResolver

This matches the HostResolverManager behavior where all requests get
silently cancelled if the manager is destroyed. Now if a
ContextHostResolver is destroyed, all requests created by that resolver
get silently cancelled, leaving requests from other resolvers alone.

Motivation:
For DoH, need to ensure that if a context (and its per-context resolver)
are destroyed, any requests potentially using that context for DoH are
cancelled. Previously, DoH always used the "primary" context and we took
special care to cancel all DNS requests on destruction of that context,
but DoH will soon use any context from the per-context resolvers.

Bug: 934402
Change-Id: I99d087bdd17ac8e2bd54efa4c425e63abf0568ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1549897Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#648713}

net/dns/BUILD.gn

@@ -356,6 +356,7 @@ source_set("mdns_client") {
 source_set("tests") {
   testonly = true
   sources = [
+    "context_host_resolver_unittest.cc",
     "dns_config_service_unittest.cc",
     "dns_config_service_win_unittest.cc",
     "dns_hosts_unittest.cc",

net/dns/context_host_resolver.cc

@@ -4,6 +4,7 @@
 
 #include "net/dns/context_host_resolver.h"
 
+#include <string>
 #include <utility>
 
 #include "base/logging.h"
@@ -17,6 +18,67 @@
 
 namespace net {
 
+// Wrapper of ResolveHostRequests that on destruction will remove itself from
+// |ContextHostResolver::active_requests_|.
+class ContextHostResolver::WrappedRequest
+    : public HostResolver::ResolveHostRequest {
+ public:
+  WrappedRequest(
+      std::unique_ptr<HostResolverManager::CancellableRequest> inner_request,
+      ContextHostResolver* resolver)
+      : inner_request_(std::move(inner_request)), resolver_(resolver) {}
+
+  ~WrappedRequest() override { Cancel(); }
+
+  void Cancel() {
+    // Cannot destroy |inner_request_| because it is still allowed to call
+    // Get...Results() methods if the request was already complete.
+    inner_request_->Cancel();
+
+    if (resolver_) {
+      resolver_->active_requests_.erase(this);
+      resolver_ = nullptr;
+    }
+  }
+
+  int Start(CompletionOnceCallback callback) override {
+    DCHECK(resolver_);
+    return inner_request_->Start(std::move(callback));
+  }
+
+  const base::Optional<AddressList>& GetAddressResults() const override {
+    return inner_request_->GetAddressResults();
+  }
+
+  const base::Optional<std::vector<std::string>>& GetTextResults()
+      const override {
+    return inner_request_->GetTextResults();
+  }
+
+  const base::Optional<std::vector<HostPortPair>>& GetHostnameResults()
+      const override {
+    return inner_request_->GetHostnameResults();
+  }
+
+  const base::Optional<HostCache::EntryStaleness>& GetStaleInfo()
+      const override {
+    return inner_request_->GetStaleInfo();
+  }
+
+  void ChangeRequestPriority(RequestPriority priority) override {
+    inner_request_->ChangeRequestPriority(priority);
+  }
+
+ private:
+  std::unique_ptr<HostResolverManager::CancellableRequest> inner_request_;
+
+  // Resolver is expected to call Cancel() on destruction, clearing the pointer
+  // before it becomes invalid.
+  ContextHostResolver* resolver_;
+
+  DISALLOW_COPY_AND_ASSIGN(WrappedRequest);
+};
+
 ContextHostResolver::ContextHostResolver(HostResolverManager* manager)
     : manager_(manager) {
   DCHECK(manager_);
@@ -31,6 +93,10 @@ ContextHostResolver::ContextHostResolver(
 ContextHostResolver::~ContextHostResolver() {
   if (owned_manager_)
     DCHECK_EQ(owned_manager_.get(), manager_);
+
+  // Silently cancel all requests associated with this resolver.
+  while (!active_requests_.empty())
+    (*active_requests_.begin())->Cancel();
 }
 
 std::unique_ptr<HostResolver::ResolveHostRequest>
@@ -39,7 +105,10 @@ ContextHostResolver::CreateRequest(
     const NetLogWithSource& source_net_log,
     const base::Optional<ResolveHostParameters>& optional_parameters) {
   // TODO(crbug.com/934402): DHCECK |context_| once universally set.
-  return manager_->CreateRequest(host, source_net_log, optional_parameters);
+  auto request = std::make_unique<WrappedRequest>(
+      manager_->CreateRequest(host, source_net_log, optional_parameters), this);
+  active_requests_.insert(request.get());
+  return request;
 }
 
 std::unique_ptr<HostResolver::MdnsListener>

net/dns/context_host_resolver.h

@@ -6,6 +6,7 @@
 #define NET_DNS_CONTEXT_HOST_RESOLVER_H_
 
 #include <memory>
+#include <unordered_set>
 #include <vector>
 
 #include "base/macros.h"
@@ -75,10 +76,20 @@ class NET_EXPORT ContextHostResolver : public HostResolver {
   void SetBaseDnsConfigForTesting(const DnsConfig& base_config);
   void SetTickClockForTesting(const base::TickClock* tick_clock);
 
+  size_t GetNumActiveRequestsForTesting() const {
+    return active_requests_.size();
+  }
+
  private:
+  class WrappedRequest;
+
   HostResolverManager* const manager_;
   std::unique_ptr<HostResolverManager> owned_manager_;
 
+  // Requests are expected to clear themselves from this set on destruction or
+  // cancellation.
+  std::unordered_set<WrappedRequest*> active_requests_;
+
   URLRequestContext* context_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(ContextHostResolver);

net/dns/host_resolver_manager.cc

@@ -490,7 +490,7 @@ const unsigned HostResolverManager::kMaximumDnsFailures = 16;
 // cancellation is initiated by the Job (OnJobCancelled) vs by the end user
 // (~RequestImpl).
 class HostResolverManager::RequestImpl
-    : public HostResolver::ResolveHostRequest,
+    : public CancellableRequest,
       public base::LinkNode<HostResolverManager::RequestImpl> {
  public:
   RequestImpl(const NetLogWithSource& source_net_log,
@@ -508,7 +508,12 @@ class HostResolverManager::RequestImpl
         resolver_(resolver),
         complete_(false) {}
 
-  ~RequestImpl() override;
+  ~RequestImpl() override {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    Cancel();
+  }
+
+  void Cancel() override;
 
   int Start(CompletionOnceCallback callback) override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -2252,7 +2257,7 @@ void HostResolverManager::SetDnsClient(std::unique_ptr<DnsClient> dns_client) {
   UpdateModeForHistogram(dns_config);
 }
 
-std::unique_ptr<HostResolver::ResolveHostRequest>
+std::unique_ptr<HostResolverManager::CancellableRequest>
 HostResolverManager::CreateRequest(
     const HostPortPair& host,
     const NetLogWithSource& net_log,
@@ -3133,10 +3138,14 @@ void HostResolverManager::UpdateModeForHistogram(const DnsConfig& dns_config) {
   }
 }
 
-HostResolverManager::RequestImpl::~RequestImpl() {
+void HostResolverManager::RequestImpl::Cancel() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (job_)
+  if (!job_)
+    return;
+
   job_->CancelRequest(this);
+  job_ = nullptr;
+  callback_.Reset();
 }
 
 void HostResolverManager::RequestImpl::ChangeRequestPriority(

net/dns/host_resolver_manager.h

@@ -87,6 +87,14 @@ class NET_EXPORT HostResolverManager
   using ResolveHostRequest = HostResolver::ResolveHostRequest;
   using ResolveHostParameters = HostResolver::ResolveHostParameters;
 
+  class CancellableRequest : public ResolveHostRequest {
+   public:
+    // If running asynchronously, silently cancels the request as if destroyed.
+    // Callbacks will never be invoked. Noop if request is already complete or
+    // never started.
+    virtual void Cancel() = 0;
+  };
+
   // Creates a HostResolver as specified by |options|. Blocking tasks are run in
   // TaskScheduler.
   //
@@ -111,8 +119,7 @@ class NET_EXPORT HostResolverManager
   // NetworkChangeNotifier.
   void SetDnsClient(std::unique_ptr<DnsClient> dns_client);
 
-  // HostResolver methods:
-  std::unique_ptr<ResolveHostRequest> CreateRequest(
+  std::unique_ptr<CancellableRequest> CreateRequest(
       const HostPortPair& host,
       const NetLogWithSource& net_log,
       const base::Optional<ResolveHostParameters>& optional_parameters);
@@ -159,6 +166,15 @@ class NET_EXPORT HostResolverManager
 
   void SetBaseDnsConfigForTesting(const DnsConfig& base_config);
 
+  // Allows the tests to catch slots leaking out of the dispatcher.  One
+  // HostResolverManager::Job could occupy multiple PrioritizedDispatcher job
+  // slots.
+  size_t num_running_dispatcher_jobs_for_tests() const {
+    return dispatcher_->num_running_jobs();
+  }
+
+  size_t num_jobs_for_testing() const { return jobs_.size(); }
+
  protected:
   // Callback from HaveOnlyLoopbackAddresses probe.
   void SetHaveOnlyLoopbackAddresses(bool result);
@@ -332,13 +348,6 @@ class NET_EXPORT HostResolverManager
 
   int GetOrCreateMdnsClient(MDnsClient** out_client);
 
-  // Allows the tests to catch slots leaking out of the dispatcher.  One
-  // HostResolverManager::Job could occupy multiple PrioritizedDispatcher job
-  // slots.
-  size_t num_running_dispatcher_jobs_for_tests() const {
-    return dispatcher_->num_running_jobs();
-  }
-
   // Update |mode_for_histogram_|. Called when DNS config changes. |dns_config|
   // is the current DNS config and is only used if !HaveDnsConfig().
   void UpdateModeForHistogram(const DnsConfig& dns_config);

net/dns/host_resolver_manager_unittest.cc

@@ -244,13 +244,13 @@ class ResolveHostResponseHelper {
 
   ResolveHostResponseHelper() {}
   explicit ResolveHostResponseHelper(
-      std::unique_ptr<HostResolver::ResolveHostRequest> request)
+      std::unique_ptr<HostResolverManager::CancellableRequest> request)
       : request_(std::move(request)) {
     result_error_ = request_->Start(base::BindOnce(
         &ResolveHostResponseHelper::OnComplete, base::Unretained(this)));
   }
   ResolveHostResponseHelper(
-      std::unique_ptr<HostResolver::ResolveHostRequest> request,
+      std::unique_ptr<HostResolverManager::CancellableRequest> request,
       Callback custom_callback)
       : request_(std::move(request)) {
     result_error_ = request_->Start(
@@ -265,7 +265,7 @@ class ResolveHostResponseHelper {
     return result_error_;
   }
 
-  HostResolver::ResolveHostRequest* request() { return request_.get(); }
+  HostResolverManager::CancellableRequest* request() { return request_.get(); }
 
   void CancelRequest() {
     DCHECK(request_);
@@ -291,7 +291,7 @@ class ResolveHostResponseHelper {
     DCHECK(complete());
   }
 
-  std::unique_ptr<HostResolver::ResolveHostRequest> request_;
+  std::unique_ptr<HostResolverManager::CancellableRequest> request_;
   int result_error_ = ERR_IO_PENDING;
   base::RunLoop run_loop_;
 
@@ -3993,6 +3993,57 @@ TEST_F(HostResolverManagerDnsTest, DeleteWithActiveTransactions) {
   }
 }
 
+TEST_F(HostResolverManagerDnsTest, DeleteWithCompletedRequests) {
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  ResolveHostResponseHelper response(resolver_->CreateRequest(
+      HostPortPair("ok", 80), NetLogWithSource(), base::nullopt));
+
+  EXPECT_THAT(response.result_error(), IsOk());
+  EXPECT_THAT(response.request()->GetAddressResults().value().endpoints(),
+              testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 80),
+                                            CreateExpected("::1", 80)));
+
+  resolver_.reset();
+
+  // Completed requests should be unaffected by manager destruction.
+  EXPECT_THAT(response.request()->GetAddressResults().value().endpoints(),
+              testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 80),
+                                            CreateExpected("::1", 80)));
+}
+
+TEST_F(HostResolverManagerDnsTest, ExplicitCancel) {
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  ResolveHostResponseHelper response(resolver_->CreateRequest(
+      HostPortPair("4slow_4ok", 80), NetLogWithSource(), base::nullopt));
+
+  response.request()->Cancel();
+  dns_client_->CompleteDelayedTransactions();
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(response.complete());
+}
+
+TEST_F(HostResolverManagerDnsTest, ExplicitCancel_Completed) {
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  ResolveHostResponseHelper response(resolver_->CreateRequest(
+      HostPortPair("ok", 80), NetLogWithSource(), base::nullopt));
+
+  EXPECT_THAT(response.result_error(), IsOk());
+  EXPECT_THAT(response.request()->GetAddressResults().value().endpoints(),
+              testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 80),
+                                            CreateExpected("::1", 80)));
+
+  response.request()->Cancel();
+
+  // Completed requests should be unaffected by cancellation.
+  EXPECT_THAT(response.request()->GetAddressResults().value().endpoints(),
+              testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 80),
+                                            CreateExpected("::1", 80)));
+}
+
 // Cancel a request with only the IPv6 transaction active.
 TEST_F(HostResolverManagerDnsTest, CancelWithIPv6TransactionActive) {
   ChangeDnsConfig(CreateValidDnsConfig());