Socket Pools Refactor 19: Move HttpProxyConnectJob to its own file.

And rewrite most HttpProxyClientSocketPool tests in terms of it. The
only remaining tests are for HTTP auth, client certs, socket tagging,
or for cases that return a socket on failure, all of which include
some aspect of testing the socket pool as well, and will be delth with
in future CLs.

This CL also changes type of the |respect_limits| argument to
HttpProxyConnectJob to a bool, from a ClientSocketPool::RespectLimits.
HttpProxyConnectJob is otherwise unchanged.

The HttpProxyClientSocketPool class is going to be merged into the
TransportClientSocketPool class, but we'll still need these tests.

This is part of an effort to flatten the socket pools.
https://docs.google.com/document/d/1g0EA4iDqaDhNXA_mq-YK3SlSX-xRkoKvZetAQqdRrxM/edit

Bug: 472729
Change-Id: Ic0570a7e48ad012da793723bbecbdf04d3659586
Reviewed-on: https://chromium-review.googlesource.com/c/1446563
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#630576}

net/BUILD.gn

@@ -806,6 +806,8 @@ component("net") {
       "http/http_proxy_client_socket_pool.h",
       "http/http_proxy_client_socket_wrapper.cc",
       "http/http_proxy_client_socket_wrapper.h",
+      "http/http_proxy_connect_job.cc",
+      "http/http_proxy_connect_job.h",
       "http/http_request_info.cc",
       "http/http_request_info.h",
       "http/http_response_body_drainer.cc",
@@ -4940,6 +4942,7 @@ test("net_unittests") {
     "http/http_proxy_client_socket_pool_unittest.cc",
     "http/http_proxy_client_socket_unittest.cc",
     "http/http_proxy_client_socket_wrapper_unittest.cc",
+    "http/http_proxy_connect_job_unittest.cc",
     "http/http_request_headers_unittest.cc",
     "http/http_response_body_drainer_unittest.cc",
     "http/http_response_headers_unittest.cc",

net/http/http_proxy_client_socket_pool.h

@@ -5,162 +5,23 @@
 #ifndef NET_HTTP_HTTP_PROXY_CLIENT_SOCKET_POOL_H_
 #define NET_HTTP_HTTP_PROXY_CLIENT_SOCKET_POOL_H_
 
-#include <stdint.h>
-
 #include <memory>
 #include <string>
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/weak_ptr.h"
-#include "base/time/time.h"
 #include "net/base/completion_once_callback.h"
-#include "net/base/host_port_pair.h"
 #include "net/base/net_export.h"
-#include "net/http/http_auth.h"
-#include "net/http/http_response_info.h"
-#include "net/http/proxy_client_socket.h"
+#include "net/http/http_proxy_connect_job.h"
 #include "net/socket/client_socket_pool.h"
 #include "net/socket/client_socket_pool_base.h"
-#include "net/socket/ssl_client_socket.h"
-#include "net/spdy/spdy_session.h"
-#include "net/traffic_annotation/network_traffic_annotation.h"
 
 namespace net {
 
-class HttpAuthCache;
-class HttpAuthHandlerFactory;
-class HttpProxyClientSocketWrapper;
 class NetLog;
 class NetworkQualityEstimator;
 class ProxyDelegate;
-class QuicStreamFactory;
-class SSLSocketParams;
-class SpdySessionPool;
 class TransportClientSocketPool;
-class TransportSocketParams;
-
-// HttpProxySocketParams only needs the socket params for one of the proxy
-// types.  The other param must be NULL.  When using an HTTP proxy,
-// |transport_params| must be set.  When using an HTTPS proxy or QUIC proxy,
-// |ssl_params| must be set. Also, if using a QUIC proxy, |quic_version| must
-// not be quic::QUIC_VERSION_UNSUPPORTED.
-class NET_EXPORT_PRIVATE HttpProxySocketParams
-    : public base::RefCounted<HttpProxySocketParams> {
- public:
-  HttpProxySocketParams(
-      const scoped_refptr<TransportSocketParams>& transport_params,
-      const scoped_refptr<SSLSocketParams>& ssl_params,
-      quic::QuicTransportVersion quic_version,
-      const std::string& user_agent,
-      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);
-
-  const scoped_refptr<TransportSocketParams>& transport_params() const {
-    return transport_params_;
-  }
-  const scoped_refptr<SSLSocketParams>& ssl_params() const {
-    return ssl_params_;
-  }
-  quic::QuicTransportVersion quic_version() const { return quic_version_; }
-  const std::string& user_agent() const { return user_agent_; }
-  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 {
-    return traffic_annotation_;
-  }
-
- private:
-  friend class base::RefCounted<HttpProxySocketParams>;
-  ~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 std::string user_agent_;
-  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_;
-
-  DISALLOW_COPY_AND_ASSIGN(HttpProxySocketParams);
-};
-
-// HttpProxyConnectJob optionally establishes a tunnel through the proxy
-// server after connecting the underlying transport socket.
-class HttpProxyConnectJob : public ConnectJob {
- public:
-  HttpProxyConnectJob(const std::string& group_name,
-                      RequestPriority priority,
-                      const SocketTag& socket_tag,
-                      ClientSocketPool::RespectLimits respect_limits,
-                      const scoped_refptr<HttpProxySocketParams>& params,
-                      ProxyDelegate* proxy_delegate,
-                      TransportClientSocketPool* transport_pool,
-                      TransportClientSocketPool* ssl_pool,
-                      NetworkQualityEstimator* network_quality_estimator,
-                      Delegate* delegate,
-                      NetLog* net_log);
-  ~HttpProxyConnectJob() override;
-
-  // ConnectJob methods.
-  LoadState GetLoadState() const override;
-  bool HasEstablishedConnection() const override;
-
-  void GetAdditionalErrorState(ClientSocketHandle* handle) override;
-
-  // Returns the connection timeout that will be used by a HttpProxyConnectJob
-  // created with the specified parameters, given current network conditions.
-  NET_EXPORT_PRIVATE static base::TimeDelta ConnectionTimeout(
-      const HttpProxySocketParams& params,
-      const NetworkQualityEstimator* network_quality_estimator);
-
-  // Updates the field trial parameters used in calculating timeouts.
-  NET_EXPORT_PRIVATE static void UpdateFieldTrialParametersForTesting();
-
- private:
-  // Begins the tcp connection and the optional Http proxy tunnel.  If the
-  // request is not immediately servicable (likely), the request will return
-  // ERR_IO_PENDING. An OK return from this function or the callback means
-  // that the connection is established; ERR_PROXY_AUTH_REQUESTED means
-  // that the tunnel needs authentication credentials, the socket will be
-  // returned in this case, and must be release back to the pool; or
-  // a standard net error code will be returned.
-  int ConnectInternal() override;
-
-  void ChangePriorityInternal(RequestPriority priority) override;
-
-  void OnConnectComplete(int result);
-
-  int HandleConnectResult(int result);
-
-  std::unique_ptr<HttpProxyClientSocketWrapper> client_socket_;
-
-  std::unique_ptr<HttpResponseInfo> error_response_info_;
-
-  DISALLOW_COPY_AND_ASSIGN(HttpProxyConnectJob);
-};
 
 class NET_EXPORT_PRIVATE HttpProxyClientSocketPool
     : public ClientSocketPool,

net/http/http_proxy_client_socket_wrapper.cc

@@ -40,7 +40,7 @@ HttpProxyClientSocketWrapper::HttpProxyClientSocketWrapper(
     const std::string& group_name,
     RequestPriority priority,
     const SocketTag& socket_tag,
-    ClientSocketPool::RespectLimits respect_limits,
+    bool respect_limits,
     base::TimeDelta connect_timeout_duration,
     base::TimeDelta proxy_negotiation_timeout_duration,
     TransportClientSocketPool* transport_pool,
@@ -143,7 +143,7 @@ HttpProxyClientSocketWrapper::GetAdditionalErrorState() {
 }
 
 void HttpProxyClientSocketWrapper::SetPriority(RequestPriority priority) {
-  if (respect_limits_ == ClientSocketPool::RespectLimits::DISABLED) {
+  if (!respect_limits_) {
     DCHECK_EQ(MAXIMUM_PRIORITY, priority_);
     return;
   }
@@ -503,7 +503,9 @@ int HttpProxyClientSocketWrapper::DoTransportConnect() {
       group_name_,
       TransportClientSocketPool::SocketParams::CreateFromTransportSocketParams(
           transport_params_),
-      priority_, initial_socket_tag_, respect_limits_,
+      priority_, initial_socket_tag_,
+      respect_limits_ ? ClientSocketPool::RespectLimits::ENABLED
+                      : ClientSocketPool::RespectLimits::DISABLED,
       base::Bind(&HttpProxyClientSocketWrapper::OnIOComplete,
                  base::Unretained(this)),
       transport_pool_, net_log_);

net/http/http_proxy_client_socket_wrapper.h

@@ -59,7 +59,7 @@ class NET_EXPORT_PRIVATE HttpProxyClientSocketWrapper
       const std::string& group_name,
       RequestPriority priority,
       const SocketTag& socket_tag,
-      ClientSocketPool::RespectLimits respect_limits,
+      bool respect_limits,
       base::TimeDelta connect_timeout_duration,
       base::TimeDelta proxy_negotiation_timeout_duration,
       TransportClientSocketPool* transport_pool,
@@ -190,7 +190,7 @@ class NET_EXPORT_PRIVATE HttpProxyClientSocketWrapper
   const std::string group_name_;
   RequestPriority priority_;
   const SocketTag initial_socket_tag_;
-  ClientSocketPool::RespectLimits respect_limits_;
+  bool respect_limits_;
   const base::TimeDelta connect_timeout_duration_;
   const base::TimeDelta proxy_negotiation_timeout_duration_;
 

net/http/http_proxy_client_socket_wrapper_unittest.cc

@@ -286,10 +286,10 @@ TEST_P(HttpProxyClientSocketWrapperTest, QuicProxy) {
                           SSLConfig(), privacy_mode_);
   transport_params = nullptr;
 
-  client_socket_wrapper_.reset(new HttpProxyClientSocketWrapper(
+  client_socket_wrapper_ = std::make_unique<HttpProxyClientSocketWrapper>(
       /*group_name=*/std::string(), /*requiest_priority=*/DEFAULT_PRIORITY,
       /*socket_tag=*/SocketTag(),
-      /*respect_limits=*/ClientSocketPool::RespectLimits::ENABLED,
+      /*respect_limits=*/true,
       /*connect_timeout_duration=*/base::TimeDelta::FromHours(1),
       /*proxy_negotiation_timeout_duration=*/base::TimeDelta::FromHours(1),
       /*transport_pool=*/nullptr, /*ssl_pool=*/nullptr,
@@ -297,7 +297,7 @@ TEST_P(HttpProxyClientSocketWrapperTest, QuicProxy) {
       endpoint_host_port_, &http_auth_cache_, http_auth_handler_factory_.get(),
       /*spdy_session_pool=*/nullptr, quic_stream_factory_.get(),
       /*is_trusted_proxy=*/false, /*tunnel=*/true, /*proxy_delegate=*/nullptr,
-      TRAFFIC_ANNOTATION_FOR_TESTS, net_log_));
+      TRAFFIC_ANNOTATION_FOR_TESTS, net_log_);
 
   TestCompletionCallback callback;
   client_socket_wrapper_->Connect(callback.callback());
@@ -344,10 +344,10 @@ TEST_P(HttpProxyClientSocketWrapperTest, QuicProxySocketTag) {
   transport_params = nullptr;
   SocketTag tag(getuid(), 0x87654321);
 
-  client_socket_wrapper_.reset(new HttpProxyClientSocketWrapper(
+  client_socket_wrapper_ = std::make_unique<HttpProxyClientSocketWrapper>(
       /*group_name=*/std::string(), /*requiest_priority=*/DEFAULT_PRIORITY,
       /*socket_tag=*/tag,
-      /*respect_limits=*/ClientSocketPool::RespectLimits::ENABLED,
+      /*respect_limits=*/true,
       /*connect_timeout_duration=*/base::TimeDelta::FromHours(1),
       /*proxy_negotiation_timeout_duration=*/base::TimeDelta::FromHours(1),
       /*transport_pool=*/nullptr, /*ssl_pool=*/nullptr,
@@ -355,7 +355,7 @@ TEST_P(HttpProxyClientSocketWrapperTest, QuicProxySocketTag) {
       endpoint_host_port_, &http_auth_cache_, http_auth_handler_factory_.get(),
       /*spdy_session_pool=*/nullptr, quic_stream_factory_.get(),
       /*is_trusted_proxy=*/false, /*tunnel=*/true, /*proxy_delegate=*/nullptr,
-      TRAFFIC_ANNOTATION_FOR_TESTS, net_log_));
+      TRAFFIC_ANNOTATION_FOR_TESTS, net_log_);
 
   TestCompletionCallback callback;
   client_socket_wrapper_->Connect(callback.callback());

net/http/http_proxy_connect_job.h

+// Copyright 2019 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.
+
+#ifndef NET_HTTP_HTTP_PROXY_CONNECT_JOB_H_
+#define NET_HTTP_HTTP_PROXY_CONNECT_JOB_H_
+
+#include <memory>
+#include <string>
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/time/time.h"
+#include "net/base/host_port_pair.h"
+#include "net/base/net_export.h"
+#include "net/http/http_auth.h"
+#include "net/http/http_response_info.h"
+#include "net/socket/connect_job.h"
+#include "net/socket/ssl_client_socket.h"
+#include "net/spdy/spdy_session.h"
+#include "net/traffic_annotation/network_traffic_annotation.h"
+
+namespace net {
+
+class HttpAuthCache;
+class HttpAuthHandlerFactory;
+class HttpProxyClientSocketWrapper;
+class NetLog;
+class NetworkQualityEstimator;
+class ProxyDelegate;
+class SpdySessionPool;
+class SSLSocketParams;
+class TransportClientSocketPool;
+class TransportSocketParams;
+class QuicStreamFactory;
+
+// HttpProxySocketParams only needs the socket params for one of the proxy
+// types.  The other param must be NULL.  When using an HTTP proxy,
+// |transport_params| must be set.  When using an HTTPS proxy or QUIC proxy,
+// |ssl_params| must be set. Also, if using a QUIC proxy, |quic_version| must
+// not be quic::QUIC_VERSION_UNSUPPORTED.
+class NET_EXPORT_PRIVATE HttpProxySocketParams
+    : public base::RefCounted<HttpProxySocketParams> {
+ public:
+  HttpProxySocketParams(
+      const scoped_refptr<TransportSocketParams>& transport_params,
+      const scoped_refptr<SSLSocketParams>& ssl_params,
+      quic::QuicTransportVersion quic_version,
+      const std::string& user_agent,
+      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);
+
+  const scoped_refptr<TransportSocketParams>& transport_params() const {
+    return transport_params_;
+  }
+  const scoped_refptr<SSLSocketParams>& ssl_params() const {
+    return ssl_params_;
+  }
+  quic::QuicTransportVersion quic_version() const { return quic_version_; }
+  const std::string& user_agent() const { return user_agent_; }
+  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 {
+    return traffic_annotation_;
+  }
+
+ private:
+  friend class base::RefCounted<HttpProxySocketParams>;
+  ~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 std::string user_agent_;
+  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_;
+
+  DISALLOW_COPY_AND_ASSIGN(HttpProxySocketParams);
+};
+
+// HttpProxyConnectJob optionally establishes a tunnel through the proxy
+// server after connecting the underlying transport socket.
+class NET_EXPORT_PRIVATE HttpProxyConnectJob : public ConnectJob {
+ public:
+  HttpProxyConnectJob(const std::string& group_name,
+                      RequestPriority priority,
+                      const SocketTag& socket_tag,
+                      bool respect_limits,
+                      const scoped_refptr<HttpProxySocketParams>& params,
+                      ProxyDelegate* proxy_delegate,
+                      TransportClientSocketPool* transport_pool,
+                      TransportClientSocketPool* ssl_pool,
+                      NetworkQualityEstimator* network_quality_estimator,
+                      Delegate* delegate,
+                      NetLog* net_log);
+  ~HttpProxyConnectJob() override;
+
+  // ConnectJob methods.
+  LoadState GetLoadState() const override;
+  bool HasEstablishedConnection() const override;
+
+  void GetAdditionalErrorState(ClientSocketHandle* handle) override;
+
+  // Returns the connection timeout that will be used by a HttpProxyConnectJob
+  // created with the specified parameters, given current network conditions.
+  static base::TimeDelta ConnectionTimeout(
+      const HttpProxySocketParams& params,
+      const NetworkQualityEstimator* network_quality_estimator);
+
+  // Updates the field trial parameters used in calculating timeouts.
+  static void UpdateFieldTrialParametersForTesting();
+
+ private:
+  // Begins the tcp connection and the optional Http proxy tunnel.  If the
+  // request is not immediately serviceable (likely), the request will return
+  // ERR_IO_PENDING. An OK return from this function or the callback means
+  // that the connection is established; ERR_PROXY_AUTH_REQUESTED means
+  // that the tunnel needs authentication credentials, the socket will be
+  // returned in this case, and must be released back to the pool; or
+  // a standard net error code will be returned.
+  int ConnectInternal() override;
+
+  void ChangePriorityInternal(RequestPriority priority) override;
+
+  void OnConnectComplete(int result);
+
+  int HandleConnectResult(int result);
+
+  std::unique_ptr<HttpProxyClientSocketWrapper> client_socket_;
+
+  std::unique_ptr<HttpResponseInfo> error_response_info_;
+
+  DISALLOW_COPY_AND_ASSIGN(HttpProxyConnectJob);
+};
+
+}  // namespace net
+
+#endif  // NET_HTTP_HTTP_PROXY_CONNECT_JOB_H_

net/socket/ssl_connect_job.cc

@@ -252,7 +252,8 @@ int SSLConnectJob::DoTransportConnect() {
 }
 
 int SSLConnectJob::DoTransportConnectComplete(int result) {
-  // TODO(mmenke): Implement a better API to get this information.
+  // TODO(https://crbug.com/927101): Implement a better API to get this
+  // information.
   ClientSocketHandle bogus_handle;
   nested_connect_job_->GetAdditionalErrorState(&bogus_handle);
   connection_attempts_.insert(connection_attempts_.end(),
@@ -350,8 +351,8 @@ int SSLConnectJob::DoSSLConnect() {
 
   // If privacy mode is enabled and the session shard is non-empty, prefix the
   // SSL session shard with "pm/". Otherwise, prefix with "nopm/".
-  // TODO(mmenke): Consider moving this up to the socket pool layer, after
-  // giving socket pools knowledge of privacy mode.
+  // TODO(https://crbug.com/927084): Consider moving this up to the socket pool
+  // layer, after giving socket pools knowledge of privacy mode.
   SSLClientSocketContext context_with_privacy_mode(
       ssl_client_socket_context().cert_verifier,
       ssl_client_socket_context().channel_id_service,