Changed SPDY's ip connection pooling logic to only add the used IP,

rather than all the IPs for the hostname, to the SpdyAliasMap.

This is to fix the following problem:

When we establish a new SPDY connection, we add all IP addresses for
that hostname to our alias map. Therefore, if we connect to host foo
which maps to IPs X, Y, and Z, but the actual IP we connect to is X,
then if we try to get a SPDY connection to host bar which maps to
IPs Y and Z, then currently we will share the SPDY connection to host
foo on IP X.

BUG=89094
R=willchan
TEST=spdy unit tests
Review URL: http://codereview.chromium.org/7349023

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

6 files changed, 91 insertions, 40 deletions

diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 541a4f9b..863693a 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -8702,6 +8702,37 @@ TEST_F(HttpNetworkTransactionTest, ClientAuthCertCache_Proxy_Fail) {
   }
 }
 
+void IPPoolingPreloadHostCache(MockCachingHostResolver* host_resolver,
+                               SpdySessionPoolPeer* pool_peer) {
+  const int kTestPort = 443;
+  struct TestHosts {
+    std::string name;
+    std::string iplist;
+  } test_hosts[] = {
+    { "www.google.com", "127.0.0.1"},
+  };
+
+  // Preload cache entries into HostCache.
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_hosts); i++) {
+    host_resolver->rules()->AddIPLiteralRule(test_hosts[i].name,
+        test_hosts[i].iplist, "");
+
+    AddressList addresses;
+    // This test requires that the HostResolver cache be populated.  Normal
+    // code would have done this already, but we do it manually.
+    HostResolver::RequestInfo info(HostPortPair(test_hosts[i].name, kTestPort));
+    host_resolver->Resolve(
+        info, &addresses, NULL, NULL, BoundNetLog());
+
+    // Setup a HostPortProxyPair
+    HostPortProxyPair pair = HostPortProxyPair(
+        HostPortPair(test_hosts[i].name, kTestPort), ProxyServer::Direct());
+
+    const addrinfo* address = addresses.head();
+    pool_peer->AddAlias(address, pair);
+  }
+}
+
 TEST_F(HttpNetworkTransactionTest, UseIPConnectionPooling) {
   HttpStreamFactory::set_use_alternate_protocols(true);
   HttpStreamFactory::set_next_protos(kExpectedNPNString);
@@ -8780,6 +8811,8 @@ TEST_F(HttpNetworkTransactionTest, UseIPConnectionPooling) {
   AddressList ignored;
   host_resolver.Resolve(resolve_info, &ignored, NULL, NULL, BoundNetLog());
 
+  IPPoolingPreloadHostCache(&host_resolver, &pool_peer);
+
   HttpRequestInfo request2;
   request2.method = "GET";
   request2.url = GURL("https://www.gmail.com/");
@@ -8839,6 +8872,10 @@ class OneTimeCachingHostResolver : public net::HostResolver {
     return host_resolver_.RemoveObserver(observer);
   }
 
+  MockCachingHostResolver* GetMockHostResolver() {
+    return &host_resolver_;
+  }
+
  private:
   MockCachingHostResolver host_resolver_;
   const HostPortPair host_port_;
@@ -8928,6 +8965,8 @@ TEST_F(HttpNetworkTransactionTest,
   request2.load_flags = 0;
   HttpNetworkTransaction trans2(session);
 
+  IPPoolingPreloadHostCache(host_resolver.GetMockHostResolver(), &pool_peer);
+
   rv = trans2.Start(&request2, &callback, BoundNetLog());
   EXPECT_EQ(ERR_IO_PENDING, rv);
   EXPECT_EQ(OK, callback.WaitForResult());
diff --git a/net/socket/ssl_client_socket_pool_unittest.cc b/net/socket/ssl_client_socket_pool_unittest.cc
index d04ddef..9df1376 100644
--- a/net/socket/ssl_client_socket_pool_unittest.cc
+++ b/net/socket/ssl_client_socket_pool_unittest.cc
@@ -14,6 +14,7 @@
 #include "net/base/mock_host_resolver.h"
 #include "net/base/net_errors.h"
 #include "net/base/ssl_config_service_defaults.h"
+#include "net/base/sys_addrinfo.h"
 #include "net/base/test_certificate_data.h"
 #include "net/base/test_completion_callback.h"
 #include "net/http/http_auth_handler_factory.h"
@@ -26,6 +27,7 @@
 #include "net/socket/socket_test_util.h"
 #include "net/spdy/spdy_session.h"
 #include "net/spdy/spdy_session_pool.h"
+#include "net/spdy/spdy_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -655,10 +657,11 @@ TEST_F(SSLClientSocketPoolTest, IPPooling) {
     std::string name;
     std::string iplist;
     HostPortProxyPair pair;
+    AddressList addresses;
   } test_hosts[] = {
-    { "www.webkit.org",    "192.168.0.1,192.168.0.5" },
+    { "www.webkit.org",    "192.0.2.33,192.168.0.1,192.168.0.5" },
     { "code.google.com",   "192.168.0.2,192.168.0.3,192.168.0.5" },
-    { "js.webkit.org",     "192.168.0.4,192.168.0.5" },
+    { "js.webkit.org",     "192.168.0.4,192.168.0.1,192.0.2.33" },
   };
 
   host_resolver_.set_synchronous_mode(true);
@@ -669,8 +672,8 @@ TEST_F(SSLClientSocketPoolTest, IPPooling) {
     // This test requires that the HostResolver cache be populated.  Normal
     // code would have done this already, but we do it manually.
     HostResolver::RequestInfo info(HostPortPair(test_hosts[i].name, kTestPort));
-    AddressList result;
-    host_resolver_.Resolve(info, &result, NULL, NULL, BoundNetLog());
+    host_resolver_.Resolve(info, &test_hosts[i].addresses, NULL, NULL,
+                           BoundNetLog());
 
     // Setup a HostPortProxyPair
     test_hosts[i].pair = HostPortProxyPair(
@@ -712,6 +715,12 @@ TEST_F(SSLClientSocketPoolTest, IPPooling) {
   EXPECT_EQ(SSLClientSocket::NextProtoFromString(proto),
             SSLClientSocket::kProtoSPDY2);
 
+  // TODO(rtenneti): MockClientSocket::GetPeerAddress return's 0 as the port
+  // number. Fix it to return port 80 and then use GetPeerAddress to AddAlias.
+  const addrinfo* address = test_hosts[0].addresses.head();
+  SpdySessionPoolPeer pool_peer(session_->spdy_session_pool());
+  pool_peer.AddAlias(address, test_hosts[0].pair);
+
   scoped_refptr<SpdySession> spdy_session;
   rv = session_->spdy_session_pool()->GetSpdySessionFromSocket(
     test_hosts[0].pair, handle.release(), BoundNetLog(), 0,
diff --git a/net/spdy/spdy_session_pool.cc b/net/spdy/spdy_session_pool.cc
index b84d6ad..e8324f4 100644
--- a/net/spdy/spdy_session_pool.cc
+++ b/net/spdy/spdy_session_pool.cc
@@ -150,6 +150,19 @@ net::Error SpdySessionPool::GetSpdySessionFromSocket(
       make_scoped_refptr(new NetLogSourceParameter(
           "session", (*spdy_session)->net_log().source())));
 
+  // We have a new session.  Lookup 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 (g_enable_ip_pooling  && host_port_proxy_pair.second.is_direct()) {
+    AddressList addresses;
+    if (connection->socket()->GetPeerAddress(&addresses) == OK) {
+      const addrinfo* address = addresses.head();
+      AddAlias(address, host_port_proxy_pair);
+    }
+  }
+
   // Now we can initialize the session with the SSL socket.
   return (*spdy_session)->InitializeWithSocket(connection, is_secure,
                                                certificate_error_code);
@@ -302,16 +315,6 @@ SpdySessionPool::SpdySessionList*
   DCHECK(sessions_.find(pair) == sessions_.end());
   SpdySessionPool::SpdySessionList* list = new SpdySessionList();
   sessions_[pair] = list;
-
-  // We have a new session.  Lookup the IP addresses for this session so that
-  // we can match future Sessions (potentially to different domains) which can
-  // potentially be pooled with this one.
-  if (g_enable_ip_pooling) {
-    AddressList addresses;
-    if (LookupAddresses(host_port_proxy_pair, &addresses))
-      AddAliases(addresses, host_port_proxy_pair);
-  }
-
   return list;
 }
 
@@ -351,26 +354,13 @@ bool SpdySessionPool::LookupAddresses(const HostPortProxyPair& pair,
   return rv == OK;
 }
 
-void SpdySessionPool::AddAliases(const AddressList& addresses,
-                                 const HostPortProxyPair& pair) {
-  // Note:  it is possible to think of strange overlapping sets of ip addresses
-  // for hosts such that a new session can override the alias for an IP
-  // address that was previously aliased to a different host.  This is probably
-  // undesirable, but seemingly unlikely and complicated to fix.
-  //    Example:
-  //      host1 = 1.1.1.1, 1.1.1.4
-  //      host2 = 1.1.1.4, 1.1.1.5
-  //      host3 = 1.1.1.3, 1.1.1.5
-  //      Creating session1 (to host1), creates an alias for host2 to host1.
-  //      Creating session2 (to host3), overrides the alias for host2 to host3.
-
-  const addrinfo* address = addresses.head();
-  while (address) {
-    IPEndPoint endpoint;
-    endpoint.FromSockAddr(address->ai_addr, address->ai_addrlen);
-    aliases_[endpoint] = pair;
-    address = address->ai_next;
-  }
+void SpdySessionPool::AddAlias(const addrinfo* address,
+                               const HostPortProxyPair& pair) {
+  DCHECK(g_enable_ip_pooling);
+  DCHECK(address);
+  IPEndPoint endpoint;
+  endpoint.FromSockAddr(address->ai_addr, address->ai_addrlen);
+  aliases_[endpoint] = pair;
 }
 
 void SpdySessionPool::RemoveAliases(const HostPortProxyPair& pair) {
diff --git a/net/spdy/spdy_session_pool.h b/net/spdy/spdy_session_pool.h
index 74b8874..366ede4 100644
--- a/net/spdy/spdy_session_pool.h
+++ b/net/spdy/spdy_session_pool.h
@@ -163,8 +163,8 @@ class NET_API SpdySessionPool
   bool LookupAddresses(const HostPortProxyPair& pair,
                        AddressList* addresses) const;
 
-  // Add a set of |addresses| as IP-equivalent addresses for |pair|.
-  void AddAliases(const AddressList& addresses, const HostPortProxyPair& pair);
+  // Add |endpoint| as an IP-equivalent address for |pair|.
+  void AddAlias(const addrinfo* address, const HostPortProxyPair& pair);
 
   // Remove all aliases for |pair| from the aliases table.
   void RemoveAliases(const HostPortProxyPair& pair);
diff --git a/net/spdy/spdy_session_unittest.cc b/net/spdy/spdy_session_unittest.cc
index 1c1543a..351b8ed 100644
--- a/net/spdy/spdy_session_unittest.cc
+++ b/net/spdy/spdy_session_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "net/spdy/spdy_session.h"
 
+#include "net/base/ip_endpoint.h"
 #include "net/spdy/spdy_io_buffer.h"
 #include "net/spdy/spdy_session_pool.h"
 #include "net/spdy/spdy_stream.h"
@@ -416,9 +417,10 @@ void IPPoolingTest(bool clean_via_close_current_sessions) {
     std::string name;
     std::string iplist;
     HostPortProxyPair pair;
+    AddressList addresses;
   } test_hosts[] = {
-    { "www.foo.com",    "192.168.0.1,192.168.0.5" },
-    { "images.foo.com", "192.168.0.2,192.168.0.3,192.168.0.5" },
+    { "www.foo.com",    "192.0.2.33,192.168.0.1,192.168.0.5" },
+    { "images.foo.com", "192.168.0.2,192.168.0.3,192.168.0.5,192.0.2.33" },
     { "js.foo.com",     "192.168.0.4,192.168.0.3" },
   };
 
@@ -431,9 +433,8 @@ void IPPoolingTest(bool clean_via_close_current_sessions) {
     // This test requires that the HostResolver cache be populated.  Normal
     // code would have done this already, but we do it manually.
     HostResolver::RequestInfo info(HostPortPair(test_hosts[i].name, kTestPort));
-    AddressList result;
     session_deps.host_resolver->Resolve(
-        info, &result, NULL, NULL, BoundNetLog());
+        info, &test_hosts[i].addresses, NULL, NULL, BoundNetLog());
 
     // Setup a HostPortProxyPair
     test_hosts[i].pair = HostPortProxyPair(
@@ -477,6 +478,12 @@ void IPPoolingTest(bool clean_via_close_current_sessions) {
                              BoundNetLog()));
   EXPECT_EQ(OK, session->InitializeWithSocket(connection.release(), false, OK));
 
+  // TODO(rtenneti): MockClientSocket::GetPeerAddress return's 0 as the port
+  // number. Fix it to return port 80 and then use GetPeerAddress to AddAlias.
+  const addrinfo* address = test_hosts[0].addresses.head();
+  SpdySessionPoolPeer pool_peer(spdy_session_pool);
+  pool_peer.AddAlias(address, test_hosts[0].pair);
+
   // Flush the SpdySession::OnReadComplete() task.
   MessageLoop::current()->RunAllPending();
 
diff --git a/net/spdy/spdy_test_util.h b/net/spdy/spdy_test_util.h
index 5d0a27f..1ccdb37 100644
--- a/net/spdy/spdy_test_util.h
+++ b/net/spdy/spdy_test_util.h
@@ -8,9 +8,11 @@
 
 #include "base/basictypes.h"
 #include "net/base/cert_verifier.h"
+#include "net/base/host_port_pair.h"
 #include "net/base/mock_host_resolver.h"
 #include "net/base/request_priority.h"
 #include "net/base/ssl_config_service_defaults.h"
+#include "net/base/sys_addrinfo.h"
 #include "net/http/http_auth_handler_factory.h"
 #include "net/http/http_cache.h"
 #include "net/http/http_network_session.h"
@@ -376,6 +378,10 @@ class SpdySessionPoolPeer {
   explicit SpdySessionPoolPeer(SpdySessionPool* pool)
       : pool_(pool) {}
 
+  void AddAlias(const addrinfo* address, const HostPortProxyPair& pair) {
+    pool_->AddAlias(address, pair);
+  }
+
   void RemoveSpdySession(const scoped_refptr<SpdySession>& session) {
     pool_->Remove(session);
   }