Implement a 15 connection per proxy server limit.

Previously, we had a limit of 6 connections per proxy server, which was horrifically low.
Connections to all endpoint hosts remains at 6.
Basically, we have a map of socket pools, keyed by the proxy server.
Each of these proxy socket pools has a socket limit of 15 and a connection limit per host of 6.
The main TCP socket pool (non-proxied) has the standard 256 socket limit, and connection limit per host of 6.
There are two maps currently, one for HTTP proxies and one for SOCKS proxies.
Note that SSL tunnels over HTTP CONNECTs are still located within the standard http proxy socket pools.
We depend on the fact that our code never returns a non-SSL connected socket to the pool for a |connection_group|
that is to a HTTPS endpoint.  A newly connected socket from the pool will only have the TCP connection done.  An
idle socket will have both the TCP and the HTTP CONNECT and the SSL handshake done for it.

TODO(willchan): Remove the extra constructor overload for the old deprecated parameters.  Switch to using HostPortPair everywhere.
TODO(willchan): Finish SSL socket pools.
TODO(willchan): Rip out the DoInitConnection() code so it can be shared by other caller (TCP pre-warming!).
TODO(willchan): Flush out the socket pool maps when the proxy configuration changes.
TODO(willchan): Fix up global socket limits.  They're slightly broken in this case, since each pool instance has its own "global" limit, so obviously different pools don't share the same "global" limit.  This is such a minor deal since the global limits are so small for proxy servers compared to the system global limits (256 vs 15), that it doesn't have a big effect.
TODO(willchan): Drink moar b33r.

BUG=12066

Review URL: http://codereview.chromium.org/1808001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@45896 0039d316-1c4b-4281-b951-d872f2087c98

13 files changed, 396 insertions, 171 deletions

