Implement ConnectJobFactory

Refactor intended to make ConnectJob construction logic more testable.

Each ConnectJob type now has its own mockable Factory class to allow
replacing the individual construction in tests.  The new
ConnectJobFactory contains the logic that was previously in
ConnectJob::CreateConnectJob() but now in a stateful class to keep the
individual type factories and allow replacing them for tests.  The
ConnectJobFactory instances are owned by the ClientSocketPools (or by
network::ProxyResolvingClientSocketFactory for mojo socket creation).

ConnectJobFactory also replaces the previously-used
TransportClientSocketPool::ConnectJobFactory.  But where that lived in
TransportClientSocketPool and called into
ClientSocketPool::CreateConnectJob() (where ClientSocketPool added a
bit of extra logic to create an OnHostResolutionCallback), now the
ConnectJobFactory lives in the ClientSocketPool and the extra callback
stuff is done before calling into the factory.

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

net/BUILD.gn

@@ -846,6 +846,8 @@ component("net") {
     "socket/client_socket_pool_manager_impl.h",
     "socket/connect_job.cc",
     "socket/connect_job.h",
+    "socket/connect_job_factory.cc",
+    "socket/connect_job_factory.h",
     "socket/connection_attempts.h",
     "socket/datagram_client_socket.h",
     "socket/datagram_server_socket.h",
@@ -4308,6 +4310,7 @@ test("net_unittests") {
     "quic/test_quic_crypto_client_config_handle.h",
     "socket/client_socket_pool_base_unittest.cc",
     "socket/client_socket_pool_unittest.cc",
+    "socket/connect_job_factory_unittest.cc",
     "socket/connect_job_test_util.cc",
     "socket/connect_job_test_util.h",
     "socket/connect_job_unittest.cc",

net/http/http_proxy_connect_job.cc

@@ -4,6 +4,7 @@
 
 #include "net/http/http_proxy_connect_job.h"
 
+#include <memory>
 #include <utility>
 
 #include "base/bind.h"
@@ -156,6 +157,18 @@ HttpProxySocketParams::HttpProxySocketParams(
 
 HttpProxySocketParams::~HttpProxySocketParams() = default;
 
+std::unique_ptr<HttpProxyConnectJob> HttpProxyConnectJob::Factory::Create(
+    RequestPriority priority,
+    const SocketTag& socket_tag,
+    const CommonConnectJobParams* common_connect_job_params,
+    scoped_refptr<HttpProxySocketParams> params,
+    ConnectJob::Delegate* delegate,
+    const NetLogWithSource* net_log) {
+  return std::make_unique<HttpProxyConnectJob>(
+      priority, socket_tag, common_connect_job_params, std::move(params),
+      delegate, net_log);
+}
+
 HttpProxyConnectJob::HttpProxyConnectJob(
     RequestPriority priority,
     const SocketTag& socket_tag,

net/http/http_proxy_connect_job.h

@@ -88,6 +88,20 @@ class NET_EXPORT_PRIVATE HttpProxySocketParams
 class NET_EXPORT_PRIVATE HttpProxyConnectJob : public ConnectJob,
                                                public ConnectJob::Delegate {
  public:
+  class NET_EXPORT_PRIVATE Factory {
+   public:
+    Factory() = default;
+    virtual ~Factory() = default;
+
+    virtual std::unique_ptr<HttpProxyConnectJob> Create(
+        RequestPriority priority,
+        const SocketTag& socket_tag,
+        const CommonConnectJobParams* common_connect_job_params,
+        scoped_refptr<HttpProxySocketParams> params,
+        ConnectJob::Delegate* delegate,
+        const NetLogWithSource* net_log);
+  };
+
   HttpProxyConnectJob(RequestPriority priority,
                       const SocketTag& socket_tag,
                       const CommonConnectJobParams* common_connect_job_params,

net/socket/client_socket_pool.cc

@@ -4,18 +4,21 @@
 
 #include "net/socket/client_socket_pool.h"
 
+#include <memory>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/check_op.h"
 #include "base/feature_list.h"
 #include "net/base/features.h"
 #include "net/base/host_port_pair.h"
+#include "net/base/proxy_server.h"
 #include "net/dns/public/secure_dns_policy.h"
 #include "net/http/http_proxy_connect_job.h"
 #include "net/log/net_log_event_type.h"
 #include "net/log/net_log_with_source.h"
 #include "net/socket/connect_job.h"
+#include "net/socket/connect_job_factory.h"
 #include "net/socket/socks_connect_job.h"
 #include "net/socket/ssl_connect_job.h"
 #include "net/socket/stream_socket.h"
@@ -139,7 +142,13 @@ void ClientSocketPool::set_used_idle_socket_timeout(base::TimeDelta timeout) {
   g_used_idle_socket_timeout_s = timeout.InSeconds();
 }
 
-ClientSocketPool::ClientSocketPool() = default;
+ClientSocketPool::ClientSocketPool(
+    bool is_for_websockets,
+    const CommonConnectJobParams* common_connect_job_params,
+    std::unique_ptr<ConnectJobFactory> connect_job_factory)
+    : is_for_websockets_(is_for_websockets),
+      common_connect_job_params_(common_connect_job_params),
+      connect_job_factory_(std::move(connect_job_factory)) {}
 
 void ClientSocketPool::NetLogTcpClientSocketPoolRequestedSocket(
     const NetLogWithSource& net_log,
@@ -162,8 +171,6 @@ std::unique_ptr<ConnectJob> ClientSocketPool::CreateConnectJob(
     scoped_refptr<SocketParams> socket_params,
     const ProxyServer& proxy_server,
     const absl::optional<NetworkTrafficAnnotationTag>& proxy_annotation_tag,
-    bool is_for_websockets,
-    const CommonConnectJobParams* common_connect_job_params,
     RequestPriority request_priority,
     SocketTag socket_tag,
     ConnectJob::Delegate* delegate) {
@@ -174,34 +181,34 @@ std::unique_ptr<ConnectJob> ClientSocketPool::CreateConnectJob(
   OnHostResolutionCallback resolution_callback;
   if (using_ssl && proxy_server.is_direct()) {
     resolution_callback = base::BindRepeating(
-        &OnHostResolution, common_connect_job_params->spdy_session_pool,
+        &OnHostResolution, common_connect_job_params_->spdy_session_pool,
         // TODO(crbug.com/1206799): Pass along as SchemeHostPort.
         SpdySessionKey(HostPortPair::FromSchemeHostPort(group_id.destination()),
                        proxy_server, group_id.privacy_mode(),
                        SpdySessionKey::IsProxySession::kFalse, socket_tag,
                        group_id.network_isolation_key(),
                        group_id.secure_dns_policy()),
-        is_for_websockets);
+        is_for_websockets_);
   } else if (proxy_server.is_https()) {
     resolution_callback = base::BindRepeating(
-        &OnHostResolution, common_connect_job_params->spdy_session_pool,
+        &OnHostResolution, common_connect_job_params_->spdy_session_pool,
         SpdySessionKey(proxy_server.host_port_pair(), ProxyServer::Direct(),
                        group_id.privacy_mode(),
                        SpdySessionKey::IsProxySession::kTrue, socket_tag,
                        group_id.network_isolation_key(),
                        group_id.secure_dns_policy()),
-        is_for_websockets);
+        is_for_websockets_);
   }
 
   // TODO(crbug.com/1206799): Pass along as SchemeHostPort.
-  return ConnectJob::CreateConnectJob(
+  return connect_job_factory_->CreateConnectJob(
       using_ssl, HostPortPair::FromSchemeHostPort(group_id.destination()),
       proxy_server, proxy_annotation_tag,
       socket_params->ssl_config_for_origin(),
-      socket_params->ssl_config_for_proxy(), is_for_websockets,
+      socket_params->ssl_config_for_proxy(), is_for_websockets_,
       group_id.privacy_mode(), resolution_callback, request_priority,
       socket_tag, group_id.network_isolation_key(),
-      group_id.secure_dns_policy(), common_connect_job_params, delegate);
+      group_id.secure_dns_policy(), common_connect_job_params_, delegate);
 }
 
 }  // namespace net

net/socket/client_socket_pool.h

@@ -36,7 +36,7 @@ class ProcessMemoryDump;
 namespace net {
 
 class ClientSocketHandle;
-struct CommonConnectJobParams;
+class ConnectJobFactory;
 class HttpAuthController;
 class HttpResponseInfo;
 class NetLogWithSource;
@@ -341,26 +341,30 @@ class NET_EXPORT ClientSocketPool : public LowerLayeredPool {
   static void set_used_idle_socket_timeout(base::TimeDelta timeout);
 
  protected:
-  ClientSocketPool();
+  ClientSocketPool(bool is_for_websockets,
+                   const CommonConnectJobParams* common_connect_job_params,
+                   std::unique_ptr<ConnectJobFactory> connect_job_factory);
 
   void NetLogTcpClientSocketPoolRequestedSocket(const NetLogWithSource& net_log,
                                                 const GroupId& group_id);
 
   // Utility method to log a GroupId with a NetLog event.
   static base::Value NetLogGroupIdParams(const GroupId& group_id);
 
-  static std::unique_ptr<ConnectJob> CreateConnectJob(
+  std::unique_ptr<ConnectJob> CreateConnectJob(
       GroupId group_id,
       scoped_refptr<SocketParams> socket_params,
       const ProxyServer& proxy_server,
       const absl::optional<NetworkTrafficAnnotationTag>& proxy_annotation_tag,
-      bool is_for_websockets,
-      const CommonConnectJobParams* common_connect_job_params,
       RequestPriority request_priority,
       SocketTag socket_tag,
       ConnectJob::Delegate* delegate);
 
  private:
+  const bool is_for_websockets_;
+  const CommonConnectJobParams* const common_connect_job_params_;
+  const std::unique_ptr<ConnectJobFactory> connect_job_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ClientSocketPool);
 };
 

net/socket/client_socket_pool_base_unittest.cc

@@ -48,6 +48,7 @@
 #include "net/log/test_net_log_util.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/client_socket_handle.h"
+#include "net/socket/connect_job_factory.h"
 #include "net/socket/datagram_client_socket.h"
 #include "net/socket/socket_performance_watcher.h"
 #include "net/socket/socket_tag.h"
@@ -540,29 +541,10 @@ class TestConnectJob : public ConnectJob {
   DISALLOW_COPY_AND_ASSIGN(TestConnectJob);
 };
 
-class TestConnectJobFactory
-    : public TransportClientSocketPool::ConnectJobFactory {
+class TestConnectJobFactory : public ConnectJobFactory {
  public:
-  TestConnectJobFactory(MockClientSocketFactory* client_socket_factory,
-                        NetLog* net_log)
-      : common_connect_job_params_(
-            nullptr /* client_socket_factory */,
-            nullptr /* host_resolver */,
-            nullptr /* http_auth_cache */,
-            nullptr /* http_auth_handler_factory */,
-            nullptr /* spdy_session_pool */,
-            nullptr /* quic_supported_versions */,
-            nullptr /* quic_stream_factory */,
-            nullptr /* proxy_delegate */,
-            nullptr /* http_user_agent_settings */,
-            nullptr /* ssl_client_context */,
-            nullptr /* socket_performance_watcher_factory */,
-            nullptr /* network_quality_estimator */,
-            net_log,
-            nullptr /* websocket_endpoint_lock_manager */),
-        job_type_(TestConnectJob::kMockJob),
-        job_types_(nullptr),
-        client_socket_factory_(client_socket_factory) {}
+  explicit TestConnectJobFactory(MockClientSocketFactory* client_socket_factory)
+      : client_socket_factory_(client_socket_factory) {}
 
   ~TestConnectJobFactory() override = default;
 
@@ -579,12 +561,21 @@ class TestConnectJobFactory
 
   // ConnectJobFactory implementation.
 
-  std::unique_ptr<ConnectJob> NewConnectJob(
-      ClientSocketPool::GroupId group_id,
-      scoped_refptr<ClientSocketPool::SocketParams> socket_params,
+  std::unique_ptr<ConnectJob> CreateConnectJob(
+      bool using_ssl,
+      const HostPortPair& endpoint,
+      const ProxyServer& proxy_server,
       const absl::optional<NetworkTrafficAnnotationTag>& proxy_annotation_tag,
+      const SSLConfig* ssl_config_for_origin,
+      const SSLConfig* ssl_config_for_proxy,
+      bool force_tunnel,
+      PrivacyMode privacy_mode,
+      const OnHostResolutionCallback& resolution_callback,
       RequestPriority request_priority,
       SocketTag socket_tag,
+      const NetworkIsolationKey& network_isolation_key,
+      SecureDnsPolicy secure_dns_policy,
+      const CommonConnectJobParams* common_connect_job_params,
       ConnectJob::Delegate* delegate) const override {
     EXPECT_TRUE(!job_types_ || !job_types_->empty());
     TestConnectJob::JobType job_type = job_type_;
@@ -594,13 +585,12 @@ class TestConnectJobFactory
     }
     return std::make_unique<TestConnectJob>(
         job_type, request_priority, socket_tag, timeout_duration_,
-        &common_connect_job_params_, delegate, client_socket_factory_);
+        common_connect_job_params, delegate, client_socket_factory_);
   }
 
  private:
-  const CommonConnectJobParams common_connect_job_params_;
-  TestConnectJob::JobType job_type_;
-  std::list<TestConnectJob::JobType>* job_types_;
+  TestConnectJob::JobType job_type_ = TestConnectJob::kMockJob;
+  std::list<TestConnectJob::JobType>* job_types_ = nullptr;
   base::TimeDelta timeout_duration_;
   MockClientSocketFactory* const client_socket_factory_;
 
@@ -669,12 +659,12 @@ class ClientSocketPoolBaseTest : public TestWithTaskEnvironment {
       ProxyServer proxy_server = ProxyServer::Direct()) {
     DCHECK(!pool_.get());
     std::unique_ptr<TestConnectJobFactory> connect_job_factory =
-        std::make_unique<TestConnectJobFactory>(&client_socket_factory_,
-                                                &net_log_);
+        std::make_unique<TestConnectJobFactory>(&client_socket_factory_);
     connect_job_factory_ = connect_job_factory.get();
     pool_ = TransportClientSocketPool::CreateForTesting(
         max_sockets, max_sockets_per_group, unused_idle_socket_timeout,
-        used_idle_socket_timeout, proxy_server, std::move(connect_job_factory),
+        used_idle_socket_timeout, proxy_server, /*is_for_websockets=*/false,
+        &common_connect_job_params_, std::move(connect_job_factory),
         nullptr /* ssl_config_service */, enable_backup_connect_jobs);
   }
 
@@ -729,6 +719,21 @@ class ClientSocketPoolBaseTest : public TestWithTaskEnvironment {
   size_t completion_count() const { return test_base_.completion_count(); }
 
   RecordingTestNetLog net_log_;
+  const CommonConnectJobParams common_connect_job_params_{
+      nullptr /* client_socket_factory */,
+      nullptr /* host_resolver */,
+      nullptr /* http_auth_cache */,
+      nullptr /* http_auth_handler_factory */,
+      nullptr /* spdy_session_pool */,
+      nullptr /* quic_supported_versions */,
+      nullptr /* quic_stream_factory */,
+      nullptr /* proxy_delegate */,
+      nullptr /* http_user_agent_settings */,
+      nullptr /* ssl_client_context */,
+      nullptr /* socket_performance_watcher_factory */,
+      nullptr /* network_quality_estimator */,
+      &net_log_,
+      nullptr /* websocket_endpoint_lock_manager */};
   bool connect_backup_jobs_enabled_;
   MockClientSocketFactory client_socket_factory_;
   TestConnectJobFactory* connect_job_factory_;