Lazy RemoveCanonicalHost().

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 84a7a9edb47f226f4b7e94b45996038de4b1d97b..9b3d28a20aa98c2e0d265e0c4249125cecf77797 100644
--- a/net/http/http_server_properties_impl.cc
+++ b/net/http/http_server_properties_impl.cc
@@ -325,10 +325,6 @@ void HttpServerPropertiesImpl::SetBrokenAlternateProtocol(
     return;
   }
 
-  // Do not leave this host as canonical so that we don't infer the other
-  // hosts are also broken without testing them first.
-  RemoveCanonicalHost(origin);
-
   // If this is the only entry in the list, schedule an expiration task.
   // Otherwise it will be rescheduled automatically when the pending task runs.
   if (broken_alternative_services_.size() == 1) {
@@ -493,9 +489,23 @@ HttpServerPropertiesImpl::GetAlternateProtocolIterator(
     return it;

[Ryan Hamilton, 2015/03/18 22:44:36]

Do you need to (possibly) do the removal here too?

[Bence, 2015/03/19 12:44:02]

It's not necessary.  Consider foo being the canonical host for bar.  If foo has
an alternative service, and we detect it to be broken, and later we query the
alternative service of foo, we might or might not RemoveCanonicalHost(foo), but
we get back the broken alternative service in either case.  When we query the
alternative service of bar, the codepath below gets executed, foo is removed
from the canonical host list, and no alternative service is returned,
irrespectively from whether foo was removed from the canonical host list earlier
or not.

Since most hosts are not canonical hosts, I think it's better not to add a call
to RemoveCanonicalHost here.

[Ryan Hamilton, 2015/03/19 20:05:18]

Ah, I see. Ok, do we have a test which covers this case? foo is canonical for
bar, foo is broken. GetAlternateProtocol for bar does not return the broken
entry?

[Bence, 2015/03/19 21:43:09]

Yes, AlternateProtocolServerPropertiesTest.CanonicalBroken does.

 
   CanonicalHostMap::const_iterator canonical = GetCanonicalHost(server);
-  if (canonical != canonical_host_to_origin_map_.end())
-    return alternate_protocol_map_.Get(canonical->second);
+  if (canonical == canonical_host_to_origin_map_.end()) {
+    return alternate_protocol_map_.end();
+  }
+
+  const HostPortPair canonical_host_port = canonical->second;
+  it = alternate_protocol_map_.Get(canonical_host_port);
+  if (it == alternate_protocol_map_.end()) {
+    return alternate_protocol_map_.end();
+  }
+
+  const AlternativeService alternative_service(
+      it->second.protocol, canonical_host_port.host(), it->second.port);
+  if (!IsAlternativeServiceBroken(alternative_service)) {
+    return it;
+  }
 
+  RemoveCanonicalHost(canonical_host_port);
   return alternate_protocol_map_.end();
 }