Implement ContextHostResolver::OnShutdown()

Cancels pending requests causing them to never complete, same as
previous behavior for destroying the resolver. Allows stopping all
requests at the start of URLRequestContext destruction to ensure the
resolver stops working and stops any action that could depend upon
destructing context objects.

Since lots of inter-dependent context objects also have references back
to the resolver, subsequent requests are also blocked to ensure
problematic requests can not be triggered during context destruction.
Subsequently-started requests (whether created before or after the
OnShutdown() call) will always immediately fail with
ERR_CONTEXT_SHUT_DOWN. More self-consistent would be for these requests
to just hang as if they were started and cancelled, but this could
break code making assumptions about requests that should always return
a synchronous result.

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

net/dns/context_host_resolver.cc

@@ -8,8 +8,10 @@
 #include <utility>
 
 #include "base/logging.h"
+#include "base/no_destructor.h"
 #include "base/strings/string_piece.h"
 #include "base/time/tick_clock.h"
+#include "net/base/net_errors.h"
 #include "net/dns/dns_config.h"
 #include "net/dns/host_cache.h"
 #include "net/dns/host_resolver_manager.h"
@@ -19,53 +21,114 @@
 namespace net {
 
 // Wrapper of ResolveHostRequests that on destruction will remove itself from
-// |ContextHostResolver::active_requests_|.
+// |ContextHostResolver::handed_out_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) {}
+      ContextHostResolver* resolver,
+      bool shutting_down)
+      : inner_request_(std::move(inner_request)),
+        resolver_(resolver),
+        shutting_down_(shutting_down) {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(resolver_->sequence_checker_);
+  }
 
   ~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();
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    OnShutdown();
 
     if (resolver_) {
-      resolver_->active_requests_.erase(this);
+      DCHECK_EQ(1u, resolver_->handed_out_requests_.count(this));
+      resolver_->handed_out_requests_.erase(this);
       resolver_ = nullptr;
     }
   }
 
+  void OnShutdown() {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+    // Cannot destroy |inner_request_| because it is still allowed to call
+    // Get...Results() methods if the request was already complete.
+    if (inner_request_)
+      inner_request_->Cancel();
+
+    shutting_down_ = true;
+
+    // Not clearing |resolver_| so that early shutdown can be differentiated in
+    // Start() from full cancellation on resolver destruction.
+  }
+
   int Start(CompletionOnceCallback callback) override {
-    DCHECK(resolver_);
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+    if (!resolver_) {
+      // Parent resolver has been destroyed. HostResolver generally disallows
+      // calling Start() in this case, but this implementation returns
+      // ERR_FAILED to allow testing the case.
+      inner_request_ = nullptr;
+      return ERR_FAILED;
+    }
+
+    if (shutting_down_) {
+      // Shutting down but the resolver is not yet destroyed.
+      inner_request_ = nullptr;
+      return ERR_CONTEXT_SHUT_DOWN;
+    }
+
+    DCHECK(inner_request_);
     return inner_request_->Start(std::move(callback));
   }
 
   const base::Optional<AddressList>& GetAddressResults() const override {
+    if (!inner_request_) {
+      static base::NoDestructor<base::Optional<AddressList>> nullopt_result;
+      return *nullopt_result;
+    }
+
     return inner_request_->GetAddressResults();
   }
 
   const base::Optional<std::vector<std::string>>& GetTextResults()
       const override {
+    if (!inner_request_) {
+      static const base::NoDestructor<base::Optional<std::vector<std::string>>>
+          nullopt_result;
+      return *nullopt_result;
+    }
+
     return inner_request_->GetTextResults();
   }
 
   const base::Optional<std::vector<HostPortPair>>& GetHostnameResults()
       const override {
+    if (!inner_request_) {
+      static const base::NoDestructor<base::Optional<std::vector<HostPortPair>>>
+          nullopt_result;
+      return *nullopt_result;
+    }
+
     return inner_request_->GetHostnameResults();
   }
 
   const base::Optional<HostCache::EntryStaleness>& GetStaleInfo()
       const override {
+    if (!inner_request_) {
+      static const base::NoDestructor<base::Optional<HostCache::EntryStaleness>>
+          nullopt_result;
+      return *nullopt_result;
+    }
+
     return inner_request_->GetStaleInfo();
   }
 
   void ChangeRequestPriority(RequestPriority priority) override {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    DCHECK(inner_request_);
+
     inner_request_->ChangeRequestPriority(priority);
   }
 
