[Cronet] Add option to use stale DNS when network resolver returns ERR_NAME_NOT_RESOLVED

Sometimes resolving a host name will result in ERR_NAME_NOT_RESOLVED even
though the host still should resolve.  This change adds an option to use
stale DNS results in these cases.

Bug: 875845

Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I02b7a122bb9236f9ff47c73af8e4803315dcd382
Reviewed-on: https://chromium-review.googlesource.com/1179998
Commit-Queue: Paul Jensen <pauljensen@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584772}

components/cronet/stale_host_resolver.cc

@@ -36,6 +36,9 @@ enum RequestOutcome {
   // Request canceled; there was usable stale data.
   CANCELED_WITH_STALE = 5,
 
+  // Stale data returned; network got ERR_NAME_NOT_RESOLVED.
+  STALE_INSTEAD_OF_NETWORK_NAME_NOT_RESOLVED = 6,
+
   MAX_REQUEST_OUTCOME
 };
 
@@ -84,7 +87,10 @@ bool StaleEntryIsUsable(const StaleHostResolver::StaleOptions& options,
 // network data, or stale cached data.
 class StaleHostResolver::RequestImpl {
  public:
-  explicit RequestImpl(const base::TickClock* tick_clock);
+  // If |use_stale_on_name_not_resolved| and network resolution returns
+  // ERR_NAME_NOT_RESOLVED, use stale result if available.
+  explicit RequestImpl(const base::TickClock* tick_clock,
+                       bool use_stale_on_name_not_resolved);
   ~RequestImpl();
 
   // A callback for the caller to decide whether a stale entry is usable or not.
@@ -149,7 +155,10 @@ class StaleHostResolver::RequestImpl {
   void MaybeDeleteThis();
 
   void RecordSynchronousRequest();
-  void RecordNetworkRequest(int error, bool returned_stale_data);
+  void RecordNetworkRequest(
+      int error,
+      bool returned_stale_data,
+      bool returned_stale_data_instead_of_network_name_not_resolved);
   void RecordCanceledRequest();
 
   // The address list passed into |Start()| to be filled in when the request
@@ -189,9 +198,14 @@ class StaleHostResolver::RequestImpl {
   // Handle that caller can use to cancel the request before it returns.
   // Owned by the caller; cleared via |OnHandleDestroyed()| when destroyed.
   Handle* handle_;
+
+  // If |use_stale_on_name_not_resolved| and network resolution returns
+  // ERR_NAME_NOT_RESOLVED, use stale result if available.
+  const bool use_stale_on_name_not_resolved_;
 };
 
-StaleHostResolver::RequestImpl::RequestImpl(const base::TickClock* tick_clock)
+StaleHostResolver::RequestImpl::RequestImpl(const base::TickClock* tick_clock,
+                                            bool use_stale_on_name_not_resolved)
     : result_addresses_(nullptr),
       returning_result_(false),
       stale_error_(net::ERR_DNS_CACHE_MISS),
@@ -199,7 +213,8 @@ StaleHostResolver::RequestImpl::RequestImpl(const base::TickClock* tick_clock)
       tick_clock_(tick_clock),
       restore_size_(0),
       current_size_(0),
-      handle_(nullptr) {}
+      handle_(nullptr),
+      use_stale_on_name_not_resolved_(use_stale_on_name_not_resolved) {}
 
 StaleHostResolver::RequestImpl::~RequestImpl() {}
 
@@ -288,12 +303,23 @@ void StaleHostResolver::RequestImpl::OnNetworkRequestComplete(int error) {
   DCHECK(have_network_request());
   network_request_.reset();
 
-  RecordNetworkRequest(error, /* returned_stale_data = */ have_returned());
+  bool return_stale_data_instead_of_network_name_not_resolved =
+      !have_returned() && use_stale_on_name_not_resolved_ &&
+      error == net::ERR_NAME_NOT_RESOLVED && have_stale_data();
+
+  RecordNetworkRequest(
+      error, /* returned_stale_data = */ have_returned() ||
+                 return_stale_data_instead_of_network_name_not_resolved,
+      return_stale_data_instead_of_network_name_not_resolved);
 
   if (!have_returned()) {
     if (have_stale_data())
       stale_timer_.Stop();
-    ReturnResult(error, network_addresses_);
+    if (return_stale_data_instead_of_network_name_not_resolved) {
+      ReturnResult(stale_error_, stale_addresses_);
+    } else {
+      ReturnResult(error, network_addresses_);
+    }
   }
 
   if (!have_handle())
@@ -342,7 +368,8 @@ void StaleHostResolver::RequestImpl::RecordSynchronousRequest() {
 
 void StaleHostResolver::RequestImpl::RecordNetworkRequest(
     int error,
-    bool returned_stale_data) {
+    bool returned_stale_data,
+    bool returned_stale_data_instead_of_network_name_not_resolved) {
   if (have_stale_data())
     RecordTimeDelta(tick_clock_->NowTicks(), stale_timer_.desired_run_time());
 
@@ -352,7 +379,11 @@ void StaleHostResolver::RequestImpl::RecordNetworkRequest(
   }
 
   if (returned_stale_data) {
-    RecordRequestOutcome(STALE_BEFORE_NETWORK);
+    if (returned_stale_data_instead_of_network_name_not_resolved) {
+      RecordRequestOutcome(STALE_INSTEAD_OF_NETWORK_NAME_NOT_RESOLVED);
+    } else {
+      RecordRequestOutcome(STALE_BEFORE_NETWORK);
+    }
   } else if (have_stale_data()) {
     RecordRequestOutcome(NETWORK_WITH_STALE);
     RecordCacheSizes(restore_size_, current_size_);
@@ -369,7 +400,9 @@ void StaleHostResolver::RequestImpl::RecordCanceledRequest() {
 }
 
 StaleHostResolver::StaleOptions::StaleOptions()
-    : allow_other_network(false), max_stale_uses(0) {}
+    : allow_other_network(false),
+      max_stale_uses(0),
+      use_stale_on_name_not_resolved(false) {}
 
 StaleHostResolver::StaleHostResolver(
     std::unique_ptr<net::HostResolverImpl> inner_resolver,
@@ -400,7 +433,8 @@ int StaleHostResolver::Resolve(const RequestInfo& info,
   DCHECK(tick_clock_);
   StaleHostResolver::RequestImpl::StaleEntryUsableCallback usable_callback =
       base::Bind(&StaleEntryIsUsable, options_);
-  RequestImpl* request = new RequestImpl(tick_clock_);
+  RequestImpl* request =
+      new RequestImpl(tick_clock_, options_.use_stale_on_name_not_resolved);
   int rv = request->Start(inner_resolver_.get(), info, priority, addresses,
                           std::move(callback), out_req, net_log,
                           usable_callback, options_.delay);

components/cronet/stale_host_resolver.h

@@ -50,6 +50,10 @@ class StaleHostResolver : public net::HostResolver {
     // If positive, the maximum number of times a stale entry can be used. If
     // zero, there is no limit.
     int max_stale_uses;
+
+    // If network resolution returns ERR_NAME_NOT_RESOLVED, use stale result if
+    // available.
+    bool use_stale_on_name_not_resolved;
   };
 
   // Creates a StaleHostResolver that uses |inner_resolver| for actual

components/cronet/stale_host_resolver_unittest.cc

@@ -81,7 +81,9 @@ std::unique_ptr<net::DnsClient> CreateMockDnsClientForHosts() {
 
 class MockHostResolverProc : public net::HostResolverProc {
  public:
-  MockHostResolverProc() : HostResolverProc(nullptr) {}
+  // |result| is the net error code to return from resolution attempts.
+  MockHostResolverProc(int result)
+      : HostResolverProc(nullptr), result_(result) {}
 
   int Resolve(const std::string& hostname,
               net::AddressFamily address_family,
@@ -89,11 +91,15 @@ class MockHostResolverProc : public net::HostResolverProc {
               net::AddressList* address_list,
               int* os_error) override {
     *address_list = MakeAddressList(kNetworkAddress);
-    return net::OK;
+    return result_;
   }
 
  protected:
   ~MockHostResolverProc() override {}
+
+ private:
+  // Result code to return from Resolve().
+  const int result_;
 };
 
 class StaleHostResolverTest : public testing::Test {
@@ -101,7 +107,7 @@ class StaleHostResolverTest : public testing::Test {
   StaleHostResolverTest()
       : scoped_task_environment_(
             base::test::ScopedTaskEnvironment::MainThreadType::IO),
-        mock_proc_(new MockHostResolverProc()),
+        mock_proc_(new MockHostResolverProc(net::OK)),
         resolver_(nullptr),
         resolve_pending_(false),
         resolve_complete_(false) {
@@ -117,6 +123,12 @@ class StaleHostResolverTest : public testing::Test {
     options_.delay = base::TimeDelta::FromSeconds(stale_delay_sec);
   }
 
+  void SetUseStaleOnNameNotResolved() {
+    DCHECK(!resolver_);
+
+    options_.use_stale_on_name_not_resolved = true;
+  }
+
   void SetStaleUsability(int max_expired_time_sec,
                          int max_stale_uses,
                          bool allow_other_network) {
@@ -128,6 +140,12 @@ class StaleHostResolverTest : public testing::Test {
     options_.allow_other_network = allow_other_network;
   }
 
+  void SetNetResult(int result) {
+    DCHECK(!resolver_);
+
+    mock_proc_ = new MockHostResolverProc(result);
+  }
+
   std::unique_ptr<net::HostResolverImpl> CreateMockInnerResolverWithDnsClient(
       std::unique_ptr<net::DnsClient> dns_client) {
     std::unique_ptr<net::HostResolverImpl> inner_resolver(
@@ -376,6 +394,39 @@ TEST_F(StaleHostResolverTest, MAYBE_StaleCache) {
   EXPECT_EQ(kCacheAddress, resolve_addresses()[0].ToStringWithoutPort());
 }
 
+// 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) {
+  SetStaleDelay(kLongStaleDelaySec);
+  SetUseStaleOnNameNotResolved();
+  SetNetResult(net::ERR_NAME_NOT_RESOLVED);
+  CreateResolver();
+  CreateCacheEntry(kAgeExpiredSec, net::OK);
+
+  Resolve();
+  WaitForResolve();
+
+  EXPECT_TRUE(resolve_complete());
+  EXPECT_EQ(net::OK, resolve_error());
+  EXPECT_EQ(1u, resolve_addresses().size());
+  EXPECT_EQ(kCacheAddress, resolve_addresses()[0].ToStringWithoutPort());
+}
+
+// Ensure that without |use_stale_on_name_not_resolved| network resolution
+// failing causes StaleHostResolver jobs to fail with the same error code.
+TEST_F(StaleHostResolverTest, StaleCacheNameNotResolvedDisabled) {
+  SetStaleDelay(kLongStaleDelaySec);
+  SetNetResult(net::ERR_NAME_NOT_RESOLVED);
+  CreateResolver();
+  CreateCacheEntry(kAgeExpiredSec, net::OK);
+
+  Resolve();
+  WaitForResolve();
+
+  EXPECT_TRUE(resolve_complete());
+  EXPECT_EQ(net::ERR_NAME_NOT_RESOLVED, resolve_error());
+}
+
 TEST_F(StaleHostResolverTest, NetworkWithStaleCache) {
   SetStaleDelay(kLongStaleDelaySec);
   CreateResolver();

components/cronet/url_request_context_config.cc

@@ -107,6 +107,10 @@ const char kStaleDnsPersist[] = "persist_to_disk";
 // cache persistence. Default value is one minute. Only relevant if
 // "persist_to_disk" is true.
 const char kStaleDnsPersistTimer[] = "persist_delay_ms";
+// Name of boolean to allow use of stale DNS results when network resolver
+// returns ERR_NAME_NOT_RESOLVED.
+const char kStaleDnsUseStaleOnNameNotResolved[] =
+    "use_stale_on_name_not_resolved";
 
 // Rules to override DNS resolution. Intended for testing.
 // See explanation of format in net/dns/mapped_host_resolver.h.
@@ -434,6 +438,12 @@ void URLRequestContextConfig::ParseAndSetExperimentalOptions(
         int persist_timer;
         if (stale_dns_args->GetInteger(kStaleDnsPersistTimer, &persist_timer))
           host_cache_persistence_delay_ms = persist_timer;
+        bool use_stale_on_name_not_resolved;
+        if (stale_dns_args->GetBoolean(kStaleDnsUseStaleOnNameNotResolved,
+                                       &use_stale_on_name_not_resolved)) {
+          stale_dns_options.use_stale_on_name_not_resolved =
+              use_stale_on_name_not_resolved;
+        }
       }
     } else if (it.key() == kHostResolverRulesFieldTrialName) {
       const base::DictionaryValue* host_resolver_rules_args = nullptr;

tools/metrics/histograms/enums.xml

@@ -10692,6 +10692,8 @@ Called by update_net_error_codes.py.-->
   <int value="3" label="Returned stale cached result; network was too slow."/>
   <int value="4" label="Canceled; no stale cached result was available."/>
   <int value="5" label="Canceled; stale cached result was available."/>
+  <int value="6"
+      label="Returned stale cached result; network got NAME_NOT_RESOLVED"/>
 </enum>
 
 <enum name="DNSEmptyAddressListAndNoError">