ABANDONED Handle multiple AlternateProtocols for each HostPortPair.

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 2bd9ab3f1dbfc4ba904bb7b44e256742e918afe1..0bbaabf72cf285896ac0370786dcbb6e6cbf4bba 100644
--- a/net/quic/quic_stream_factory.cc
+++ b/net/quic/quic_stream_factory.cc
@@ -687,27 +687,42 @@ int QuicStreamFactory::Create(const HostPortPair& host_port_pair,
   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,
@@ -728,10 +743,11 @@ int QuicStreamFactory::Create(const HostPortPair& host_port_pair,
 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,
@@ -1228,10 +1244,11 @@ int64 QuicStreamFactory::GetServerNetworkStatsSmoothedRttInMicroseconds(
 }
 
 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(
@@ -1261,10 +1278,14 @@ void QuicStreamFactory::InitializeCachedStateInCryptoConfig(
 
   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;
+          }
         }
       }
     }
@@ -1321,13 +1342,21 @@ void QuicStreamFactory::ProcessGoingAwaySession(
   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
@@ -1335,7 +1364,6 @@ void QuicStreamFactory::ProcessGoingAwaySession(
   // 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
@@ -1343,14 +1371,12 @@ void QuicStreamFactory::ProcessGoingAwaySession(
   // 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