@@ -75,6 +138,9 @@ class ContextHostResolver::WrappedRequest
   // Resolver is expected to call Cancel() on destruction, clearing the pointer
   // before it becomes invalid.
   ContextHostResolver* resolver_;
+  bool shutting_down_ = false;
+
+  SEQUENCE_CHECKER(sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(WrappedRequest);
 };
@@ -108,12 +174,21 @@ ContextHostResolver::~ContextHostResolver() {
     manager_->RemoveHostCacheInvalidator(host_cache_->invalidator());
 
   // Silently cancel all requests associated with this resolver.
-  while (!active_requests_.empty())
-    (*active_requests_.begin())->Cancel();
+  while (!handed_out_requests_.empty())
+    (*handed_out_requests_.begin())->Cancel();
 }
 
 void ContextHostResolver::OnShutdown() {
-  // TODO(crbug.com/1006902): Implement.
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  for (auto* active_request : handed_out_requests_)
+    active_request->OnShutdown();
+
+  context_ = nullptr;
+  shutting_down_ = true;
+
+  // TODO(crbug.com/1006902): Cancel DoH prober requests too if using
+  // |context_|.
 }
 
 std::unique_ptr<HostResolver::ResolveHostRequest>
@@ -121,11 +196,17 @@ ContextHostResolver::CreateRequest(
     const HostPortPair& host,
     const NetLogWithSource& source_net_log,
     const base::Optional<ResolveHostParameters>& optional_parameters) {
-  auto request = std::make_unique<WrappedRequest>(
-      manager_->CreateRequest(host, source_net_log, optional_parameters,
-                              context_, host_cache_.get()),
-      this);
-  active_requests_.insert(request.get());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  std::unique_ptr<HostResolverManager::CancellableRequest> inner_request;
+  if (!shutting_down_) {
+    inner_request = manager_->CreateRequest(
+        host, source_net_log, optional_parameters, context_, host_cache_.get());
+  }
+
+  auto request = std::make_unique<WrappedRequest>(std::move(inner_request),
+                                                  this, shutting_down_);
+  handed_out_requests_.insert(request.get());
   return request;
 }
 
