Add timeout determination to ResolveContext

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

net/dns/resolve_context.cc

@@ -91,6 +91,9 @@ 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)) {}
@@ -263,6 +266,31 @@ base::TimeDelta ResolveContext::NextDohFallbackPeriod(
       0 /* num_backoffs */);
 }
 
+base::TimeDelta ResolveContext::SecureTransactionTimeout(
+    SecureDnsMode secure_dns_mode,
+    const DnsSession* session) {
+  // Currently only implemented for Secure mode as other modes are assumed to
+  // always use aggressive timeouts. If that ever changes, need to implement
+  // only accounting for available DoH servers when not Secure mode.
+  DCHECK_EQ(secure_dns_mode, SecureDnsMode::kSecure);
+
+  if (!IsCurrentSession(session))
+    return kMinTransactionTimeout;
+
+  // Should not need to call if there are no DoH servers configured.
+  DCHECK(!doh_server_stats_.empty());
+
+  base::TimeDelta shortest_fallback_period = base::TimeDelta::Max();
+  for (const ServerStats& stats : doh_server_stats_) {
+    shortest_fallback_period =
+        std::min(shortest_fallback_period,
+                 NextFallbackPeriodHelper(&stats, 0 /* num_backoffs */));
+  }
+
+  return std::max(kMinTransactionTimeout,
+                  shortest_fallback_period * kTimeoutMultiplier);
+}
+
 void ResolveContext::RegisterDohStatusObserver(DohStatusObserver* observer) {
   DCHECK(observer);
   doh_status_observers_.AddObserver(observer);
@@ -365,7 +393,7 @@ ResolveContext::ServerStats* ResolveContext::GetServerStats(
 }
 
 base::TimeDelta ResolveContext::NextFallbackPeriodHelper(
-    ServerStats* server_stats,
+    const ServerStats* server_stats,
     int num_backoffs) {
   // Respect initial fallback period (from config or field trial) if it exceeds
   // max.

net/dns/resolve_context.h

@@ -43,6 +43,13 @@ 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
@@ -129,6 +136,18 @@ class NET_EXPORT_PRIVATE ResolveContext : public base::CheckedObserver {
   base::TimeDelta NextDohFallbackPeriod(size_t doh_server_index,
                                         const DnsSession* session);
 
+  // Return a timeout for a transaction (from Transaction::Start()). Expected
+  // that the transaction will skip waiting for this timeout if it is using
+  // fast timeouts, and also expected that transactions will always wait for all
+  // attempts to run for at least their fallback period before dying with
+  // timeout.
+  //
+  // Currently only implemented for secure transactions as insecure transactions
+  // are assumed to always use aggressive timeouts (but a
+  // ClassicTransactionTimeout() could be implemented if ever needed).
+  base::TimeDelta SecureTransactionTimeout(SecureDnsMode secure_dns_mode,
+                                           const DnsSession* session);
+
   void RegisterDohStatusObserver(DohStatusObserver* observer);
   void UnregisterDohStatusObserver(const DohStatusObserver* observer);
 
@@ -200,7 +219,7 @@ class NET_EXPORT_PRIVATE ResolveContext : public base::CheckedObserver {
   ServerStats* GetServerStats(size_t server_index, bool is_doh_server);
 
   // Return the fallback period for the next query.
-  base::TimeDelta NextFallbackPeriodHelper(ServerStats* server_stats,
+  base::TimeDelta NextFallbackPeriodHelper(const ServerStats* server_stats,
                                            int attempt);
 
   // Record the time to perform a query.

net/dns/resolve_context_unittest.cc

@@ -1048,6 +1048,106 @@ TEST_F(ResolveContextTest, FallbackPeriod_DifferentSession) {
                                               0 /* attempt */, session2.get()));
 }
 
+// Expect minimum timeout will be used when fallback period is small.
+TEST_F(ResolveContextTest, TransactionTimeout_SmallFallbackPeriod) {
+  ResolveContext context(nullptr /* url_request_context */,
+                         false /* enable_caching */);
+  DnsConfig config =
+      CreateDnsConfig(0 /* num_servers */, 1 /* num_doh_servers */);
+  config.fallback_period = base::TimeDelta();
+  scoped_refptr<DnsSession> session = CreateDnsSession(config);
+  context.InvalidateCachesAndPerSessionData(session.get(),
+                                            false /* network_change */);
+
+  EXPECT_EQ(
+      context.SecureTransactionTimeout(SecureDnsMode::kSecure, session.get()),
+      ResolveContext::kMinTransactionTimeout);
+}
+
+// Expect multiplier on fallback period to be used when larger than minimum
+// timeout.
+TEST_F(ResolveContextTest, TransactionTimeout_LongFallbackPeriod) {
+  ResolveContext context(nullptr /* url_request_context */,
+                         false /* enable_caching */);
+  const base::TimeDelta kFallbackPeriod = base::TimeDelta::FromMinutes(5);
+  DnsConfig config =
+      CreateDnsConfig(0 /* num_servers */, 1 /* num_doh_servers */);
+  config.fallback_period = kFallbackPeriod;
+  scoped_refptr<DnsSession> session = CreateDnsSession(config);
+  context.InvalidateCachesAndPerSessionData(session.get(),
+                                            false /* network_change */);
+
+  base::TimeDelta expected =
+      kFallbackPeriod * ResolveContext::kTimeoutMultiplier;
+  ASSERT_GT(expected, ResolveContext::kMinTransactionTimeout);
+
+  EXPECT_EQ(
+      context.SecureTransactionTimeout(SecureDnsMode::kSecure, session.get()),
+      expected);
+}
+
+TEST_F(ResolveContextTest, TransactionTimeout_LongRtt) {
+  ResolveContext context(nullptr /* url_request_context */,
+                         false /* enable_caching */);
+  DnsConfig config =
+      CreateDnsConfig(0 /* num_servers */, 2 /* num_doh_servers */);
+  config.fallback_period = base::TimeDelta();
+  scoped_refptr<DnsSession> session = CreateDnsSession(config);
+  context.InvalidateCachesAndPerSessionData(session.get(),
+                                            false /* network_change */);
+
+  // Record long RTTs for only 1 server.
+  for (int i = 0; i < 50; ++i) {
+    context.RecordRtt(1u /* server_index */, true /* is_doh_server */,
+                      base::TimeDelta::FromMinutes(10), OK, session.get());
+  }
+
+  // No expected change from recording RTT to single server because lowest
+  // fallback period is used.
+  EXPECT_EQ(
+      context.SecureTransactionTimeout(SecureDnsMode::kSecure, session.get()),
+      ResolveContext::kMinTransactionTimeout);
+
+  // Record long RTTs for remaining server.
+  for (int i = 0; i < 50; ++i) {
+    context.RecordRtt(0u /* server_index */, true /* is_doh_server */,
+                      base::TimeDelta::FromMinutes(10), OK, session.get());
+  }
+
+  // Expect longer timeouts.
+  EXPECT_GT(
+      context.SecureTransactionTimeout(SecureDnsMode::kSecure, session.get()),
+      ResolveContext::kMinTransactionTimeout);
+}
+
+TEST_F(ResolveContextTest, TransactionTimeout_DifferentSession) {
+  const base::TimeDelta kFallbackPeriod = base::TimeDelta::FromMinutes(5);
+  DnsConfig config1 =
+      CreateDnsConfig(0 /* num_servers */, 1 /* num_doh_servers */);
+  config1.fallback_period = kFallbackPeriod;
+  scoped_refptr<DnsSession> session1 = CreateDnsSession(config1);
+
+  DnsConfig config2 =
+      CreateDnsConfig(2 /* num_servers */, 2 /* num_doh_servers */);
+  scoped_refptr<DnsSession> session2 = CreateDnsSession(config2);
+
+  ResolveContext context(nullptr /* url_request_context */,
+                         false /* enable_caching */);
+  context.InvalidateCachesAndPerSessionData(session1.get(),
+                                            true /* network_change */);
+
+  // 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);
+
+  // Expect timeout always minimum with wrong session.
+  EXPECT_EQ(
+      context.SecureTransactionTimeout(SecureDnsMode::kSecure, session2.get()),
+      ResolveContext::kMinTransactionTimeout);
+}
+
 // Ensures that reported negative RTT values don't cause a crash. Regression
 // test for https://crbug.com/753568.
 TEST_F(ResolveContextTest, NegativeRtt) {