Don't clear DNS cache when closing last incognito Window.

The DNS cache is now a per-NetworkContext cache, instead of a global,
so clearing the cache when closing incognito is no longer useful -
destroying the incognito profile will destroy its own DNS cache.

This CL also updates the network::mojom::HostResolver API to allow stale
DNS lookups. I'm adding a lot of tests for DNS cache isolation, and
this option is needed to avoid tests racily failing due to DNS cache
eviction, particularly on slower bots. This option was already
supported by the HostResolver; it just wasn't being exposed outside
the network service.

Bug: 1042354
Change-Id: I8cd4332f6d28b5cc783b256550f720807cfb9c83
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2023507Reviewed-by: Eric Orth <ericorth@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736009}

chrome/browser/net/dns_probe_runner.cc

@@ -95,7 +95,8 @@ void DnsProbeRunner::RunProbe(base::OnceClosure callback) {
       network::mojom::ResolveHostParameters::New();
   parameters->dns_query_type = net::DnsQueryType::A;
   parameters->source = net::HostResolverSource::DNS;
-  parameters->allow_cached_response = false;
+  parameters->cache_usage =
+      network::mojom::ResolveHostParameters::CacheUsage::DISALLOWED;
 
   // Use transient NIKs - don't want cached responses anyways, so no benefit
   // from sharing a cache, beyond multiple probes not evicting anything.

chrome/browser/net/network_context_configuration_browsertest.cc

@@ -14,6 +14,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/guid.h"
 #include "base/location.h"
+#include "base/optional.h"
 #include "base/path_service.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_util.h"
@@ -59,6 +60,7 @@
 #include "content/public/test/simple_url_loader_test_helper.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/system/data_pipe_utils.h"
+#include "net/base/address_list.h"
 #include "net/base/filename_util.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/load_flags.h"
@@ -67,6 +69,7 @@
 #include "net/cookies/cookie_options.h"
 #include "net/cookies/cookie_util.h"
 #include "net/dns/mock_host_resolver.h"
+#include "net/dns/public/resolve_error_info.h"
 #include "net/http/http_response_headers.h"
 #include "net/reporting/reporting_policy.h"
 #include "net/ssl/ssl_config.h"
@@ -85,9 +88,11 @@
 #include "services/network/public/cpp/network_connection_tracker.h"
 #include "services/network/public/cpp/simple_url_loader.h"
 #include "services/network/public/mojom/cookie_manager.mojom.h"
+#include "services/network/public/mojom/host_resolver.mojom.h"
 #include "services/network/public/mojom/network_service.mojom.h"
 #include "services/network/public/mojom/url_loader.mojom.h"
 #include "services/network/public/mojom/url_loader_factory.mojom.h"
+#include "services/network/test/test_dns_util.h"
 #include "services/network/test/test_url_loader_client.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -104,6 +109,9 @@
 
 namespace {
 
+constexpr char kHostname[] = "foo.test";
+constexpr char kAddress[] = "5.8.13.21";
+
 const char kCacheRandomPath[] = "/cacherandom";
 
 // Path using a ControllableHttpResponse that's part of the test fixture.
@@ -236,6 +244,8 @@ class NetworkContextConfigurationBrowserTest
     // Used in a bunch of proxy tests. Should not resolve.
     host_resolver()->AddSimulatedFailure("does.not.resolve.test");
 
+    host_resolver()->AddRule(kHostname, kAddress);
+
     controllable_http_response_ =
         std::make_unique<net::test_server::ControllableHttpResponse>(
             embedded_test_server(), kControllablePath);
@@ -1109,6 +1119,54 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest, DiskCache) {
   }
 }
 
