Implement DNS transaction timeout

Unless |fast_timeout| is used (which will maintain previous behavior
of timing out immediately after the last attempt), on fallback of the
last attempt, a timer is run to time out the transaction.

Also doing the same on errors with the last attempt as long as at least
one previous attempt is still pending. And the transaction will also
return errors immediately if it was from the last pending attempt.

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

net/dns/dns_transaction.cc

@@ -4,6 +4,7 @@
 
 #include "net/dns/dns_transaction.h"
 
+#include <algorithm>
 #include <memory>
 #include <set>
 #include <string>
@@ -31,6 +32,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_checker.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/timer/elapsed_timer.h"
 #include "base/timer/timer.h"
 #include "base/values.h"
 #include "build/build_config.h"
@@ -130,8 +132,7 @@ base::Value NetLogStartParams(const std::string& hostname, uint16_t qtype) {
 // matches. Logging is done in the socket and in the outer DnsTransaction.
 class DnsAttempt {
  public:
-  explicit DnsAttempt(size_t server_index)
-      : result_(ERR_FAILED), server_index_(server_index) {}
+  explicit DnsAttempt(size_t server_index) : server_index_(server_index) {}
 
   virtual ~DnsAttempt() = default;
   // Starts the attempt. Returns ERR_IO_PENDING if cannot complete synchronously
@@ -165,21 +166,10 @@ class DnsAttempt {
     return dict;
   }
 
-  void set_result(int result) { result_ = result; }
-
   // True if current attempt is pending (waiting for server response).
-  bool is_pending() const { return result_ == ERR_IO_PENDING; }
-
-  // True if attempt is completed (received server response).
-  bool is_completed() const {
-    return (result_ == OK) || (result_ == ERR_NAME_NOT_RESOLVED) ||
-           (result_ == ERR_DNS_SERVER_REQUIRES_TCP);
-  }
+  virtual bool IsPending() const = 0;
 
  private:
-  // Result of last operation.
-  int result_;
-
   const size_t server_index_;
 
   DISALLOW_COPY_AND_ASSIGN(DnsAttempt);
@@ -223,6 +213,8 @@ class DnsUDPAttempt : public DnsAttempt {
     return socket_->NetLog();
   }
 
+  bool IsPending() const override { return next_state_ != STATE_NONE; }
+
  private:
   enum State {
     STATE_SEND_QUERY,
@@ -257,13 +249,9 @@ class DnsUDPAttempt : public DnsAttempt {
       }
     } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
 
-    set_result(rv);
-
-    if (rv == ERR_IO_PENDING)
-      return rv;
-
-    if (rv == OK)
+    if (rv != ERR_IO_PENDING)
       DCHECK_EQ(STATE_NONE, next_state_);
+
     return rv;
   }
 
@@ -527,6 +515,8 @@ class DnsHTTPAttempt : public DnsAttempt, public URLRequest::Delegate {
     }
   }
 
