Make DNS timeout params finch-controllable

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

net/base/features.cc

@@ -17,6 +17,16 @@ const base::Feature kAcceptLanguageHeader{"AcceptLanguageHeader",
 const base::Feature kCapReferrerToOriginOnCrossOrigin{
     "CapReferrerToOriginOnCrossOrigin", base::FEATURE_DISABLED_BY_DEFAULT};
 
+const base::Feature kDnsTransactionDynamicTimeouts{
+    "DnsTransactionDynamicTimeouts", base::FEATURE_DISABLED_BY_DEFAULT};
+
+const base::FeatureParam<double> kDnsTransactionTimeoutMultiplier{
+    &kDnsTransactionDynamicTimeouts, "DnsTransactionTimeoutMultiplier", 7.5};
+
+const base::FeatureParam<base::TimeDelta> kDnsMinTransactionTimeout{
+    &kDnsTransactionDynamicTimeouts, "DnsMinTransactionTimeout",
+    base::TimeDelta::FromSeconds(12)};
+
 const base::Feature kDnsHttpssvc{"DnsHttpssvc",
                                  base::FEATURE_DISABLED_BY_DEFAULT};
 

net/base/features.h

@@ -28,6 +28,16 @@ NET_EXPORT extern const base::Feature kCapReferrerToOriginOnCrossOrigin;
 // Enables TLS 1.3 early data.
 NET_EXPORT extern const base::Feature kEnableTLS13EarlyData;
 
+// Support for altering the parameters used for DNS transaction timeout. See
+// ResolveContext::SecureTransactionTimeout().
+NET_EXPORT extern const base::Feature kDnsTransactionDynamicTimeouts;
+// Multiplier applied to current fallback periods in determining a transaction
+// timeout.
+NET_EXPORT extern const base::FeatureParam<double>
+    kDnsTransactionTimeoutMultiplier;
+NET_EXPORT extern const base::FeatureParam<base::TimeDelta>
+    kDnsMinTransactionTimeout;
+
 // Enables DNS queries for HTTPSSVC or INTEGRITY records, depending on feature
 // parameters. These queries will only be made over DoH. HTTPSSVC responses may
 // cause us to upgrade the URL to HTTPS and/or to attempt QUIC.

net/dns/resolve_context.cc

@@ -19,6 +19,7 @@
 #include "base/no_destructor.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/stringprintf.h"
+#include "net/base/features.h"
 #include "net/base/network_change_notifier.h"
 #include "net/dns/dns_server_iterator.h"
 #include "net/dns/dns_session.h"
@@ -91,9 +92,6 @@ static std::unique_ptr<base::SampleVector> GetRttHistogram(
 
 }  // namespace
 
-// static
-const base::TimeDelta ResolveContext::kMinTransactionTimeout;
-
 ResolveContext::ServerStats::ServerStats(
     std::unique_ptr<base::SampleVector> buckets)
     : last_failure_count(0), rtt_histogram(std::move(buckets)) {}
@@ -274,8 +272,11 @@ base::TimeDelta ResolveContext::SecureTransactionTimeout(
   // only accounting for available DoH servers when not Secure mode.
   DCHECK_EQ(secure_dns_mode, SecureDnsMode::kSecure);
 
+  DCHECK_GE(features::kDnsMinTransactionTimeout.Get(), base::TimeDelta());
+  DCHECK_GE(features::kDnsTransactionTimeoutMultiplier.Get(), 0.0);
+
   if (!IsCurrentSession(session))
-    return kMinTransactionTimeout;
+    return features::kDnsMinTransactionTimeout.Get();
 
   // Should not need to call if there are no DoH servers configured.
   DCHECK(!doh_server_stats_.empty());
@@ -287,8 +288,13 @@ base::TimeDelta ResolveContext::SecureTransactionTimeout(
                  NextFallbackPeriodHelper(&stats, 0 /* num_backoffs */));
   }
 
-  return std::max(kMinTransactionTimeout,
-                  shortest_fallback_period * kTimeoutMultiplier);
+  DCHECK_GE(shortest_fallback_period, base::TimeDelta());
+  base::TimeDelta ratio_based_timeout =
+      shortest_fallback_period *
+      features::kDnsTransactionTimeoutMultiplier.Get();
+
+  return std::max(features::kDnsMinTransactionTimeout.Get(),
+                  ratio_based_timeout);
 }
 
 void ResolveContext::RegisterDohStatusObserver(DohStatusObserver* observer) {

net/dns/resolve_context.h

@@ -43,13 +43,6 @@ class NET_EXPORT_PRIVATE ResolveContext : public base::CheckedObserver {
   // failures, and the outcome of fallback queries is not taken into account.
   static const int kAutomaticModeFailureLimit = 10;
 
-  // Multiplier applied to current fallback periods in determining a transaction
-  // timeout.
-  static constexpr float kTimeoutMultiplier = 7.5;
-
-  static constexpr base::TimeDelta kMinTransactionTimeout =
-      base::TimeDelta::FromSeconds(12);
-
   class DohStatusObserver : public base::CheckedObserver {
    public:
     // Notification indicating that the current session for which DoH servers

net/dns/resolve_context_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "net/base/address_list.h"
+#include "net/base/features.h"
 #include "net/base/ip_address.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/mock_network_change_notifier.h"
@@ -1061,7 +1062,7 @@ TEST_F(ResolveContextTest, TransactionTimeout_SmallFallbackPeriod) {
 
   EXPECT_EQ(
       context.SecureTransactionTimeout(SecureDnsMode::kSecure, session.get()),
-      ResolveContext::kMinTransactionTimeout);
+      features::kDnsMinTransactionTimeout.Get());
 }
 
 // Expect multiplier on fallback period to be used when larger than minimum
@@ -1078,8 +1079,8 @@ TEST_F(ResolveContextTest, TransactionTimeout_LongFallbackPeriod) {
                                             false /* network_change */);
 
   base::TimeDelta expected =
-      kFallbackPeriod * ResolveContext::kTimeoutMultiplier;
-  ASSERT_GT(expected, ResolveContext::kMinTransactionTimeout);
+      kFallbackPeriod * features::kDnsTransactionTimeoutMultiplier.Get();
+  ASSERT_GT(expected, features::kDnsMinTransactionTimeout.Get());
 
   EXPECT_EQ(
       context.SecureTransactionTimeout(SecureDnsMode::kSecure, session.get()),
@@ -1106,7 +1107,7 @@ TEST_F(ResolveContextTest, TransactionTimeout_LongRtt) {
   // fallback period is used.
   EXPECT_EQ(
       context.SecureTransactionTimeout(SecureDnsMode::kSecure, session.get()),
-      ResolveContext::kMinTransactionTimeout);
+      features::kDnsMinTransactionTimeout.Get());
 
   // Record long RTTs for remaining server.
   for (int i = 0; i < 50; ++i) {
@@ -1117,7 +1118,7 @@ TEST_F(ResolveContextTest, TransactionTimeout_LongRtt) {
   // Expect longer timeouts.
   EXPECT_GT(
       context.SecureTransactionTimeout(SecureDnsMode::kSecure, session.get()),
-      ResolveContext::kMinTransactionTimeout);
+      features::kDnsMinTransactionTimeout.Get());
 }
 
 TEST_F(ResolveContextTest, TransactionTimeout_DifferentSession) {
@@ -1139,13 +1140,13 @@ TEST_F(ResolveContextTest, TransactionTimeout_DifferentSession) {
   // Confirm that if session data were used, the timeout would be higher than
   // the min.
   base::TimeDelta multiplier_expected =
-      kFallbackPeriod * ResolveContext::kTimeoutMultiplier;
-  ASSERT_GT(multiplier_expected, ResolveContext::kMinTransactionTimeout);
+      kFallbackPeriod * features::kDnsTransactionTimeoutMultiplier.Get();
+  ASSERT_GT(multiplier_expected, features::kDnsMinTransactionTimeout.Get());
 
   // Expect timeout always minimum with wrong session.
   EXPECT_EQ(
       context.SecureTransactionTimeout(SecureDnsMode::kSecure, session2.get()),
-      ResolveContext::kMinTransactionTimeout);
+      features::kDnsMinTransactionTimeout.Get());
 }
 
 // Ensures that reported negative RTT values don't cause a crash. Regression