Minor DnsSession cleanups

Bunch of minor changes/fixes in advance of moving ServerStats, server
error and timeout tracking, and related logic from DnsSession to
ResolveContext:
*Updated method names/comments to match feedback from an earlier
 partial move of the DoH server selection logic. Eg we now have
 ServerIndexToUse() instead of NextGoodServerIndex().
*For consistency, FirstServerIndex() (previously NextFirstServerIndex())
 is now used for both DoH and non-DoH and just returns 0u for DoH.
*Unified server index types wherever I could find them to always be
 size_t. It's essentially a vector index, so the Chrome style guide says
 that should use size_t.
*Slightly modernized the global base::BucketRanges used in ServerStats
 to be shared using a static method with NoDestructor rather than the
 static field lazy initializer stuff that was used in DnsSession.
*Deleted the non-histogram logic for tracking round-trip times in
 ServerStats. This was dead code as only the histogram-based logic is
 ever used for determining timeouts.
*Converted some millisecond constants/variables to TimeDelta because
 units are evil and result in crashed spacecraft.
*Removed some DnsClient code to clear ServerStats on connection type
 change based on a trial name that doesn't seem to ever be enabled.
 Couldn't find that name string anywhere else in any Google code.
*Convert LastFailure times from base::Time to base::TimeTicks.
*Rename RecordRTTForHistogram() to RecordRttForUma(). "Histogram" is a
 very ambiguous term here because DnsSession is using histogram logic to
 track RTT estimation.

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

net/dns/dns_client.cc

@@ -7,7 +7,6 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/rand_util.h"
 #include "base/values.h"
@@ -249,9 +248,6 @@ class DnsClientImpl : public DnsClient,
       NetworkChangeNotifier::ConnectionType type) override {
     if (session_) {
       session_->UpdateTimeouts(type);
-      const char* kTrialName = "AsyncDnsFlushServerStatsOnConnectionTypeChange";
-      if (base::FieldTrialList::FindFullName(kTrialName) == "enable")
-        session_->InitializeServerStats();
     }
   }
 

net/dns/dns_session.h

@@ -10,11 +10,9 @@
 #include <memory>
 #include <vector>
 
-#include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "base/metrics/bucket_ranges.h"
 #include "base/time/time.h"
 #include "net/base/net_export.h"
 #include "net/base/network_change_notifier.h"
@@ -22,10 +20,6 @@
 #include "net/dns/dns_config.h"
 #include "net/dns/dns_socket_pool.h"
 