+  bool IsPending() const override { return !callback_.is_null(); }
+
  private:
   void ResponseCompleted(int net_error) {
     request_.reset();
@@ -620,7 +610,6 @@ class DnsTCPAttempt : public DnsAttempt {
     int rv = socket_->Connect(
         base::BindOnce(&DnsTCPAttempt::OnIOComplete, base::Unretained(this)));
     if (rv == ERR_IO_PENDING) {
-      set_result(rv);
       return rv;
     }
     return DoLoop(rv);
@@ -637,6 +626,8 @@ class DnsTCPAttempt : public DnsAttempt {
     return socket_->NetLog();
   }
 
+  bool IsPending() const override { return next_state_ != STATE_NONE; }
+
  private:
   enum State {
     STATE_CONNECT_COMPLETE,
@@ -683,9 +674,9 @@ class DnsTCPAttempt : public DnsAttempt {
       }
     } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
 
-    set_result(rv);
-    if (rv == OK)
+    if (rv != ERR_IO_PENDING)
       DCHECK_EQ(STATE_NONE, next_state_);
+
     return rv;
   }
 
@@ -1054,6 +1045,7 @@ class DnsTransactionImpl : public DnsTransaction,
         secure_(secure),
         secure_dns_mode_(secure_dns_mode),
         callback_(std::move(callback)),
+        fast_timeout_(fast_timeout),
         net_log_(net_log),
         qnames_initial_size_(0),
         attempts_count_(0),
@@ -1089,6 +1081,7 @@ class DnsTransactionImpl : public DnsTransaction,
     DCHECK(attempts_.empty());
     net_log_.BeginEvent(NetLogEventType::DNS_TRANSACTION,
                         [&] { return NetLogStartParams(hostname_, qtype_); });
+    time_from_start_ = std::make_unique<base::ElapsedTimer>();
     AttemptResult result(PrepareSearch(), nullptr);
     if (result.rv == OK) {
       qnames_initial_size_ = qnames_.size();
@@ -1211,6 +1204,8 @@ class DnsTransactionImpl : public DnsTransaction,
   }
 
   AttemptResult MakeAttempt() {
+    DCHECK(MoreAttemptsAllowed());
+
     DnsConfig config = session_->config();
     if (secure_) {
       DCHECK_GT(config.dns_over_https_servers.size(), 0u);
@@ -1430,6 +1425,8 @@ class DnsTransactionImpl : public DnsTransaction,
   // Resolves the result of a DnsAttempt until a terminal result is reached
   // or it will complete asynchronously (ERR_IO_PENDING).
   AttemptResult ProcessAttemptResult(AttemptResult result) {
+    DCHECK(!callback_.is_null());
+
     while (result.rv != ERR_IO_PENDING) {
       LogResponse(result.attempt);
 
@@ -1463,17 +1460,25 @@ class DnsTransactionImpl : public DnsTransaction,
           break;
         case ERR_CONNECTION_REFUSED:
         case ERR_DNS_TIMED_OUT:
+          timer_.Stop();
+
           if (result.attempt) {
+            DCHECK(result.attempt == attempts_.back().get());
             resolve_context_->RecordServerFailure(
                 result.attempt->server_index(), secure_ /* is_doh_server */,
                 result.rv, session_.get());
           }
           if (MoreAttemptsAllowed()) {
             result = MakeAttempt();
-          } else {
-            return result;
+            break;
           }
-          break;
+
+          if (!fast_timeout_ && AnyAttemptPending()) {
+            StartTimeoutTimer();
+            return AttemptResult(ERR_IO_PENDING, nullptr);
+          }
+
+          return result;
         case ERR_DNS_SERVER_REQUIRES_TCP:
           result = RetryUdpAttemptAsTcp(result.attempt);
           break;
@@ -1491,28 +1496,43 @@ class DnsTransactionImpl : public DnsTransaction,
           // 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);
+          if (result.attempt == attempts_.back().get()) {
+            timer_.Stop();
+            resolve_context_->RecordServerFailure(
+                result.attempt->server_index(), secure_ /* is_doh_server */,
+                result.rv, session_.get());
+
+            if (MoreAttemptsAllowed()) {
+              result = MakeAttempt();
+              break;
+            }
+
+            if (fast_timeout_) {
+              return result;
+            }
+
+            // No more attempts can be made, but there may be other attempts
+            // still pending, so start the timeout timer.
+            StartTimeoutTimer();
           }
 
-          resolve_context_->RecordServerFailure(result.attempt->server_index(),
-                                                secure_ /* is_doh_server */,
-                                                result.rv, session_.get());
-          if (!MoreAttemptsAllowed()) {
-            return result;
+          // If any attempts are still pending, continue to wait for them.
+          if (AnyAttemptPending()) {
+            DCHECK(timer_.IsRunning());
+            return AttemptResult(ERR_IO_PENDING, nullptr);
           }
-          result = MakeAttempt();
-          break;
+
+          return result;
       }
     }
     return result;
   }
 
-  // Clears and cancels all non-completed attempts. If |leave_attempt| is not
-  // null, it is not cleared even if complete.
+  // Clears and cancels all pending attempts. If |leave_attempt| is not
+  // null, that attempt is not cleared even if pending.
   void ClearAttempts(const DnsAttempt* leave_attempt) {
     for (auto it = attempts_.begin(); it != attempts_.end();) {
-      if (!(*it)->is_completed() && it->get() != leave_attempt) {
+      if ((*it)->IsPending() && it->get() != leave_attempt) {
         it = attempts_.erase(it);
       } else {
         ++it;
@@ -1520,6 +1540,13 @@ class DnsTransactionImpl : public DnsTransaction,
     }
   }
 
+  bool AnyAttemptPending() {
+    return std::any_of(attempts_.begin(), attempts_.end(),
+                       [](std::unique_ptr<DnsAttempt>& attempt) {
+                         return attempt->IsPending();
+                       });
+  }
+
   void OnFallbackPeriodExpired() {
     if (callback_.is_null())
       return;
@@ -1530,6 +1557,29 @@ class DnsTransactionImpl : public DnsTransaction,
       DoCallback(result);
   }
 
+  void StartTimeoutTimer() {
+    DCHECK(!fast_timeout_);
+    DCHECK(!timer_.IsRunning());
+    DCHECK(!callback_.is_null());
+
+    // Timeout determination not currently implemented for Classic DNS because
+    // such transactions are assumed to always use fast timeout. If that ever
+    // changes, need to implement a ResolveContext::ClassicTransactionTimeout().
+    DCHECK(secure_);
+
+    base::TimeDelta timeout = resolve_context_->SecureTransactionTimeout(
+        secure_dns_mode_, session_.get());
+    timeout -= time_from_start_->Elapsed();
+
+    timer_.Start(FROM_HERE, timeout, this, &DnsTransactionImpl::OnTimeout);
+  }
+
+  void OnTimeout() {
+    if (callback_.is_null())
+      return;
+    DoCallback(AttemptResult(ERR_DNS_TIMED_OUT, nullptr));
+  }
+
   scoped_refptr<DnsSession> session_;
   std::string hostname_;
   uint16_t qtype_;
@@ -1539,6 +1589,11 @@ class DnsTransactionImpl : public DnsTransaction,
   // Cleared in DoCallback.
   DnsTransactionFactory::CallbackType callback_;
 
+  // When true, transaction should time out immediately on expiration of the
+  // last attempt fallback period rather than waiting the overall transaction
+  // timeout period.
+  const bool fast_timeout_;
+
   NetLogWithSource net_log_;
 
   // Search list of fully-qualified DNS names to query next (in DNS format).
@@ -1557,6 +1612,7 @@ class DnsTransactionImpl : public DnsTransaction,
   std::unique_ptr<DnsServerIterator> dns_server_iterator_;
 
   base::OneShotTimer timer_;
+  std::unique_ptr<base::ElapsedTimer> time_from_start_;
 
   // TODO(ericorth@chromium.org): Use base::UnownedPtr once available.
   ResolveContext* resolve_context_;

net/dns/dns_transaction.h

@@ -104,14 +104,12 @@ class NET_EXPORT_PRIVATE DnsTransactionFactory {
   // |secure| specifies whether DNS lookups should be performed using DNS-over-
   // HTTPS (DoH) or using plaintext DNS.
   //
-  // |fast_timeout| Not yet implemented. When true (and implemented), the
-  // transaction will timeout quickly after making its DNS attempts, without
-  // necessarily waiting long enough to allow slower-than-average requests to
-  // complete. Intended as an optimization for cases where the caller has
-  // reasonable fallback options to the transaction and it would be beneficial
-  // to move on to those options sooner on signals that the transaction is
-  // potentially slow or problematic.
-  // TODO(crbug.com/1109792): Implement it.
+  // When |fast_timeout| is true, the transaction will timeout quickly after
+  // making its DNS attempts, without necessarily waiting long enough to allow
+  // slower-than-average requests to complete. Intended as an optimization for
+  // cases where the caller has reasonable fallback options to the transaction
+  // and it would be beneficial to move on to those options sooner on signals
+  // that the transaction is potentially slow or problematic.
   virtual std::unique_ptr<DnsTransaction> CreateTransaction(
       const std::string& hostname,
       uint16_t qtype,