Rename most "timeouts" used by built-in resolver to "fallback period"

These were never true timeouts but merely the period of time after
querying a resolver before we would try another server or retry. The
original query would keep going.

Renaming these to avoid confusing myself as I add more logic for
transaction timeouts (that actually will be timeouts for the overall
transaction).

Bug: 1109792
Change-Id: Icafcce30c5250a9164cd8b779a8ddf1b9fdbfbf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2446526
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Auto-Submit: Eric Orth <ericorth@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813896}

net/dns/dns_config.cc

@@ -10,8 +10,8 @@
 
 namespace net {
 
-// Default values are taken from glibc resolv.h except timeout which is set to
-// |kDnsDefaultTimeoutMs|.
+// Default values are taken from glibc resolv.h except |fallback_period| which
+// is set to |kDnsDefaultFallbackPeriod|.
 DnsConfig::DnsConfig() : DnsConfig(std::vector<IPEndPoint>()) {}
 
 DnsConfig::DnsConfig(const DnsConfig& other) = default;
@@ -25,7 +25,7 @@ DnsConfig::DnsConfig(std::vector<IPEndPoint> nameservers)
       unhandled_options(false),
       append_to_multi_label_name(true),
       ndots(1),
-      timeout(kDnsDefaultTimeout),
+      fallback_period(kDnsDefaultFallbackPeriod),
       attempts(2),
       doh_attempts(1),
       rotate(false),
@@ -57,7 +57,7 @@ bool DnsConfig::EqualsIgnoreHosts(const DnsConfig& d) const {
          (dns_over_tls_hostname == d.dns_over_tls_hostname) &&
          (search == d.search) && (unhandled_options == d.unhandled_options) &&
          (append_to_multi_label_name == d.append_to_multi_label_name) &&
-         (ndots == d.ndots) && (timeout == d.timeout) &&
+         (ndots == d.ndots) && (fallback_period == d.fallback_period) &&
          (attempts == d.attempts) && (doh_attempts == d.doh_attempts) &&
          (rotate == d.rotate) && (use_local_ipv6 == d.use_local_ipv6) &&
          (dns_over_https_servers == d.dns_over_https_servers) &&
@@ -74,7 +74,7 @@ void DnsConfig::CopyIgnoreHosts(const DnsConfig& d) {
   unhandled_options = d.unhandled_options;
   append_to_multi_label_name = d.append_to_multi_label_name;
   ndots = d.ndots;
-  timeout = d.timeout;
+  fallback_period = d.fallback_period;
   attempts = d.attempts;
   doh_attempts = d.doh_attempts;
   rotate = d.rotate;
@@ -103,7 +103,7 @@ base::Value DnsConfig::ToValue() const {
   dict.SetBoolKey("unhandled_options", unhandled_options);
   dict.SetBoolKey("append_to_multi_label_name", append_to_multi_label_name);
   dict.SetIntKey("ndots", ndots);
-  dict.SetDoubleKey("timeout", timeout.InSecondsF());
+  dict.SetDoubleKey("timeout", fallback_period.InSecondsF());
   dict.SetIntKey("attempts", attempts);
   dict.SetIntKey("doh_attempts", doh_attempts);
   dict.SetBoolKey("rotate", rotate);

net/dns/dns_config.h

@@ -22,8 +22,9 @@ class Value;
 
 namespace net {
 
-// Default to 1 second timeout (before exponential backoff).
-constexpr base::TimeDelta kDnsDefaultTimeout = base::TimeDelta::FromSeconds(1);
+// Default to 1 second fallback period (before exponential backoff).
+constexpr base::TimeDelta kDnsDefaultFallbackPeriod =
+    base::TimeDelta::FromSeconds(1);
 
 // DnsConfig stores configuration of the system resolver.
 struct NET_EXPORT DnsConfig {
@@ -78,7 +79,7 @@ struct NET_EXPORT DnsConfig {
   // Minimum number of dots before global resolution precedes |search|.
   int ndots;
   // Time between retransmissions, see res_state.retrans.
-  base::TimeDelta timeout;
+  base::TimeDelta fallback_period;
   // Maximum number of attempts, see res_state.retry.
   int attempts;
   // Maximum number of times a DoH server is attempted per attempted per DNS

net/dns/dns_config_service_posix.cc

@@ -189,8 +189,8 @@ ConfigParsePosixResult ReadDnsConfig(DnsConfig* dns_config) {
       return error;
   }
 #endif  // defined(OS_MAC)
-  // Override timeout value to match default setting on Windows.
-  dns_config->timeout = kDnsDefaultTimeout;
+  // Override |fallback_period| value to match default setting on Windows.
+  dns_config->fallback_period = kDnsDefaultFallbackPeriod;
   return result;
 #else  // defined(OS_ANDROID)
   dns_config->nameservers.clear();
@@ -537,7 +537,7 @@ ConfigParsePosixResult ConvertResStateToDnsConfig(const struct __res_state& res,
   }
 
   dns_config->ndots = res.ndots;
-  dns_config->timeout = base::TimeDelta::FromSeconds(res.retrans);
+  dns_config->fallback_period = base::TimeDelta::FromSeconds(res.retrans);
   dns_config->attempts = res.retry;
 #if defined(RES_ROTATE)
   dns_config->rotate = res.options & RES_ROTATE;

net/dns/dns_config_service_posix_unittest.cc

@@ -118,7 +118,7 @@ void CloseResState(res_state res) {
 
 void InitializeExpectedConfig(DnsConfig* config) {
   config->ndots = 2;
-  config->timeout = base::TimeDelta::FromSeconds(4);
+  config->fallback_period = base::TimeDelta::FromSeconds(4);
   config->attempts = 7;
   config->rotate = true;
   config->append_to_multi_label_name = true;

net/dns/dns_transaction.cc

@@ -1030,9 +1030,9 @@ class DnsOverHttpsProbeRunner : public DnsProbeRunner {
 
 // Implements DnsTransaction. Configuration is supplied by DnsSession.
 // The suffix list is built according to the DnsConfig from the session.
-// The timeout for each DnsUDPAttempt is given by
-// ResolveContext::NextClassicTimeout. The first server to attempt on each query
-// is given by ResolveContext::NextFirstServerIndex, and the order is
+// The fallback period for each DnsUDPAttempt is given by
+// ResolveContext::NextClassicFallbackPeriod(). The first server to attempt on
+// each query is given by ResolveContext::NextFirstServerIndex, and the order is
 // round-robin afterwards. Each server is attempted DnsConfig::attempts times.
 class DnsTransactionImpl : public DnsTransaction,
                            public base::SupportsWeakPtr<DnsTransactionImpl> {
@@ -1243,9 +1243,11 @@ class DnsTransactionImpl : public DnsTransaction,
     }
 
     if (result.rv == ERR_IO_PENDING) {
-      base::TimeDelta timeout = resolve_context_->NextClassicTimeout(
-          server_index, attempt_number, session_.get());
-      timer_.Start(FROM_HERE, timeout, this, &DnsTransactionImpl::OnTimeout);
+      base::TimeDelta fallback_period =
+          resolve_context_->NextClassicFallbackPeriod(
+              server_index, attempt_number, session_.get());
+      timer_.Start(FROM_HERE, fallback_period, this,
+                   &DnsTransactionImpl::OnFallbackPeriodExpired);
     }
 
     return result;
@@ -1303,9 +1305,10 @@ class DnsTransactionImpl : public DnsTransaction,
         &DnsTransactionImpl::OnAttemptComplete, base::Unretained(this),
         attempt_number, true /* record_rtt */, base::TimeTicks::Now()));
     if (rv == ERR_IO_PENDING) {
-      base::TimeDelta timeout =
-          resolve_context_->NextDohTimeout(doh_server_index, session_.get());
-      timer_.Start(FROM_HERE, timeout, this, &DnsTransactionImpl::OnTimeout);
+      base::TimeDelta fallback_period = resolve_context_->NextDohFallbackPeriod(
+          doh_server_index, session_.get());
+      timer_.Start(FROM_HERE, fallback_period, this,
+                   &DnsTransactionImpl::OnFallbackPeriodExpired);
     }
     return AttemptResult(rv, attempts_.back().get());
   }
@@ -1332,9 +1335,10 @@ class DnsTransactionImpl : public DnsTransaction,
     RecordAttemptUma(DnsAttemptType::kTcpTruncationRetry);
 
     if (result.rv == ERR_IO_PENDING) {
-      // On TCP upgrade, use 2x the upgraded timeout.
-      base::TimeDelta timeout = timer_.GetCurrentDelay() * 2;
-      timer_.Start(FROM_HERE, timeout, this, &DnsTransactionImpl::OnTimeout);
+      // On TCP upgrade, use 2x the upgraded fallback period.
+      base::TimeDelta fallback_period = timer_.GetCurrentDelay() * 2;
+      timer_.Start(FROM_HERE, fallback_period, this,
+                   &DnsTransactionImpl::OnFallbackPeriodExpired);
     }
 
     return result;
@@ -1481,11 +1485,11 @@ class DnsTransactionImpl : public DnsTransaction,
           DCHECK(result.attempt);
 
           // If attempt is not the most recent attempt, means this error is for
-          // an attempt that already timed out and was treated as complete but
-          // allowed to continue attempting in parallel with new attempts (see
-          // the ERR_DNS_TIMED_OUT case above). As the failure was already
-          // recorded at timeout time and is no longer being waited on, ignore
-          // this failure.
+          // a previous attempt that passed its fallback period and was treated
+          // as complete but allowed to continue attempting in parallel with new
+          // attempts (see the ERR_DNS_TIMED_OUT case above). As the failure was
+          // already recorded at fallback time and is no longer being waited on,
+          // ignore this failure.
           if (result.attempt != attempts_.back().get()) {
             return AttemptResult(ERR_IO_PENDING, nullptr);
           }
@@ -1515,7 +1519,7 @@ class DnsTransactionImpl : public DnsTransaction,
     }
   }
 
-  void OnTimeout() {
+  void OnFallbackPeriodExpired() {
     if (callback_.is_null())
       return;
     DCHECK(!attempts_.empty());

net/dns/dns_transaction_unittest.cc

@@ -63,7 +63,7 @@ namespace net {
 
 namespace {
 
-base::TimeDelta kTimeout = base::TimeDelta::FromSeconds(1);
+base::TimeDelta kFallbackPeriod = base::TimeDelta::FromSeconds(1);
 
 const char kMockHostname[] = "mock.http";
 
@@ -715,7 +715,7 @@ class DnsTransactionTestBase : public testing::Test {
   }
 
   // Add expected query of |dotted_name| and |qtype| and no response.
-  void AddQueryAndTimeout(
+  void AddHangingQuery(
       const char* dotted_name,
       uint16_t qtype,
       DnsQuery::PaddingStrategy padding_strategy =
@@ -880,8 +880,8 @@ class DnsTransactionTestBase : public testing::Test {
     ConfigureNumServers(1);
     // and no retransmissions,
     config_.attempts = 1;
-    // and an arbitrary timeout.
-    config_.timeout = kTimeout;
+    // and an arbitrary fallback period.
+    config_.fallback_period = kFallbackPeriod;
 
     request_context_ = std::make_unique<TestURLRequestContext>();
     resolve_context_ = std::make_unique<ResolveContext>(
@@ -1179,35 +1179,39 @@ TEST_F(DnsTransactionTestWithMockTime, Timeout) {
   config_.attempts = 3;
   ConfigureFactory();
 
-  AddQueryAndTimeout(kT0HostName, kT0Qtype);
-  AddQueryAndTimeout(kT0HostName, kT0Qtype);
-  AddQueryAndTimeout(kT0HostName, kT0Qtype);
+  AddHangingQuery(kT0HostName, kT0Qtype);
+  AddHangingQuery(kT0HostName, kT0Qtype);
+  AddHangingQuery(kT0HostName, kT0Qtype);
 
   TransactionHelper helper0(kT0HostName, kT0Qtype, false /* secure */,
                             ERR_DNS_TIMED_OUT, resolve_context_.get());
 
-  // Finish when the third attempt times out.
+  // Finish when the third attempt expires its fallback period.
   EXPECT_FALSE(helper0.Run(transaction_factory_.get()));
-  FastForwardBy(resolve_context_->NextClassicTimeout(0, 0, session_.get()));
+  FastForwardBy(
+      resolve_context_->NextClassicFallbackPeriod(0, 0, session_.get()));
   EXPECT_FALSE(helper0.has_completed());
-  FastForwardBy(resolve_context_->NextClassicTimeout(0, 1, session_.get()));
+  FastForwardBy(
+      resolve_context_->NextClassicFallbackPeriod(0, 1, session_.get()));
   EXPECT_FALSE(helper0.has_completed());
-  FastForwardBy(resolve_context_->NextClassicTimeout(0, 2, session_.get()));
+  FastForwardBy(
+      resolve_context_->NextClassicFallbackPeriod(0, 2, session_.get()));
   EXPECT_TRUE(helper0.has_completed());
 }
 
 TEST_F(DnsTransactionTestWithMockTime, ServerFallbackAndRotate) {
-  // Test that we fallback on both server failure and timeout.
+  // Test that we fallback on both server failure and fallback period
+  // expiration.
   config_.attempts = 2;
   // The next request should start from the next server.
   config_.rotate = true;
   ConfigureNumServers(3);
   ConfigureFactory();
 
   // Responses for first request.
-  AddQueryAndTimeout(kT0HostName, kT0Qtype);
+  AddHangingQuery(kT0HostName, kT0Qtype);
   AddAsyncQueryAndRcode(kT0HostName, kT0Qtype, dns_protocol::kRcodeSERVFAIL);
-  AddQueryAndTimeout(kT0HostName, kT0Qtype);
+  AddHangingQuery(kT0HostName, kT0Qtype);
   AddAsyncQueryAndRcode(kT0HostName, kT0Qtype, dns_protocol::kRcodeSERVFAIL);
   AddAsyncQueryAndRcode(kT0HostName, kT0Qtype, dns_protocol::kRcodeNXDOMAIN);
   // Responses for second request.
@@ -2473,29 +2477,29 @@ TEST_F(DnsTransactionTest, HttpsPostLookupWithLog) {
   EXPECT_EQ(observer.dict_count(), 3);
 }
 
-// Test for when a slow DoH response is delayed until after the initial timeout
-// and no more attempts are configured.
+// Test for when a slow DoH response is delayed until after the initial fallback
+// period and no more attempts are configured.
 TEST_F(DnsTransactionTestWithMockTime, SlowHttpsResponse_SingleAttempt) {
   config_.doh_attempts = 1;
   ConfigureDohServers(false /* use_post */);
 
-  AddQueryAndTimeout(kT0HostName, kT0Qtype,
-                     DnsQuery::PaddingStrategy::BLOCK_LENGTH_128, 0 /* id */,
-                     false /* enqueue_transaction_id */);
+  AddHangingQuery(kT0HostName, kT0Qtype,
+                  DnsQuery::PaddingStrategy::BLOCK_LENGTH_128, 0 /* id */,
+                  false /* enqueue_transaction_id */);
 
   TransactionHelper helper(kT0HostName, kT0Qtype, true /* secure */,
                            ERR_DNS_TIMED_OUT, resolve_context_.get());
   ASSERT_FALSE(helper.Run(transaction_factory_.get()));
 
-  // Only one attempt configured, so expect immediate failure after timeout
+  // Only one attempt configured, so expect immediate failure after fallback
   // period.
-  FastForwardBy(resolve_context_->NextDohTimeout(0 /* doh_server_index */,
-                                                 session_.get()));
+  FastForwardBy(resolve_context_->NextDohFallbackPeriod(
+      0 /* doh_server_index */, session_.get()));
   EXPECT_TRUE(helper.has_completed());
 }
 
-// Test for when a slow DoH response is delayed until after the initial timeout
-// but a retry is configured.
+// Test for when a slow DoH response is delayed until after the initial fallback
+// period but a retry is configured.
 TEST_F(DnsTransactionTestWithMockTime, SlowHttpsResponse_TwoAttempts) {
   config_.doh_attempts = 2;
   ConfigureDohServers(false /* use_post */);
@@ -2517,12 +2521,12 @@ TEST_F(DnsTransactionTestWithMockTime, SlowHttpsResponse_TwoAttempts) {
   ASSERT_TRUE(sequenced_socket_data->IsPaused());
 
   // Another attempt configured, so transaction should not fail after initial
-  // timeout. Setup the second attempt to never receive a response.
-  AddQueryAndTimeout(kT0HostName, kT0Qtype,
-                     DnsQuery::PaddingStrategy::BLOCK_LENGTH_128, 0 /* id */,
-                     false /* enqueue_transaction_id */);
-  FastForwardBy(resolve_context_->NextDohTimeout(0 /* doh_server_index */,
-                                                 session_.get()));
+  // fallback period. Setup the second attempt to never receive a response.
+  AddHangingQuery(kT0HostName, kT0Qtype,
+                  DnsQuery::PaddingStrategy::BLOCK_LENGTH_128, 0 /* id */,
+                  false /* enqueue_transaction_id */);
+  FastForwardBy(resolve_context_->NextDohFallbackPeriod(
+      0 /* doh_server_index */, session_.get()));
   EXPECT_FALSE(helper.has_completed());
 
   // Expect first attempt to continue in parallel with retry, so expect the

net/dns/fuzzed_host_resolver_util.cc

@@ -132,10 +132,10 @@ DnsConfig GetFuzzedDnsConfig(FuzzedDataProvider* data_provider) {
   config.ndots = data_provider->ConsumeIntegralInRange(0, 3);
   config.attempts = data_provider->ConsumeIntegralInRange(1, 3);
 
-  // Timeouts don't really work for fuzzing. Even a timeout of 0 milliseconds
-  // will be increased after the first timeout, resulting in inconsistent
-  // behavior.
-  config.timeout = base::TimeDelta::FromDays(10);
+  // Fallback periods don't really work for fuzzing. Even a period of 0
+  // milliseconds will be increased after the first expiration, resulting in
+  // inconsistent behavior.
+  config.fallback_period = base::TimeDelta::FromDays(10);
 
   config.rotate = data_provider->ConsumeBool();
 

net/dns/host_resolver_manager_unittest.cc

@@ -7066,8 +7066,8 @@ TEST_F(HostResolverManagerDnsTest, SetDnsConfigOverrides) {
   overrides.append_to_multi_label_name = false;
   const int ndots = 5;
   overrides.ndots = ndots;
-  const base::TimeDelta timeout = base::TimeDelta::FromSeconds(10);
-  overrides.timeout = timeout;
+  const base::TimeDelta fallback_period = base::TimeDelta::FromSeconds(10);
+  overrides.fallback_period = fallback_period;
   const int attempts = 20;
   overrides.attempts = attempts;
   const int doh_attempts = 19;
@@ -7097,7 +7097,7 @@ TEST_F(HostResolverManagerDnsTest, SetDnsConfigOverrides) {
   EXPECT_EQ(search, overridden_config->search);
   EXPECT_FALSE(overridden_config->append_to_multi_label_name);
   EXPECT_EQ(ndots, overridden_config->ndots);
-  EXPECT_EQ(timeout, overridden_config->timeout);
+  EXPECT_EQ(fallback_period, overridden_config->fallback_period);
   EXPECT_EQ(attempts, overridden_config->attempts);
   EXPECT_EQ(doh_attempts, overridden_config->doh_attempts);
   EXPECT_TRUE(overridden_config->rotate);
@@ -7179,7 +7179,8 @@ TEST_F(HostResolverManagerDnsTest, SetDnsConfigOverrides_PartialOverride) {
   EXPECT_EQ(original_config.hosts, overridden_config->hosts);
   EXPECT_TRUE(overridden_config->append_to_multi_label_name);
   EXPECT_EQ(original_config.ndots, overridden_config->ndots);
-  EXPECT_EQ(original_config.timeout, overridden_config->timeout);
+  EXPECT_EQ(original_config.fallback_period,
+            overridden_config->fallback_period);
   EXPECT_EQ(original_config.attempts, overridden_config->attempts);
   EXPECT_TRUE(overridden_config->rotate);
   EXPECT_FALSE(overridden_config->use_local_ipv6);