diff --git a/net/base/host_port_pair.h b/net/base/host_port_pair.h
index 547e898..35d244e 100644
--- a/net/base/host_port_pair.h
+++ b/net/base/host_port_pair.h
@@ -15,11 +15,12 @@ struct HostPortPair {
   // If |in_host| represents an IPv6 address, it should not bracket the address.
   HostPortPair(const std::string& in_host, uint16 in_port);
 
+  // TODO(willchan): Define a functor instead.
   // Comparator function so this can be placed in a std::map.
   bool operator<(const HostPortPair& other) const {
-    if (host != other.host)
-      return host < other.host;
-    return port < other.port;
+    if (port != other.port)
+      return port < other.port;
+    return host < other.host;
   }
 
   // ToString() will convert the HostPortPair to "host:port".  If |host| is an
diff --git a/net/http/http_network_session.cc b/net/http/http_network_session.cc
index 5f19ce2..2d05a55 100644
--- a/net/http/http_network_session.cc
+++ b/net/http/http_network_session.cc
@@ -4,22 +4,36 @@
 
 #include "net/http/http_network_session.h"
 
+#include <utility>
+
 #include "base/logging.h"
+#include "base/stl_util-inl.h"
+#include "base/string_util.h"
 #include "net/http/http_auth_handler_factory.h"
 #include "net/http/url_security_manager.h"
 #include "net/spdy/spdy_session_pool.h"
 
 namespace net {
 
-// static
-int HttpNetworkSession::max_sockets_ = 256;
+namespace {
 
-// static
-int HttpNetworkSession::max_sockets_per_group_ = 6;
+// Total limit of sockets.
+int g_max_sockets = 256;
 
-// static
-uint16 HttpNetworkSession::g_fixed_http_port = 0;
-uint16 HttpNetworkSession::g_fixed_https_port = 0;
+// Default to allow up to 6 connections per host. Experiment and tuning may
+// try other values (greater than 0).  Too large may cause many problems, such
+// as home routers blocking the connections!?!?  See http://crbug.com/12066.
+int g_max_sockets_per_group = 6;
+
+// The max number of sockets to allow per proxy server.  This applies both to
+// http and SOCKS proxies.  See http://crbug.com/12066 for details about proxy
+// server connection limits.
+int g_max_sockets_per_proxy_server = 15;
+
+uint16 g_fixed_http_port = 0;
+uint16 g_fixed_https_port = 0;
+
+}  // namespace
 
 // TODO(vandebo) when we've completely converted to pools, the base TCP
 // pool name should get changed to TCP instead of Transport.
@@ -33,15 +47,8 @@ HttpNetworkSession::HttpNetworkSession(
     HttpAuthHandlerFactory* http_auth_handler_factory)
     : network_change_notifier_(network_change_notifier),
       tcp_socket_pool_(new TCPClientSocketPool(
-          max_sockets_, max_sockets_per_group_, "Transport",
+          g_max_sockets, g_max_sockets_per_group, "Transport",
           host_resolver, client_socket_factory, network_change_notifier_)),
-      socks_socket_pool_(new SOCKSClientSocketPool(
-          max_sockets_, max_sockets_per_group_, "SOCKS", host_resolver,
-          new TCPClientSocketPool(max_sockets_, max_sockets_per_group_,
-                                  "TCPForSOCKS", host_resolver,
-                                  client_socket_factory,
-                                  network_change_notifier_),
-          network_change_notifier_)),
       socket_factory_(client_socket_factory),
       host_resolver_(host_resolver),
       proxy_service_(proxy_service),
@@ -55,19 +62,82 @@ HttpNetworkSession::HttpNetworkSession(
 HttpNetworkSession::~HttpNetworkSession() {
 }
 
+const scoped_refptr<TCPClientSocketPool>&
+HttpNetworkSession::GetSocketPoolForHTTPProxy(const HostPortPair& http_proxy) {
+  HTTPProxySocketPoolMap::const_iterator it =
+      http_proxy_socket_pool_.find(http_proxy);
+  if (it != http_proxy_socket_pool_.end())
+    return it->second;
+
+  std::pair<HTTPProxySocketPoolMap::iterator, bool> ret =
+      http_proxy_socket_pool_.insert(
+          std::make_pair(
+              http_proxy,
+              new TCPClientSocketPool(
+                  g_max_sockets_per_proxy_server, g_max_sockets_per_group,
+                  "HTTPProxy", host_resolver_, socket_factory_,
+                  network_change_notifier_)));
+
+  return ret.first->second;
+}
+
+const scoped_refptr<SOCKSClientSocketPool>&
+HttpNetworkSession::GetSocketPoolForSOCKSProxy(
+    const HostPortPair& socks_proxy) {
+  SOCKSSocketPoolMap::const_iterator it = socks_socket_pool_.find(socks_proxy);
+  if (it != socks_socket_pool_.end())
+    return it->second;
+
+  std::pair<SOCKSSocketPoolMap::iterator, bool> ret =
+      socks_socket_pool_.insert(
+          std::make_pair(
+              socks_proxy,
+              new SOCKSClientSocketPool(
+                  g_max_sockets_per_proxy_server, g_max_sockets_per_group,
+                  "SOCKS", host_resolver_,
+                  new TCPClientSocketPool(g_max_sockets_per_proxy_server,
+                                          g_max_sockets_per_group,
+                                          "TCPForSOCKS", host_resolver_,
+                                          socket_factory_,
+                                          network_change_notifier_),
+                  network_change_notifier_)));
+
+  return ret.first->second;
+}
+
 // static
 void HttpNetworkSession::set_max_sockets_per_group(int socket_count) {
-  DCHECK(0 < socket_count);
+  DCHECK_LT(0, socket_count);
   // The following is a sanity check... but we should NEVER be near this value.
-  DCHECK(100 > socket_count);
-  max_sockets_per_group_ = socket_count;
+  DCHECK_GT(100, socket_count);
+  g_max_sockets_per_group = socket_count;
+}
+
+// static
+uint16 HttpNetworkSession::fixed_http_port() {
+  return g_fixed_http_port;
+}
+
+// static
+void HttpNetworkSession::set_fixed_http_port(uint16 port) {
+  g_fixed_http_port = port;
+}
+
+// static
+uint16 HttpNetworkSession::fixed_https_port() {
+  return g_fixed_https_port;
+}
+
+// static
+void HttpNetworkSession::set_fixed_https_port(uint16 port) {
+  g_fixed_https_port = port;
 }
 
 // TODO(vandebo) when we've completely converted to pools, the base TCP
 // pool name should get changed to TCP instead of Transport.
 void HttpNetworkSession::ReplaceTCPSocketPool() {
-  tcp_socket_pool_ = new TCPClientSocketPool(max_sockets_,
-                                             max_sockets_per_group_,
+  tcp_socket_pool_ = new TCPClientSocketPool(g_max_sockets,
+                                             g_max_sockets_per_group,
                                              "Transport",
                                              host_resolver_,
                                              socket_factory_,
diff --git a/net/http/http_network_session.h b/net/http/http_network_session.h
index 8f85ce7..9674b21 100644
--- a/net/http/http_network_session.h
+++ b/net/http/http_network_session.h
@@ -5,8 +5,10 @@
 #ifndef NET_HTTP_HTTP_NETWORK_SESSION_H_
 #define NET_HTTP_HTTP_NETWORK_SESSION_H_
 
+#include <map>
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
+#include "net/base/host_port_pair.h"
 #include "net/base/host_resolver.h"
 #include "net/base/ssl_client_auth_cache.h"
 #include "net/base/ssl_config_service.h"
@@ -21,8 +23,9 @@ namespace net {
 
 class ClientSocketFactory;
 class HttpAuthHandlerFactory;
-class SpdySessionPool;
+class HttpNetworkSessionPeer;
 class NetworkChangeNotifier;
+class SpdySessionPool;
 
 // This class holds session objects used by HttpNetworkTransaction objects.
 class HttpNetworkSession : public base::RefCounted<HttpNetworkSession> {
@@ -60,9 +63,13 @@ class HttpNetworkSession : public base::RefCounted<HttpNetworkSession> {
   const scoped_refptr<TCPClientSocketPool>& tcp_socket_pool() {
     return tcp_socket_pool_;
   }
-  const scoped_refptr<SOCKSClientSocketPool>& socks_socket_pool() {
-    return socks_socket_pool_;
-  }
+
+  const scoped_refptr<SOCKSClientSocketPool>& GetSocketPoolForSOCKSProxy(
+      const HostPortPair& socks_proxy);
+
+  const scoped_refptr<TCPClientSocketPool>& GetSocketPoolForHTTPProxy(
+      const HostPortPair& http_proxy);
+
   // SSL sockets come from the socket_factory().
   ClientSocketFactory* socket_factory() { return socket_factory_; }
   HostResolver* host_resolver() { return host_resolver_; }
@@ -81,35 +88,30 @@ class HttpNetworkSession : public base::RefCounted<HttpNetworkSession> {
 
   static void set_max_sockets_per_group(int socket_count);
 
-  static uint16 fixed_http_port() { return g_fixed_http_port; }
-  static void set_fixed_http_port(uint16 port) { g_fixed_http_port = port; }
+  static uint16 fixed_http_port();
+  static void set_fixed_http_port(uint16 port);
 
-  static uint16 fixed_https_port() { return g_fixed_https_port; }
-  static void set_fixed_https_port(uint16 port) { g_fixed_https_port = port; }
+  static uint16 fixed_https_port();
+  static void set_fixed_https_port(uint16 port);
 
  private:
+  typedef std::map<HostPortPair, scoped_refptr<TCPClientSocketPool> >
+      HTTPProxySocketPoolMap;
+  typedef std::map<HostPortPair, scoped_refptr<SOCKSClientSocketPool> >
+      SOCKSSocketPoolMap;
+
   friend class base::RefCounted<HttpNetworkSession>;
-  FRIEND_TEST(HttpNetworkTransactionTest, GroupNameForProxyConnections);
+  friend class HttpNetworkSessionPeer;
 
   ~HttpNetworkSession();
 
-  // Total limit of sockets. Not a constant to allow experiments.
-  static int max_sockets_;
-
-  // Default to allow up to 6 connections per host. Experiment and tuning may
-  // try other values (greater than 0).  Too large may cause many problems, such
-  // as home routers blocking the connections!?!?
-  static int max_sockets_per_group_;
-
-  static uint16 g_fixed_http_port;
-  static uint16 g_fixed_https_port;
-
   HttpAuthCache auth_cache_;
   SSLClientAuthCache ssl_client_auth_cache_;
   HttpAlternateProtocols alternate_protocols_;
   NetworkChangeNotifier* const network_change_notifier_;
   scoped_refptr<TCPClientSocketPool> tcp_socket_pool_;
-  scoped_refptr<SOCKSClientSocketPool> socks_socket_pool_;
+  HTTPProxySocketPoolMap http_proxy_socket_pool_;
+  SOCKSSocketPoolMap socks_socket_pool_;
   ClientSocketFactory* socket_factory_;
   scoped_refptr<HostResolver> host_resolver_;
   scoped_refptr<ProxyService> proxy_service_;
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 4a789b6..f10a852 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -701,27 +701,20 @@ int HttpNetworkTransaction::DoInitConnection() {
   // Determine the host and port to connect to.
   std::string connection_group;
 
-  // |endpoint| indicates the final destination endpoint.
-  HostPortPair endpoint;
-  endpoint.host = request_->url.HostNoBrackets();
-  endpoint.port = request_->url.EffectiveIntPort();
+  // |endpoint_| indicates the final destination endpoint.
+  endpoint_ = HostPortPair(request_->url.HostNoBrackets(),
+                           request_->url.EffectiveIntPort());
 
-  if (!proxy_info_.is_direct()) {
-    ProxyServer proxy_server = proxy_info_.proxy_server();
-    connection_group = "proxy/" + proxy_server.ToURI() + "/";
-    peer_.host = proxy_server.HostNoBrackets();
-    peer_.port = proxy_server.port();
-  } else {
-    peer_ = endpoint;
+  if (proxy_info_.is_direct()) {
     if (alternate_protocol_mode_ == kUnspecified) {
       const HttpAlternateProtocols& alternate_protocols =
           session_->alternate_protocols();
-      if (alternate_protocols.HasAlternateProtocolFor(peer_)) {
+      if (alternate_protocols.HasAlternateProtocolFor(endpoint_)) {
         HttpAlternateProtocols::PortProtocolPair alternate =
-            alternate_protocols.GetAlternateProtocolFor(peer_);
+            alternate_protocols.GetAlternateProtocolFor(endpoint_);
         if (alternate.protocol != HttpAlternateProtocols::BROKEN) {
           DCHECK_EQ(HttpAlternateProtocols::NPN_SPDY_1, alternate.protocol);
-          peer_.port = alternate.port;
+          endpoint_.port = alternate.port;
           using_ssl_ = true;
           alternate_protocol_ = HttpAlternateProtocols::NPN_SPDY_1;
           alternate_protocol_mode_ = kUsingAlternateProtocol;
@@ -733,53 +726,67 @@ int HttpNetworkTransaction::DoInitConnection() {
   // Use the fixed testing ports if they've been provided.
   if (using_ssl_) {
     if (session_->fixed_https_port() != 0)
-      peer_.port = session_->fixed_https_port();
+      endpoint_.port = session_->fixed_https_port();
   } else if (session_->fixed_http_port() != 0) {
-    peer_.port = session_->fixed_http_port();
+    endpoint_.port = session_->fixed_http_port();
   }
 
   // Check first if we have a spdy session for this group.  If so, then go
   // straight to using that.
-  if (session_->spdy_session_pool()->HasSession(peer_)) {
+  if (session_->spdy_session_pool()->HasSession(endpoint_)) {
     using_spdy_ = true;
     return OK;
   }
 
-  // For a connection via HTTP proxy not using CONNECT, the connection
-  // is to the proxy server only. For all other cases
-  // (direct, HTTP proxy CONNECT, SOCKS), the connection is up to the
-  // url endpoint. Hence we append the url data into the connection_group.
-  // Note that the url endpoint may be different in the Alternate-Protocol case.
-  if (proxy_info_.is_direct())
-    connection_group = peer_.ToString();
-  else if (using_ssl_ || proxy_info_.is_socks())
-    connection_group.append(endpoint.ToString());
-
+  connection_group = endpoint_.ToString();
   DCHECK(!connection_group.empty());
 
   // If the user is refreshing the page, bypass the host cache.
   bool disable_resolver_cache = request_->load_flags & LOAD_BYPASS_CACHE ||
     request_->load_flags & LOAD_DISABLE_CACHE;
 
-  TCPSocketParams tcp_params(peer_.host, peer_.port, request_->priority,
-                             request_->referrer, disable_resolver_cache);
-
   int rv;
-  if (!proxy_info_.is_socks()) {
+  if (!proxy_info_.is_direct()) {
+    ProxyServer proxy_server = proxy_info_.proxy_server();
+    HostPortPair proxy_host_port_pair(proxy_server.HostNoBrackets(),
+                                      proxy_server.port());
+
+    TCPSocketParams tcp_params(proxy_host_port_pair, request_->priority,
+                               request_->referrer, disable_resolver_cache);
+
+    if (proxy_info_.is_socks()) {
+      const char* socks_version;
+      bool socks_v5;
+      if (proxy_info_.proxy_server().scheme() == ProxyServer::SCHEME_SOCKS5) {
+        socks_version = "5";
+        socks_v5 = true;
+      } else {
+        socks_version = "4";
+        socks_v5 = false;
+      }
+
+      connection_group =
+          StringPrintf("socks%s/%s", socks_version, connection_group.c_str());
+
+      SOCKSSocketParams socks_params(tcp_params, socks_v5, endpoint_,
+                                     request_->priority, request_->referrer);
+
+      rv = connection_->Init(
+          connection_group, socks_params, request_->priority,
+          &io_callback_,
+          session_->GetSocketPoolForSOCKSProxy(proxy_host_port_pair), net_log_);
+    } else {
+      rv = connection_->Init(
+          connection_group, tcp_params, request_->priority,
+          &io_callback_,
+          session_->GetSocketPoolForHTTPProxy(proxy_host_port_pair), net_log_);
+    }
+  } else {
+    TCPSocketParams tcp_params(endpoint_, request_->priority,
+                               request_->referrer, disable_resolver_cache);
     rv = connection_->Init(connection_group, tcp_params, request_->priority,
                            &io_callback_, session_->tcp_socket_pool(),
                            net_log_);
-  } else {
-    bool socks_v5 = proxy_info_.proxy_server().scheme() ==
-                    ProxyServer::SCHEME_SOCKS5;
-    SOCKSSocketParams socks_params(tcp_params, socks_v5,
-                                   request_->url.HostNoBrackets(),
-                                   request_->url.EffectiveIntPort(),
-                                   request_->priority, request_->referrer);
-
-    rv = connection_->Init(connection_group, socks_params, request_->priority,
-                           &io_callback_, session_->socks_socket_pool(),
-                           net_log_);
   }
 
   return rv;
@@ -1248,15 +1255,15 @@ int HttpNetworkTransaction::DoSpdySendRequest() {
       session_->spdy_session_pool();
   scoped_refptr<SpdySession> spdy_session;
 
-  if (spdy_pool->HasSession(peer_)) {
-    spdy_session = spdy_pool->Get(peer_, session_);
+  if (spdy_pool->HasSession(endpoint_)) {
+    spdy_session = spdy_pool->Get(endpoint_, session_);
   } else {
     // SPDY is negotiated using the TLS next protocol negotiation (NPN)
     // extension, so |connection_| must contain an SSLClientSocket.
     DCHECK(using_ssl_);
     CHECK(connection_->socket());
     spdy_session = spdy_pool->GetSpdySessionFromSSLSocket(
-        peer_, session_, connection_.release());
+        endpoint_, session_, connection_.release());
   }
 
   CHECK(spdy_session.get());
diff --git a/net/http/http_network_transaction.h b/net/http/http_network_transaction.h
index eb6f06e..3217fac 100644
--- a/net/http/http_network_transaction.h
+++ b/net/http/http_network_transaction.h
@@ -362,9 +362,9 @@ class HttpNetworkTransaction : public HttpTransaction {
   // The next state in the state machine.
   State next_state_;
 
-  // The hostname and port of the peer.  This is not necessarily the one
-  // specified by the URL, due to Alternate-Protocol or proxies.
-  HostPortPair peer_;
+  // The hostname and port of the endpoint.  This is not necessarily the one
+  // specified by the URL, due to Alternate-Protocol or fixed testing ports.
+  HostPortPair endpoint_;
 
   DISALLOW_COPY_AND_ASSIGN(HttpNetworkTransaction);
 };
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 6058d8f..7d6db6f 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -2,9 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "net/http/http_network_transaction.h"
+
 #include <math.h>  // ceil
 #include <vector>
 
+#include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
@@ -19,7 +22,6 @@
 #include "net/http/http_auth_handler_ntlm.h"
 #include "net/http/http_basic_stream.h"
 #include "net/http/http_network_session.h"
-#include "net/http/http_network_transaction.h"
 #include "net/http/http_stream.h"
 #include "net/http/http_transaction_unittest.h"
 #include "net/proxy/proxy_config_service_fixed.h"
@@ -37,6 +39,34 @@
 
 namespace net {
 
+class HttpNetworkSessionPeer {
+ public:
+  explicit HttpNetworkSessionPeer(
+      const scoped_refptr<HttpNetworkSession>& session)
+      : session_(session) {}
+
+  void SetTCPSocketPool(const scoped_refptr<TCPClientSocketPool>& pool) {
+    session_->tcp_socket_pool_ = pool;
+  }
+
+  void SetSocketPoolForSOCKSProxy(
+      const HostPortPair& socks_proxy,
+      const scoped_refptr<SOCKSClientSocketPool>& pool) {
+    session_->socks_socket_pool_[socks_proxy] = pool;
+  }
+
+  void SetSocketPoolForHTTPProxy(
+      const HostPortPair& http_proxy,
+      const scoped_refptr<TCPClientSocketPool>& pool) {
+    session_->http_proxy_socket_pool_[http_proxy] = pool;
+  }
+
+ private:
+  const scoped_refptr<HttpNetworkSession> session_;
+
+  DISALLOW_COPY_AND_ASSIGN(HttpNetworkSessionPeer);
+};
+
 // Helper to manage the lifetimes of the dependencies for a
 // HttpNetworkTransaction.
 class SessionDependencies {
@@ -190,13 +220,12 @@ std::string MockGetHostName() {
   return "WTC-WIN7";
 }
 
-template<typename EmulatedClientSocketPool, typename SocketSourceType>
+template<typename EmulatedClientSocketPool>
 class CaptureGroupNameSocketPool : public EmulatedClientSocketPool {
  public:
-  CaptureGroupNameSocketPool(HttpNetworkSession* session,
-                             SocketSourceType* socket_source)
+  CaptureGroupNameSocketPool(HttpNetworkSession* session)
       : EmulatedClientSocketPool(0, 0, "CaptureGroupNameTestPool",
-                                 session->host_resolver(), socket_source,
+                                 session->host_resolver(), NULL,
                                  NULL) {}
   const std::string last_group_name_received() const {
     return last_group_name_;
@@ -237,9 +266,9 @@ class CaptureGroupNameSocketPool : public EmulatedClientSocketPool {
   std::string last_group_name_;
 };
 
-typedef CaptureGroupNameSocketPool<TCPClientSocketPool, ClientSocketFactory>
+typedef CaptureGroupNameSocketPool<TCPClientSocketPool>
     CaptureGroupNameTCPSocketPool;
-typedef CaptureGroupNameSocketPool<SOCKSClientSocketPool, TCPClientSocketPool>
+typedef CaptureGroupNameSocketPool<SOCKSClientSocketPool>
     CaptureGroupNameSOCKSSocketPool;
 //-----------------------------------------------------------------------------
 
@@ -3673,61 +3702,165 @@ TEST_F(HttpNetworkTransactionTest, SOCKS5_SSL_GET) {
 }
 
 // Tests that for connection endpoints the group names are correctly set.
-TEST_F(HttpNetworkTransactionTest, GroupNameForProxyConnections) {
-  const struct {
-    const std::string proxy_server;
-    const std::string url;
-    const std::string expected_group_name;
-  } tests[] = {
+
+struct GroupNameTest {
+  std::string proxy_server;
+  std::string url;
+  std::string expected_group_name;
+};
+
+scoped_refptr<HttpNetworkSession> SetupSessionForGroupNameTests(
+    const std::string& proxy_server) {
+  SessionDependencies session_deps(CreateFixedProxyService(proxy_server));
+  scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps));
+
+  HttpAlternateProtocols* alternate_protocols =
+      session->mutable_alternate_protocols();
+  alternate_protocols->SetAlternateProtocolFor(
+      HostPortPair("host.with.alternate", 80), 443,
+      HttpAlternateProtocols::NPN_SPDY_1);
+
+  return session;
+}
+
+int GroupNameTransactionHelper(
+    const std::string& url,
+    const scoped_refptr<HttpNetworkSession>& session) {
+  scoped_ptr<HttpTransaction> trans(new HttpNetworkTransaction(session));
+
+  HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL(url);
+  request.load_flags = 0;
+
+  TestCompletionCallback callback;
+
+  // We do not complete this request, the dtor will clean the transaction up.
+  return trans->Start(&request, &callback, BoundNetLog());
+}
+
+TEST_F(HttpNetworkTransactionTest, GroupNameForDirectConnections) {
+  const GroupNameTest tests[] = {
     {
-      "",  // no proxy (direct)
+      "",  // unused
       "http://www.google.com/direct",
       "www.google.com:80",
     },
     {
-      "",  // no proxy (direct)
+      "",  // unused
       "http://[2001:1418:13:1::25]/direct",
       "[2001:1418:13:1::25]:80",
     },
-    {
-      "http_proxy",
-      "http://www.google.com/http_proxy_normal",
-      "proxy/http_proxy:80/",
-    },
-    {
-      "socks4://socks_proxy:1080",
-      "http://www.google.com/socks4_direct",
-      "proxy/socks4://socks_proxy:1080/www.google.com:80",
-    },
 
     // SSL Tests
     {
-      "",
+      "",  // unused
       "https://www.google.com/direct_ssl",
       "www.google.com:443",
     },
     {
-      "http_proxy",
-      "https://www.google.com/http_connect_ssl",
-      "proxy/http_proxy:80/www.google.com:443",
-    },
-    {
-      "socks4://socks_proxy:1080",
-      "https://www.google.com/socks4_ssl",
-      "proxy/socks4://socks_proxy:1080/www.google.com:443",
+      "",  // unused
+      "https://[2001:1418:13:1::25]/direct",
+      "[2001:1418:13:1::25]:443",
     },
     {
-      "",  // no proxy (direct)
+      "",  // unused
       "http://host.with.alternate/direct",
       "host.with.alternate:443",
     },
+  };
+
+  HttpNetworkTransaction::SetUseAlternateProtocols(true);
 
-    // TODO(willchan): Uncomment these tests when they work.
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
+    scoped_refptr<HttpNetworkSession> session(
+        SetupSessionForGroupNameTests(tests[i].proxy_server));
+
+    HttpNetworkSessionPeer peer(session);
+    scoped_refptr<CaptureGroupNameTCPSocketPool> tcp_conn_pool(
+        new CaptureGroupNameTCPSocketPool(session.get()));
+    peer.SetTCPSocketPool(tcp_conn_pool);
+
+    EXPECT_EQ(ERR_IO_PENDING,
+              GroupNameTransactionHelper(tests[i].url, session));
+    EXPECT_EQ(tests[i].expected_group_name,
+              tcp_conn_pool->last_group_name_received());
+  }
+
+  HttpNetworkTransaction::SetUseAlternateProtocols(false);
+}
+
+TEST_F(HttpNetworkTransactionTest, GroupNameForHTTPProxyConnections) {
+  const GroupNameTest tests[] = {
+    {
+      "http_proxy",
+      "http://www.google.com/http_proxy_normal",
+      "www.google.com:80",
+    },
+
+    // SSL Tests
+    {
+      "http_proxy",
+      "https://www.google.com/http_connect_ssl",
+      "www.google.com:443",
+    },
+
+// TODO(willchan): Uncomment these tests when they work.
 //    {
 //      "http_proxy",
 //      "http://host.with.alternate/direct",
 //      "proxy/http_proxy:80/host.with.alternate:443",
 //    },
+  };
+
+  HttpNetworkTransaction::SetUseAlternateProtocols(true);
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
+    scoped_refptr<HttpNetworkSession> session(
+        SetupSessionForGroupNameTests(tests[i].proxy_server));
+
+    HttpNetworkSessionPeer peer(session);
+
+    scoped_refptr<CaptureGroupNameTCPSocketPool> http_proxy_pool(
+        new CaptureGroupNameTCPSocketPool(session.get()));
+    peer.SetSocketPoolForHTTPProxy(
+        HostPortPair("http_proxy", 80), http_proxy_pool);
+
+    EXPECT_EQ(ERR_IO_PENDING,
+              GroupNameTransactionHelper(tests[i].url, session));
+    EXPECT_EQ(tests[i].expected_group_name,
+              http_proxy_pool->last_group_name_received());
+  }
+
+  HttpNetworkTransaction::SetUseAlternateProtocols(false);
+}
+
+TEST_F(HttpNetworkTransactionTest, GroupNameForSOCKSConnections) {
+  const GroupNameTest tests[] = {
+    {
+      "socks4://socks_proxy:1080",
+      "http://www.google.com/socks4_direct",
+      "socks4/www.google.com:80",
+    },
+    {
+      "socks5://socks_proxy:1080",
+      "http://www.google.com/socks5_direct",
+      "socks5/www.google.com:80",
+    },
+
+    // SSL Tests
+    {
+      "socks4://socks_proxy:1080",
+      "https://www.google.com/socks4_ssl",
+      "socks4/www.google.com:443",
+    },
+    {
+      "socks5://socks_proxy:1080",
+      "https://www.google.com/socks5_ssl",
+      "socks5/www.google.com:443",
+    },
+
+// TODO(willchan): Uncomment these tests when they work.
 //    {
 //      "socks4://socks_proxy:1080",
 //      "http://host.with.alternate/direct",
@@ -3738,40 +3871,21 @@ TEST_F(HttpNetworkTransactionTest, GroupNameForProxyConnections) {
   HttpNetworkTransaction::SetUseAlternateProtocols(true);
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
-    SessionDependencies session_deps(
-        CreateFixedProxyService(tests[i].proxy_server));
-
-    scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps));
-
-    HttpAlternateProtocols* alternate_protocols =
-        session->mutable_alternate_protocols();
-    alternate_protocols->SetAlternateProtocolFor(
-        HostPortPair("host.with.alternate", 80), 443,
-        HttpAlternateProtocols::NPN_SPDY_1);
+    scoped_refptr<HttpNetworkSession> session(
+        SetupSessionForGroupNameTests(tests[i].proxy_server));
+    HttpNetworkSessionPeer peer(session);
 
-    scoped_refptr<CaptureGroupNameTCPSocketPool> tcp_conn_pool(
-        new CaptureGroupNameTCPSocketPool(session.get(),
-                                          session->socket_factory()));
-    session->tcp_socket_pool_ = tcp_conn_pool.get();
     scoped_refptr<CaptureGroupNameSOCKSSocketPool> socks_conn_pool(
-        new CaptureGroupNameSOCKSSocketPool(session.get(),
-                                            tcp_conn_pool.get()));
-    session->socks_socket_pool_ = socks_conn_pool.get();
+        new CaptureGroupNameSOCKSSocketPool(session.get()));
+    peer.SetSocketPoolForSOCKSProxy(
+        HostPortPair("socks_proxy", 1080), socks_conn_pool);
 
     scoped_ptr<HttpTransaction> trans(new HttpNetworkTransaction(session));
 
-    HttpRequestInfo request;
-    request.method = "GET";
-    request.url = GURL(tests[i].url);
-    request.load_flags = 0;
-
-    TestCompletionCallback callback;
-
-    // We do not complete this request, the dtor will clean the transaction up.
-    EXPECT_EQ(ERR_IO_PENDING, trans->Start(&request, &callback, BoundNetLog()));
-    std::string allgroups = tcp_conn_pool->last_group_name_received() +
-                            socks_conn_pool->last_group_name_received();
-    EXPECT_EQ(tests[i].expected_group_name, allgroups);
+    EXPECT_EQ(ERR_IO_PENDING,
+              GroupNameTransactionHelper(tests[i].url, session));
+    EXPECT_EQ(tests[i].expected_group_name,
+              socks_conn_pool->last_group_name_received());
   }
 
   HttpNetworkTransaction::SetUseAlternateProtocols(false);
diff --git a/net/proxy/proxy_server.cc b/net/proxy/proxy_server.cc
index 6c47fee..4903880 100644
--- a/net/proxy/proxy_server.cc
+++ b/net/proxy/proxy_server.cc
@@ -84,6 +84,13 @@ std::string ProxyServer::host_and_port() const {
   return host_ + ":" + IntToString(port_);
 }
 
+HostPortPair ProxyServer::host_port_pair() const {
+  // Doesn't make sense to call this if the URI scheme doesn't
+  // have concept of a host.
+  DCHECK(is_valid() && !is_direct());
+  return HostPortPair(host_, port_);
+}
+
 // static
 ProxyServer ProxyServer::FromURI(const std::string& uri,
                                  Scheme default_scheme) {
diff --git a/net/proxy/proxy_server.h b/net/proxy/proxy_server.h
index d079396..d7a94f0 100644
--- a/net/proxy/proxy_server.h
+++ b/net/proxy/proxy_server.h
@@ -12,6 +12,7 @@
 #endif
 
 #include <string>
+#include "net/base/host_port_pair.h"
 
 namespace net {
 
@@ -65,8 +66,13 @@ class ProxyServer {
   int port() const;
 
   // Returns the <host>":"<port> string for the proxy server.
+  // TODO(willchan): Remove in favor of host_port_pair().
   std::string host_and_port() const;
 
+  // TODO(willchan): Change to const HostPortPair& after refactoring |host_| and
+  // |port_| here.
+  HostPortPair host_port_pair() const;
+
   // Parse from an input with format:
   //   [<scheme>"://"]<server>[":"<port>]
   //
diff --git a/net/socket/socks_client_socket_pool.h b/net/socket/socks_client_socket_pool.h
index 2c30600..a786dc1 100644
--- a/net/socket/socks_client_socket_pool.h
+++ b/net/socket/socks_client_socket_pool.h
@@ -11,6 +11,7 @@
 #include "base/compiler_specific.h"
 #include "base/scoped_ptr.h"
 #include "base/time.h"
+#include "net/base/host_port_pair.h"
 #include "net/base/host_resolver.h"
 #include "net/proxy/proxy_server.h"
 #include "net/socket/client_socket_pool_base.h"
@@ -25,10 +26,10 @@ class ConnectJobFactory;
 class SOCKSSocketParams {
  public:
   SOCKSSocketParams(const TCPSocketParams& proxy_server, bool socks_v5,
-                    const std::string& destination_host, int destination_port,
+                    const HostPortPair& host_port_pair,
                     RequestPriority priority, const GURL& referrer)
       : tcp_params_(proxy_server),
-        destination_(destination_host, destination_port),
+        destination_(host_port_pair.host, host_port_pair.port),
         socks_v5_(socks_v5) {
     // The referrer is used by the DNS prefetch system to correlate resolutions
     // with the page that triggered them. It doesn't impact the actual addresses
diff --git a/net/socket/socks_client_socket_pool_unittest.cc b/net/socket/socks_client_socket_pool_unittest.cc
index 577a39b..89530f6 100644
--- a/net/socket/socks_client_socket_pool_unittest.cc
+++ b/net/socket/socks_client_socket_pool_unittest.cc
@@ -168,14 +168,17 @@ class SOCKSClientSocketPoolTest : public ClientSocketPoolTest {
   };
 
   SOCKSClientSocketPoolTest()
-      : ignored_tcp_socket_params_("proxy", 80, MEDIUM, GURL(), false),
+      : ignored_tcp_socket_params_(
+            HostPortPair("proxy", 80), MEDIUM, GURL(), false),
         tcp_socket_pool_(new MockTCPClientSocketPool(
             kMaxSockets, kMaxSocketsPerGroup, "MockTCP",
             &tcp_client_socket_factory_, &tcp_notifier_)),
-        ignored_socket_params_(ignored_tcp_socket_params_, true, "host", 80,
-            MEDIUM, GURL()),
-        pool_(new SOCKSClientSocketPool(kMaxSockets, kMaxSocketsPerGroup,
-            "SOCKSUnitTest", NULL, tcp_socket_pool_.get(), &socks_notifier_)) {
+        ignored_socket_params_(ignored_tcp_socket_params_, true,
+                               HostPortPair("host", 80),
+                               MEDIUM, GURL()),
+        pool_(new SOCKSClientSocketPool(
+            kMaxSockets, kMaxSocketsPerGroup, "SOCKSUnitTest", NULL,
+            tcp_socket_pool_.get(), &socks_notifier_)) {
   }
 
   int StartRequest(const std::string& group_name, RequestPriority priority) {
diff --git a/net/socket/tcp_client_socket_pool.h b/net/socket/tcp_client_socket_pool.h
index 76950c3..30158f6 100644
--- a/net/socket/tcp_client_socket_pool.h
+++ b/net/socket/tcp_client_socket_pool.h
@@ -12,6 +12,7 @@
 #include "base/scoped_ptr.h"
 #include "base/time.h"
 #include "base/timer.h"
+#include "net/base/host_port_pair.h"
 #include "net/base/host_resolver.h"
 #include "net/socket/client_socket_pool_base.h"
 #include "net/socket/client_socket_pool.h"
@@ -22,9 +23,24 @@ class ClientSocketFactory;
 
 class TCPSocketParams {
  public:
+  TCPSocketParams(const HostPortPair& host_port_pair, RequestPriority priority,
+                  const GURL& referrer, bool disable_resolver_cache)
+      :  destination_(host_port_pair.host, host_port_pair.port) {
+    Initialize(priority, referrer, disable_resolver_cache);
+  }
+
+  // TODO(willchan): Update all unittests so we don't need this.
   TCPSocketParams(const std::string& host, int port, RequestPriority priority,
                   const GURL& referrer, bool disable_resolver_cache)
       :  destination_(host, port) {
+    Initialize(priority, referrer, disable_resolver_cache);
+  }
+
+  HostResolver::RequestInfo destination() const { return destination_; }
+
+ private:
+  void Initialize(RequestPriority priority, const GURL& referrer,
+                  bool disable_resolver_cache) {
     // The referrer is used by the DNS prefetch system to correlate resolutions
     // with the page that triggered them. It doesn't impact the actual addresses
     // that we resolve to.
@@ -34,9 +50,6 @@ class TCPSocketParams {
       destination_.set_allow_cached_response(false);
   }
 
-  HostResolver::RequestInfo destination() const { return destination_; }
-
- private:
   HostResolver::RequestInfo destination_;
 };
 
diff --git a/net/socket/tcp_client_socket_pool_unittest.cc b/net/socket/tcp_client_socket_pool_unittest.cc
index 4986f32..6df566ba 100644
--- a/net/socket/tcp_client_socket_pool_unittest.cc
+++ b/net/socket/tcp_client_socket_pool_unittest.cc
@@ -256,7 +256,8 @@ class MockClientSocketFactory : public ClientSocketFactory {
 class TCPClientSocketPoolTest : public ClientSocketPoolTest {
  protected:
   TCPClientSocketPoolTest()
-      : ignored_socket_params_("ignored", 80, MEDIUM, GURL(), false),
+      : ignored_socket_params_(
+            HostPortPair("ignored", 80), MEDIUM, GURL(), false),
         host_resolver_(new MockHostResolver),
         pool_(new TCPClientSocketPool(kMaxSockets,
                                       kMaxSocketsPerGroup,
@@ -281,7 +282,7 @@ class TCPClientSocketPoolTest : public ClientSocketPoolTest {
 TEST_F(TCPClientSocketPoolTest, Basic) {
   TestCompletionCallback callback;
   ClientSocketHandle handle;
-  TCPSocketParams dest("www.google.com", 80, LOW, GURL(), false);
+  TCPSocketParams dest(HostPortPair("www.google.com", 80), LOW, GURL(), false);
   int rv = handle.Init("a", dest, LOW, &callback, pool_, BoundNetLog());
   EXPECT_EQ(ERR_IO_PENDING, rv);
   EXPECT_FALSE(handle.is_initialized());
diff --git a/net/spdy/spdy_network_transaction.cc b/net/spdy/spdy_network_transaction.cc
index 6a50fb2..b0f35af 100644
--- a/net/spdy/spdy_network_transaction.cc
+++ b/net/spdy/spdy_network_transaction.cc
@@ -225,9 +225,9 @@ int SpdyNetworkTransaction::DoInitConnection() {
   std::string connection_group = "spdy.";
   connection_group.append(host);
 
-  TCPSocketParams tcp_params(host, port, request_->priority, request_->referrer,
-                             false);
   HostPortPair host_port_pair(host, port);
+  TCPSocketParams tcp_params(host_port_pair, request_->priority,
+                             request_->referrer, false);
 
   spdy_ = session_->spdy_session_pool()->Get(host_port_pair, session_);
   DCHECK(spdy_);