Refactoring using_proxy_, using_tunnel_, using_socks_proxy_ into a single enum ProxyConfig in HttpNetworkTransaction.

Fixing http://crbug.com/14982
In case of SOCKS proxies we set the url to append to the group name so that the socks endpoint is at the target server and not the socks proxy server and so can be effectively reused by the socket pool.

BUG=14982
TEST=unittests

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

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

4 files changed, 145 insertions, 23 deletions

diff --git a/net/http/http_network_session.h b/net/http/http_network_session.h
index b17b99d..a084ec7 100644
--- a/net/http/http_network_session.h
+++ b/net/http/http_network_session.h
@@ -44,6 +44,8 @@ class HttpNetworkSession : public base::RefCounted<HttpNetworkSession> {
   static void set_max_sockets_per_group(int socket_count);
 
  private:
+  FRIEND_TEST(HttpNetworkTransactionTest, GroupNameForProxyConnections);
+
   // 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!?!?
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 8d2821b..c1d0cef 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -140,9 +140,7 @@ HttpNetworkTransaction::HttpNetworkTransaction(HttpNetworkSession* session,
       connection_(session->connection_pool()),
       reused_socket_(false),
       using_ssl_(false),
-      using_proxy_(false),
-      using_tunnel_(false),
-      using_socks_proxy_(false),
+      proxy_mode_(kDirectConnection),
       establishing_tunnel_(false),
       reading_body_from_socket_(false),
       request_headers_(new RequestHeaders()),
@@ -562,17 +560,22 @@ int HttpNetworkTransaction::DoInitConnection() {
   next_state_ = STATE_INIT_CONNECTION_COMPLETE;
 
   using_ssl_ = request_->url.SchemeIs("https");
-  using_socks_proxy_ = !proxy_info_.is_direct() &&
-      proxy_info_.proxy_server().is_socks();
-  using_proxy_ = !proxy_info_.is_direct() && !using_ssl_ && !using_socks_proxy_;
-  using_tunnel_ = !proxy_info_.is_direct() && using_ssl_ && !using_socks_proxy_;
+
+  if (proxy_info_.is_direct())
+    proxy_mode_ = kDirectConnection;
+  else if (proxy_info_.proxy_server().is_socks())
+    proxy_mode_ = kSOCKSProxy;
+  else if (using_ssl_)
+    proxy_mode_ = kHTTPProxyUsingTunnel;
+  else
+    proxy_mode_ = kHTTPProxy;
 
   // Build the string used to uniquely identify connections of this type.
   // Determine the host and port to connect to.
   std::string connection_group;
   std::string host;
   int port;
-  if (using_proxy_ || using_tunnel_ || using_socks_proxy_) {
+  if (proxy_mode_ != kDirectConnection) {
     ProxyServer proxy_server = proxy_info_.proxy_server();
     connection_group = "proxy/" + proxy_server.ToURI() + "/";
     host = proxy_server.HostNoBrackets();
@@ -581,7 +584,12 @@ int HttpNetworkTransaction::DoInitConnection() {
     host = request_->url.HostNoBrackets();
     port = request_->url.EffectiveIntPort();
   }
-  if (!using_proxy_ && !using_socks_proxy_)
+
+  // 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 upto the
+  // url endpoint. Hence we append the url data into the connection_group.
+  if (proxy_mode_ != kHTTPProxy)
     connection_group.append(request_->url.GetOrigin().spec());
 
   DCHECK(!connection_group.empty());
@@ -620,13 +628,13 @@ int HttpNetworkTransaction::DoInitConnectionComplete(int result) {
     // Now we have a TCP connected socket.  Perform other connection setup as
     // needed.
     LogTCPConnectedMetrics();
-    if (using_socks_proxy_)
+    if (proxy_mode_ == kSOCKSProxy)
       next_state_ = STATE_SOCKS_CONNECT;
-    else if (using_ssl_ && !using_tunnel_) {
+    else if (using_ssl_ && proxy_mode_ == kDirectConnection) {
       next_state_ = STATE_SSL_CONNECT;
     } else {
       next_state_ = STATE_WRITE_HEADERS;
-      if (using_tunnel_)
+      if (proxy_mode_ == kHTTPProxyUsingTunnel)
         establishing_tunnel_ = true;
     }
   }
@@ -635,9 +643,7 @@ int HttpNetworkTransaction::DoInitConnectionComplete(int result) {
 }
 
 int HttpNetworkTransaction::DoSOCKSConnect() {
-  DCHECK(using_socks_proxy_);
-  DCHECK(!using_proxy_);
-  DCHECK(!using_tunnel_);
+  DCHECK_EQ(kSOCKSProxy, proxy_mode_);
 
   next_state_ = STATE_SOCKS_CONNECT_COMPLETE;
 
@@ -653,9 +659,7 @@ int HttpNetworkTransaction::DoSOCKSConnect() {
 }
 
 int HttpNetworkTransaction::DoSOCKSConnectComplete(int result) {
-  DCHECK(using_socks_proxy_);
-  DCHECK(!using_proxy_);
-  DCHECK(!using_tunnel_);
+  DCHECK_EQ(kSOCKSProxy, proxy_mode_);
 
   if (result == OK) {
     if (using_ssl_) {
@@ -730,7 +734,8 @@ int HttpNetworkTransaction::DoWriteHeaders() {
       if (request_->upload_data)
         request_body_stream_.reset(new UploadDataStream(request_->upload_data));
       BuildRequestHeaders(request_, authorization_headers,
-                          request_body_stream_.get(), using_proxy_,
+                          request_body_stream_.get(),
+                          proxy_mode_ == kHTTPProxy,
                           &request_headers_->headers_);
     }
   }
@@ -1496,7 +1501,7 @@ int HttpNetworkTransaction::ReconsiderProxyAfterError(int error) {
 }
 
 bool HttpNetworkTransaction::ShouldApplyProxyAuth() const {
-  return using_proxy_ || establishing_tunnel_;
+  return (proxy_mode_ == kHTTPProxy) || establishing_tunnel_;
 }
 
 bool HttpNetworkTransaction::ShouldApplyServerAuth() const {
diff --git a/net/http/http_network_transaction.h b/net/http/http_network_transaction.h
index 9e6d53f..63ac243 100644
--- a/net/http/http_network_transaction.h
+++ b/net/http/http_network_transaction.h
@@ -116,6 +116,13 @@ class HttpNetworkTransaction : public HttpTransaction {
     STATE_NONE
   };
 
+  enum ProxyMode {
+    kDirectConnection,  // If using a direct connection
+    kHTTPProxy,  // If using a proxy for HTTP (not HTTPS)
+    kHTTPProxyUsingTunnel,  // If using a tunnel for HTTPS
+    kSOCKSProxy,  // If using a SOCKS proxy
+  };
+
   void DoCallback(int result);
   void OnIOComplete(int result);
 
@@ -307,9 +314,7 @@ class HttpNetworkTransaction : public HttpTransaction {
   bool reused_socket_;
 
   bool using_ssl_;     // True if handling a HTTPS request
-  bool using_proxy_;   // True if using a proxy for HTTP (not HTTPS)
-  bool using_tunnel_;  // True if using a tunnel for HTTPS
-  bool using_socks_proxy_;  // True if using a SOCKS proxy
+  ProxyMode proxy_mode_;
 
   // True while establishing a tunnel.  This allows the HTTP CONNECT
   // request/response to reuse the STATE_WRITE_HEADERS,
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 57ea425..ba68db1 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -164,6 +164,45 @@ std::string MockGetHostName() {
   return "WTC-WIN7";
 }
 
+class CaptureGroupNameSocketPool : public ClientSocketPool {
+ public:
+  CaptureGroupNameSocketPool() {
+  }
+  virtual int RequestSocket(const std::string& group_name,
+                            const HostResolver::RequestInfo& resolve_info,
+                            int priority,
+                            ClientSocketHandle* handle,
+                            CompletionCallback* callback) {
+    last_group_name_ = group_name;
+    return ERR_IO_PENDING;
+  }
+
+  const std::string last_group_name_received() const {
+    return last_group_name_;
+  }
+
+  virtual void CancelRequest(const std::string& group_name,
+                             const ClientSocketHandle* handle) { }
+  virtual void ReleaseSocket(const std::string& group_name,
+                             ClientSocket* socket) {}
+  virtual void CloseIdleSockets() {}
+  virtual HostResolver* GetHostResolver() const {
+    return NULL;
+  }
+  virtual int IdleSocketCount() const {
+    return 0;
+  }
+  virtual int IdleSocketCountInGroup(const std::string& group_name) const {
+    return 0;
+  }
+  virtual LoadState GetLoadState(const std::string& group_name,
+                                 const ClientSocketHandle* handle) const {
+    return LOAD_STATE_IDLE;
+  }
+ protected:
+  std::string last_group_name_;
+};
+
 //-----------------------------------------------------------------------------
 
 TEST_F(HttpNetworkTransactionTest, Basic) {
@@ -3115,6 +3154,77 @@ TEST_F(HttpNetworkTransactionTest, SOCKS4_SSL_GET) {
   EXPECT_EQ("Payload", response_text);
 }
 
+// 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[] = {
+    {
+      "",  // no proxy (direct)
+      "http://www.google.com/direct",
+      "http://www.google.com/",
+    },
+    {
+      "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/http://www.google.com/",
+    },
+
+    // SSL Tests
+    {
+      "",
+      "https://www.google.com/direct_ssl",
+      "https://www.google.com/",
+    },
+    {
+      "http_proxy",
+      "https://www.google.com/http_connect_ssl",
+      "proxy/http_proxy:80/https://www.google.com/",
+    },
+    {
+      "socks4://socks_proxy:1080",
+      "https://www.google.com/socks4_ssl",
+      "proxy/socks4://socks_proxy:1080/https://www.google.com/",
+    },
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
+    SessionDependencies session_deps;
+    session_deps.proxy_service.reset(CreateFixedProxyService(
+        tests[i].proxy_server));
+
+    scoped_refptr<CaptureGroupNameSocketPool> conn_pool(
+        new CaptureGroupNameSocketPool());
+
+    scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps));
+    session->connection_pool_ = conn_pool.get();
+
+    scoped_ptr<HttpTransaction> trans(
+        new HttpNetworkTransaction(
+            session.get(),
+            &session_deps.socket_factory));
+
+    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));
+    EXPECT_EQ(tests[i].expected_group_name,
+              conn_pool->last_group_name_received());
+  }
+}
+
 TEST_F(HttpNetworkTransactionTest, ReconsiderProxyAfterFailedConnection) {
   scoped_refptr<RuleBasedHostMapper> host_mapper(new RuleBasedHostMapper());
   ScopedHostMapper scoped_host_mapper(host_mapper.get());