Cleanup dead code in DnsSocketPool

Remove everything related to the "default" socket pool that is
completely unused.  Also simplify the DnsSocketPool interface for the
remaining much simpler usage of the class.

Also delete dns_session_unittest.cc since it was really only testing
that allocated sockets were properly freed in the pool, but that no
longer matters and it wasn't worth maintaining the mockability and hooks
in the pool needed for the test.

Bug: 1116579
Change-Id: Ib322bffb653e85a9c33a4b3835f4067fd8d29530
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2357615
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Dan McArdle <dmcardle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800248}

net/dns/BUILD.gn

@@ -391,7 +391,6 @@ source_set("tests") {
     "dns_hosts_unittest.cc",
     "dns_query_unittest.cc",
     "dns_response_unittest.cc",
-    "dns_session_unittest.cc",
     "dns_socket_pool_unittest.cc",
     "dns_transaction_unittest.cc",
     "dns_udp_tracker_unittest.cc",

net/dns/dns_client.cc

@@ -245,8 +245,8 @@ class DnsClientImpl : public DnsClient {
     if (new_effective_config) {
       DCHECK(new_effective_config.value().IsValid());
 
-      std::unique_ptr<DnsSocketPool> socket_pool(
-          DnsSocketPool::CreateNull(socket_factory_, rand_int_callback_));
+      auto socket_pool = std::make_unique<DnsSocketPool>(
+          socket_factory_, new_effective_config.value().nameservers, net_log_);
       session_ =
           new DnsSession(std::move(new_effective_config).value(),
                          std::move(socket_pool), rand_int_callback_, net_log_);

net/dns/dns_session.cc

@@ -48,7 +48,6 @@ DnsSession::DnsSession(const DnsConfig& config,
                                          0,
                                          std::numeric_limits<uint16_t>::max())),
       net_log_(net_log) {
-  socket_pool_->Initialize(&config_.nameservers, net_log);
   UMA_HISTOGRAM_CUSTOM_COUNTS("AsyncDNS.ServerCount",
                               config_.nameservers.size(), 1, 10, 11);
 }
@@ -65,7 +64,7 @@ std::unique_ptr<DnsSession::SocketLease> DnsSession::AllocateSocket(
     const NetLogSource& source) {
   std::unique_ptr<DatagramClientSocket> socket;
 
-  socket = socket_pool_->AllocateSocket(server_index);
+  socket = socket_pool_->CreateConnectedUdpSocket(server_index);
   if (!socket.get())
     return std::unique_ptr<SocketLease>();
 
@@ -79,7 +78,7 @@ std::unique_ptr<DnsSession::SocketLease> DnsSession::AllocateSocket(
 std::unique_ptr<StreamSocket> DnsSession::CreateTCPSocket(
     size_t server_index,
     const NetLogSource& source) {
-  return socket_pool_->CreateTCPSocket(server_index, source);
+  return socket_pool_->CreateTcpSocket(server_index, source);
 }
 
 void DnsSession::InvalidateWeakPtrsForTesting() {
@@ -92,8 +91,6 @@ void DnsSession::FreeSocket(size_t server_index,
   DCHECK(socket.get());
 
   socket->NetLog().EndEvent(NetLogEventType::SOCKET_IN_USE);
-
-  socket_pool_->FreeSocket(server_index, std::move(socket));
 }
 
 }  // namespace net

net/dns/dns_session.h

@@ -38,6 +38,8 @@ class NET_EXPORT_PRIVATE DnsSession : public base::RefCounted<DnsSession> {
  public:
   typedef base::RepeatingCallback<int()> RandCallback;
 
+  // TODO(crbug.com/1116579): Remove this unnecessary abstraction now that DNS
+  // sockets are not pooled and do not need a special release signal.
   class NET_EXPORT_PRIVATE SocketLease {
    public:
     SocketLease(scoped_refptr<DnsSession> session,
@@ -71,11 +73,13 @@ class NET_EXPORT_PRIVATE DnsSession : public base::RefCounted<DnsSession> {
 
   // Allocate a socket, already connected to the server address.
   // When the SocketLease is destroyed, the socket will be freed.
+  // TODO(crbug.com/1116579): Remove this and directly call into DnsSocketPool.
   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.
+  // TODO(crbug.com/1116579): Remove this and directly call into DnsSocketPool.
   std::unique_ptr<StreamSocket> CreateTCPSocket(size_t server_index,
                                                 const NetLogSource& source);
 

net/dns/dns_session_unittest.cc

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "net/dns/dns_session.h"
-
-#include <list>
-#include <memory>
-#include <string>
-#include <utility>
-#include <vector>
-
-#include "base/bind.h"
-#include "base/memory/ptr_util.h"
-#include "base/rand_util.h"
-#include "base/run_loop.h"
-#include "base/strings/stringprintf.h"
-#include "net/base/ip_address.h"
-#include "net/base/mock_network_change_notifier.h"
-#include "net/base/net_errors.h"
-#include "net/dns/dns_socket_pool.h"
-#include "net/dns/public/dns_protocol.h"
-#include "net/dns/resolve_context.h"
-#include "net/log/net_log_source.h"
-#include "net/socket/socket_performance_watcher.h"
-#include "net/socket/socket_test_util.h"
-#include "net/socket/ssl_client_socket.h"
-#include "net/socket/stream_socket.h"
-#include "net/test/test_with_task_environment.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace net {
-
-namespace {
-
-class TestClientSocketFactory : public ClientSocketFactory {
- public:
-  ~TestClientSocketFactory() override;
-
-  std::unique_ptr<DatagramClientSocket> CreateDatagramClientSocket(
-      DatagramSocket::BindType bind_type,
-      NetLog* net_log,
-      const NetLogSource& source) override;
-
-  std::unique_ptr<TransportClientSocket> CreateTransportClientSocket(
-      const AddressList& addresses,
-      std::unique_ptr<SocketPerformanceWatcher>,
-      NetLog*,
-      const NetLogSource&) override {
-    NOTIMPLEMENTED();
-    return nullptr;
-  }
-
-  std::unique_ptr<SSLClientSocket> CreateSSLClientSocket(
-      SSLClientContext* context,
-      std::unique_ptr<StreamSocket> stream_socket,
-      const HostPortPair& host_and_port,
-      const SSLConfig& ssl_config) override {
-    NOTIMPLEMENTED();
-    return nullptr;
-  }
-
-  std::unique_ptr<ProxyClientSocket> CreateProxyClientSocket(
-      std::unique_ptr<StreamSocket> stream_socket,
-      const std::string& user_agent,
-      const HostPortPair& endpoint,
-      const ProxyServer& proxy_server,
-      HttpAuthController* http_auth_controller,
-      bool tunnel,
-      bool using_spdy,
-      NextProto negotiated_protocol,
-      ProxyDelegate* proxy_delegate,
-      const NetworkTrafficAnnotationTag& traffic_annotation) override {
-    NOTIMPLEMENTED();
-    return nullptr;
-  }
-
- private:
-  std::list<std::unique_ptr<SocketDataProvider>> data_providers_;
-};
-
-struct PoolEvent {
-  enum { ALLOCATE, FREE } action;
-  size_t server_index;
-};
-
-class DnsSessionTest : public TestWithTaskEnvironment {
- public:
-  void OnSocketAllocated(size_t server_index);
-  void OnSocketFreed(size_t server_index);
-
- protected:
-  void Initialize(size_t num_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_;
-  std::unique_ptr<TestClientSocketFactory> test_client_socket_factory_;
-  scoped_refptr<DnsSession> session_;
-  NetLogSource source_;
-
- private:
-  bool ExpectEvent(const PoolEvent& event);
-  std::list<PoolEvent> events_;
-};
-
-class MockDnsSocketPool : public DnsSocketPool {
- public:
-  MockDnsSocketPool(ClientSocketFactory* factory, DnsSessionTest* test)
-      : DnsSocketPool(factory, base::BindRepeating(&base::RandInt)),
-        test_(test) {}
-
-  ~MockDnsSocketPool() override = default;
-
-  void Initialize(const std::vector<IPEndPoint>* nameservers,
-                  NetLog* net_log) override {
-    InitializeInternal(nameservers, net_log);
-  }
-
-  std::unique_ptr<DatagramClientSocket> AllocateSocket(
-      size_t server_index) override {
-    test_->OnSocketAllocated(server_index);
-    return CreateConnectedSocket(server_index);
-  }
-
-  void FreeSocket(size_t server_index,
-                  std::unique_ptr<DatagramClientSocket> socket) override {
-    test_->OnSocketFreed(server_index);
-  }
-
- private:
-  DnsSessionTest* test_;
-};
-
-void DnsSessionTest::Initialize(size_t num_servers) {
-  CHECK_LT(num_servers, 256u);
-  config_.nameservers.clear();
-  config_.dns_over_https_servers.clear();
-  for (unsigned char i = 0; i < num_servers; ++i) {
-    IPEndPoint dns_endpoint(IPAddress(192, 168, 1, i),
-                            dns_protocol::kDefaultPort);
-    config_.nameservers.push_back(dns_endpoint);
-  }
-
-  test_client_socket_factory_.reset(new TestClientSocketFactory());
-
-  DnsSocketPool* dns_socket_pool =
-      new MockDnsSocketPool(test_client_socket_factory_.get(), this);
-
-  session_ =
-      new DnsSession(config_, std::unique_ptr<DnsSocketPool>(dns_socket_pool),
-                     base::BindRepeating(&base::RandInt), nullptr /* NetLog */);
-
-  events_.clear();
-}
-
-std::unique_ptr<DnsSession::SocketLease> DnsSessionTest::Allocate(
-    size_t server_index) {
-  return session_->AllocateSocket(server_index, source_);
-}
-
-bool DnsSessionTest::DidAllocate(size_t server_index) {
-  PoolEvent expected_event = { PoolEvent::ALLOCATE, server_index };
-  return ExpectEvent(expected_event);
-}
-
-bool DnsSessionTest::DidFree(size_t server_index) {
-  PoolEvent expected_event = { PoolEvent::FREE, server_index };
-  return ExpectEvent(expected_event);
-}
-
-bool DnsSessionTest::NoMoreEvents() {
-  return events_.empty();
-}
-
-void DnsSessionTest::OnSocketAllocated(size_t server_index) {
-  PoolEvent event = { PoolEvent::ALLOCATE, server_index };
-  events_.push_back(event);
-}
-
-void DnsSessionTest::OnSocketFreed(size_t server_index) {
-  PoolEvent event = { PoolEvent::FREE, server_index };
-  events_.push_back(event);
-}
-
-bool DnsSessionTest::ExpectEvent(const PoolEvent& expected) {
-  if (events_.empty()) {
-    return false;
-  }
-
-  const PoolEvent actual = events_.front();
-  if ((expected.action != actual.action)
-      || (expected.server_index != actual.server_index)) {
-    return false;
-  }
-  events_.pop_front();
-
-  return true;
-}
-
-std::unique_ptr<DatagramClientSocket>
-TestClientSocketFactory::CreateDatagramClientSocket(
-    DatagramSocket::BindType bind_type,
-    NetLog* net_log,
-    const NetLogSource& source) {
-  // We're not actually expecting to send or receive any data, so use the
-  // simplest SocketDataProvider with no data supplied.
-  SocketDataProvider* data_provider = new StaticSocketDataProvider();
-  data_providers_.push_back(base::WrapUnique(data_provider));
-  std::unique_ptr<MockUDPClientSocket> socket(
-      new MockUDPClientSocket(data_provider, net_log));
-  return std::move(socket);
-}
-
-TestClientSocketFactory::~TestClientSocketFactory() = default;
-
-TEST_F(DnsSessionTest, AllocateFree) {
-  std::unique_ptr<DnsSession::SocketLease> lease1, lease2;
-
-  Initialize(2 /* num_servers */);
-  EXPECT_TRUE(NoMoreEvents());
-
-  lease1 = Allocate(0);
-  EXPECT_TRUE(DidAllocate(0));
-  EXPECT_TRUE(NoMoreEvents());
-
-  lease2 = Allocate(1);
-  EXPECT_TRUE(DidAllocate(1));
-  EXPECT_TRUE(NoMoreEvents());
-
-  lease1.reset();
-  EXPECT_TRUE(DidFree(0));
-  EXPECT_TRUE(NoMoreEvents());
-
-  lease2.reset();
-  EXPECT_TRUE(DidFree(1));
-  EXPECT_TRUE(NoMoreEvents());
-}
-
-}  // namespace
-
-}  // namespace net

net/dns/dns_socket_pool.cc

@@ -5,8 +5,8 @@
 #include "net/dns/dns_socket_pool.h"
 
 #include "base/logging.h"
-#include "base/macros.h"
 #include "base/rand_util.h"
+#include "build/build_config.h"
 #include "net/base/address_list.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_errors.h"
@@ -19,205 +19,55 @@ namespace net {
 
 namespace {
 
-// When we initialize the SocketPool, we allocate kInitialPoolSize sockets.
-// When we allocate a socket, we ensure we have at least kAllocateMinSize
-// sockets to choose from.  Freed sockets are not retained.
-
-// On Windows, we can't request specific (random) ports, since that will
-// trigger firewall prompts, so request default ones, but keep a pile of
-// them.  Everywhere else, request fresh, random ports each time.
+// On Windows, can't request specific (random) ports, since that will trigger
+// firewall prompts, so request default ones (but experimentally, the OS appears
+// to still allocate random ports).
 #if defined(OS_WIN)
 const DatagramSocket::BindType kBindType = DatagramSocket::DEFAULT_BIND;
-const size_t kInitialPoolSize = 256;
-const size_t kAllocateMinSize = 256;
 #else
 const DatagramSocket::BindType kBindType = DatagramSocket::RANDOM_BIND;
-const size_t kInitialPoolSize = 0;
-const size_t kAllocateMinSize = 1;
 #endif
 
 } // namespace
 
 DnsSocketPool::DnsSocketPool(ClientSocketFactory* socket_factory,
-                             const RandIntCallback& rand_int_callback)
+                             std::vector<IPEndPoint> nameservers,
+                             NetLog* net_log)
     : socket_factory_(socket_factory),
-      rand_int_callback_(rand_int_callback),
-      net_log_(nullptr),
-      nameservers_(nullptr),
-      initialized_(false) {}
-
-void DnsSocketPool::InitializeInternal(
-    const std::vector<IPEndPoint>* nameservers,
-    NetLog* net_log) {
-  DCHECK(nameservers);
-  DCHECK(!initialized_);
-
-  net_log_ = net_log;
-  nameservers_ = nameservers;
-  initialized_ = true;
+      net_log_(net_log),
+      nameservers_(std::move(nameservers)) {
+  DCHECK(socket_factory_);
 }
 
-std::unique_ptr<StreamSocket> DnsSocketPool::CreateTCPSocket(
-    size_t server_index,
-    const NetLogSource& source) {
-  DCHECK_LT(server_index, nameservers_->size());
-
-  return std::unique_ptr<StreamSocket>(
-      socket_factory_->CreateTransportClientSocket(
-          AddressList((*nameservers_)[server_index]), nullptr, net_log_,
-          source));
-}
+DnsSocketPool::~DnsSocketPool() = default;
 
-std::unique_ptr<DatagramClientSocket> DnsSocketPool::CreateConnectedSocket(
+std::unique_ptr<DatagramClientSocket> DnsSocketPool::CreateConnectedUdpSocket(
     size_t server_index) {
-  DCHECK_LT(server_index, nameservers_->size());
+  DCHECK_LT(server_index, nameservers_.size());
 
   std::unique_ptr<DatagramClientSocket> socket;
 
   NetLogSource no_source;
   socket = socket_factory_->CreateDatagramClientSocket(kBindType, net_log_,
                                                        no_source);
+  DCHECK(socket);
 
-  if (socket.get()) {
-    int rv = socket->Connect((*nameservers_)[server_index]);
-    if (rv != OK) {
-      DVLOG(1) << "Failed to connect socket: " << rv;
-      socket.reset();
-    }
-  } else {
-    DVLOG(1) << "Failed to create socket.";
-  }
-
-  return socket;
-}
-
-int DnsSocketPool::GetRandomInt(int min, int max) {
-  return rand_int_callback_.Run(min, max);
-}
-
-class NullDnsSocketPool : public DnsSocketPool {
- public:
-  NullDnsSocketPool(ClientSocketFactory* factory,
-                    const RandIntCallback& rand_int_callback)
-      : DnsSocketPool(factory, rand_int_callback) {}
-
-  void Initialize(const std::vector<IPEndPoint>* nameservers,
-                  NetLog* net_log) override {
-    InitializeInternal(nameservers, net_log);
-  }
-
-  std::unique_ptr<DatagramClientSocket> AllocateSocket(
-      size_t server_index) override {
-    return CreateConnectedSocket(server_index);
-  }
-
-  void FreeSocket(size_t server_index,
-                  std::unique_ptr<DatagramClientSocket> socket) override {}
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(NullDnsSocketPool);
-};
-
-// static
-std::unique_ptr<DnsSocketPool> DnsSocketPool::CreateNull(
-    ClientSocketFactory* factory,
-    const RandIntCallback& rand_int_callback) {
-  return std::unique_ptr<DnsSocketPool>(
-      new NullDnsSocketPool(factory, rand_int_callback));
-}
-
-class DefaultDnsSocketPool : public DnsSocketPool {
- public:
-  DefaultDnsSocketPool(ClientSocketFactory* factory,
-                       const RandIntCallback& rand_int_callback)
-      : DnsSocketPool(factory, rand_int_callback) {}
-
-  ~DefaultDnsSocketPool() override;
-
-  void Initialize(const std::vector<IPEndPoint>* nameservers,
-                  NetLog* net_log) override;
-
-  std::unique_ptr<DatagramClientSocket> AllocateSocket(
-      size_t server_index) override;
-
-  void FreeSocket(size_t server_index,
-                  std::unique_ptr<DatagramClientSocket> socket) override;
-
- private:
-  void FillPool(size_t server_index, size_t size);
-
-  typedef std::vector<std::unique_ptr<DatagramClientSocket>> SocketVector;
-
-  std::vector<SocketVector> pools_;
-
-  DISALLOW_COPY_AND_ASSIGN(DefaultDnsSocketPool);
-};
-
-DnsSocketPool::~DnsSocketPool() = default;
-
-// static
-std::unique_ptr<DnsSocketPool> DnsSocketPool::CreateDefault(
-    ClientSocketFactory* factory,
-    const RandIntCallback& rand_int_callback) {
-  return std::unique_ptr<DnsSocketPool>(
-      new DefaultDnsSocketPool(factory, rand_int_callback));
-}
-
-void DefaultDnsSocketPool::Initialize(
-    const std::vector<IPEndPoint>* nameservers,
-    NetLog* net_log) {
-  InitializeInternal(nameservers, net_log);
-
-  DCHECK(pools_.empty());
-  const size_t num_servers = nameservers->size();
-  pools_.resize(num_servers);
-  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(
-    size_t server_index) {
-  DCHECK_LT(server_index, pools_.size());
-  SocketVector& pool = pools_[server_index];
-
-  FillPool(server_index, kAllocateMinSize);
-  if (pool.size() == 0) {
-    DVLOG(1) << "No DNS sockets available in pool " << server_index << "!";
-    return std::unique_ptr<DatagramClientSocket>();
-  }
-
-  if (pool.size() < kAllocateMinSize) {
-    DVLOG(1) << "Low DNS port entropy: wanted " << kAllocateMinSize
-             << " sockets to choose from, but only have " << pool.size()
-             << " in pool " << server_index << ".";
+  int rv = socket->Connect(nameservers_[server_index]);
+  if (rv != OK) {
+    DVLOG(1) << "Failed to connect socket: " << rv;
+    socket.reset();
   }
 
-  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();
-
   return socket;
 }
 
-void DefaultDnsSocketPool::FreeSocket(
+std::unique_ptr<StreamSocket> DnsSocketPool::CreateTcpSocket(
     size_t server_index,
-    std::unique_ptr<DatagramClientSocket> socket) {
-  DCHECK_LT(server_index, pools_.size());
-}
-
-void DefaultDnsSocketPool::FillPool(size_t server_index, size_t size) {
-  SocketVector& pool = pools_[server_index];
+    const NetLogSource& source) {
+  DCHECK_LT(server_index, nameservers_.size());
 
-  for (size_t pool_index = pool.size(); pool_index < size; ++pool_index) {
-    std::unique_ptr<DatagramClientSocket> socket =
-        CreateConnectedSocket(server_index);
-    if (!socket)
-      break;
-    pool.push_back(std::move(socket));
-  }
+  return socket_factory_->CreateTransportClientSocket(
+      AddressList(nameservers_[server_index]), nullptr, net_log_, source);
 }
 
 } // namespace net

net/dns/dns_socket_pool.h

@@ -8,7 +8,6 @@
 #include <memory>
 #include <vector>
 
-#include "base/macros.h"
 #include "net/base/net_export.h"
 #include "net/base/rand_callback.h"
 
@@ -24,73 +23,32 @@ class StreamSocket;
 // A DnsSocketPool is an abstraction layer around a ClientSocketFactory that
 // allows preallocation, reuse, or other strategies to manage sockets connected
 // to DNS servers.
+//
+// TODO(crbug.com/1116579): Rename since this doesn't do any pooling.
 class NET_EXPORT_PRIVATE DnsSocketPool {
  public:
-  virtual ~DnsSocketPool();
+  DnsSocketPool(ClientSocketFactory* factory,
+                std::vector<IPEndPoint> nameservers,
+                NetLog* net_log);
+  ~DnsSocketPool();
 
-  // Creates a DnsSocketPool that implements the default strategy for managing
-  // sockets.  (This varies by platform; see DnsSocketPoolImpl in
-  // dns_socket_pool.cc for details.)
-  static std::unique_ptr<DnsSocketPool> CreateDefault(
-      ClientSocketFactory* factory,
-      const RandIntCallback& rand_int_callback);
+  DnsSocketPool(const DnsSocketPool&) = delete;
+  DnsSocketPool& operator=(const DnsSocketPool&) = delete;
 
-  // Creates a DnsSocketPool that implements a "null" strategy -- no sockets are
-  // preallocated, allocation requests are satisfied by calling the factory
-  // directly, and returned sockets are deleted immediately.
-  static std::unique_ptr<DnsSocketPool> CreateNull(
-      ClientSocketFactory* factory,
-      const RandIntCallback& rand_int_callback);
-
-  // Initializes the DnsSocketPool.  |nameservers| is the list of nameservers
-  // for which the DnsSocketPool will manage sockets; |net_log| is the NetLog
-  // used when constructing sockets with the factory.
-  //
-  // Initialize may not be called more than once, and must be called before
-  // calling AllocateSocket or FreeSocket.
-  virtual void Initialize(
-      const std::vector<IPEndPoint>* nameservers,
-      NetLog* net_log) = 0;
-
-  // Allocates a socket that is already connected to the nameserver referenced
-  // by |server_index|.  May return a std::unique_ptr to NULL if no sockets are
-  // available to reuse and the factory fails to produce a socket (or produces
-  // one on which Connect fails).
-  virtual std::unique_ptr<DatagramClientSocket> AllocateSocket(
-      size_t server_index) = 0;
-
-  // Frees a socket allocated by AllocateSocket.  |server_index| must be the
-  // same index passed to AllocateSocket.
-  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(size_t server_index,
-                                                const NetLogSource& source);
-
- protected:
-  DnsSocketPool(ClientSocketFactory* socket_factory,
-                const RandIntCallback& rand_int_callback);
-
-  void InitializeInternal(
-      const std::vector<IPEndPoint>* nameservers,
-      NetLog* net_log);
-
-  std::unique_ptr<DatagramClientSocket> CreateConnectedSocket(
+  // Creates a UDP client socket that is already connected to the nameserver
+  // referenced by |server_index|. Returns null on error connecting the socket.
+  std::unique_ptr<DatagramClientSocket> CreateConnectedUdpSocket(
       size_t server_index);
 
-  // Returns a random int in the specified range.
-  int GetRandomInt(int min, int max);
+  // Creates a StreamSocket for TCP to the nameserver referenced by
+  // |server_index|. Does not connect the seocket.
+  std::unique_ptr<StreamSocket> CreateTcpSocket(size_t server_index,
+                                                const NetLogSource& source);
 
  private:
-  ClientSocketFactory* socket_factory_;
-  const RandIntCallback rand_int_callback_;
-  NetLog* net_log_;
-  const std::vector<IPEndPoint>* nameservers_;
-  bool initialized_;
-
-  DISALLOW_COPY_AND_ASSIGN(DnsSocketPool);
+  ClientSocketFactory* const socket_factory_;
+  NetLog* const net_log_;
+  const std::vector<IPEndPoint> nameservers_;
 };
 
 } // namespace net

net/dns/dns_socket_pool_unittest.cc

@@ -4,105 +4,72 @@
 
 #include "net/dns/dns_socket_pool.h"
 
-#include "base/bind.h"
-#include "base/callback.h"
-#include "base/memory/weak_ptr.h"
-#include "net/base/rand_callback.h"
+#include "net/base/ip_endpoint.h"
+#include "net/base/net_errors.h"
+#include "net/log/net_log_source.h"
 #include "net/socket/client_socket_factory.h"
+#include "net/socket/datagram_client_socket.h"
+#include "net/socket/socket_test_util.h"
+#include "net/test/gtest_util.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
 namespace {
 
-class DummyObject {
- public:
-  DummyObject() {}
+const IPEndPoint kEndpoint0(IPAddress(1, 2, 3, 4), 578);
+const IPEndPoint kEndpoint1(IPAddress(2, 3, 4, 5), 678);
 
-  base::WeakPtr<DummyObject> GetWeakPtr() { return weak_factory_.GetWeakPtr(); }
-
-  bool HasWeakPtrs() const { return weak_factory_.HasWeakPtrs(); }
-
- private:
-  base::WeakPtrFactory<DummyObject> weak_factory_{this};
-
-  DISALLOW_COPY_AND_ASSIGN(DummyObject);
-};
-
-class DummyRandIntCallback {
- public:
-  DummyRandIntCallback() = default;
-
-  RandIntCallback MakeCallback() {
-    return base::BindRepeating(&DummyRandIntCallback::GetRandInt,
-                               dummy_.GetWeakPtr());
-  }
-
-  bool HasRefs() const { return dummy_.HasWeakPtrs(); }
-
- private:
-  static int GetRandInt(base::WeakPtr<DummyObject> dummy, int from, int to) {
-    // Chosen by fair dice roll. Guaranteed to be random.
-    return 4;
+class DnsSocketPoolTest : public ::testing::Test {
+ protected:
+  void SetUp() override {
+    pool_ = std::make_unique<DnsSocketPool>(&socket_factory_, nameservers_,
+                                            nullptr /* net_log */);
   }
 
-  DummyObject dummy_;
-
-  DISALLOW_COPY_AND_ASSIGN(DummyRandIntCallback);
+  MockClientSocketFactory socket_factory_;
+  std::vector<IPEndPoint> nameservers_ = {kEndpoint0, kEndpoint1};
+  std::unique_ptr<DnsSocketPool> pool_;
 };
 
-// Since the below tests rely upon it, make sure that DummyRandIntCallback
-// can reliably tell whether there are other refs to the callback it returns.
-
-// A const reference to the callback shouldn't keep the callback referenced.
-TEST(DummyRandIntCallbackTest, Referenced) {
-  DummyRandIntCallback dummy;
+TEST_F(DnsSocketPoolTest, CreateConnectedUdpSocket) {
+  // Prep socket factory for a single do-nothing socket.
+  StaticSocketDataProvider data_provider;
+  socket_factory_.AddSocketDataProvider(&data_provider);
 
-  RandIntCallback original = dummy.MakeCallback();
-  EXPECT_TRUE(dummy.HasRefs());
-  const RandIntCallback& reference = original;
-  EXPECT_TRUE(dummy.HasRefs());
+  std::unique_ptr<DatagramClientSocket> socket =
+      pool_->CreateConnectedUdpSocket(1 /* server_index */);
 
-  EXPECT_EQ(4, reference.Run(0, 6));
+  ASSERT_TRUE(socket);
 
-  original.Reset();
-  EXPECT_FALSE(dummy.HasRefs());
+  IPEndPoint peer_address;
+  ASSERT_THAT(socket->GetPeerAddress(&peer_address), test::IsOk());
+  EXPECT_EQ(peer_address, kEndpoint1);
 }
 
-// A copy of the callback should keep the callback referenced.
-TEST(DummyRandIntCallbackTest, Copied) {
-  DummyRandIntCallback dummy;
+TEST_F(DnsSocketPoolTest, CreateConnectedUdpSocket_ConnectError) {
+  // Prep socket factory for a single socket with connection failure.
+  MockConnect connect_data;
+  connect_data.result = ERR_INSUFFICIENT_RESOURCES;
+  StaticSocketDataProvider data_provider;
+  data_provider.set_connect_data(connect_data);
+  socket_factory_.AddSocketDataProvider(&data_provider);
 
-  RandIntCallback original = dummy.MakeCallback();
-  EXPECT_TRUE(dummy.HasRefs());
-  RandIntCallback copy = original;
-  EXPECT_TRUE(dummy.HasRefs());
+  std::unique_ptr<DatagramClientSocket> socket =
+      pool_->CreateConnectedUdpSocket(0 /* server_index */);
 
-  EXPECT_EQ(4, copy.Run(0, 6));
-
-  original.Reset();
-  EXPECT_TRUE(dummy.HasRefs());
+  EXPECT_FALSE(socket);
 }
 
-class DnsSocketPoolTest : public ::testing::Test {
- protected:
-  DummyRandIntCallback dummy_;
-  std::unique_ptr<DnsSocketPool> pool_;
-};
-
-// Make sure that the DnsSocketPools returned by CreateDefault and CreateNull
-// both retain (by copying the RandIntCallback object, instead of taking a
-// reference) the RandIntCallback used for creating sockets.
+TEST_F(DnsSocketPoolTest, CreateTcpSocket) {
+  // Prep socket factory for a single do-nothing socket.
+  StaticSocketDataProvider data_provider;
+  socket_factory_.AddSocketDataProvider(&data_provider);
 
-TEST_F(DnsSocketPoolTest, DefaultCopiesCallback) {
-  pool_ = DnsSocketPool::CreateDefault(ClientSocketFactory::GetDefaultFactory(),
-                                       dummy_.MakeCallback());
-  EXPECT_TRUE(dummy_.HasRefs());
-}
+  std::unique_ptr<StreamSocket> socket =
+      pool_->CreateTcpSocket(1 /* server_index */, NetLogSource());
 
-TEST_F(DnsSocketPoolTest, NullCopiesCallback) {
-  pool_ = DnsSocketPool::CreateNull(ClientSocketFactory::GetDefaultFactory(),
-                                    dummy_.MakeCallback());
-  EXPECT_TRUE(dummy_.HasRefs());
+  EXPECT_TRUE(socket);
 }
 
 }  // namespace

net/dns/dns_test_util.cc

@@ -725,10 +725,13 @@ scoped_refptr<DnsSession> MockDnsClient::BuildSession() {
   auto null_random_callback =
       base::BindRepeating([](int, int) -> int { IMMEDIATE_CRASH(); });
 
+  auto socket_pool = std::make_unique<DnsSocketPool>(
+      &socket_factory_, effective_config_.value().nameservers,
+      nullptr /* net_log */);
+
   return base::MakeRefCounted<DnsSession>(
-      effective_config_.value(),
-      DnsSocketPool::CreateNull(&socket_factory_, null_random_callback),
-      null_random_callback, nullptr /* NetLog */);
+      effective_config_.value(), std::move(socket_pool), null_random_callback,
+      nullptr /* net_log */);
 }
 
 }  // namespace net

net/dns/dns_transaction_unittest.cc

@@ -638,8 +638,8 @@ class DnsTransactionTestBase : public testing::Test {
     socket_factory_.reset(new TestSocketFactory());
     session_ = new DnsSession(
         config_,
-        DnsSocketPool::CreateNull(socket_factory_.get(),
-                                  base::BindRepeating(base::RandInt)),
+        std::make_unique<DnsSocketPool>(
+            socket_factory_.get(), config_.nameservers, nullptr /* net_log */),
         base::BindRepeating(&DnsTransactionTestBase::GetNextId,
                             base::Unretained(this)),
         nullptr /* NetLog */);

net/dns/resolve_context_unittest.cc

@@ -46,8 +46,8 @@ class ResolveContextTest : public TestWithTaskEnvironment {
   scoped_refptr<DnsSession> CreateDnsSession(const DnsConfig& config) {
     auto null_random_callback =
         base::BindRepeating([](int, int) -> int { IMMEDIATE_CRASH(); });
-    std::unique_ptr<DnsSocketPool> dns_socket_pool =
-        DnsSocketPool::CreateNull(socket_factory_.get(), null_random_callback);
+    auto dns_socket_pool = std::make_unique<DnsSocketPool>(
+        socket_factory_.get(), config.nameservers, nullptr /* net_log */);
 
     return base::MakeRefCounted<DnsSession>(config, std::move(dns_socket_pool),
                                             null_random_callback,