Include the scheme in the key for QUIC the session map.

Include the scheme in the key for QUIC the session map.

BUG=350533
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255904

[ramant (doing other things), 2014-03-07 23:42:56]

lgtm

https://codereview.chromium.org/190063008/diff/1/net/quic/quic_stream_factory...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/190063008/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory_test.cc:382: &factory_, host_port_proxy_pair_,
is_https_));
nit: 4 spaces indentation on line# 382

[Ryan Hamilton, 2014-03-08 00:58:34]

Thanks, Raman.

eroman: can you review net-internals?

https://codereview.chromium.org/190063008/diff/1/net/quic/quic_stream_factory...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/190063008/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory_test.cc:382: &factory_, host_port_proxy_pair_,
is_https_));
On 2014/03/07 23:42:56, ramant wrote:
> nit: 4 spaces indentation on line# 382

Done.

[eroman, 2014-03-08 01:02:42]

lgtm

[eroman, 2014-03-08 01:04:25]

https://codereview.chromium.org/190063008/diff/60001/net/quic/quic_stream_fac...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/190063008/diff/60001/net/quic/quic_stream_fac...
net/quic/quic_stream_factory_test.cc:167: EXPECT_EQ(NULL,
CreateIfSessionExists(destination, net_log_).get());
BTW I seem to remember having issues with this pattern (where on some platforms
NULL is not understood to be a pointer type and the template magic fails...

If so, try EXPECT_FALSE() instead

[Ryan Hamilton, 2014-03-08 01:48:26]

On 2014/03/08 01:04:25, eroman wrote:
>
https://codereview.chromium.org/190063008/diff/60001/net/quic/quic_stream_fac...
> File net/quic/quic_stream_factory_test.cc (right):
> 
>
https://codereview.chromium.org/190063008/diff/60001/net/quic/quic_stream_fac...
> net/quic/quic_stream_factory_test.cc:167: EXPECT_EQ(NULL,
> CreateIfSessionExists(destination, net_log_).get());
> BTW I seem to remember having issues with this pattern (where on some
platforms
> NULL is not understood to be a pointer type and the template magic fails...
> 
> If so, try EXPECT_FALSE() instead

Good point!  I've had that experience before too.  I based the new test off of
an existing test.  I wonder why it's working?  *boggle*

Thanks for the review!

[Ryan Hamilton, 2014-03-08 01:48:27]

On 2014/03/08 01:04:25, eroman wrote:
>
https://codereview.chromium.org/190063008/diff/60001/net/quic/quic_stream_fac...
> File net/quic/quic_stream_factory_test.cc (right):
> 
>
https://codereview.chromium.org/190063008/diff/60001/net/quic/quic_stream_fac...
> net/quic/quic_stream_factory_test.cc:167: EXPECT_EQ(NULL,
> CreateIfSessionExists(destination, net_log_).get());
> BTW I seem to remember having issues with this pattern (where on some
platforms
> NULL is not understood to be a pointer type and the template magic fails...
> 
> If so, try EXPECT_FALSE() instead

Good point!  I've had that experience before too.  I based the new test off of
an existing test.  I wonder why it's working?  *boggle*

Thanks for the review!

[Ryan Hamilton, 2014-03-08 01:48:39]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-03-08 11:18:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/190063008/60001

[commit-bot: I haz the power, 2014-03-08 13:20:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-08 13:20:40]

Retried try job too often on win_x64_rel for step(s) base_unittests,
chrome_elf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[Ryan Hamilton, 2014-03-08 14:35:04]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-03-08 14:38:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/190063008/60001

[commit-bot: I haz the power, 2014-03-08 15:11:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-08 15:11:11]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Ryan Hamilton, 2014-03-08 15:33:23]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-03-08 15:33:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/190063008/60001

[commit-bot: I haz the power, 2014-03-08 16:27:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-08 16:27:28]

Retried try job too often on mac_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Ryan Hamilton, 2014-03-09 18:04:27]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-03-09 18:04:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/190063008/80001

[commit-bot: I haz the power, 2014-03-09 18:04:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-09 18:04:46]

Failed to apply patch for net/quic/quic_stream_factory.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file net/quic/quic_stream_factory.cc
  Hunk #16 FAILED at 497.
  Hunk #17 succeeded at 551 (offset 3 lines).
  Hunk #18 succeeded at 589 (offset 3 lines).
  Hunk #19 succeeded at 601 (offset 3 lines).
  Hunk #20 succeeded at 661 (offset 3 lines).
  Hunk #21 succeeded at 688 (offset 3 lines).
  Hunk #22 succeeded at 717 (offset 3 lines).
  Hunk #23 succeeded at 742 (offset 3 lines).
  Hunk #24 succeeded at 768 (offset 3 lines).
  1 out of 24 hunks FAILED -- saving rejects to file
net/quic/quic_stream_factory.cc.rej

Patch:       net/quic/quic_stream_factory.cc
Index: net/quic/quic_stream_factory.cc
diff --git a/net/quic/quic_stream_factory.cc b/net/quic/quic_stream_factory.cc
index
f445cd3e8f7a07579cc0fe7e5cab6cf3fa55ea5f..08effdffc2a2170fd840995908dafba635a666f4
100644
--- a/net/quic/quic_stream_factory.cc
+++ b/net/quic/quic_stream_factory.cc
@@ -38,6 +38,57 @@ using std::vector;
 
 namespace net {
 
+QuicStreamFactory::SessionKey::SessionKey() {}
+
+QuicStreamFactory::SessionKey::SessionKey(
+    HostPortProxyPair host_port_proxy_pair,
+    bool is_https)
+    : host_port_proxy_pair(host_port_proxy_pair),
+      is_https(is_https) {}
+
+QuicStreamFactory::SessionKey::~SessionKey() {}
+
+bool QuicStreamFactory::SessionKey::operator<(
+    const QuicStreamFactory::SessionKey &other) const {
+  if (!host_port_proxy_pair.first.Equals(other.host_port_proxy_pair.first)) {
+    return host_port_proxy_pair.first < other.host_port_proxy_pair.first;
+  }
+  if (!(host_port_proxy_pair.second == other.host_port_proxy_pair.second)) {
+    return host_port_proxy_pair.second < other.host_port_proxy_pair.second;
+  }
+  return is_https < other.is_https;
+}
+
+bool QuicStreamFactory::SessionKey::operator==(
+    const QuicStreamFactory::SessionKey& other) const {
+  return is_https == other.is_https &&
+      host_port_proxy_pair.second == other.host_port_proxy_pair.second &&
+      host_port_proxy_pair.first.Equals(other.host_port_proxy_pair.first);
+};
+
+QuicStreamFactory::IpAliasKey::IpAliasKey() {}
+
+QuicStreamFactory::IpAliasKey::IpAliasKey(IPEndPoint ip_endpoint,
+                                          bool is_https)
+    : ip_endpoint(ip_endpoint),
+      is_https(is_https) {}
+
+QuicStreamFactory::IpAliasKey::~IpAliasKey() {}
+
+bool QuicStreamFactory::IpAliasKey::operator<(
+    const QuicStreamFactory::IpAliasKey& other) const {
+  if (!(ip_endpoint == other.ip_endpoint)) {
+    return ip_endpoint < other.ip_endpoint;
+  }
+  return is_https < other.is_https;
+}
+
+bool QuicStreamFactory::IpAliasKey::operator==(
+    const QuicStreamFactory::IpAliasKey& other) const {
+  return is_https == other.is_https &&
+      ip_endpoint == other.ip_endpoint;
+};
+
 // Responsible for creating a new QUIC session to the specified server, and
 // for notifying any associated requests when complete.
 class QuicStreamFactory::Job {
@@ -66,8 +117,8 @@ class QuicStreamFactory::Job {
     return callback_;
   }
 
-  const HostPortProxyPair& host_port_proxy_pair() const {
-    return host_port_proxy_pair_;
+  const SessionKey session_key() const {
+    return session_key_;
   }
 
  private:
@@ -82,8 +133,8 @@ class QuicStreamFactory::Job {
 
   QuicStreamFactory* factory_;
   SingleRequestHostResolver host_resolver_;
-  const HostPortProxyPair host_port_proxy_pair_;
   bool is_https_;
+  SessionKey session_key_;
   bool is_post_;
   CertVerifier* cert_verifier_;
   const BoundNetLog net_log_;
@@ -102,8 +153,8 @@ QuicStreamFactory::Job::Job(QuicStreamFactory* factory,
                             const BoundNetLog& net_log)
     : factory_(factory),
       host_resolver_(host_resolver),
-      host_port_proxy_pair_(host_port_proxy_pair),
       is_https_(is_https),
+      session_key_(host_port_proxy_pair, is_https),
       is_post_(method == "POST"),
       cert_verifier_(cert_verifier),
       net_log_(net_log),
@@ -159,7 +210,7 @@ void QuicStreamFactory::Job::OnIOComplete(int rv) {
 int QuicStreamFactory::Job::DoResolveHost() {
   io_state_ = STATE_RESOLVE_HOST_COMPLETE;
   return host_resolver_.Resolve(
-      HostResolver::RequestInfo(host_port_proxy_pair_.first),
+      HostResolver::RequestInfo(session_key_.host_port_proxy_pair.first),
       DEFAULT_PRIORITY,
       &address_list_,
       base::Bind(&QuicStreamFactory::Job::OnIOComplete,
base::Unretained(this)),
@@ -170,11 +221,11 @@ int QuicStreamFactory::Job::DoResolveHostComplete(int rv)
{
   if (rv != OK)
     return rv;
 
-  DCHECK(!factory_->HasActiveSession(host_port_proxy_pair_));
+  DCHECK(!factory_->HasActiveSession(session_key_));
 
   // Inform the factory of this resolution, which will set up
   // a session alias, if possible.
-  if (factory_->OnResolution(host_port_proxy_pair_, address_list_)) {
+  if (factory_->OnResolution(session_key_, address_list_)) {
     return OK;
   }
 
@@ -199,8 +250,8 @@ int QuicStreamRequest::Request(const HostPortProxyPair&
host_port_proxy_pair,
   DCHECK(!stream_);
   DCHECK(callback_.is_null());
   DCHECK(factory_);
-  int rv = factory_->Create(
-      host_port_proxy_pair, is_https, method, cert_verifier, net_log, this);
+  int rv = factory_->Create(host_port_proxy_pair, is_https,
+                            method, cert_verifier, net_log, this);
   if (rv == ERR_IO_PENDING) {
     host_port_proxy_pair_ = host_port_proxy_pair;
     is_https_ = is_https;
@@ -233,7 +284,7 @@ scoped_ptr<QuicHttpStream>
QuicStreamRequest::ReleaseStream() {
 int QuicStreamFactory::Job::DoConnect() {
   io_state_ = STATE_CONNECT_COMPLETE;
 
-  int rv = factory_->CreateSession(host_port_proxy_pair_, is_https_,
+  int rv = factory_->CreateSession(session_key_.host_port_proxy_pair,
is_https_,
       cert_verifier_, address_list_, net_log_, &session_);
   if (rv != OK) {
     DCHECK(rv != ERR_IO_PENDING);
@@ -256,17 +307,17 @@ int QuicStreamFactory::Job::DoConnectComplete(int rv) {
   if (rv != OK)
     return rv;
 
-  DCHECK(!factory_->HasActiveSession(host_port_proxy_pair_));
+  DCHECK(!factory_->HasActiveSession(session_key_));
   // There may well now be an active session for this IP.  If so, use the
   // existing session instead.
   AddressList address(session_->connection()->peer_address());
-  if (factory_->OnResolution(host_port_proxy_pair_, address)) {
+  if (factory_->OnResolution(session_key_, address)) {
     session_->connection()->SendConnectionClose(QUIC_NO_ERROR);
     session_ = NULL;
     return OK;
   }
 
-  factory_->ActivateSession(host_port_proxy_pair_, session_);
+  factory_->ActivateSession(session_key_, session_);
 
   return OK;
 }
@@ -319,13 +370,14 @@ int QuicStreamFactory::Create(const HostPortProxyPair&
host_port_proxy_pair,
                               CertVerifier* cert_verifier,
                               const BoundNetLog& net_log,
                               QuicStreamRequest* request) {
-  if (HasActiveSession(host_port_proxy_pair)) {
-    request->set_stream(CreateIfSessionExists(host_port_proxy_pair, net_log));
+  SessionKey session_key(host_port_proxy_pair, is_https);
+  if (HasActiveSession(session_key)) {
+    request->set_stream(CreateIfSessionExists(session_key, net_log));
     return OK;
   }
 
-  if (HasActiveJob(host_port_proxy_pair)) {
-    Job* job = active_jobs_[host_port_proxy_pair];
+  if (HasActiveJob(session_key)) {
+    Job* job = active_jobs_[session_key];
     active_requests_[request] = job;
     job_requests_map_[job].insert(request);
     return ERR_IO_PENDING;
@@ -333,8 +385,7 @@ int QuicStreamFactory::Create(const HostPortProxyPair&
host_port_proxy_pair,
 
   // Create crypto config and start the process of loading QUIC server
   // information from disk cache.
-  QuicCryptoClientConfig* crypto_config =
-      GetOrCreateCryptoConfig(host_port_proxy_pair);
+  QuicCryptoClientConfig* crypto_config = GetOrCreateCryptoConfig(session_key);
   DCHECK(crypto_config);
 
   scoped_ptr<Job> job(new Job(this, host_resolver_, host_port_proxy_pair,
@@ -345,32 +396,33 @@ int QuicStreamFactory::Create(const HostPortProxyPair&
host_port_proxy_pair,
   if (rv == ERR_IO_PENDING) {
     active_requests_[request] = job.get();
     job_requests_map_[job.get()].insert(request);
-    active_jobs_[host_port_proxy_pair] = job.release();
+    active_jobs_[session_key] = job.release();
   }
   if (rv == OK) {
-    DCHECK(HasActiveSession(host_port_proxy_pair));
-    request->set_stream(CreateIfSessionExists(host_port_proxy_pair, net_log));
+    DCHECK(HasActiveSession(session_key));
+    request->set_stream(CreateIfSessionExists(session_key, net_log));
   }
   return rv;
 }
 
 bool QuicStreamFactory::OnResolution(
-    const HostPortProxyPair& host_port_proxy_pair,
+    const SessionKey& session_key,
     const AddressList& address_list) {
-  DCHECK(!HasActiveSession(host_port_proxy_pair));
+  DCHECK(!HasActiveSession(session_key));
   for (size_t i = 0; i < address_list.size(); ++i) {
     const IPEndPoint& address = address_list[i];
-    if (!ContainsKey(ip_aliases_, address))
+    const IpAliasKey ip_alias_key(address, session_key.is_https);
+    if (!ContainsKey(ip_aliases_, ip_alias_key))
       continue;
 
-    const SessionSet& sessions = ip_aliases_[address];
+    const SessionSet& sessions = ip_aliases_[ip_alias_key];
     for (SessionSet::const_iterator i = sessions.begin();
          i != sessions.end(); ++i) {
       QuicClientSession* session = *i;
-      if (!session->CanPool(host_port_proxy_pair.first.host()))
+      if (!session->CanPool(session_key.host_port_proxy_pair.first.host()))
         continue;
-      active_sessions_[host_port_proxy_pair] = session;
-      session_aliases_[session].insert(host_port_proxy_pair);
+      active_sessions_[session_key] = session;
+      session_aliases_[session].insert(session_key);
       return true;
     }
   }
@@ -384,8 +436,8 @@ void QuicStreamFactory::OnJobComplete(Job* job, int rv) {
    …
(message too large)

[Ryan Hamilton, 2014-03-09 18:24:24]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-03-09 18:24:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/190063008/110001

[commit-bot: I haz the power, 2014-03-10 07:27:32]

Change committed as 255904