@@ -147,6 +228,8 @@ void ContextHostResolver::SetRequestContext(
     URLRequestContext* request_context) {
   DCHECK(request_context);
   DCHECK(!context_);
+  DCHECK(!shutting_down_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   context_ = request_context;
 }

net/dns/context_host_resolver.h

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/sequence_checker.h"
 #include "net/base/net_export.h"
 #include "net/dns/host_resolver.h"
 
@@ -67,7 +68,7 @@ class NET_EXPORT ContextHostResolver : public HostResolver {
   void SetTickClockForTesting(const base::TickClock* tick_clock);
 
   size_t GetNumActiveRequestsForTesting() const {
-    return active_requests_.size();
+    return handed_out_requests_.size();
   }
 
  private:
@@ -77,12 +78,20 @@ class NET_EXPORT ContextHostResolver : public HostResolver {
   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_;
+  // cancellation.  Requests in an early shutdown state (from
+  // HostResolver::OnShutdown()) are still in this set, so they can be notified
+  // on resolver destruction.
+  std::unordered_set<WrappedRequest*> handed_out_requests_;
 
   URLRequestContext* context_ = nullptr;
   std::unique_ptr<HostCache> host_cache_;
 
+  // If true, the context is shutting down. Subsequent request Start() calls
+  // will always fail immediately with ERR_CONTEXT_SHUT_DOWN.
+  bool shutting_down_ = false;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+
   DISALLOW_COPY_AND_ASSIGN(ContextHostResolver);
 };
 

net/dns/context_host_resolver_unittest.cc

@@ -21,6 +21,7 @@
 #include "net/dns/dns_util.h"
 #include "net/dns/host_cache.h"
 #include "net/dns/host_resolver_manager.h"
+#include "net/dns/host_resolver_source.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/dns/public/dns_protocol.h"
 #include "net/log/net_log_with_source.h"
@@ -100,7 +101,7 @@ TEST_F(ContextHostResolverTest, Resolve) {
 
 // Test that destroying a request silently cancels that request.
 TEST_F(ContextHostResolverTest, DestroyRequest) {
-  // Setup delayed results for "example.com".
+  // Set up delayed results for "example.com".
   MockDnsClientRuleList rules;
   rules.emplace_back("example.com", dns_protocol::kTypeA, false /* secure */,
                      MockDnsClientRule::Result(BuildTestDnsResponse(
@@ -134,7 +135,7 @@ TEST_F(ContextHostResolverTest, DestroyRequest) {
 
 // Test that cancelling a resolver cancels its (and only its) requests.
 TEST_F(ContextHostResolverTest, DestroyResolver) {
-  // Setup delayed results for "example.com" and "google.com".
+  // Set up delayed results for "example.com" and "google.com".
   MockDnsClientRuleList rules;
   rules.emplace_back("example.com", dns_protocol::kTypeA, false /* secure */,
                      MockDnsClientRule::Result(BuildTestDnsResponse(
@@ -187,7 +188,7 @@ TEST_F(ContextHostResolverTest, DestroyResolver) {
 // Test that cancelling a resolver cancels its (and only its) requests, even if
 // those requests shared a job (same query) with another resolver's requests.
 TEST_F(ContextHostResolverTest, DestroyResolver_RemainingRequests) {
-  // Setup delayed results for "example.com".
+  // Set up delayed results for "example.com".
   MockDnsClientRuleList rules;
   rules.emplace_back("example.com", dns_protocol::kTypeA, false /* secure */,
                      MockDnsClientRule::Result(BuildTestDnsResponse(
@@ -260,6 +261,142 @@ TEST_F(ContextHostResolverTest, DestroyResolver_CompletedRequests) {
               testing::ElementsAre(kEndpoint));
 }
 
+// Test a request created before resolver destruction but not yet started.
+TEST_F(ContextHostResolverTest, DestroyResolver_DelayedStartRequest) {
+  // Set up delayed result for "example.com".
+  MockDnsClientRuleList rules;
+  rules.emplace_back("example.com", dns_protocol::kTypeA, false /* secure */,
+                     MockDnsClientRule::Result(BuildTestDnsResponse(
+                         "example.com", IPAddress(2, 3, 4, 5))),
+                     true /* delay */);
+  rules.emplace_back("example.com", dns_protocol::kTypeAAAA, false /* secure */,
+                     MockDnsClientRule::Result(MockDnsClientRule::EMPTY),
+                     false /* delay */);
+
+  auto resolver = std::make_unique<ContextHostResolver>(
+      manager_.get(), nullptr /* host_cache */);
+  std::unique_ptr<HostResolver::ResolveHostRequest> request =
+      resolver->CreateRequest(HostPortPair("example.com", 100),
+                              NetLogWithSource(), base::nullopt);
+
+  resolver = nullptr;
+
+  TestCompletionCallback callback;
+  int rv = request->Start(callback.callback());
+
+  EXPECT_THAT(callback.GetResult(rv), test::IsError(ERR_FAILED));
+  EXPECT_FALSE(request->GetAddressResults());
+}
+
+TEST_F(ContextHostResolverTest, OnShutdown_PendingRequest) {
+  // Set up delayed result for "example.com".
+  MockDnsClientRuleList rules;
+  rules.emplace_back("example.com", dns_protocol::kTypeA, false /* secure */,
+                     MockDnsClientRule::Result(BuildTestDnsResponse(
+                         "example.com", IPAddress(2, 3, 4, 5))),
+                     true /* delay */);
+  rules.emplace_back("example.com", dns_protocol::kTypeAAAA, false /* secure */,
+                     MockDnsClientRule::Result(MockDnsClientRule::EMPTY),
+                     false /* delay */);
+  SetMockDnsRules(std::move(rules));
+
+  auto resolver = std::make_unique<ContextHostResolver>(
+      manager_.get(), nullptr /* host_cache */);
+  std::unique_ptr<HostResolver::ResolveHostRequest> request =
+      resolver->CreateRequest(HostPortPair("example.com", 100),
+                              NetLogWithSource(), base::nullopt);
+
+  TestCompletionCallback callback;
+  int rv = request->Start(callback.callback());
+
+  // Trigger shutdown before allowing request to complete.
+  resolver->OnShutdown();
+  dns_client_->CompleteDelayedTransactions();
+
+  // Ensure request never completes.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_THAT(rv, test::IsError(ERR_IO_PENDING));
+  EXPECT_FALSE(callback.have_result());
+}
+
+TEST_F(ContextHostResolverTest, OnShutdown_CompletedRequests) {
+  MockDnsClientRuleList rules;
+  rules.emplace_back("example.com", dns_protocol::kTypeA, false /* secure */,
+                     MockDnsClientRule::Result(BuildTestDnsResponse(
+                         "example.com", kEndpoint.address())),
+                     false /* delay */);
+  rules.emplace_back("example.com", dns_protocol::kTypeAAAA, false /* secure */,
+                     MockDnsClientRule::Result(MockDnsClientRule::EMPTY),
+                     false /* delay */);
+  SetMockDnsRules(std::move(rules));
+
+  auto resolver = std::make_unique<ContextHostResolver>(
+      manager_.get(), nullptr /* host_cache */);
+  std::unique_ptr<HostResolver::ResolveHostRequest> request =
+      resolver->CreateRequest(HostPortPair("example.com", 100),
+                              NetLogWithSource(), base::nullopt);
+
+  // Complete request and then shutdown the resolver.
+  TestCompletionCallback callback;
+  int rv = request->Start(callback.callback());
+  ASSERT_THAT(callback.GetResult(rv), test::IsOk());
+  resolver->OnShutdown();
+
+  // Expect completed results are still available.
+  EXPECT_THAT(request->GetAddressResults().value().endpoints(),
+              testing::ElementsAre(kEndpoint));
+}
+
+TEST_F(ContextHostResolverTest, OnShutdown_SubsequentRequests) {
+  auto resolver = std::make_unique<ContextHostResolver>(
+      manager_.get(), nullptr /* host_cache */);
+  resolver->OnShutdown();
+
+  std::unique_ptr<HostResolver::ResolveHostRequest> request1 =
+      resolver->CreateRequest(HostPortPair("example.com", 100),
+                              NetLogWithSource(), base::nullopt);
+  std::unique_ptr<HostResolver::ResolveHostRequest> request2 =
+      resolver->CreateRequest(HostPortPair("127.0.0.1", 100),
+                              NetLogWithSource(), base::nullopt);
+
+  TestCompletionCallback callback1;
+  int rv1 = request1->Start(callback1.callback());
+  TestCompletionCallback callback2;
+  int rv2 = request2->Start(callback2.callback());
+
+  EXPECT_THAT(callback1.GetResult(rv1), test::IsError(ERR_CONTEXT_SHUT_DOWN));
+  EXPECT_FALSE(request1->GetAddressResults());
+  EXPECT_THAT(callback2.GetResult(rv2), test::IsError(ERR_CONTEXT_SHUT_DOWN));
+  EXPECT_FALSE(request2->GetAddressResults());
+}
+
+// Test a request created before shutdown but not yet started.
+TEST_F(ContextHostResolverTest, OnShutdown_DelayedStartRequest) {
+  // Set up delayed result for "example.com".
+  MockDnsClientRuleList rules;
+  rules.emplace_back("example.com", dns_protocol::kTypeA, false /* secure */,
+                     MockDnsClientRule::Result(BuildTestDnsResponse(
+                         "example.com", IPAddress(2, 3, 4, 5))),
+                     true /* delay */);
+  rules.emplace_back("example.com", dns_protocol::kTypeAAAA, false /* secure */,
+                     MockDnsClientRule::Result(MockDnsClientRule::EMPTY),
+                     false /* delay */);
+
+  auto resolver = std::make_unique<ContextHostResolver>(
+      manager_.get(), nullptr /* host_cache */);
+  std::unique_ptr<HostResolver::ResolveHostRequest> request =
+      resolver->CreateRequest(HostPortPair("example.com", 100),
+                              NetLogWithSource(), base::nullopt);
+
+  resolver->OnShutdown();
+
+  TestCompletionCallback callback;
+  int rv = request->Start(callback.callback());
+
+  EXPECT_THAT(callback.GetResult(rv), test::IsError(ERR_CONTEXT_SHUT_DOWN));
+  EXPECT_FALSE(request->GetAddressResults());
+}
+
 TEST_F(ContextHostResolverTest, ResolveFromCache) {
   base::SimpleTestTickClock clock;
   clock.Advance(base::TimeDelta::FromDays(62));  // Arbitrary non-zero time.