ABANDONED Handle multiple AlternateProtocols for each HostPortPair.

net/quic/quic_stream_factory.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/quic_stream_factory.h"
 
 #include <set>
 
 #include "base/cpu.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 669 matching lines @@
     active_requests_[request] = server_id;
     job_requests_map_[server_id].insert(request);
     return ERR_IO_PENDING;
   }
 
   // TODO(rtenneti): |task_runner_| is used by the Job. Initialize task_runner_
   // in the constructor after WebRequestActionWithThreadsTest.* tests are fixed.
   if (!task_runner_)
     task_runner_ = base::MessageLoop::current()->message_loop_proxy().get();
 
-  QuicServerInfo* quic_server_info = nullptr;
-  if (quic_server_info_factory_) {
-    bool load_from_disk_cache = !disable_disk_cache_;
-    if (http_server_properties_) {
-      const AlternateProtocolMap& alternate_protocol_map =
-          http_server_properties_->alternate_protocol_map();
-      AlternateProtocolMap::const_iterator it =
-          alternate_protocol_map.Peek(server_id.host_port_pair());
-      if (it == alternate_protocol_map.end() || it->second.protocol != QUIC) {
-        // If there is no entry for QUIC, consider that as a new server and
-        // don't wait for Cache thread to load the data for that server.
+  bool was_alternate_protocol_recently_broken = false;
+  bool load_from_disk_cache = !disable_disk_cache_;
+  if (http_server_properties_) {
+    const AlternateProtocolMap& alternate_protocol_map =
+        http_server_properties_->alternate_protocol_map();
+    AlternateProtocolMap::const_iterator it =
+        alternate_protocol_map.Peek(server_id.host_port_pair());
+    // If there is no entry for QUIC, consider that as a new server and
+    // don't wait for Cache thread to load the data for that server.
+    if (it == alternate_protocol_map.end()) {
+      load_from_disk_cache = false;
+    } else {
+      AlternateProtocols alternate_protocols = it->second;
+      AlternateProtocols::iterator alternate;
+      for (alternate = alternate_protocols.begin();
+           alternate != alternate_protocols.end(); ++alternate) {
+        if (alternate->protocol == QUIC) {
+          was_alternate_protocol_recently_broken =
+              WasAlternateProtocolRecentlyBroken(server_id, *alternate);
+          break;
+        }
+      }
+      if (alternate == alternate_protocols.end()) {
         load_from_disk_cache = false;
       }
     }
-    if (load_from_disk_cache && CryptoConfigCacheIsEmpty(server_id)) {
-      quic_server_info = quic_server_info_factory_->GetForServer(server_id);
-    }
+  }

[Ryan Hamilton, 2015/02/27 18:20:48]

There's a lot going on here, I think it might make sense to put this into a
helper method.

[Bence, 2015/02/27 19:31:41]

Can I do that later?  A lot of things will change soon.  For example, we will
not be fishing for the alternative service here, but construct one from
host_port_pair.  Also, I don't like messing around with
HttpServerProperties::alternate_protocol_map_ outside the class.  So the method
you're proposing will really have to live in HttpServerProperties.

+
+  QuicServerInfo* quic_server_info = nullptr;
+  if (quic_server_info_factory_ && load_from_disk_cache &&
+      CryptoConfigCacheIsEmpty(server_id)) {
+    quic_server_info = quic_server_info_factory_->GetForServer(server_id);
   }
 
   scoped_ptr<Job> job(new Job(this, host_resolver_, host_port_pair, is_https,
-                              WasAlternateProtocolRecentlyBroken(server_id),
+                              was_alternate_protocol_recently_broken,
                               privacy_mode, method == "POST" /* is_post */,
                               quic_server_info, net_log));
   int rv = job->Run(base::Bind(&QuicStreamFactory::OnJobComplete,
                                base::Unretained(this), job.get()));
   if (rv == ERR_IO_PENDING) {
     active_requests_[request] = server_id;
     job_requests_map_[server_id].insert(request);
     active_jobs_[server_id].insert(job.release());
     return rv;
   }
   if (rv == OK) {
     DCHECK(HasActiveSession(server_id));
     request->set_stream(CreateIfSessionExists(server_id, net_log));
   }
   return rv;
 }
 
 void QuicStreamFactory::CreateAuxilaryJob(const QuicServerId server_id,
                                           bool is_post,
                                           const BoundNetLog& net_log) {
-  Job* aux_job = new Job(this, host_resolver_, server_id.host_port_pair(),
-                         server_id.is_https(),
-                         WasAlternateProtocolRecentlyBroken(server_id),
-                         server_id.privacy_mode(), is_post, nullptr, net_log);
+  const AlternateProtocolInfo alternate_protocol(server_id.port(), QUIC, 0.0);

[Ryan Hamilton, 2015/02/27 18:20:48]

It seems like this should be done *in* WasAlternateProtocol broken, since from
the perspective of the QuicStreamFactory, there is only one alternate protocol
we're talking about. You could consider renaming the method to be
WasQuicRecentlyBroken, if you wish.

[Bence, 2015/02/27 19:31:41]

I think this just underlines the issue that we need to change
WasAlternateProtocolRecentlyBroken semantics to operate on the alternative
service, and not on the origin.  (Heck, we might not even know the origin in
this method.)

I like the WasQuicRecentlyBroken idea.

+  Job* aux_job = new Job(
+      this, host_resolver_, server_id.host_port_pair(), server_id.is_https(),
+      WasAlternateProtocolRecentlyBroken(server_id, alternate_protocol),
+      server_id.privacy_mode(), is_post, nullptr, net_log);
   active_jobs_[server_id].insert(aux_job);
   task_runner_->PostTask(FROM_HERE,
                          base::Bind(&QuicStreamFactory::Job::RunAuxilaryJob,
                                     aux_job->GetWeakPtr()));
 }
 
 bool QuicStreamFactory::OnResolution(
     const QuicServerId& server_id,
     const AddressList& address_list) {
   DCHECK(!HasActiveSession(server_id));
@@ skipping 476 matching lines @@
     return 0;
   const ServerNetworkStats* stats =
       http_server_properties_->GetServerNetworkStats(
           server_id.host_port_pair());
   if (stats == nullptr)
     return 0;
   return stats->srtt.InMicroseconds();
 }
 
 bool QuicStreamFactory::WasAlternateProtocolRecentlyBroken(
-    const QuicServerId& server_id) const {
+    const QuicServerId& server_id,
+    const AlternateProtocolInfo& alternate_protocol) const {
   return http_server_properties_ &&
          http_server_properties_->WasAlternateProtocolRecentlyBroken(
-             server_id.host_port_pair());
+             server_id.host_port_pair(), alternate_protocol);
 }
 
 bool QuicStreamFactory::CryptoConfigCacheIsEmpty(
     const QuicServerId& server_id) {
   QuicCryptoClientConfig::CachedState* cached =
       crypto_config_.LookupOrCreate(server_id);
   return cached->IsEmpty();
 }
 
 void QuicStreamFactory::InitializeCachedStateInCryptoConfig(
     const QuicServerId& server_id,
     const scoped_ptr<QuicServerInfo>& server_info) {
   // TODO(vadimt): Remove ScopedTracker below once crbug.com/422516 is fixed.
   tracked_objects::ScopedTracker tracking_profile1(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "422516 QuicStreamFactory::InitializeCachedStateInCryptoConfig1"));
 
   // |server_info| will be NULL, if a non-empty server config already exists in
   // the memory cache. This is a minor optimization to avoid LookupOrCreate.
   if (!server_info)
     return;
 
   QuicCryptoClientConfig::CachedState* cached =
       crypto_config_.LookupOrCreate(server_id);
   if (!cached->IsEmpty())
     return;
 
   if (http_server_properties_) {
     if (quic_supported_servers_at_startup_.empty()) {
-      for (const std::pair<const HostPortPair, AlternateProtocolInfo>&
-               key_value : http_server_properties_->alternate_protocol_map()) {
-        if (key_value.second.protocol == QUIC) {
-          quic_supported_servers_at_startup_.insert(key_value.first);
+      for (const std::pair<const HostPortPair, AlternateProtocols>& key_value :
+           http_server_properties_->alternate_protocol_map()) {
+        for (const AlternateProtocolInfo& alternate_protocol :
+             key_value.second) {
+          if (alternate_protocol.protocol == QUIC) {
+            quic_supported_servers_at_startup_.insert(key_value.first);
+            break;
+          }
         }
       }
     }
 
     // TODO(rtenneti): Delete the following histogram after collecting stats.
     // If the AlternateProtocolMap contained an entry for this host, check if
     // the disk cache contained an entry for it.
     if (ContainsKey(quic_supported_servers_at_startup_,
                     server_id.host_port_pair())) {
       UMA_HISTOGRAM_BOOLEAN(
@@ skipping 36 matching lines @@
                                                    network_stats);
     return;
   }
 
   UMA_HISTOGRAM_COUNTS("Net.QuicHandshakeNotConfirmedNumPacketsReceived",
                        stats.packets_received);
 
   if (!session_was_active)
     return;
 
+  // A QUIC alternate job was started iff there is a non-broken QUIC alternate
+  // protocol for the server.

[Ryan Hamilton, 2015/02/27 18:20:48]

I don't understand this comment.

[Bence, 2015/02/27 19:31:41]

I am substituting new lines 1350--1359 for old lines 1327--1330, this is what
I'm explaining here.

Hey, this all have to be scratched, since we will not know what server (origin)
was.

   const HostPortPair& server = server_id.host_port_pair();
-  // Don't try to change the alternate-protocol state, if the
-  // alternate-protocol state is unknown.
-  const AlternateProtocolInfo alternate =
-      http_server_properties_->GetAlternateProtocol(server);
-  if (alternate.protocol == UNINITIALIZED_ALTERNATE_PROTOCOL)
+  const AlternateProtocols alternate_protocols =
+      http_server_properties_->GetAlternateProtocols(server);
+  AlternateProtocols::const_iterator alternate;
+  for (alternate = alternate_protocols.begin();

[Ryan Hamilton, 2015/02/27 18:20:48]

range-base for loop, please.

[Bence, 2015/02/27 19:31:41]

Note that in this case I couldn't do it, because range-based loops declare the
running variable in loop scope only, I couldn't do the check on line 1357.

+       alternate != alternate_protocols.end(); ++alternate) {
+    if (!alternate->is_broken && alternate->protocol == QUIC) {
+      break;
+    }
+  }
+  if (alternate == alternate_protocols.end()) {
     return;
+  }

[Ryan Hamilton, 2015/02/27 18:20:48]

As per the design doc you wrote up, I think this code needs to be reassessed. We
know the host:port of the server we're talking to an we know that we're speaking
QUIC. We want to (possibly) mark QUIC as broken to this server. But it looks
like we're going to end up checking the origin->server map looking for an origin
with this server's host:port pair. I think this might not be what we want to do?

(The current code may well be confused in this respect. In current practice the
host:port of the origin is always the host:port of the alternate, so the current
code may well work by accident not design)

[Bence, 2015/02/27 19:31:41]

Absolutely.  Let's revisit this once we work out the SetBroken/WasBroken
semantics.

 
   // TODO(rch):  In the special case where the session has received no
   // packets from the peer, we should consider blacklisting this
   // differently so that we still race TCP but we don't consider the
   // session connected until the handshake has been confirmed.
   HistogramBrokenAlternateProtocolLocation(
       BROKEN_ALTERNATE_PROTOCOL_LOCATION_QUIC_STREAM_FACTORY);
-  DCHECK_EQ(QUIC, alternate.protocol);
 
   // Since the session was active, there's no longer an
   // HttpStreamFactoryImpl::Job running which can mark it broken, unless the
   // TCP job also fails. So to avoid not using QUIC when we otherwise could,
   // we mark it as broken, and then immediately re-enable it. This leaves
   // QUIC as "recently broken" which means that 0-RTT will be disabled but
   // we'll still race.
-  http_server_properties_->SetBrokenAlternateProtocol(server);
-  http_server_properties_->ClearAlternateProtocol(server);
-  http_server_properties_->SetAlternateProtocol(
-      server, alternate.port, alternate.protocol, 1);
-  DCHECK_EQ(QUIC,
-            http_server_properties_->GetAlternateProtocol(server).protocol);
+  http_server_properties_->SetBrokenAlternateProtocol(server, *alternate);
+  http_server_properties_->RemoveAlternateProtocol(server, *alternate);
+  http_server_properties_->AddAlternateProtocol(server, alternate->port,
+                                                alternate->protocol, 1);
   DCHECK(http_server_properties_->WasAlternateProtocolRecentlyBroken(
-      server));
+      server, *alternate));
 }
 
 }  // namespace net