ABANDONED Handle multiple AlternateProtocols for each HostPortPair.

net/http/http_server_properties_impl.cc

Index: net/http/http_server_properties_impl.cc
diff --git a/net/http/http_server_properties_impl.cc b/net/http/http_server_properties_impl.cc
index 103d0e1eaa36dad0eb2ab29c6f257bf2ec653ccd..349a7183ab6104d41080d9652f726cc5ff96adb5 100644
--- a/net/http/http_server_properties_impl.cc
+++ b/net/http/http_server_properties_impl.cc
@@ -49,14 +49,23 @@ void HttpServerPropertiesImpl::InitializeSpdyServers(
 
 void HttpServerPropertiesImpl::InitializeAlternateProtocolServers(
     AlternateProtocolMap* alternate_protocol_map) {
-  // Keep all the ALTERNATE_PROTOCOL_BROKEN ones since those don't
-  // get persisted.
-  for (AlternateProtocolMap::iterator it = alternate_protocol_map_.begin();
-       it != alternate_protocol_map_.end();) {
-    AlternateProtocolMap::iterator old_it = it;
-    ++it;
-    if (old_it->second.protocol != ALTERNATE_PROTOCOL_BROKEN) {
-      alternate_protocol_map_.Erase(old_it);
+  // Keep all the ALTERNATE_PROTOCOL_BROKEN ones since those don't get
+  // persisted.

[Ryan Hamilton, 2014/11/03 19:00:30]

I think you might want to move this to the second for loop.

[Bence, 2014/11/03 22:32:26]

Done.

+  for (AlternateProtocolMap::iterator alternate_protocols =
+           alternate_protocol_map_.begin();
+       alternate_protocols != alternate_protocol_map_.end();) {
+    for (AlternateProtocols::iterator it = alternate_protocols->second.begin();
+         it != alternate_protocols->second.end();) {
+      if (it->protocol != ALTERNATE_PROTOCOL_BROKEN) {
+        it = alternate_protocols->second.erase(it);
+      } else {
+        ++it;
+      }
+    }
+    if (alternate_protocols->second.size() == 0) {
+      alternate_protocols = alternate_protocol_map_.Erase(alternate_protocols);
+    } else {
+      ++alternate_protocols;
     }
   }
 
@@ -204,10 +213,18 @@ bool HttpServerPropertiesImpl::HasAlternateProtocol(
     const HostPortPair& server) {
   if (g_forced_alternate_protocol)
     return true;
-  AlternateProtocolMap::const_iterator it = alternate_protocol_map_.Get(server);
-  if (it != alternate_protocol_map_.end() &&
-      it->second.probability >= alternate_protocol_probability_threshold_) {
-    return true;
+  AlternateProtocolMap::const_iterator alternate_protocols =
+      alternate_protocol_map_.Get(server);
+  if (alternate_protocols != alternate_protocol_map_.end()) {
+    for (AlternateProtocols::const_iterator alternate_protocol =
+             alternate_protocols->second.begin();
+         alternate_protocol != alternate_protocols->second.end();
+         alternate_protocol++) {
+      if (alternate_protocol->probability >=
+          alternate_protocol_probability_threshold_) {
+        return true;
+      }
+    }
   }
 
   return GetCanonicalHost(server) != canonical_host_to_origin_map_.end();
@@ -226,8 +243,7 @@ std::string HttpServerPropertiesImpl::GetCanonicalSuffix(
   return std::string();
 }
 
-AlternateProtocolInfo
-HttpServerPropertiesImpl::GetAlternateProtocol(
+AlternateProtocols HttpServerPropertiesImpl::GetAlternateProtocol(
     const HostPortPair& server) {
   DCHECK(HasAlternateProtocol(server));
 
@@ -243,10 +259,11 @@ HttpServerPropertiesImpl::GetAlternateProtocol(
 
   // We must be forcing an alternate.
   DCHECK(g_forced_alternate_protocol);
-  return *g_forced_alternate_protocol;
+  return AlternateProtocols(/* size = */ 1,

[Ryan Hamilton, 2014/11/03 19:00:30]

I'm familiar wit this convention of "naming" literals via comments, but I
usually see it without whitespace:

/*size=*/

[Bence, 2014/11/03 22:32:26]

Done.

+                            /* val = */ *g_forced_alternate_protocol);

[Ryan Hamilton, 2014/11/03 19:00:30]

Since the variable already has a readable name, I don't think you need the
"val=" comment.

[Bence, 2014/11/03 22:32:26]

Done.

 }
 
-void HttpServerPropertiesImpl::SetAlternateProtocol(
+void HttpServerPropertiesImpl::AddAlternateProtocol(
     const HostPortPair& server,
     uint16 alternate_port,
     AlternateProtocol alternate_protocol,
@@ -259,27 +276,39 @@ void HttpServerPropertiesImpl::SetAlternateProtocol(
   AlternateProtocolInfo alternate(alternate_port,
                                   alternate_protocol,
                                   alternate_probability);
-  if (HasAlternateProtocol(server)) {
-    const AlternateProtocolInfo existing_alternate =
-        GetAlternateProtocol(server);
-
-    if (existing_alternate.protocol == ALTERNATE_PROTOCOL_BROKEN) {
-      DVLOG(1) << "Ignore alternate protocol since it's known to be broken.";
-      return;
+  // Do not call HasAlternateProtocol(), because we don't care about
+  // |g_forced_alternate_protocol|, or whether the alternate protocol
+  // probability is higher than the threshold.
+  AlternateProtocolMap::iterator map_it = alternate_protocol_map_.Get(server);
+  if (map_it != alternate_protocol_map_.end()) {
+    AlternateProtocols existing_alternates = map_it->second;
+    AlternateProtocols::iterator it;
+    for (it = existing_alternates.begin(); it != existing_alternates.end();
+         ++it) {
+      if (it->EqualsModuloProbabilityAndProtocol(alternate)) {

[Ryan Hamilton, 2014/11/04 18:56:22]

I don't think I understand why this is the right tests. If I advertised that I
speak, say, both HTTP/2 and QUIC on port 443 I would expect that both of these
entries would be added. I think the only thing you want to exclude is
probability.

[Bence, 2014/11/04 21:55:50]

Darn, you are right.  The problem with the current design is that if an
alternate protocol is broken, we overwrite the protocol with
ALTERNATE_PROTOCOL_BROKEN, so later we'll have no idea if it was the HTTP/2 or
the QUIC one that was broken.  Like if I test for protocol equality here, the
ALTERNATE_PROTOCOL_BROKEN case six lines below will never get executed.

I propose that I remove |ALTERNATE_PROTOCOL_BROKEN| from |enum
AlternateProtocol| and add |bool broken_| to |struct AlternateProtocolInfo|,
what do you think?

[Ryan Hamilton, 2014/11/04 22:13:17]

s/broken_/is_broken_/ but yeah, this sounds great. (I think we may have
discussed something like this over the VC a while back.

It might be a good idea to make this change as a small stand-alone change which
you then rebase your current CL on top of. Two small CLs is typically easier to
review than one larger CL.

+        break;
+      }
     }
 
-    if (alternate_protocol != ALTERNATE_PROTOCOL_BROKEN &&
-        !existing_alternate.Equals(alternate)) {
-      LOG(WARNING) << "Changing the alternate protocol for: "
-                   << server.ToString()
-                   << " from [Port: " << existing_alternate.port
-                   << ", Protocol: " << existing_alternate.protocol
-                   << ", Probability: " << existing_alternate.probability
-                   << "] to [Port: " << alternate_port
-                   << ", Protocol: " << alternate_protocol
-                   << ", Probability: " << alternate_probability
-                   << "].";
+    if (it != existing_alternates.end()) {
+      if (it->protocol == ALTERNATE_PROTOCOL_BROKEN) {
+        DVLOG(1) << "Ignore alternate protocol since it's known to be broken.";
+        return;
+      }
+      if (it->probability != alternate_probability) {
+        LOG(WARNING) << "Changing the probability of alternate protocol for: "
+                     << server.ToString()
+                     << " Port: " << it->port
+                     << ", Protocol: " << it->protocol
+                     << ", Probability from: " << it->probability
+                     << " to: " << alternate_probability << ".";
+        it->probability = alternate_probability;
+      }
+    } else {
+      existing_alternates.push_back(alternate);
     }
+
+    alternate_protocol_map_.Put(server, existing_alternates);
   } else {
     if (alternate_probability >= alternate_protocol_probability_threshold_) {
       // TODO(rch): Consider the case where multiple requests are started
@@ -287,10 +316,10 @@ void HttpServerPropertiesImpl::SetAlternateProtocol(
       // would reach this code, whereas all of them should should have.
       HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_MAPPING_MISSING);
     }
+    alternate_protocol_map_.Put(
+        server, AlternateProtocols(/* size = */ 1, /* val = */ alternate));
   }
 
-  alternate_protocol_map_.Put(server, alternate);
-
   // If this host ends with a canonical suffix, then set it as the
   // canonical host.
   for (size_t i = 0; i < canonical_suffixes_.size(); ++i) {
@@ -304,16 +333,26 @@ void HttpServerPropertiesImpl::SetAlternateProtocol(
 }
 
 void HttpServerPropertiesImpl::SetBrokenAlternateProtocol(
-    const HostPortPair& server) {
-  AlternateProtocolMap::iterator it = alternate_protocol_map_.Get(server);
-  if (it != alternate_protocol_map_.end()) {
-    it->second.protocol = ALTERNATE_PROTOCOL_BROKEN;
-  } else {
-    AlternateProtocolInfo alternate(server.port(),
-                                    ALTERNATE_PROTOCOL_BROKEN,
-                                    1);
-    alternate_protocol_map_.Put(server, alternate);
+    const HostPortPair& server,
+    const AlternateProtocolInfo& broken_alternate_protocol) {
+  AddAlternateProtocol(server,
+                       broken_alternate_protocol.port,
+                       broken_alternate_protocol.protocol,
+                       broken_alternate_protocol.probability);
+  AlternateProtocolMap::iterator alternate_protocols =
+      alternate_protocol_map_.Get(server);
+  DCHECK(alternate_protocols != alternate_protocol_map_.end());
+  AlternateProtocols::iterator it;
+  for (it = alternate_protocols->second.begin();
+       it != alternate_protocols->second.end();
+       ++it) {
+    if (it->Equals(broken_alternate_protocol)) {
+      it->protocol = ALTERNATE_PROTOCOL_BROKEN;
+      break;
+    }
   }
+  DCHECK(it != alternate_protocols->second.end());
+
   int count = ++broken_alternate_protocol_map_[server];
   base::TimeDelta delay =
       base::TimeDelta::FromSeconds(kBrokenAlternateProtocolDelaySecs);
@@ -322,37 +361,62 @@ void HttpServerPropertiesImpl::SetBrokenAlternateProtocol(
   entry.when = base::TimeTicks::Now() + delay * (1 << (count - 1));
   broken_alternate_protocol_list_.push_back(entry);
 
-  // Do not leave this host as canonical so that we don't infer the other
-  // hosts are also broken without testing them first.
+  // Do not leave this host as canonical so that we don't infer the other hosts
+  // are also broken without testing them first.
   RemoveCanonicalHost(server);
 
   // If this is the only entry in the list, schedule an expiration task.
-  // Otherwse it will be rescheduled automatically when the pending
-  // task runs.
+  // Otherwise it will be rescheduled automatically when the pending task runs.
   if (broken_alternate_protocol_list_.size() == 1) {
     ScheduleBrokenAlternateProtocolMappingsExpiration();
   }
 }
 
 bool HttpServerPropertiesImpl::WasAlternateProtocolRecentlyBroken(
-    const HostPortPair& server) {
+    const HostPortPair& server,
+    const AlternateProtocolInfo& alternate_protocol) {
   return ContainsKey(broken_alternate_protocol_map_, server);
 }
 
 void HttpServerPropertiesImpl::ConfirmAlternateProtocol(
-    const HostPortPair& server) {
+    const HostPortPair& server,
+    const AlternateProtocolInfo& alternate_protocol) {
   broken_alternate_protocol_map_.erase(server);
 }
 
 void HttpServerPropertiesImpl::ClearAlternateProtocol(
     const HostPortPair& server) {
   AlternateProtocolMap::iterator it = alternate_protocol_map_.Peek(server);
-  if (it != alternate_protocol_map_.end())
+  if (it != alternate_protocol_map_.end()) {
     alternate_protocol_map_.Erase(it);
+  }
 
   RemoveCanonicalHost(server);
 }
 
+void HttpServerPropertiesImpl::RemoveAlternateProtocol(
+    const HostPortPair& server,
+    const AlternateProtocolInfo& alternate_protocol) {
+  AlternateProtocolMap::iterator alternate_protocols =
+      alternate_protocol_map_.Peek(server);
+  if (alternate_protocols == alternate_protocol_map_.end()) {
+    return;
+  }
+  for (AlternateProtocols::iterator it = alternate_protocols->second.begin();
+       it != alternate_protocols->second.end();) {
+    if (it->Equals(alternate_protocol)) {
+      it = alternate_protocols->second.erase(it);
+      break;
+    } else {
+      ++it;
+    }
+  }
+  if (alternate_protocols->second.size() == 0) {
+    alternate_protocol_map_.Erase(alternate_protocols);
+    RemoveCanonicalHost(server);
+  }
+}
+
 const AlternateProtocolMap&
 HttpServerPropertiesImpl::alternate_protocol_map() const {
   return alternate_protocol_map_;