Remove HttpNetworkSession argument from CreateSocketParamsAndGetGroupId.

Instead, this CL adds pointers to several global objects are to
CommonConnectJobParams, and moves the logic for testing_fixed_http_port
over to the calling function - it affects the endpoint used, which
affects part of the group_id, and hence is fine to keep around.

In order to avoid pooling sockets created with different SocketParams,
we're working on making it so there's a 1-to-1 mapping between GroupIds
and SocketParams. To that end, we're removing objects used to create
SocketParams.

Bug: 921369
Change-Id: Ib5b234ab8f4cc0c45c97d5a4c4e46e6e885536b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1531083
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#642470}

net/http/http_network_session.cc

@@ -492,7 +492,9 @@ CommonConnectJobParams HttpNetworkSession::CreateCommonConnectJobParams(
   return CommonConnectJobParams(
       context_.client_socket_factory ? context_.client_socket_factory
                                      : ClientSocketFactory::GetDefaultFactory(),
-      context_.host_resolver, context_.proxy_delegate,
+      context_.host_resolver, &http_auth_cache_,
+      context_.http_auth_handler_factory, &spdy_session_pool_,
+      &quic_stream_factory_, context_.proxy_delegate,
       context_.http_user_agent_settings,
       CreateClientSocketContext(context_, &ssl_client_session_cache_),
       CreateClientSocketContext(context_,

net/http/http_network_transaction_unittest.cc

@@ -381,6 +381,10 @@ class HttpNetworkTransactionTest : public PlatformTest,
         dummy_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_stream_factory */,
             nullptr /* proxy_delegate */,
             nullptr /* http_user_agent_settings */,
             SSLClientSocketContext(),

net/http/http_proxy_client_socket_pool_unittest.cc

@@ -71,46 +71,40 @@ class HttpProxyClientSocketPoolTest
     : public ::testing::TestWithParam<HttpProxyType>,
       public WithScopedTaskEnvironment {
  protected:
-  HttpProxyClientSocketPoolTest()
-      : common_connect_job_params_(
-            &socket_factory_,
-            session_deps_.host_resolver.get(),
-            nullptr /* proxy_delegate */,
-            nullptr /* http_user_agent_settings */,
-            SSLClientSocketContext(
-                session_deps_.cert_verifier.get(),
-                session_deps_.channel_id_service.get(),
-                session_deps_.transport_security_state.get(),
-                session_deps_.cert_transparency_verifier.get(),
-                session_deps_.ct_policy_enforcer.get(),
-                nullptr /* ssl_client_session_cache */),
-            SSLClientSocketContext(
-                session_deps_.cert_verifier.get(),
-                session_deps_.channel_id_service.get(),
-                session_deps_.transport_security_state.get(),
-                session_deps_.cert_transparency_verifier.get(),
-                session_deps_.ct_policy_enforcer.get(),
-                nullptr /* ssl_client_session_cache_privacy_mode */),
-            nullptr /* socket_performance_watcher_factory */,
-            &estimator_,
-            nullptr /* net_log */,
-            nullptr /* websocket_lock_endpoint_manager */),
-        pool_(std::make_unique<TransportClientSocketPool>(
-            kMaxSockets,
-            kMaxSocketsPerGroup,
-            kUnusedIdleSocketTimeout,
-            &common_connect_job_params_,
-            session_deps_.ssl_config_service.get())) {
+  HttpProxyClientSocketPoolTest() {
     session_deps_.host_resolver->set_synchronous_mode(true);
     session_ = CreateNetworkSession();
+    common_connect_job_params_ = std::make_unique<CommonConnectJobParams>(
+        &socket_factory_, session_deps_.host_resolver.get(),
+        session_->http_auth_cache(),
+        session_deps_.http_auth_handler_factory.get(),
+        session_->spdy_session_pool(), session_->quic_stream_factory(),
+        nullptr /* proxy_delegate */, nullptr /* http_user_agent_settings */,
+        SSLClientSocketContext(session_deps_.cert_verifier.get(),
+                               session_deps_.channel_id_service.get(),
+                               session_deps_.transport_security_state.get(),
+                               session_deps_.cert_transparency_verifier.get(),
+                               session_deps_.ct_policy_enforcer.get(),
+                               nullptr /* ssl_client_session_cache */),
+        SSLClientSocketContext(
+            session_deps_.cert_verifier.get(),
+            session_deps_.channel_id_service.get(),
+            session_deps_.transport_security_state.get(),
+            session_deps_.cert_transparency_verifier.get(),
+            session_deps_.ct_policy_enforcer.get(),
+            nullptr /* ssl_client_session_cache_privacy_mode */),
+        nullptr /* socket_performance_watcher_factory */, &estimator_,
+        nullptr /* net_log */, nullptr /* websocket_lock_endpoint_manager */);
+    InitPoolWithProxyDelegate(nullptr);
   }
 
   ~HttpProxyClientSocketPoolTest() override = default;
 
   void InitPoolWithProxyDelegate(ProxyDelegate* proxy_delegate) {
     pool_ = std::make_unique<TransportClientSocketPool>(
         kMaxSockets, kMaxSocketsPerGroup, kUnusedIdleSocketTimeout,
-        &common_connect_job_params_, session_deps_.ssl_config_service.get());
+        common_connect_job_params_.get(),
+        session_deps_.ssl_config_service.get());
   }
 
   void AddAuthToCache() {
@@ -153,9 +147,6 @@ class HttpProxyClientSocketPoolTest
                 CreateHttpProxyParams(), CreateHttpsProxyParams(),
                 quic::QUIC_VERSION_UNSUPPORTED,
                 HostPortPair("www.google.com", 443),
-                session_->http_auth_cache(),
-                session_->http_auth_handler_factory(),
-                session_->spdy_session_pool(), session_->quic_stream_factory(),
                 /*is_trusted_proxy=*/false, /*tunnel=*/true,
                 TRAFFIC_ANNOTATION_FOR_TESTS));
   }
@@ -207,7 +198,7 @@ class HttpProxyClientSocketPoolTest
 
   std::unique_ptr<HttpNetworkSession> session_;
 
-  const CommonConnectJobParams common_connect_job_params_;
+  std::unique_ptr<CommonConnectJobParams> common_connect_job_params_;
 
   base::HistogramTester histogram_tester_;
 

net/http/http_proxy_connect_job.cc

@@ -134,21 +134,13 @@ HttpProxySocketParams::HttpProxySocketParams(
     const scoped_refptr<SSLSocketParams>& ssl_params,
     quic::QuicTransportVersion quic_version,
     const HostPortPair& endpoint,
-    HttpAuthCache* http_auth_cache,
-    HttpAuthHandlerFactory* http_auth_handler_factory,
-    SpdySessionPool* spdy_session_pool,
-    QuicStreamFactory* quic_stream_factory,
     bool is_trusted_proxy,
     bool tunnel,
     const NetworkTrafficAnnotationTag traffic_annotation)
     : transport_params_(transport_params),
       ssl_params_(ssl_params),
       quic_version_(quic_version),
-      spdy_session_pool_(spdy_session_pool),
-      quic_stream_factory_(quic_stream_factory),
       endpoint_(endpoint),
-      http_auth_cache_(tunnel ? http_auth_cache : nullptr),
-      http_auth_handler_factory_(tunnel ? http_auth_handler_factory : nullptr),
       is_trusted_proxy_(is_trusted_proxy),
       tunnel_(tunnel),
       traffic_annotation_(traffic_annotation) {
@@ -194,8 +186,8 @@ HttpProxyConnectJob::HttpProxyConnectJob(
                     HttpAuth::AUTH_PROXY,
                     GURL((params_->ssl_params() ? "https://" : "http://") +
                          GetDestination().ToString()),
-                    params_->http_auth_cache(),
-                    params_->http_auth_handler_factory(),
+                    common_connect_job_params->http_auth_cache,
+                    common_connect_job_params->http_auth_handler_factory,
                     host_resolver())
               : nullptr),
       weak_ptr_factory_(this) {}
@@ -489,7 +481,7 @@ int HttpProxyConnectJob::DoSSLConnect() {
         params_->ssl_params()->GetDirectConnectionParams()->destination(),
         ProxyServer::Direct(), PRIVACY_MODE_DISABLED,
         SpdySessionKey::IsProxySession::kTrue, socket_tag());
-    if (params_->spdy_session_pool()->FindAvailableSession(
+    if (common_connect_job_params()->spdy_session_pool->FindAvailableSession(
             key, /* enable_ip_based_pooling = */ true,
             /* is_websocket = */ false, net_log())) {
       using_spdy_ = true;
@@ -611,18 +603,19 @@ int HttpProxyConnectJob::DoSpdyProxyCreateStream() {
       ProxyServer::Direct(), PRIVACY_MODE_DISABLED,
       SpdySessionKey::IsProxySession::kTrue, socket_tag());
   base::WeakPtr<SpdySession> spdy_session =
-      params_->spdy_session_pool()->FindAvailableSession(
+      common_connect_job_params()->spdy_session_pool->FindAvailableSession(
           key, /* enable_ip_based_pooling = */ true,
           /* is_websocket = */ false, net_log());
   // It's possible that a session to the proxy has recently been created
   if (spdy_session) {
     nested_connect_job_.reset();
   } else {
     // Create a session direct to the proxy itself
-    spdy_session =
-        params_->spdy_session_pool()->CreateAvailableSessionFromSocket(
-            key, params_->is_trusted_proxy(), nested_connect_job_->PassSocket(),
-            nested_connect_job_->connect_timing(), net_log());
+    spdy_session = common_connect_job_params()
+                       ->spdy_session_pool->CreateAvailableSessionFromSocket(
+                           key, params_->is_trusted_proxy(),
+                           nested_connect_job_->PassSocket(),
+                           nested_connect_job_->connect_timing(), net_log());
     DCHECK(spdy_session);
     nested_connect_job_.reset();
   }
@@ -664,8 +657,8 @@ int HttpProxyConnectJob::DoQuicProxyCreateSession() {
   next_state_ = STATE_QUIC_PROXY_CREATE_STREAM;
   const HostPortPair& proxy_server =
       ssl_params->GetDirectConnectionParams()->destination();
-  quic_stream_request_ =
-      std::make_unique<QuicStreamRequest>(params_->quic_stream_factory());
+  quic_stream_request_ = std::make_unique<QuicStreamRequest>(
+      common_connect_job_params()->quic_stream_factory);
   return quic_stream_request_->Request(
       proxy_server, params_->quic_version(), ssl_params->privacy_mode(),
       kH2QuicTunnelPriority, socket_tag(),

net/http/http_proxy_connect_job.h

@@ -24,14 +24,11 @@
 
 namespace net {
 
-class HttpAuthCache;
 class HttpAuthController;
-class HttpAuthHandlerFactory;
 class HttpResponseInfo;
 class NetworkQualityEstimator;
 class SocketTag;
 class ProxyClientSocket;
-class SpdySessionPool;
 class SpdyStreamRequest;
 class SSLSocketParams;
 class TransportSocketParams;
@@ -50,10 +47,6 @@ class NET_EXPORT_PRIVATE HttpProxySocketParams
       const scoped_refptr<SSLSocketParams>& ssl_params,
       quic::QuicTransportVersion quic_version,
       const HostPortPair& endpoint,
-      HttpAuthCache* http_auth_cache,
-      HttpAuthHandlerFactory* http_auth_handler_factory,
-      SpdySessionPool* spdy_session_pool,
-      QuicStreamFactory* quic_stream_factory,
       bool is_trusted_proxy,
       bool tunnel,
       const NetworkTrafficAnnotationTag traffic_annotation);
@@ -66,14 +59,6 @@ class NET_EXPORT_PRIVATE HttpProxySocketParams
   }
   quic::QuicTransportVersion quic_version() const { return quic_version_; }
   const HostPortPair& endpoint() const { return endpoint_; }
-  HttpAuthCache* http_auth_cache() const { return http_auth_cache_; }
-  HttpAuthHandlerFactory* http_auth_handler_factory() const {
-    return http_auth_handler_factory_;
-  }
-  SpdySessionPool* spdy_session_pool() { return spdy_session_pool_; }
-  QuicStreamFactory* quic_stream_factory() const {
-    return quic_stream_factory_;
-  }
   bool is_trusted_proxy() const { return is_trusted_proxy_; }
   bool tunnel() const { return tunnel_; }
   const NetworkTrafficAnnotationTag traffic_annotation() const {
@@ -87,11 +72,7 @@ class NET_EXPORT_PRIVATE HttpProxySocketParams
   const scoped_refptr<TransportSocketParams> transport_params_;
   const scoped_refptr<SSLSocketParams> ssl_params_;
   quic::QuicTransportVersion quic_version_;
-  SpdySessionPool* spdy_session_pool_;
-  QuicStreamFactory* quic_stream_factory_;
   const HostPortPair endpoint_;
-  HttpAuthCache* const http_auth_cache_;
-  HttpAuthHandlerFactory* const http_auth_handler_factory_;
   const bool is_trusted_proxy_;
   const bool tunnel_;
   const NetworkTrafficAnnotationTag traffic_annotation_;

net/http/http_proxy_connect_job_unittest.cc

@@ -135,8 +135,6 @@ class HttpProxyConnectJobTest : public ::testing::TestWithParam<HttpProxyType>,
         CreateHttpProxyParams(), CreateHttpsProxyParams(),
         quic::QUIC_VERSION_UNSUPPORTED,
         HostPortPair(kEndpointHost, tunnel ? 443 : 80),
-        session_->http_auth_cache(), session_->http_auth_handler_factory(),
-        session_->spdy_session_pool(), session_->quic_stream_factory(),
         /*is_trusted_proxy=*/false, tunnel, TRAFFIC_ANNOTATION_FOR_TESTS);
   }
 

net/socket/client_socket_pool_base_unittest.cc

@@ -537,6 +537,10 @@ class TestConnectJobFactory
       : 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_stream_factory */,
             nullptr /* proxy_delegate */,
             nullptr /* http_user_agent_settings */,
             SSLClientSocketContext(),

net/socket/client_socket_pool_manager.cc

@@ -67,39 +67,44 @@ static_assert(base::size(g_max_sockets_per_proxy_server) ==
 
 // The meat of the implementation for the InitSocketHandleForHttpRequest,
 // InitSocketHandleForRawConnect and PreconnectSocketsForHttpRequest methods.
+//
+// DO NOT ADD ANY ARGUMENTS TO THIS METHOD.
+//
+// TODO(https://crbug.com/921369) In order to resolve longstanding issues
+// related to pooling distinguishable sockets together, reduce the arguments to
+// just those that are used to populate |connection_group|, and those used to
+// locate the socket pool to use.
 scoped_refptr<TransportClientSocketPool::SocketParams>
 CreateSocketParamsAndGetGroupId(
     ClientSocketPoolManager::SocketGroupType group_type,
     const HostPortPair& endpoint,
+    // This argument should be removed.
     int request_load_flags,
-    HttpNetworkSession* session,
     const ProxyInfo& proxy_info,
+    // This argument should be removed.
     quic::QuicTransportVersion quic_version,
+    // This argument should be removed.
     const SSLConfig& ssl_config_for_origin,
+    // This argument should be removed.
     const SSLConfig& ssl_config_for_proxy,
+    // This argument should be removed.
     bool force_tunnel,
     PrivacyMode privacy_mode,
+    // TODO(https://crbug.com/912727):  This argument should be removed.
     const OnHostResolutionCallback& resolution_callback,
     ClientSocketPool::GroupId* connection_group) {
   scoped_refptr<HttpProxySocketParams> http_proxy_params;
   scoped_refptr<SOCKSSocketParams> socks_params;
 
   const bool using_ssl = group_type == ClientSocketPoolManager::SSL_GROUP;
-  HostPortPair origin_host_port = endpoint;
-
-  if (!using_ssl && session->params().testing_fixed_http_port != 0) {
-    origin_host_port.set_port(session->params().testing_fixed_http_port);
-  } else if (using_ssl && session->params().testing_fixed_https_port != 0) {
-    origin_host_port.set_port(session->params().testing_fixed_https_port);
-  }
 
   // LOAD_BYPASS_CACHE should bypass the host cache as well as the HTTP cache.
   // Other cache-related load flags should not have this effect.
   bool disable_resolver_cache = request_load_flags & LOAD_BYPASS_CACHE;
 
   // Build the string used to uniquely identify connections of this type.
   // Determine the host and port to connect to.
-  DCHECK(!origin_host_port.IsEmpty());
+  DCHECK(!endpoint.IsEmpty());
   ClientSocketPool::SocketType socket_type =
       ClientSocketPool::SocketType::kHttp;
 
@@ -118,7 +123,7 @@ CreateSocketParamsAndGetGroupId(
   }
 
   *connection_group = ClientSocketPool::GroupId(
-      origin_host_port, socket_type, privacy_mode == PRIVACY_MODE_ENABLED);
+      endpoint, socket_type, privacy_mode == PRIVACY_MODE_ENABLED);
 
   if (!proxy_info.is_direct()) {
     ProxyServer proxy_server = proxy_info.proxy_server();
@@ -144,16 +149,14 @@ CreateSocketParamsAndGetGroupId(
       }
 
       http_proxy_params = new HttpProxySocketParams(
-          proxy_tcp_params, ssl_params, quic_version, origin_host_port,
-          session->http_auth_cache(), session->http_auth_handler_factory(),
-          session->spdy_session_pool(), session->quic_stream_factory(),
+          proxy_tcp_params, ssl_params, quic_version, endpoint,
           proxy_server.is_trusted_proxy(), force_tunnel || using_ssl,
           NetworkTrafficAnnotationTag(proxy_info.traffic_annotation()));
     } else {
       DCHECK(proxy_info.is_socks());
       socks_params = new SOCKSSocketParams(
           proxy_tcp_params, proxy_server.scheme() == ProxyServer::SCHEME_SOCKS5,
-          origin_host_port,
+          endpoint,
           NetworkTrafficAnnotationTag(proxy_info.traffic_annotation()));
     }
   }
@@ -163,11 +166,11 @@ CreateSocketParamsAndGetGroupId(
     scoped_refptr<TransportSocketParams> ssl_tcp_params;
     if (proxy_info.is_direct()) {
       ssl_tcp_params = base::MakeRefCounted<TransportSocketParams>(
-          origin_host_port, disable_resolver_cache, resolution_callback);
+          endpoint, disable_resolver_cache, resolution_callback);
     }
     scoped_refptr<SSLSocketParams> ssl_params =
         base::MakeRefCounted<SSLSocketParams>(
-            ssl_tcp_params, socks_params, http_proxy_params, origin_host_port,
+            ssl_tcp_params, socks_params, http_proxy_params, endpoint,
             ssl_config_for_origin, privacy_mode);
     return TransportClientSocketPool::SocketParams::CreateFromSSLSocketParams(
         std::move(ssl_params));
@@ -185,7 +188,7 @@ CreateSocketParamsAndGetGroupId(
 
   DCHECK(proxy_info.is_direct());
   scoped_refptr<TransportSocketParams> tcp_params = new TransportSocketParams(
-      origin_host_port, disable_resolver_cache, resolution_callback);
+      endpoint, disable_resolver_cache, resolution_callback);
   return TransportClientSocketPool::SocketParams::
       CreateFromTransportSocketParams(std::move(tcp_params));
 }
@@ -210,10 +213,19 @@ int InitSocketPoolHelper(
     const OnHostResolutionCallback& resolution_callback,
     CompletionOnceCallback callback,
     const ClientSocketPool::ProxyAuthCallback& proxy_auth_callback) {
+  bool using_ssl = group_type == ClientSocketPoolManager::SSL_GROUP;
+  HostPortPair origin_host_port = endpoint;
+
+  if (!using_ssl && session->params().testing_fixed_http_port != 0) {
+    origin_host_port.set_port(session->params().testing_fixed_http_port);
+  } else if (using_ssl && session->params().testing_fixed_https_port != 0) {
+    origin_host_port.set_port(session->params().testing_fixed_https_port);
+  }
+
   ClientSocketPool::GroupId connection_group;
   scoped_refptr<TransportClientSocketPool::SocketParams> socket_params =
       CreateSocketParamsAndGetGroupId(
-          group_type, endpoint, request_load_flags, session, proxy_info,
+          group_type, origin_host_port, request_load_flags, proxy_info,
           quic_version, ssl_config_for_origin, ssl_config_for_proxy,
           force_tunnel, privacy_mode, resolution_callback, &connection_group);