+// Make sure that NetworkContexts have separate DNS caches.
+IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest,
+                       DnsCacheIsolation) {
+  net::NetworkIsolationKey network_isolation_key =
+      net::NetworkIsolationKey::CreateTransient();
+  net::HostPortPair host_port_pair(kHostname, 0);
+  // Resolve |host_port_pair|, which should succeed and put it in the
+  // NetworkContext's cache.
+  network::DnsLookupResult result =
+      network::BlockingDnsLookup(network_context(), host_port_pair,
+                                 nullptr /* params */, network_isolation_key);
+  EXPECT_EQ(net::OK, result.error);
+  ASSERT_TRUE(result.resolved_addresses.has_value());
+  ASSERT_EQ(1u, result.resolved_addresses->size());
+  EXPECT_EQ(kAddress,
+            result.resolved_addresses.value()[0].ToStringWithoutPort());
+  // Make a cache-only request for the same hostname, for each other network
+  // context, and make sure no result is returned.
+  ForEachOtherContext(
+      base::BindLambdaForTesting([&](NetworkContextType network_context_type) {
+        network::mojom::ResolveHostParametersPtr params =
+            network::mojom::ResolveHostParameters::New();
+        // Cache only lookup.
+        params->cache_usage =
+            network::mojom::ResolveHostParameters::CacheUsage::STALE_ALLOWED;
+        params->source = net::HostResolverSource::LOCAL_ONLY;
+        network::DnsLookupResult result = network::BlockingDnsLookup(
+            GetNetworkContextForContextType(network_context_type),
+            host_port_pair, std::move(params), network_isolation_key);
+        EXPECT_EQ(net::ERR_NAME_NOT_RESOLVED, result.error);
+      }));
+  // Do a cache-only lookup using the original network context, which should
+  // return the same result it initially did.
+  network::mojom::ResolveHostParametersPtr params =
+      network::mojom::ResolveHostParameters::New();
+  // Cache only lookup.
+  params->source = net::HostResolverSource::LOCAL_ONLY;
+  params->cache_usage =
+      network::mojom::ResolveHostParameters::CacheUsage::STALE_ALLOWED;
+  result = network::BlockingDnsLookup(network_context(), host_port_pair,
+                                      std::move(params), network_isolation_key);
+  EXPECT_EQ(net::OK, result.error);
+  ASSERT_TRUE(result.resolved_addresses.has_value());
+  ASSERT_EQ(1u, result.resolved_addresses->size());
+  EXPECT_EQ(kAddress,
+            result.resolved_addresses.value()[0].ToStringWithoutPort());
+}
+
 // Visits a URL with an HSTS header, and makes sure it is respected.
 IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest, PRE_Hsts) {
   if (IsRestartStateWithInProcessNetworkService())

chrome/browser/profiles/off_the_record_profile_impl.cc

@@ -71,7 +71,6 @@
 #include "media/mojo/services/video_decode_perf_history.h"
 #include "net/http/transport_security_state.h"
 #include "ppapi/buildflags/buildflags.h"
-#include "services/network/public/mojom/network_context.mojom.h"
 #include "storage/browser/database/database_tracker.h"
 
 #if defined(OS_ANDROID)
@@ -190,12 +189,6 @@ OffTheRecordProfileImpl::~OffTheRecordProfileImpl() {
   ChromePluginServiceFilter::GetInstance()->UnregisterProfile(this);
 #endif
 
-  // Clears any data the network stack contains that may be related to the
-  // OTR session. Must be done before DestroyBrowserContextServices, since
-  // the NetworkContext is managed by one such service.
-  GetDefaultStoragePartition(this)->GetNetworkContext()->ClearHostCache(
-      nullptr, network::mojom::NetworkContext::ClearHostCacheCallback());
-
   FullBrowserTransitionManager::Get()->OnProfileDestroyed(this);
 
   // The SimpleDependencyManager should always be passed after the

services/network/host_resolver.cc

@@ -36,10 +36,20 @@ ConvertOptionalParameters(
   parameters.dns_query_type = mojo_parameters->dns_query_type;
   parameters.initial_priority = mojo_parameters->initial_priority;
   parameters.source = mojo_parameters->source;
-  parameters.cache_usage =
-      mojo_parameters->allow_cached_response
-          ? net::HostResolver::ResolveHostParameters::CacheUsage::ALLOWED
-          : net::HostResolver::ResolveHostParameters::CacheUsage::DISALLOWED;
+  switch (mojo_parameters->cache_usage) {
+    case mojom::ResolveHostParameters::CacheUsage::ALLOWED:
+      parameters.cache_usage =
+          net::HostResolver::ResolveHostParameters::CacheUsage::ALLOWED;
+      break;
+    case mojom::ResolveHostParameters::CacheUsage::STALE_ALLOWED:
+      parameters.cache_usage =
+          net::HostResolver::ResolveHostParameters::CacheUsage::STALE_ALLOWED;
+      break;
+    case mojom::ResolveHostParameters::CacheUsage::DISALLOWED:
+      parameters.cache_usage =
+          net::HostResolver::ResolveHostParameters::CacheUsage::DISALLOWED;
+      break;
+  }
   parameters.include_canonical_name = mojo_parameters->include_canonical_name;
   parameters.loopback_only = mojo_parameters->loopback_only;
   parameters.is_speculative = mojo_parameters->is_speculative;

services/network/host_resolver_unittest.cc

@@ -12,8 +12,8 @@
 #include "base/optional.h"
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
-#include "base/test/simple_test_tick_clock.h"
 #include "base/test/task_environment.h"
+#include "base/time/time.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -39,11 +39,12 @@ namespace {
 
 class HostResolverTest : public testing::Test {
  public:
-  HostResolverTest()
-      : task_environment_(base::test::TaskEnvironment::MainThreadType::IO) {}
+  HostResolverTest() = default;
 
  protected:
-  base::test::TaskEnvironment task_environment_;
+  base::test::TaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME,
+      base::test::TaskEnvironment::MainThreadType::IO};
 };
 
 net::IPEndPoint CreateExpectedEndPoint(const std::string& address,
@@ -401,8 +402,6 @@ TEST_F(HostResolverTest, SeparateCacheBySource) {
       kDomain, kAnyResultOriginal);
   inner_resolver->rules_map()[net::HostResolverSource::SYSTEM]->AddRule(
       kDomain, kSystemResultOriginal);
-  base::SimpleTestTickClock test_clock;
-  inner_resolver->set_tick_clock(&test_clock);
 
   HostResolver resolver(inner_resolver.get(), net::NetLog::Get());
 
@@ -473,8 +472,6 @@ TEST_F(HostResolverTest, CacheDisabled) {
   constexpr char kResultOriginal[] = "1.2.3.4";
   auto inner_resolver = std::make_unique<net::MockCachingHostResolver>();
   inner_resolver->rules()->AddRule(kDomain, kResultOriginal);
-  base::SimpleTestTickClock test_clock;
-  inner_resolver->set_tick_clock(&test_clock);
 
   HostResolver resolver(inner_resolver.get(), net::NetLog::Get());
 
@@ -502,7 +499,8 @@ TEST_F(HostResolverTest, CacheDisabled) {
   TestResolveHostClient cached_client(&pending_cached_client, &cached_run_loop);
   mojom::ResolveHostParametersPtr cached_parameters =
       mojom::ResolveHostParameters::New();
-  cached_parameters->allow_cached_response = true;
+  cached_parameters->cache_usage =
+      mojom::ResolveHostParameters::CacheUsage::ALLOWED;
   resolver.ResolveHost(net::HostPortPair(kDomain, 80),
                        net::NetworkIsolationKey(), std::move(cached_parameters),
                        std::move(pending_cached_client));
@@ -519,7 +517,78 @@ TEST_F(HostResolverTest, CacheDisabled) {
                                         &uncached_run_loop);
   mojom::ResolveHostParametersPtr uncached_parameters =
       mojom::ResolveHostParameters::New();
-  uncached_parameters->allow_cached_response = false;
+  uncached_parameters->cache_usage =
+      mojom::ResolveHostParameters::CacheUsage::DISALLOWED;
+  resolver.ResolveHost(
+      net::HostPortPair(kDomain, 80), net::NetworkIsolationKey(),
+      std::move(uncached_parameters), std::move(pending_uncached_client));
+  uncached_run_loop.Run();
+
+  EXPECT_EQ(net::OK, uncached_client.result_error());
+  EXPECT_THAT(uncached_client.result_addresses().value().endpoints(),
+              testing::ElementsAre(CreateExpectedEndPoint(kResultFresh, 80)));
+}
+
+TEST_F(HostResolverTest, CacheStaleAllowed) {
+  constexpr char kDomain[] = "example.com";
+  constexpr char kResultOriginal[] = "1.2.3.4";
+  auto inner_resolver = std::make_unique<net::MockCachingHostResolver>();
+  inner_resolver->rules()->AddRule(kDomain, kResultOriginal);
+
+  HostResolver resolver(inner_resolver.get(), net::NetLog::Get());
+
+  // Load result into cache.
+  base::RunLoop run_loop;
+  mojo::PendingRemote<mojom::ResolveHostClient> pending_client;
+  TestResolveHostClient client(&pending_client, &run_loop);
+  resolver.ResolveHost(net::HostPortPair(kDomain, 80),
+                       net::NetworkIsolationKey(), nullptr,
+                       std::move(pending_client));
+  run_loop.Run();
+  ASSERT_EQ(net::OK, client.result_error());
+  EXPECT_THAT(
+      client.result_addresses().value().endpoints(),
+      testing::ElementsAre(CreateExpectedEndPoint(kResultOriginal, 80)));
+
+  // Change |inner_resolver| rules to ensure results are coming from cache or
+  // not based on whether they resolve to the old or new value.
+  constexpr char kResultFresh[] = "111.222.1.1";
+  inner_resolver->rules()->ClearRules();
+  inner_resolver->rules()->AddRule(kDomain, kResultFresh);
+
+  // MockHostResolver gives cache entries a 1 min TTL, so simulate a day
+  // passing, which is more than long enough for the cached results to become
+  // stale.
+  task_environment_.FastForwardBy(base::TimeDelta::FromDays(1));
+
+  // Fetching stale results returns the original cached value.
+  base::RunLoop cached_run_loop;
+  mojo::PendingRemote<mojom::ResolveHostClient> pending_cached_client;
+  TestResolveHostClient cached_client(&pending_cached_client, &cached_run_loop);
+  mojom::ResolveHostParametersPtr cached_parameters =
+      mojom::ResolveHostParameters::New();
+  cached_parameters->cache_usage =
+      mojom::ResolveHostParameters::CacheUsage::STALE_ALLOWED;
+  resolver.ResolveHost(net::HostPortPair(kDomain, 80),
+                       net::NetworkIsolationKey(), std::move(cached_parameters),
+                       std::move(pending_cached_client));
+  cached_run_loop.Run();
+
+  EXPECT_EQ(net::OK, cached_client.result_error());
+  EXPECT_THAT(
+      cached_client.result_addresses().value().endpoints(),
+      testing::ElementsAre(CreateExpectedEndPoint(kResultOriginal, 80)));
+
+  // Resolution where only non-stale cache usage is allowed returns the new
+  // value.
+  base::RunLoop uncached_run_loop;
+  mojo::PendingRemote<mojom::ResolveHostClient> pending_uncached_client;
+  TestResolveHostClient uncached_client(&pending_uncached_client,
+                                        &uncached_run_loop);
+  mojom::ResolveHostParametersPtr uncached_parameters =
+      mojom::ResolveHostParameters::New();
+  uncached_parameters->cache_usage =
+      mojom::ResolveHostParameters::CacheUsage::ALLOWED;
   resolver.ResolveHost(
       net::HostPortPair(kDomain, 80), net::NetworkIsolationKey(),
       std::move(uncached_parameters), std::move(pending_uncached_client));
@@ -537,8 +606,6 @@ TEST_F(HostResolverTest, CacheDisabled_ErrorResults) {
   constexpr char kResult[] = "1.2.3.4";
   auto inner_resolver = std::make_unique<net::MockCachingHostResolver>();
   inner_resolver->rules()->AddRule(kDomain, kResult);
-  base::SimpleTestTickClock test_clock;
-  inner_resolver->set_tick_clock(&test_clock);
 
   HostResolver resolver(inner_resolver.get(), net::NetLog::Get());
 
@@ -563,7 +630,8 @@ TEST_F(HostResolverTest, CacheDisabled_ErrorResults) {
   TestResolveHostClient cached_client(&pending_cached_client, &cached_run_loop);
   mojom::ResolveHostParametersPtr cached_parameters =
       mojom::ResolveHostParameters::New();
-  cached_parameters->allow_cached_response = true;
+  cached_parameters->cache_usage =
+      mojom::ResolveHostParameters::CacheUsage::ALLOWED;
   resolver.ResolveHost(net::HostPortPair(kDomain, 80),
                        net::NetworkIsolationKey(), std::move(cached_parameters),
                        std::move(pending_cached_client));
@@ -576,7 +644,8 @@ TEST_F(HostResolverTest, CacheDisabled_ErrorResults) {
                                         &uncached_run_loop);
   mojom::ResolveHostParametersPtr uncached_parameters =
       mojom::ResolveHostParameters::New();
-  uncached_parameters->allow_cached_response = false;
+  uncached_parameters->cache_usage =
+      mojom::ResolveHostParameters::CacheUsage::DISALLOWED;
   resolver.ResolveHost(
       net::HostPortPair(kDomain, 80), net::NetworkIsolationKey(),
       std::move(uncached_parameters), std::move(pending_uncached_client));

services/network/public/mojom/host_resolver.mojom

@@ -219,8 +219,23 @@ struct ResolveHostParameters {
   // literals, etc.
   Source source = Source.ANY;
 
-  // If |false|, results will not come from the host cache.
-  bool allow_cached_response = true;
+  // Cache usage mode for ResolveHostRequest. Values corresponding to
+  // HostResolver::ResolveHostParameters::CacheUsage
+  enum CacheUsage {
+    // Results may come from the host cache if non-stale.
+    ALLOWED,
+
+    // Results may come from the host cache even if stale (by expiration or
+    // network changes).
+    STALE_ALLOWED,
+
+    // Results will not come from the host cache.
+    DISALLOWED,
+  };
+
+  // Indicates what DNS cache entries, if any, can be used to provide a
+  // response.
+  CacheUsage cache_usage = CacheUsage.ALLOWED;
 
   // If set, the outstanding request can be controlled, eg cancelled, via the
   // handle.