Make sure the key into the spdy session pool identifies the actual proxy used, and not the full list of possible proxies for the URL.

BUG=52668
TEST=SpdyNetworkTransactionTest.DirectConnectProxyReconnect
Review URL: http://codereview.chromium.org/3192011

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

7 files changed, 77 insertions, 22 deletions

diff --git a/net/http/http_stream_request.cc b/net/http/http_stream_request.cc
index 6ee73d2..3e9b157 100644
--- a/net/http/http_stream_request.cc
+++ b/net/http/http_stream_request.cc
@@ -423,7 +423,7 @@ int HttpStreamRequest::DoInitConnection() {
 
   // Check first if we have a spdy session for this group.  If so, then go
   // straight to using that.
-  HostPortProxyPair pair(endpoint_, proxy_info()->ToPacString());
+  HostPortProxyPair pair(endpoint_, proxy_info()->proxy_server().ToPacString());
   if (session_->spdy_session_pool()->HasSession(pair)) {
     using_spdy_ = true;
     next_state_ = STATE_INIT_STREAM;
@@ -684,7 +684,8 @@ int HttpStreamRequest::DoInitStream() {
       session_->spdy_session_pool();
   scoped_refptr<SpdySession> spdy_session;
 
-  HostPortProxyPair pair(endpoint_, proxy_info()->ToPacString());
+  HostPortProxyPair pair(endpoint_,
+                         proxy_info()->proxy_server().ToPacString());
   if (session_->spdy_session_pool()->HasSession(pair)) {
     spdy_session =
         session_->spdy_session_pool()->Get(pair, session_, net_log_);
diff --git a/net/proxy/proxy_info.h b/net/proxy/proxy_info.h
index 69c87c8..d6a891f 100644
--- a/net/proxy/proxy_info.h
+++ b/net/proxy/proxy_info.h
@@ -77,7 +77,7 @@ class ProxyInfo {
 
   // Returns the first valid proxy server. is_empty() must be false to be able
   // to call this function.
-  ProxyServer proxy_server() const { return proxy_list_.Get(); }
+  const ProxyServer& proxy_server() const { return proxy_list_.Get(); }
 
   // See description in ProxyList::ToPacString().
   std::string ToPacString() const;
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index 48e1fca..54fe171 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -83,6 +83,37 @@ class ProxyResolverNull : public ProxyResolver {
   }
 };
 
+// ProxyResolver that simulates a PAC script which returns
+// |pac_string| for every single URL.
+class ProxyResolverFromPacString : public ProxyResolver {
+ public:
+  ProxyResolverFromPacString(const std::string& pac_string)
+      : ProxyResolver(false /*expects_pac_bytes*/),
+        pac_string_(pac_string) {}
+
+  virtual int GetProxyForURL(const GURL& url,
+                             ProxyInfo* results,
+                             CompletionCallback* callback,
+                             RequestHandle* request,
+                             const BoundNetLog& net_log) {
+    results->UsePacString(pac_string_);
+    return OK;
+  }
+
+  virtual void CancelRequest(RequestHandle request) {
+    NOTREACHED();
+  }
+
+  virtual int SetPacScript(
+      const scoped_refptr<ProxyResolverScriptData>& pac_script,
+      CompletionCallback* callback) {
+    return OK;
+  }
+
+ private:
+  const std::string pac_string_;
+};
+
 // This factory creates V8ProxyResolvers with appropriate javascript bindings.
 class ProxyResolverFactoryForV8 : public ProxyResolverFactory {
  public:
@@ -360,6 +391,23 @@ ProxyService* ProxyService::CreateNull() {
                           NULL);
 }
 
+// static
+ProxyService* ProxyService::CreateFixedFromPacResult(
+    const std::string& pac_string) {
+
+  // We need the settings to contain an "automatic" setting, otherwise the
+  // ProxyResolver dependency we give it will never be used.
+  scoped_ptr<ProxyConfigService> proxy_config_service(
+      new ProxyConfigServiceFixed(ProxyConfig::CreateAutoDetect()));
+
+  scoped_ptr<ProxyResolver> proxy_resolver(
+      new ProxyResolverFromPacString(pac_string));
+
+  return new ProxyService(proxy_config_service.release(),
+                          proxy_resolver.release(),
+                          NULL);
+}
+
 int ProxyService::ResolveProxy(const GURL& raw_url,
                                ProxyInfo* result,
                                CompletionCallback* callback,
diff --git a/net/proxy/proxy_service.h b/net/proxy/proxy_service.h
index 3a3f7da..8f30327 100644
--- a/net/proxy/proxy_service.h
+++ b/net/proxy/proxy_service.h
@@ -178,8 +178,16 @@ class ProxyService : public base::RefCountedThreadSafe<ProxyService>,
 
   // Creates a proxy service that always fails to fetch the proxy configuration,
   // so it falls back to direct connect.
+  // TODO(eroman): Rename to CreateDirect().
   static ProxyService* CreateNull();
 
+  // This method is used by tests to create a ProxyService that returns a
+  // hardcoded proxy fallback list (|pac_string|) for every URL.
+  //
+  // |pac_string| is a list of proxy servers, in the format that a PAC script
+  // would return it. For example, "PROXY foobar:99; SOCKS fml:2; DIRECT"
+  static ProxyService* CreateFixedFromPacResult(const std::string& pac_string);
+
   // Creates a config service appropriate for this platform that fetches the
   // system proxy settings.
   static ProxyConfigService* CreateSystemProxyConfigService(
diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc
index 49f41ed..61630c3 100644
--- a/net/spdy/spdy_network_transaction_unittest.cc
+++ b/net/spdy/spdy_network_transaction_unittest.cc
@@ -3860,7 +3860,7 @@ TEST_P(SpdyNetworkTransactionTest, ProxyConnect) {
   NormalSpdyTransactionHelper helper(CreateGetRequest(),
                                      BoundNetLog(), GetParam());
   helper.session_deps().reset(new SpdySessionDependencies(
-      net::SpdyCreateFixedProxyService("myproxy:70")));
+      ProxyService::CreateFixedFromPacResult("PROXY myproxy:70")));
   helper.SetSession(SpdySessionDependencies::SpdyCreateSession(
       helper.session_deps().get()));
   helper.RunPreTestSetup();
@@ -3957,6 +3957,16 @@ TEST_P(SpdyNetworkTransactionTest, DirectConnectProxyReconnect) {
   // we can use the same pool in the second transaction.
   NormalSpdyTransactionHelper helper(CreateGetRequest(),
                                      BoundNetLog(), GetParam());
+
+  // Use a proxy service which returns a proxy fallback list from DIRECT to
+  // myproxy:70. For this test there will be no fallback, so it is equivalent
+  // to simply DIRECT. The reason for appending the second proxy is to verify
+  // that the session pool key used does is just "DIRECT".
+  helper.session_deps().reset(new SpdySessionDependencies(
+      ProxyService::CreateFixedFromPacResult("DIRECT; PROXY myproxy:70")));
+  helper.SetSession(SpdySessionDependencies::SpdyCreateSession(
+      helper.session_deps().get()));
+
   scoped_refptr<SpdySessionPool> spdy_session_pool =
       helper.session_deps()->spdy_session_pool;
   helper.RunPreTestSetup();
@@ -4000,10 +4010,10 @@ TEST_P(SpdyNetworkTransactionTest, DirectConnectProxyReconnect) {
 
   // Check that the SpdySession is still in the SpdySessionPool.
   HostPortPair host_port_pair("www.google.com", helper.port());
-  HostPortProxyPair pair(host_port_pair, "DIRECT");
-  EXPECT_TRUE(spdy_session_pool->HasSession(pair));
-  HostPortProxyPair nonexistent_pair(host_port_pair, "PROXY www.foo.com");
-  EXPECT_FALSE(spdy_session_pool->HasSession(nonexistent_pair));
+  HostPortProxyPair session_pool_key_direct(host_port_pair, "DIRECT");
+  EXPECT_TRUE(spdy_session_pool->HasSession(session_pool_key_direct));
+  HostPortProxyPair session_pool_key_proxy(host_port_pair, "PROXY www.foo.com");
+  EXPECT_FALSE(spdy_session_pool->HasSession(session_pool_key_proxy));
 
   // Set up data for the proxy connection.
   const char kConnect443[] = {"CONNECT www.google.com:443 HTTP/1.1\r\n"
@@ -4079,8 +4089,8 @@ TEST_P(SpdyNetworkTransactionTest, DirectConnectProxyReconnect) {
   request_proxy.url = GURL("http://www.google.com/foo.dat");
   request_proxy.load_flags = 0;
   scoped_ptr<SpdySessionDependencies> ssd_proxy(
-      new SpdySessionDependencies(net::SpdyCreateFixedProxyService(
-          "myproxy:70")));
+      new SpdySessionDependencies(
+          ProxyService::CreateFixedFromPacResult("PROXY myproxy:70")));
   // Ensure that this transaction uses the same SpdySessionPool.
   ssd_proxy->spdy_session_pool = spdy_session_pool;
   scoped_refptr<HttpNetworkSession> session_proxy =
diff --git a/net/spdy/spdy_test_util.cc b/net/spdy/spdy_test_util.cc
index f57fb18..5a0b884 100644
--- a/net/spdy/spdy_test_util.cc
+++ b/net/spdy/spdy_test_util.cc
@@ -745,14 +745,6 @@ int CombineFrames(const spdy::SpdyFrame** frames, int num_frames,
   return total_len;
 }
 
-// This creates a proxy for testing purposes.
-// |proxy| should be in the form "myproxy:70".
-ProxyService* SpdyCreateFixedProxyService(const std::string& proxy) {
-  net::ProxyConfig proxy_config;
-  proxy_config.proxy_rules().ParseFromString(proxy);
-  return ProxyService::CreateFixed(proxy_config);
-}
-
 const SpdyHeaderInfo make_spdy_header(spdy::SpdyControlType type) {
   const SpdyHeaderInfo kHeader = {
     type,                         // Kind = Syn
diff --git a/net/spdy/spdy_test_util.h b/net/spdy/spdy_test_util.h
index 79010e23..c2e15e0 100644
--- a/net/spdy/spdy_test_util.h
+++ b/net/spdy/spdy_test_util.h
@@ -379,10 +379,6 @@ class SpdyURLRequestContext : public URLRequestContext {
   scoped_refptr<SpdySessionPool> spdy_session_pool_;
 };
 
-// This creates a proxy for testing purposes.
-// |proxy| should be in the form "myproxy:70".
-ProxyService* SpdyCreateFixedProxyService(const std::string& proxy);
-
 const SpdyHeaderInfo make_spdy_header(spdy::SpdyControlType type);
 }  // namespace net