Socket Pools Refactor 23: Let SpdySession take a StreamSocket.

This CL allows a SpdySession to be created by passing a StreamSocket
to its constructor directly, in addition to keeping the previous
constructor, which takes a ClientSocketHandle. The new constructor
is intended for use where there's no socket pool below the session.

In particular, for H2 proxies, the SpdySessions are created below
the highest socket pool layer, at the ConnectJob layer. Once the socket
pools are flight, this will become the only socket pool layer, so
we'll only be dealing with a raw StreamSockets, rather than
ClientSocketHandles.

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

Change-Id: Ibe722bad9814879504757c03e8ad3c135d123971
Reviewed-on: https://chromium-review.googlesource.com/c/1453107
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629882}

net/http/http_proxy_client_socket.cc

@@ -472,8 +472,8 @@ int HttpProxyClientSocket::DoReadHeadersComplete(int result) {
       if (!is_https_proxy_ || !SanitizeProxyRedirect(&response_))
         return ERR_TUNNEL_CONNECTION_FAILED;
 
-      transport_.reset();
       http_stream_parser_.reset();
+      transport_.reset();
       return ERR_HTTPS_PROXY_TUNNEL_RESPONSE;
 
     case 407:  // Proxy Authentication Required

net/http/http_proxy_client_socket_wrapper.cc

@@ -670,7 +670,7 @@ int HttpProxyClientSocketWrapper::DoSpdyProxyCreateStream() {
     }
   } else {
     // Create a session direct to the proxy itself
-    spdy_session = spdy_session_pool_->CreateAvailableSessionFromSocket(
+    spdy_session = spdy_session_pool_->CreateAvailableSessionFromSocketHandle(
         key, is_trusted_proxy_, std::move(transport_socket_handle_), net_log_);
     DCHECK(spdy_session);
   }

net/http/http_stream_factory_job.cc

@@ -1279,7 +1279,7 @@ int HttpStreamFactory::Job::DoCreateStream() {
       !spdy_session_direct_ && proxy_info_.proxy_server().is_trusted_proxy();
 
   base::WeakPtr<SpdySession> spdy_session =
-      session_->spdy_session_pool()->CreateAvailableSessionFromSocket(
+      session_->spdy_session_pool()->CreateAvailableSessionFromSocketHandle(
           spdy_session_key_, is_trusted_proxy, std::move(connection_),
           net_log_);
 

net/socket/connect_job_test_util.cc

@@ -4,6 +4,8 @@
 
 #include "net/socket/connect_job_test_util.h"
 
+#include <utility>
+
 #include "base/logging.h"
 #include "base/run_loop.h"
 #include "net/socket/stream_socket.h"
@@ -49,4 +51,8 @@ void TestConnectJobDelegate::StartJobExpectingResult(ConnectJob* connect_job,
   }
 }
 
+std::unique_ptr<StreamSocket> TestConnectJobDelegate::ReleaseSocket() {
+  return std::move(socket_);
+}
+
 }  // namespace net

net/socket/connect_job_test_util.h

@@ -46,6 +46,8 @@ class TestConnectJobDelegate : public ConnectJob::Delegate {
 
   StreamSocket* socket() { return socket_.get(); }
 
+  std::unique_ptr<StreamSocket> ReleaseSocket();
+
  private:
   const SocketExpected socket_expected_;
   bool has_result_ = false;

net/spdy/spdy_http_stream.h

@@ -14,6 +14,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "net/base/completion_once_callback.h"
+#include "net/base/load_timing_info.h"
 #include "net/base/net_export.h"
 #include "net/log/net_log_source.h"
 #include "net/spdy/multiplexed_http_stream.h"

net/spdy/spdy_proxy_client_socket_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
 #include "net/base/address_list.h"
+#include "net/base/load_timing_info.h"
 #include "net/base/test_completion_callback.h"
 #include "net/base/winsock_init.h"
 #include "net/dns/mock_host_resolver.h"
@@ -24,9 +25,15 @@
 #include "net/log/test_net_log_entry.h"
 #include "net/log/test_net_log_util.h"
 #include "net/socket/client_socket_factory.h"
+#include "net/socket/connect_job_test_util.h"
 #include "net/socket/socket_tag.h"
 #include "net/socket/socket_test_util.h"
+#include "net/socket/socks_connect_job.h"
+#include "net/socket/ssl_client_socket.h"
+#include "net/socket/ssl_connect_job.h"
+#include "net/socket/stream_socket.h"
 #include "net/socket/tcp_client_socket.h"
+#include "net/socket/transport_connect_job.h"
 #include "net/spdy/buffered_spdy_framer.h"
 #include "net/spdy/spdy_http_utils.h"
 #include "net/spdy/spdy_session_pool.h"
@@ -46,6 +53,8 @@ using net::test::IsOk;
 
 //-----------------------------------------------------------------------------
 
+namespace net {
+
 namespace {
 
 static const char kRequestUrl[] = "https://www.google.com/";
@@ -72,9 +81,57 @@ static const int kLen333 = kLen3 + kLen3 + kLen3;
 
 static const char kRedirectUrl[] = "https://example.com/";
 
-}  // anonymous namespace
+// Creates a SpdySession with a StreamSocket, instead of a ClientSocketHandle.
+base::WeakPtr<SpdySession> CreateSpdyProxySession(
+    HttpNetworkSession* http_session,
+    SpdySessionDependencies* session_deps,
+    const SpdySessionKey& key) {
+  EXPECT_FALSE(http_session->spdy_session_pool()->FindAvailableSession(
+      key, true /* enable_ip_based_pooling */, false /* is_websocket */,
+      NetLogWithSource()));
+
+  auto transport_params = base::MakeRefCounted<TransportSocketParams>(
+      key.host_port_pair(), false /* disable_resolver_cache */,
+      OnHostResolutionCallback());
+
+  SSLConfig ssl_config;
+  auto ssl_params = base::MakeRefCounted<SSLSocketParams>(
+      transport_params, nullptr, nullptr, key.host_port_pair(), ssl_config,
+      key.privacy_mode());
+  TestConnectJobDelegate connect_job_delegate;
+  SSLConnectJob connect_job(
+      MEDIUM,
+      CommonConnectJobParams(
+          "group_name", SocketTag(), true /* respect_limits */,
+          session_deps->socket_factory.get(), session_deps->host_resolver.get(),
+          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_arg */,
+                                 std::string() /* ssl_session_cache_shard_arg */
+                                 ),
+          nullptr /* socket_performance_watcher_factory */,
+          nullptr /* network_quality_estimator */, session_deps->net_log,
+          nullptr /* websocket_endpoint_lock_manager */),
+      ssl_params, nullptr /* socks_pool */, nullptr /* http_proxy_pool */,
+      &connect_job_delegate);
+  connect_job_delegate.StartJobExpectingResult(&connect_job, OK,
+                                               false /* expect_sync_result */);
+
+  base::WeakPtr<SpdySession> spdy_session =
+      http_session->spdy_session_pool()->CreateAvailableSessionFromSocket(
+          key, false /* is_trusted_proxy */,
+          connect_job_delegate.ReleaseSocket(), LoadTimingInfo::ConnectTiming(),
+          NetLogWithSource());
+  // Failure is reported asynchronously.
+  EXPECT_TRUE(spdy_session);
+  EXPECT_TRUE(HasSpdySession(http_session->spdy_session_pool(), key));
+  return spdy_session;
+}
 
-namespace net {
+}  // namespace
 
 class SpdyProxyClientSocketTest : public PlatformTest,
                                   public WithScopedTaskEnvironment,
@@ -206,8 +263,9 @@ void SpdyProxyClientSocketTest::Initialize(base::span<const MockRead> reads,
   session_ = SpdySessionDependencies::SpdyCreateSession(&session_deps_);
 
   // Creates the SPDY session and stream.
-  spdy_session_ = CreateSpdySession(session_.get(), endpoint_spdy_session_key_,
-                                    NetLogWithSource());
+  spdy_session_ = CreateSpdyProxySession(session_.get(), &session_deps_,
+                                         endpoint_spdy_session_key_);
+
   base::WeakPtr<SpdyStream> spdy_stream(
       CreateStreamSynchronously(
           SPDY_BIDIRECTIONAL_STREAM, spdy_session_, url_, LOWEST,
@@ -413,9 +471,8 @@ SpdyProxyClientSocketTest::ConstructConnectErrorReplyFrame() {
 spdy::SpdySerializedFrame SpdyProxyClientSocketTest::ConstructBodyFrame(
     const char* data,
     int length) {
-  return spdy_util_.ConstructSpdyDataFrame(kStreamId,
-                                           base::StringPiece(data, length),
-                                           /*fin=*/false);
+  return spdy_util_.ConstructSpdyDataFrame(
+      kStreamId, base::StringPiece(data, length), false /* fin */);
 }
 
 // ----------- Connect

net/spdy/spdy_session.h

@@ -27,11 +27,11 @@
 #include "net/base/host_port_pair.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_states.h"
+#include "net/base/load_timing_info.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_export.h"
 #include "net/base/request_priority.h"
 #include "net/log/net_log_source.h"
-#include "net/socket/client_socket_handle.h"
 #include "net/socket/client_socket_pool.h"
 #include "net/socket/next_proto.h"
 #include "net/socket/ssl_client_socket.h"
@@ -86,7 +86,6 @@ const int kYieldAfterDurationMilliseconds = 20;
 const spdy::SpdyStreamId kFirstStreamId = 1;
 const spdy::SpdyStreamId kLastStreamId = 0x7fffffff;
 
-struct LoadTimingInfo;
 class NetLog;
 class NetworkQualityEstimator;
 class SpdyStream;
@@ -348,10 +347,19 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
   // |pool| is the SpdySessionPool that owns us.  Its lifetime must
   // strictly be greater than |this|.
   //
-  // The session begins reading from |connection| on a subsequent event loop
-  // iteration, so the SpdySession may close immediately afterwards if the first
-  // read of |connection| fails.
-  void InitializeWithSocket(std::unique_ptr<ClientSocketHandle> connection,
+  // The session begins reading from |client_socket_handle| on a subsequent
+  // event loop iteration, so the SpdySession may close immediately afterwards
+  // if the first read of |client_socket_handle| fails.
+  void InitializeWithSocketHandle(
+      std::unique_ptr<ClientSocketHandle> client_socket_handle,
+      SpdySessionPool* pool);
+
+  // Just like InitializeWithSocketHandle(), but for use when the session is not
+  // on top of a socket pool, but instead directly on top of a socket, which the
+  // session has sole ownership of, and is responsible for deleting directly
+  // itself.
+  void InitializeWithSocket(std::unique_ptr<StreamSocket> stream_socket,
+                            const LoadTimingInfo::ConnectTiming& connect_timing,
                             SpdySessionPool* pool);
 
   // Check to see if this SPDY session can support an additional domain.
@@ -476,9 +484,7 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
 
   // Returns true if the underlying transport socket ever had any reads or
   // writes.
-  bool WasEverUsed() const {
-    return connection_->socket()->WasEverUsed();
-  }
+  bool WasEverUsed() const { return socket_->WasEverUsed(); }
 
   // Returns the load timing information from the perspective of the given
   // stream.  If it's not the first stream, the connection is considered reused
@@ -595,6 +601,9 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
     WRITE_STATE_DO_WRITE_COMPLETE,
   };
 
+  // Has the shared logic for the other two Initialize methods that call it.
+  void InitializeInternal(SpdySessionPool* pool);
+
   // Called by SpdyStreamRequest to start a request to create a
   // stream. If OK is returned, then |stream| will be filled in with a
   // valid stream. If ERR_IO_PENDING is returned, then
@@ -937,8 +946,18 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
   TransportSecurityState* transport_security_state_;
   SSLConfigService* ssl_config_service_;
 
-  // The socket handle for this session.
-  std::unique_ptr<ClientSocketHandle> connection_;
+  // One of these two owns the socket for this session, which is stored in
+  // |socket_|. If |client_socket_handle_| is non-null, this session is on top
+  // of a socket in a socket pool. If |owned_stream_socket_| is non-null, this
+  // session is directly on top of a socket, which is not in a socket pool.
+  std::unique_ptr<ClientSocketHandle> client_socket_handle_;
+  std::unique_ptr<StreamSocket> owned_stream_socket_;
+
+  // This is non-null only if |owned_stream_socket_| is non-null.
+  std::unique_ptr<LoadTimingInfo::ConnectTiming> connect_timing_;
+
+  // The socket for this session.
+  StreamSocket* socket_;
 
   // The read buffer used to read data from the socket.
   // Non-null if there is a Read() pending.

net/spdy/spdy_session_pool.cc

@@ -98,48 +98,38 @@ SpdySessionPool::~SpdySessionPool() {
   CertDatabase::GetInstance()->RemoveObserver(this);
 }
 
-base::WeakPtr<SpdySession> SpdySessionPool::CreateAvailableSessionFromSocket(
+base::WeakPtr<SpdySession>
+SpdySessionPool::CreateAvailableSessionFromSocketHandle(
     const SpdySessionKey& key,
     bool is_trusted_proxy,
-    std::unique_ptr<ClientSocketHandle> connection,
+    std::unique_ptr<ClientSocketHandle> client_socket_handle,
     const NetLogWithSource& net_log) {
   TRACE_EVENT0(NetTracingCategory(),
-               "SpdySessionPool::CreateAvailableSessionFromSocket");
-
-  UMA_HISTOGRAM_ENUMERATION(
-      "Net.SpdySessionGet", IMPORTED_FROM_SOCKET, SPDY_SESSION_GET_MAX);
-
-  auto new_session = std::make_unique<SpdySession>(
-      key, http_server_properties_, transport_security_state_,
-      ssl_config_service_, quic_supported_versions_,
-      enable_sending_initial_data_, enable_ping_based_connection_checking_,
-      support_ietf_format_quic_altsvc_, is_trusted_proxy,
-      session_max_recv_window_size_, initial_settings_, greased_http2_frame_,
-      time_func_, push_delegate_, network_quality_estimator_,
-      net_log.net_log());
+               "SpdySessionPool::CreateAvailableSessionFromSocketHandle");
 
-  new_session->InitializeWithSocket(std::move(connection), this);
+  std::unique_ptr<SpdySession> new_session =
+      CreateSession(key, is_trusted_proxy, net_log.net_log());
+  new_session->InitializeWithSocketHandle(std::move(client_socket_handle),
+                                          this);
+  return InsertSession(key, std::move(new_session), net_log);
+}
 
-  base::WeakPtr<SpdySession> available_session = new_session->GetWeakPtr();
-  sessions_.insert(new_session.release());
-  MapKeyToAvailableSession(key, available_session);
+base::WeakPtr<SpdySession> SpdySessionPool::CreateAvailableSessionFromSocket(
+    const SpdySessionKey& key,
+    bool is_trusted_proxy,
+    std::unique_ptr<StreamSocket> socket_stream,
+    const LoadTimingInfo::ConnectTiming& connect_timing,
+    const NetLogWithSource& net_log) {
+  TRACE_EVENT0(NetTracingCategory(),
+               "SpdySessionPool::CreateAvailableSessionFromSocket");
 
-  net_log.AddEvent(
-      NetLogEventType::HTTP2_SESSION_POOL_IMPORTED_SESSION_FROM_SOCKET,
-      available_session->net_log().source().ToEventParametersCallback());
+  std::unique_ptr<SpdySession> new_session =
+      CreateSession(key, is_trusted_proxy, net_log.net_log());
 
-  // Look up the IP address for this session so that we can match
-  // future sessions (potentially to different domains) which can
-  // potentially be pooled with this one. Because GetPeerAddress()
-  // reports the proxy's address instead of the origin server, check
-  // to see if this is a direct connection.
-  if (key.proxy_server().is_direct()) {
-    IPEndPoint address;
-    if (available_session->GetPeerAddress(&address) == OK)
-      aliases_.insert(AliasMap::value_type(address, key));
-  }
+  new_session->InitializeWithSocket(std::move(socket_stream), connect_timing,
+                                    this);
 
-  return available_session;
+  return InsertSession(key, std::move(new_session), net_log);
 }
 
 base::WeakPtr<SpdySession> SpdySessionPool::FindAvailableSession(
@@ -602,4 +592,46 @@ void SpdySessionPool::CloseCurrentSessionsHelper(Error error,
   }
 }
 
+std::unique_ptr<SpdySession> SpdySessionPool::CreateSession(
+    const SpdySessionKey& key,
+    bool is_trusted_proxy,
+    NetLog* net_log) {
+  UMA_HISTOGRAM_ENUMERATION("Net.SpdySessionGet", IMPORTED_FROM_SOCKET,
+                            SPDY_SESSION_GET_MAX);
+
+  return std::make_unique<SpdySession>(
+      key, http_server_properties_, transport_security_state_,
+      ssl_config_service_, quic_supported_versions_,
+      enable_sending_initial_data_, enable_ping_based_connection_checking_,
+      support_ietf_format_quic_altsvc_, is_trusted_proxy,
+      session_max_recv_window_size_, initial_settings_, greased_http2_frame_,
+      time_func_, push_delegate_, network_quality_estimator_, net_log);
+}
+
+base::WeakPtr<SpdySession> SpdySessionPool::InsertSession(
+    const SpdySessionKey& key,
+    std::unique_ptr<SpdySession> new_session,
+    const NetLogWithSource& source_net_log) {
+  base::WeakPtr<SpdySession> available_session = new_session->GetWeakPtr();
+  sessions_.insert(new_session.release());
+  MapKeyToAvailableSession(key, available_session);
+
+  source_net_log.AddEvent(
+      NetLogEventType::HTTP2_SESSION_POOL_IMPORTED_SESSION_FROM_SOCKET,
+      available_session->net_log().source().ToEventParametersCallback());
+
+  // Look up the IP address for this session so that we can match
+  // future sessions (potentially to different domains) which can
+  // potentially be pooled with this one. Because GetPeerAddress()
+  // reports the proxy's address instead of the origin server, check
+  // to see if this is a direct connection.
+  if (key.proxy_server().is_direct()) {
+    IPEndPoint address;
+    if (available_session->GetPeerAddress(&address) == OK)
+      aliases_.insert(AliasMap::value_type(address, key));
+  }
+
+  return available_session;
+}
+
 }  // namespace net

net/spdy/spdy_session_pool.h

@@ -20,6 +20,7 @@
 #include "base/optional.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/ip_endpoint.h"
+#include "net/base/load_timing_info.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_export.h"
 #include "net/base/network_change_notifier.h"
@@ -48,6 +49,7 @@ class HttpStreamRequest;
 class NetLogWithSource;
 class NetworkQualityEstimator;
 class SpdySession;
+class StreamSocket;
 class TransportSecurityState;
 
 // This is a very simple pool for open SpdySessions.
@@ -95,12 +97,28 @@ class NET_EXPORT SpdySessionPool
   // not already be a session for the given key.
   //
   // Returns the new SpdySession. Note that the SpdySession begins reading from
-  // |connection| on a subsequent event loop iteration, so it may be closed
-  // immediately afterwards if the first read of |connection| fails.
+  // |client_socket_handle| on a subsequent event loop iteration, so it may be
+  // closed immediately afterwards if the first read of |client_socket_handle|
+  // fails.
+  base::WeakPtr<SpdySession> CreateAvailableSessionFromSocketHandle(
+      const SpdySessionKey& key,
+      bool is_trusted_proxy,
+      std::unique_ptr<ClientSocketHandle> client_socket_handle,
+      const NetLogWithSource& net_log);
+
+  // Just like the above method, except it takes a SocketStream instead of a
+  // ClientSocketHandle, and separate connect timing information. When this
+  // constructor is used, there is no socket pool beneath the SpdySession.
+  // Instead, the session takes exclusive ownership of the underting socket, and
+  // destroying the session will directly destroy the socket, as opposed to
+  // disconnected it and then returning it to the socket pool. This is intended
+  // for use with H2 proxies, which are layered beneath the socket pools and
+  // can have sockets above them for tunnels, which are put in a socket pool.
   base::WeakPtr<SpdySession> CreateAvailableSessionFromSocket(
       const SpdySessionKey& key,
       bool is_trusted_proxy,
-      std::unique_ptr<ClientSocketHandle> connection,
+      std::unique_ptr<StreamSocket> socket_stream,
+      const LoadTimingInfo::ConnectTiming& connect_timing,
       const NetLogWithSource& net_log);
 
   // If there is an available session for |key|, return it.
@@ -254,6 +272,18 @@ class NET_EXPORT SpdySessionPool
                                   const std::string& description,
                                   bool idle_only);
 
+  // Creates a new session. The session must be initialized before
+  // InsertSession() is invoked.
+  std::unique_ptr<SpdySession> CreateSession(const SpdySessionKey& key,
+                                             bool is_trusted_proxy,
+                                             NetLog* net_log);
+  // Adds a new session previously created with CreateSession to the pool.
+  // |source_net_log| is the NetLog for the object that created the session.
+  base::WeakPtr<SpdySession> InsertSession(
+      const SpdySessionKey& key,
+      std::unique_ptr<SpdySession> new_session,
+      const NetLogWithSource& source_net_log);
+
   HttpServerProperties* http_server_properties_;
 
   TransportSecurityState* transport_security_state_;

net/spdy/spdy_stream.cc

@@ -20,6 +20,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/memory_usage_estimator.h"
 #include "base/values.h"
+#include "net/base/load_timing_info.h"
 #include "net/log/net_log.h"
 #include "net/log/net_log_capture_mode.h"
 #include "net/log/net_log_event_type.h"

net/spdy/spdy_test_util_common.cc

@@ -509,7 +509,7 @@ base::WeakPtr<SpdySession> CreateSpdySessionHelper(
   EXPECT_THAT(rv, IsOk());
 
   base::WeakPtr<SpdySession> spdy_session =
-      http_session->spdy_session_pool()->CreateAvailableSessionFromSocket(
+      http_session->spdy_session_pool()->CreateAvailableSessionFromSocketHandle(
           key, is_trusted_proxy, std::move(connection), net_log);
   // Failure is reported asynchronously.
   EXPECT_TRUE(spdy_session);
@@ -612,7 +612,7 @@ base::WeakPtr<SpdySession> CreateFakeSpdySessionHelper(
   handle->SetSocket(std::make_unique<FakeSpdySessionClientSocket>(
       expected_status == OK ? ERR_IO_PENDING : expected_status));
   base::WeakPtr<SpdySession> spdy_session =
-      pool->CreateAvailableSessionFromSocket(
+      pool->CreateAvailableSessionFromSocketHandle(
           key,
           /*is_trusted_proxy=*/false, std::move(handle), NetLogWithSource());
   // Failure is reported asynchronously.