-namespace base {
-class BucketRanges;
-}
-
 namespace net {
 
 class DatagramClientSocket;
@@ -53,17 +47,17 @@ class NET_EXPORT_PRIVATE DnsSession : public base::RefCounted<DnsSession> {
   class NET_EXPORT_PRIVATE SocketLease {
    public:
     SocketLease(scoped_refptr<DnsSession> session,
-                unsigned server_index,
+                size_t server_index,
                 std::unique_ptr<DatagramClientSocket> socket);
     ~SocketLease();
 
-    unsigned server_index() const { return server_index_; }
+    size_t server_index() const { return server_index_; }
 
     DatagramClientSocket* socket() { return socket_.get(); }
 
    private:
     scoped_refptr<DnsSession> session_;
-    unsigned server_index_;
+    size_t server_index_;
     std::unique_ptr<DatagramClientSocket> socket_;
 
     DISALLOW_COPY_AND_ASSIGN(SocketLease);
@@ -83,31 +77,38 @@ class NET_EXPORT_PRIVATE DnsSession : public base::RefCounted<DnsSession> {
   // Return the next random query ID.
   uint16_t NextQueryId() const;
 
-  // Return the index of the first configured server to use on first attempt.
-  unsigned NextFirstServerIndex();
+  // TODO(crbug.com/1045507): Rework the server index selection logic and
+  // interface to not be susceptible to race conditions on server
+  // availability/failure-tracking changing between attempts. As-is, this code
+  // can easily result in working servers getting skipped and failing servers
+  // getting extra attempts (further inflating the failure tracking).
+
+  // Return the (potentially rotating) index of the first configured server (to
+  // be passed to [Doh]ServerIndexToUse()).
+  size_t FirstServerIndex(bool doh_server);
 
-  // Start with |server_index| and find the index of the next known
-  // good non-dns-over-https server to use on this attempt. Returns
-  // |server_index| if this server is below the failure limit, or if there are
-  // no other servers below the failure limit or with an older last failure.
-  unsigned NextGoodServerIndex(unsigned server_index);
+  // Find the index of a non-DoH server to use for this attempt.  Starts from
+  // |starting_server| and finds the first eligible server (wrapping around as
+  // necessary) below failure limits, or if no eligible servers are below
+  // failure limits, the one with the oldest last failure.
+  size_t ServerIndexToUse(size_t starting_server);
 
   // TODO(b/1022059): Remove once all server stats are moved to ResolveContext.
-  base::Time GetLastDohFailure(size_t server_index);
+  base::TimeTicks GetLastDohFailure(size_t server_index);
   int GetLastDohFailureCount(size_t server_index);
 
   // Record that server failed to respond (due to SRV_FAIL or timeout). If
   // |is_doh_server| and the number of failures has surpassed a threshold,
   // sets the DoH probe state to unavailable.
-  void RecordServerFailure(unsigned server_index,
+  void RecordServerFailure(size_t server_index,
                            bool is_doh_server,
                            ResolveContext* resolve_context);
 
   // Record that server responded successfully.
-  void RecordServerSuccess(unsigned server_index, bool is_doh_server);
+  void RecordServerSuccess(size_t server_index, bool is_doh_server);
 
   // Record how long it took to receive a response from the server.
-  void RecordRTT(unsigned server_index,
+  void RecordRtt(size_t server_index,
                  bool is_doh_server,
                  bool is_validated_doh_server,
                  base::TimeDelta rtt,
@@ -115,19 +116,19 @@ class NET_EXPORT_PRIVATE DnsSession : public base::RefCounted<DnsSession> {
 
   // Return the timeout for the next query. |attempt| counts from 0 and is used
   // for exponential backoff.
-  base::TimeDelta NextTimeout(unsigned server_index, int attempt);
+  base::TimeDelta NextTimeout(size_t server_index, int attempt);
 
   // Return the timeout for the next DoH query.
-  base::TimeDelta NextDohTimeout(unsigned doh_server_index);
+  base::TimeDelta NextDohTimeout(size_t doh_server_index);
 
   // Allocate a socket, already connected to the server address.
   // When the SocketLease is destroyed, the socket will be freed.
-  std::unique_ptr<SocketLease> AllocateSocket(unsigned server_index,
+  std::unique_ptr<SocketLease> AllocateSocket(size_t server_index,
                                               const NetLogSource& source);
 
   // Creates a StreamSocket from the factory for a transaction over TCP. These
   // sockets are not pooled.
-  std::unique_ptr<StreamSocket> CreateTCPSocket(unsigned server_index,
+  std::unique_ptr<StreamSocket> CreateTCPSocket(size_t server_index,
                                                 const NetLogSource& source);
 
   base::WeakPtr<DnsSession> GetWeakPtr() {
@@ -142,22 +143,22 @@ class NET_EXPORT_PRIVATE DnsSession : public base::RefCounted<DnsSession> {
   ~DnsSession();
 
   // Release a socket.
-  void FreeSocket(unsigned server_index,
+  void FreeSocket(size_t server_index,
                   std::unique_ptr<DatagramClientSocket> socket);
 
   // Returns the ServerStats for the designated server. Returns nullptr if no
   // ServerStats found.
-  ServerStats* GetServerStats(unsigned server_index, bool is_doh_server);
+  ServerStats* GetServerStats(size_t server_index, bool is_doh_server);
 
   // Return the timeout for the next query.
   base::TimeDelta NextTimeoutHelper(ServerStats* server_stats, int attempt);
 
   // Record the time to perform a query.
-  void RecordRTTForHistogram(unsigned server_index,
-                             bool is_doh_server,
-                             bool is_validated_doh_server,
-                             base::TimeDelta rtt,
-                             int rv);
+  void RecordRttForUma(size_t server_index,
+                       bool is_doh_server,
+                       bool is_validated_doh_server,
+                       base::TimeDelta rtt,
+                       int rv);
 
   const DnsConfig config_;
   std::unique_ptr<DnsSocketPool> socket_pool_;
@@ -179,12 +180,6 @@ class NET_EXPORT_PRIVATE DnsSession : public base::RefCounted<DnsSession> {
   // Track runtime statistics of each DoH server.
   std::vector<std::unique_ptr<ServerStats>> doh_server_stats_;
 
-  // Buckets shared for all |ServerStats::rtt_histogram|.
-  struct RttBuckets : public base::BucketRanges {
-    RttBuckets();
-  };
-  static base::LazyInstance<RttBuckets>::Leaky rtt_buckets_;
-
   base::WeakPtrFactory<DnsSession> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(DnsSession);

net/dns/dns_session_unittest.cc

@@ -80,19 +80,19 @@ class TestClientSocketFactory : public ClientSocketFactory {
 
 struct PoolEvent {
   enum { ALLOCATE, FREE } action;
-  unsigned server_index;
+  size_t server_index;
 };
 
 class DnsSessionTest : public TestWithTaskEnvironment {
  public:
-  void OnSocketAllocated(unsigned server_index);
-  void OnSocketFreed(unsigned server_index);
+  void OnSocketAllocated(size_t server_index);
+  void OnSocketFreed(size_t server_index);
 
  protected:
-  void Initialize(unsigned num_servers, unsigned num_doh_servers);
-  std::unique_ptr<DnsSession::SocketLease> Allocate(unsigned server_index);
-  bool DidAllocate(unsigned server_index);
-  bool DidFree(unsigned server_index);
+  void Initialize(size_t num_servers, size_t num_doh_servers);
+  std::unique_ptr<DnsSession::SocketLease> Allocate(size_t server_index);
+  bool DidAllocate(size_t server_index);
+  bool DidFree(size_t server_index);
   bool NoMoreEvents();
 
   DnsConfig config_;
@@ -118,12 +118,12 @@ class MockDnsSocketPool : public DnsSocketPool {
   }
 
   std::unique_ptr<DatagramClientSocket> AllocateSocket(
-      unsigned server_index) override {
+      size_t server_index) override {
     test_->OnSocketAllocated(server_index);
     return CreateConnectedSocket(server_index);
   }
 
-  void FreeSocket(unsigned server_index,
+  void FreeSocket(size_t server_index,
                   std::unique_ptr<DatagramClientSocket> socket) override {
     test_->OnSocketFreed(server_index);
   }
@@ -132,9 +132,8 @@ class MockDnsSocketPool : public DnsSocketPool {
   DnsSessionTest* test_;
 };
 
-void DnsSessionTest::Initialize(unsigned num_servers,
-                                unsigned num_doh_servers) {
-  CHECK(num_servers < 256u);
+void DnsSessionTest::Initialize(size_t num_servers, size_t num_doh_servers) {
+  CHECK_LT(num_servers, 256u);
   config_.nameservers.clear();
   config_.dns_over_https_servers.clear();
   for (unsigned char i = 0; i < num_servers; ++i) {
@@ -163,16 +162,16 @@ void DnsSessionTest::Initialize(unsigned num_servers,
 }
 
 std::unique_ptr<DnsSession::SocketLease> DnsSessionTest::Allocate(
-    unsigned server_index) {
+    size_t server_index) {
   return session_->AllocateSocket(server_index, source_);
 }
 
-bool DnsSessionTest::DidAllocate(unsigned server_index) {
+bool DnsSessionTest::DidAllocate(size_t server_index) {
   PoolEvent expected_event = { PoolEvent::ALLOCATE, server_index };
   return ExpectEvent(expected_event);
 }
 
-bool DnsSessionTest::DidFree(unsigned server_index) {
+bool DnsSessionTest::DidFree(size_t server_index) {
   PoolEvent expected_event = { PoolEvent::FREE, server_index };
   return ExpectEvent(expected_event);
 }
@@ -181,12 +180,12 @@ bool DnsSessionTest::NoMoreEvents() {
   return events_.empty();
 }
 
-void DnsSessionTest::OnSocketAllocated(unsigned server_index) {
+void DnsSessionTest::OnSocketAllocated(size_t server_index) {
   PoolEvent event = { PoolEvent::ALLOCATE, server_index };
   events_.push_back(event);
 }
 
-void DnsSessionTest::OnSocketFreed(unsigned server_index) {
+void DnsSessionTest::OnSocketFreed(size_t server_index) {
   PoolEvent event = { PoolEvent::FREE, server_index };
   events_.push_back(event);
 }
@@ -280,13 +279,13 @@ TEST_F(DnsSessionTest, HistogramTimeoutLong) {
 // test for https://crbug.com/753568.
 TEST_F(DnsSessionTest, NegativeRtt) {
   Initialize(2 /* num_servers */, 2 /* num_doh_servers */);
-  session_->RecordRTT(0, false /* is_doh_server */,
+  session_->RecordRtt(0, false /* is_doh_server */,
                       false /* is_validated_doh_server */,
                       base::TimeDelta::FromMilliseconds(-1), OK /* rv */);
-  session_->RecordRTT(0, true /* is_doh_server */,
+  session_->RecordRtt(0, true /* is_doh_server */,
                       false /* is_validated_doh_server */,
                       base::TimeDelta::FromMilliseconds(-1), OK /* rv */);
-  session_->RecordRTT(0, true /* is_doh_server */,
+  session_->RecordRtt(0, true /* is_doh_server */,
                       true /* is_validated_doh_server */,
                       base::TimeDelta::FromMilliseconds(-1), OK /* rv */);
 }
@@ -339,4 +338,4 @@ TEST_F(DnsSessionTest, DohProbeNonConsecutiveFailures) {
 
 }  // namespace
 
-} // namespace net
+}  // namespace net

net/dns/dns_socket_pool.cc

@@ -28,12 +28,12 @@ namespace {
 // them.  Everywhere else, request fresh, random ports each time.
 #if defined(OS_WIN)
 const DatagramSocket::BindType kBindType = DatagramSocket::DEFAULT_BIND;
-const unsigned kInitialPoolSize = 256;
-const unsigned kAllocateMinSize = 256;
+const size_t kInitialPoolSize = 256;
+const size_t kAllocateMinSize = 256;
 #else
 const DatagramSocket::BindType kBindType = DatagramSocket::RANDOM_BIND;
-const unsigned kInitialPoolSize = 0;
-const unsigned kAllocateMinSize = 1;
+const size_t kInitialPoolSize = 0;
+const size_t kAllocateMinSize = 1;
 #endif
 
 } // namespace
@@ -58,7 +58,7 @@ void DnsSocketPool::InitializeInternal(
 }
 
 std::unique_ptr<StreamSocket> DnsSocketPool::CreateTCPSocket(
-    unsigned server_index,
+    size_t server_index,
     const NetLogSource& source) {
   DCHECK_LT(server_index, nameservers_->size());
 
@@ -69,7 +69,7 @@ std::unique_ptr<StreamSocket> DnsSocketPool::CreateTCPSocket(
 }
 
 std::unique_ptr<DatagramClientSocket> DnsSocketPool::CreateConnectedSocket(
-    unsigned server_index) {
+    size_t server_index) {
   DCHECK_LT(server_index, nameservers_->size());
 
   std::unique_ptr<DatagramClientSocket> socket;
@@ -107,11 +107,11 @@ class NullDnsSocketPool : public DnsSocketPool {
   }
 
   std::unique_ptr<DatagramClientSocket> AllocateSocket(
-      unsigned server_index) override {
+      size_t server_index) override {
     return CreateConnectedSocket(server_index);
   }
 
-  void FreeSocket(unsigned server_index,
+  void FreeSocket(size_t server_index,
                   std::unique_ptr<DatagramClientSocket> socket) override {}
 
  private:
@@ -138,13 +138,13 @@ class DefaultDnsSocketPool : public DnsSocketPool {
                   NetLog* net_log) override;
 
   std::unique_ptr<DatagramClientSocket> AllocateSocket(
-      unsigned server_index) override;
+      size_t server_index) override;
 
-  void FreeSocket(unsigned server_index,
+  void FreeSocket(size_t server_index,
                   std::unique_ptr<DatagramClientSocket> socket) override;
 
  private:
-  void FillPool(unsigned server_index, unsigned size);
+  void FillPool(size_t server_index, size_t size);
 
   typedef std::vector<std::unique_ptr<DatagramClientSocket>> SocketVector;
 
@@ -169,16 +169,16 @@ void DefaultDnsSocketPool::Initialize(
   InitializeInternal(nameservers, net_log);
 
   DCHECK(pools_.empty());
-  const unsigned num_servers = nameservers->size();
+  const size_t num_servers = nameservers->size();
   pools_.resize(num_servers);
-  for (unsigned server_index = 0; server_index < num_servers; ++server_index)
+  for (size_t server_index = 0; server_index < num_servers; ++server_index)
     FillPool(server_index, kInitialPoolSize);
 }
 
 DefaultDnsSocketPool::~DefaultDnsSocketPool() = default;
 
 std::unique_ptr<DatagramClientSocket> DefaultDnsSocketPool::AllocateSocket(
-    unsigned server_index) {
+    size_t server_index) {
   DCHECK_LT(server_index, pools_.size());
   SocketVector& pool = pools_[server_index];
 
@@ -194,7 +194,7 @@ std::unique_ptr<DatagramClientSocket> DefaultDnsSocketPool::AllocateSocket(
              << " in pool " << server_index << ".";
   }
 
-  unsigned socket_index = GetRandomInt(0, pool.size() - 1);
+  size_t socket_index = GetRandomInt(0, pool.size() - 1);
   std::unique_ptr<DatagramClientSocket> socket = std::move(pool[socket_index]);
   pool[socket_index] = std::move(pool.back());
   pool.pop_back();
@@ -203,15 +203,15 @@ std::unique_ptr<DatagramClientSocket> DefaultDnsSocketPool::AllocateSocket(
 }
 
 void DefaultDnsSocketPool::FreeSocket(
-    unsigned server_index,
+    size_t server_index,
     std::unique_ptr<DatagramClientSocket> socket) {
   DCHECK_LT(server_index, pools_.size());
 }
 
-void DefaultDnsSocketPool::FillPool(unsigned server_index, unsigned size) {
+void DefaultDnsSocketPool::FillPool(size_t server_index, size_t size) {
   SocketVector& pool = pools_[server_index];
 
-  for (unsigned pool_index = pool.size(); pool_index < size; ++pool_index) {
+  for (size_t pool_index = pool.size(); pool_index < size; ++pool_index) {
     std::unique_ptr<DatagramClientSocket> socket =
         CreateConnectedSocket(server_index);
     if (!socket)

net/dns/dns_socket_pool.h

@@ -57,16 +57,16 @@ class NET_EXPORT_PRIVATE DnsSocketPool {
   // available to reuse and the factory fails to produce a socket (or produces
   // one on which Connect fails).
   virtual std::unique_ptr<DatagramClientSocket> AllocateSocket(
-      unsigned server_index) = 0;
+      size_t server_index) = 0;
 
   // Frees a socket allocated by AllocateSocket.  |server_index| must be the
   // same index passed to AllocateSocket.
-  virtual void FreeSocket(unsigned server_index,
+  virtual void FreeSocket(size_t server_index,
                           std::unique_ptr<DatagramClientSocket> socket) = 0;
 
   // Creates a StreamSocket from the factory for a transaction over TCP. These
   // sockets are not pooled.
-  std::unique_ptr<StreamSocket> CreateTCPSocket(unsigned server_index,
+  std::unique_ptr<StreamSocket> CreateTCPSocket(size_t server_index,
                                                 const NetLogSource& source);
 
  protected:
@@ -78,7 +78,7 @@ class NET_EXPORT_PRIVATE DnsSocketPool {
       NetLog* net_log);
 
   std::unique_ptr<DatagramClientSocket> CreateConnectedSocket(
-      unsigned server_index);
+      size_t server_index);
 
   // Returns a random int in the specified range.
   int GetRandomInt(int min, int max);

net/dns/dns_transaction.cc

@@ -183,7 +183,7 @@ class DnsAttempt {
 
 class DnsUDPAttempt : public DnsAttempt {
  public:
-  DnsUDPAttempt(unsigned server_index,
+  DnsUDPAttempt(size_t server_index,
                 std::unique_ptr<DnsSession::SocketLease> socket_lease,
                 std::unique_ptr<DnsQuery> query)
       : DnsAttempt(server_index),
@@ -326,7 +326,7 @@ class DnsUDPAttempt : public DnsAttempt {
 
 class DnsHTTPAttempt : public DnsAttempt, public URLRequest::Delegate {
  public:
-  DnsHTTPAttempt(unsigned doh_server_index,
+  DnsHTTPAttempt(size_t doh_server_index,
                  std::unique_ptr<DnsQuery> query,
                  const string& server_template,
                  const GURL& gurl_without_parameters,
@@ -588,7 +588,7 @@ void ConstructDnsHTTPAttempt(DnsSession* session,
 
 class DnsTCPAttempt : public DnsAttempt {
  public:
-  DnsTCPAttempt(unsigned server_index,
+  DnsTCPAttempt(size_t server_index,
                 std::unique_ptr<StreamSocket> socket,
                 std::unique_ptr<DnsQuery> query)
       : DnsAttempt(server_index),
@@ -984,7 +984,7 @@ class DnsOverHttpsProbeRunner : public DnsProbeRunner {
         // so the ServerStats have not been updated yet.
         session_->RecordServerSuccess(doh_server_index,
                                       true /* is_doh_server */);
-        session_->RecordRTT(doh_server_index, true /* is_doh_server */,
+        session_->RecordRtt(doh_server_index, true /* is_doh_server */,
                             false /* is_validated_doh_server */,
                             base::TimeTicks::Now() - query_start_time, rv);
         context_->SetProbeSuccess(doh_server_index, true /* success */,
@@ -1192,7 +1192,7 @@ class DnsTransactionImpl : public DnsTransaction,
   // next nameserver.
   AttemptResult MakeUDPAttempt() {
     DCHECK(!secure_);
-    unsigned attempt_number = attempts_.size();
+    size_t attempt_number = attempts_.size();
 
     uint16_t id = session_->NextQueryId();
     std::unique_ptr<DnsQuery> query;
@@ -1204,10 +1204,10 @@ class DnsTransactionImpl : public DnsTransaction,
 
     const DnsConfig& config = session_->config();
 
-    unsigned non_doh_server_index =
+    size_t non_doh_server_index =
         (first_server_index_ + attempt_number) % config.nameservers.size();
     // Skip over known failed servers.
-    non_doh_server_index = session_->NextGoodServerIndex(non_doh_server_index);
+    non_doh_server_index = session_->ServerIndexToUse(non_doh_server_index);
 
     std::unique_ptr<DnsSession::SocketLease> lease =
         session_->AllocateSocket(non_doh_server_index, net_log_.source());
@@ -1245,7 +1245,9 @@ class DnsTransactionImpl : public DnsTransaction,
     // have configured and search for the next good server.
     base::Optional<size_t> doh_server_index =
         resolve_context_->DohServerIndexToUse(
-            doh_attempts_ % session_->config().dns_over_https_servers.size(),
+            first_server_index_ +
+                doh_attempts_ %
+                    session_->config().dns_over_https_servers.size(),
             secure_dns_mode_, session_.get());
     // Do not construct an attempt if there is no DoH server that we should send
     // a request to.
@@ -1275,7 +1277,7 @@ class DnsTransactionImpl : public DnsTransaction,
     DCHECK(previous_attempt);
     DCHECK(!had_tcp_attempt_);
 
-    unsigned server_index = previous_attempt->server_index();
+    size_t server_index = previous_attempt->server_index();
 
     std::unique_ptr<StreamSocket> socket(
         session_->CreateTCPSocket(server_index, net_log_.source()));
@@ -1319,9 +1321,7 @@ class DnsTransactionImpl : public DnsTransaction,
     net_log_.BeginEventWithStringParams(NetLogEventType::DNS_TRANSACTION_QUERY,
                                         "qname", dotted_qname);
 
-    first_server_index_ = session_->config().nameservers.empty()
-                              ? 0
-                              : session_->NextFirstServerIndex();
+    first_server_index_ = session_->FirstServerIndex(secure_);
     attempts_.clear();
     had_tcp_attempt_ = false;
     return MakeAttempt();
@@ -1337,7 +1337,7 @@ class DnsTransactionImpl : public DnsTransaction,
       bool is_validated_doh_server =
           secure_ && resolve_context_->GetDohServerAvailability(
                          attempt->server_index(), session_.get());
-      session_->RecordRTT(attempt->server_index(), secure_ /* is_doh_server */,
+      session_->RecordRtt(attempt->server_index(), secure_ /* is_doh_server */,
                           is_validated_doh_server,
                           base::TimeTicks::Now() - start, rv);
     }
@@ -1490,7 +1490,7 @@ class DnsTransactionImpl : public DnsTransaction,
   bool had_tcp_attempt_;
 
   // Index of the first server to try on each search query.
-  int first_server_index_;
+  size_t first_server_index_;
 
   base::OneShotTimer timer_;
 

net/dns/dns_transaction_unittest.cc

@@ -573,10 +573,10 @@ class DnsTransactionTestBase : public testing::Test {
   ~DnsTransactionTestBase() override = default;
 
   // Generates |nameservers| for DnsConfig.
-  void ConfigureNumServers(unsigned num_servers) {
+  void ConfigureNumServers(size_t num_servers) {
     CHECK_LE(num_servers, 255u);
     config_.nameservers.clear();
-    for (unsigned i = 0; i < num_servers; ++i) {
+    for (size_t i = 0; i < num_servers; ++i) {
       config_.nameservers.push_back(
           IPEndPoint(IPAddress(192, 168, 1, i), dns_protocol::kDefaultPort));
     }
@@ -586,21 +586,21 @@ class DnsTransactionTestBase : public testing::Test {
   // accept GET or POST requests based on use_post. If a
   // ResponseModifierCallback is provided it will be called to construct the
   // HTTPResponse.
-  void ConfigureDohServers(bool use_post, unsigned num_doh_servers = 1) {
+  void ConfigureDohServers(bool use_post, size_t num_doh_servers = 1) {
     GURL url(URLRequestMockDohJob::GetMockHttpsUrl("doh_test"));
     URLRequestFilter* filter = URLRequestFilter::GetInstance();
     filter->AddHostnameInterceptor(url.scheme(), url.host(),
                                    std::make_unique<DohJobInterceptor>(this));
     CHECK_LE(num_doh_servers, 255u);
-    for (unsigned i = 0; i < num_doh_servers; ++i) {
+    for (size_t i = 0; i < num_doh_servers; ++i) {
       std::string server_template(URLRequestMockDohJob::GetMockHttpsUrl(
-                                      base::StringPrintf("doh_test_%d", i)) +
+                                      base::StringPrintf("doh_test_%zu", i)) +
                                   "{?dns}");
       config_.dns_over_https_servers.push_back(
           DnsConfig::DnsOverHttpsServerConfig(server_template, use_post));
     }
     ConfigureFactory();
-    for (unsigned i = 0; i < num_doh_servers; ++i) {
+    for (size_t i = 0; i < num_doh_servers; ++i) {
       resolve_context_->SetProbeSuccess(i, true /* success */, session_.get());
     }
   }
@@ -725,7 +725,7 @@ class DnsTransactionTestBase : public testing::Test {
 
   // Checks if the sockets were connected in the order matching the indices in
   // |servers|.
-  void CheckServerOrder(const unsigned* servers, size_t num_attempts) {
+  void CheckServerOrder(const size_t* servers, size_t num_attempts) {
     ASSERT_EQ(num_attempts, socket_factory_->remote_endpoints_.size());
     auto num_insecure_nameservers = session_->config().nameservers.size();
     for (size_t i = 0; i < num_attempts; ++i) {
@@ -1160,7 +1160,7 @@ TEST_F(DnsTransactionTestWithMockTime, ServerFallbackAndRotate) {
   EXPECT_TRUE(helper0.has_completed());
   EXPECT_TRUE(helper1.Run(transaction_factory_.get()));
 
-  unsigned kOrder[] = {
+  size_t kOrder[] = {
       0, 1, 2, 0, 1,  // The first transaction.
       1, 2, 0,        // The second transaction starts from the next server.
   };
@@ -1191,7 +1191,7 @@ TEST_F(DnsTransactionTest, SuffixSearchAboveNdots) {
   EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
 
   // Also check if suffix search causes server rotation.
-  unsigned kOrder0[] = {0, 1, 0, 1};
+  size_t kOrder0[] = {0, 1, 0, 1};
   CheckServerOrder(kOrder0, base::size(kOrder0));
 }
 
@@ -1730,7 +1730,7 @@ TEST_F(DnsTransactionTest, HttpsMarkHttpsBad) {
   // UDP server 0 is our only UDP server, so it will be good. HTTPS
   // servers 0 and 1 failed and will be marked bad. HTTPS server 2 succeeded
   // so will be good.
-  EXPECT_EQ(session_->NextGoodServerIndex(0), 0u);
+  EXPECT_EQ(session_->ServerIndexToUse(0), 0u);
   EXPECT_THAT(resolve_context_->DohServerIndexToUse(
                   0, DnsConfig::SecureDnsMode::AUTOMATIC, session_.get()),
               testing::Optional(2u));
@@ -1740,7 +1740,7 @@ TEST_F(DnsTransactionTest, HttpsMarkHttpsBad) {
   EXPECT_THAT(resolve_context_->DohServerIndexToUse(
                   2, DnsConfig::SecureDnsMode::AUTOMATIC, session_.get()),
               testing::Optional(2u));
-  unsigned kOrder0[] = {1, 2, 3};
+  size_t kOrder0[] = {1, 2, 3};
   CheckServerOrder(kOrder0, base::size(kOrder0));
 
   EXPECT_TRUE(helper1.RunUntilDone(transaction_factory_.get()));
@@ -1749,7 +1749,7 @@ TEST_F(DnsTransactionTest, HttpsMarkHttpsBad) {
   // server 0 then had the oldest failure so would be the next good server and
   // failed so is marked bad. Next attempt was HTTPS server 1, which succeeded
   // so is good.
-  EXPECT_EQ(session_->NextGoodServerIndex(0), 0u);
+  EXPECT_EQ(session_->ServerIndexToUse(0), 0u);
   EXPECT_THAT(resolve_context_->DohServerIndexToUse(
                   0, DnsConfig::SecureDnsMode::AUTOMATIC, session_.get()),
               testing::Optional(1u));
@@ -1759,7 +1759,7 @@ TEST_F(DnsTransactionTest, HttpsMarkHttpsBad) {
   EXPECT_THAT(resolve_context_->DohServerIndexToUse(
                   2, DnsConfig::SecureDnsMode::AUTOMATIC, session_.get()),
               testing::Optional(1u));
-  unsigned kOrder1[] = {
+  size_t kOrder1[] = {
       1, 2, 3, /* transaction0 */
       3, 1, 2  /* transaction1 */
   };
@@ -1778,7 +1778,7 @@ TEST_F(DnsTransactionTest, HttpsPostFailThenHTTPFallback) {
   TransactionHelper helper0(kT0HostName, kT0Qtype, true /* secure */,
                             kT0RecordCount, resolve_context_.get());
   EXPECT_TRUE(helper0.RunUntilDone(transaction_factory_.get()));
-  unsigned kOrder0[] = {1, 2};
+  size_t kOrder0[] = {1, 2};
   CheckServerOrder(kOrder0, base::size(kOrder0));
 }
 
@@ -1797,7 +1797,7 @@ TEST_F(DnsTransactionTest, HttpsPostFailTwice) {
                             ERR_FAILED, resolve_context_.get());
   SetDohJobMakerCallback(base::BindRepeating(DohJobMakerCallbackFailStart));
   EXPECT_TRUE(helper0.RunUntilDone(transaction_factory_.get()));
-  unsigned kOrder0[] = {1, 2};
+  size_t kOrder0[] = {1, 2};
   CheckServerOrder(kOrder0, base::size(kOrder0));
 }
 
@@ -1817,7 +1817,7 @@ TEST_F(DnsTransactionTest, HttpsNotAvailableThenHttpFallback) {
   TransactionHelper helper0(kT0HostName, kT0Qtype, true /* secure */,
                             kT0RecordCount, resolve_context_.get());
   EXPECT_TRUE(helper0.RunUntilDone(transaction_factory_.get()));
-  unsigned kOrder0[] = {2};
+  size_t kOrder0[] = {2};
   CheckServerOrder(kOrder0, base::size(kOrder0));
   EXPECT_THAT(resolve_context_->DohServerIndexToUse(
                   0, DnsConfig::SecureDnsMode::AUTOMATIC, session_.get()),
@@ -1848,7 +1848,7 @@ TEST_F(DnsTransactionTest, HttpsFailureThenNotAvailable_Automatic) {
   TransactionHelper helper0(kT0HostName, kT0Qtype, true /* secure */,
                             ERR_CONNECTION_REFUSED, resolve_context_.get());
   EXPECT_TRUE(helper0.RunUntilDone(transaction_factory_.get()));
-  unsigned kOrder0[] = {1};
+  size_t kOrder0[] = {1};
   CheckServerOrder(kOrder0, base::size(kOrder0));
   EXPECT_THAT(resolve_context_->DohServerIndexToUse(
                   0, DnsConfig::SecureDnsMode::AUTOMATIC, session_.get()),
@@ -1890,7 +1890,7 @@ TEST_F(DnsTransactionTest, HttpsFailureThenNotAvailable_Secure) {
   TransactionHelper helper0(kT0HostName, kT0Qtype, true /* secure */,
                             ERR_CONNECTION_REFUSED, resolve_context_.get());
   EXPECT_TRUE(helper0.RunUntilDone(transaction_factory_.get()));
-  unsigned kOrder0[] = {1, 2, 3};
+  size_t kOrder0[] = {1, 2, 3};
   CheckServerOrder(kOrder0, base::size(kOrder0));
   EXPECT_THAT(resolve_context_->DohServerIndexToUse(
                   0, DnsConfig::SecureDnsMode::SECURE, session_.get()),

net/dns/resolve_context.cc

@@ -30,7 +30,7 @@ base::Optional<size_t> ResolveContext::DohServerIndexToUse(
 
   CHECK_LT(starting_doh_server_index, doh_server_availability_.size());
   size_t index = starting_doh_server_index;
-  base::Time oldest_server_failure;
+  base::TimeTicks oldest_server_failure;
   base::Optional<size_t> oldest_available_server_failure_index;
 
   do {
@@ -47,7 +47,7 @@ base::Optional<size_t> ResolveContext::DohServerIndexToUse(
         return index;
       }
       // Track oldest failed available server.
-      base::Time cur_server_failure =
+      base::TimeTicks cur_server_failure =
           current_session_->GetLastDohFailure(index);
       if (!oldest_available_server_failure_index ||
           cur_server_failure < oldest_server_failure) {