From ff78689d2f2421a9d581bdaf12090f78bedf0541 Mon Sep 17 00:00:00 2001
From: bnc <bnc@chromium.org>
Date: Thu, 23 Jul 2015 03:39:46 -0700
Subject: [PATCH] Remove broken alternative service from the map upon
 expiration.

When a broken alternative service expires, its every occurrence should be removed from the map.  Before this CL, only a bogus call to ClearAlternativeServices() happened.

As of Patch Set 8, I locally verified that the test fails without the change to http_server_properties_impl.cc, but passes with the change.

BUG=505413

Review URL: https://codereview.chromium.org/1211303003

Cr-Commit-Position: refs/heads/master@{#340067}
---
 net/http/http_server_properties_impl.cc          | 35 +++++++++++++++++---
 net/http/http_server_properties_impl_unittest.cc | 41 ++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/net/http/http_server_properties_impl.cc b/net/http/http_server_properties_impl.cc
index a7030f8753ae..7bdd30ec84db 100644
--- a/net/http/http_server_properties_impl.cc
+++ b/net/http/http_server_properties_impl.cc
@@ -653,11 +653,38 @@ void HttpServerPropertiesImpl::ExpireBrokenAlternateProtocolMappings() {
       break;
     }
 
-    const AlternativeService alternative_service = it->first;
+    const AlternativeService expired_alternative_service = it->first;
     broken_alternative_services_.erase(it);
-    // TODO(bnc): Make sure broken alternative services are not in the mapping.
-    ClearAlternativeServices(
-        HostPortPair(alternative_service.host, alternative_service.port));
+
+    // Remove every occurrence of |expired_alternative_service| from
+    // |alternative_service_map_|.
+    for (AlternativeServiceMap::iterator map_it =
+             alternative_service_map_.begin();
+         map_it != alternative_service_map_.end();) {
+      for (AlternativeServiceInfoVector::iterator it = map_it->second.begin();
+           it != map_it->second.end();) {
+        AlternativeService alternative_service(it->alternative_service);
+        // Empty hostname in map means hostname of key: substitute before
+        // comparing to |expired_alternative_service|.
+        if (alternative_service.host.empty()) {
+          alternative_service.host = map_it->first.host();
+        }
+        if (alternative_service == expired_alternative_service) {
+          it = map_it->second.erase(it);
+          continue;
+        }
+        ++it;
+      }
+      // If an origin has an empty list of alternative services, then remove it
+      // from both |canonical_host_to_origin_map_| and
+      // |alternative_service_map_|.
+      if (map_it->second.empty()) {
+        RemoveCanonicalHost(map_it->first);
+        map_it = alternative_service_map_.Erase(map_it);
+        continue;
+      }
+      ++map_it;
+    }
   }
   ScheduleBrokenAlternateProtocolMappingsExpiration();
 }
diff --git a/net/http/http_server_properties_impl_unittest.cc b/net/http/http_server_properties_impl_unittest.cc
index a17684558ec7..bb3dc091796c 100644
--- a/net/http/http_server_properties_impl_unittest.cc
+++ b/net/http/http_server_properties_impl_unittest.cc
@@ -731,6 +731,47 @@ TEST_F(AlternateProtocolServerPropertiesTest,
   EXPECT_TRUE(impl_.WasAlternativeServiceRecentlyBroken(alternative_service));
 }
 
+// Regression test for https://crbug.com/505413.
+TEST_F(AlternateProtocolServerPropertiesTest, RemoveExpiredBrokenAltSvc) {
+  HostPortPair foo_host_port_pair("foo", 443);
+  AlternativeService bar_alternative_service(QUIC, "bar", 443);
+  impl_.SetAlternativeService(foo_host_port_pair, bar_alternative_service, 1.0);
+  EXPECT_TRUE(HasAlternativeService(foo_host_port_pair));
+
+  HostPortPair bar_host_port_pair1("bar", 80);
+  AlternativeService nohost_alternative_service(QUIC, "", 443);
+  impl_.SetAlternativeService(bar_host_port_pair1, nohost_alternative_service,
+                              1.0);
+  EXPECT_TRUE(HasAlternativeService(bar_host_port_pair1));
+
+  HostPortPair bar_host_port_pair2("bar", 443);
+  AlternativeService baz_alternative_service(QUIC, "baz", 1234);
+  impl_.SetAlternativeService(bar_host_port_pair2, baz_alternative_service,
+                              1.0);
+  EXPECT_TRUE(HasAlternativeService(bar_host_port_pair2));
+
+  // Mark "bar:443" as broken.
+  base::TimeTicks past =
+      base::TimeTicks::Now() - base::TimeDelta::FromSeconds(42);
+  HttpServerPropertiesImplPeer::AddBrokenAlternativeServiceWithExpirationTime(
+      impl_, bar_alternative_service, past);
+
+  // Expire brokenness of "bar:443".
+  HttpServerPropertiesImplPeer::ExpireBrokenAlternateProtocolMappings(impl_);
+
+  // "foo:443" should have no alternative service now.
+  EXPECT_FALSE(HasAlternativeService(foo_host_port_pair));
+  // "bar:80" should have no alternative service now.
+  EXPECT_FALSE(HasAlternativeService(bar_host_port_pair1));
+  // The alternative service of "bar:443" should be unaffected.
+  EXPECT_TRUE(HasAlternativeService(bar_host_port_pair2));
+
+  EXPECT_TRUE(
+      impl_.WasAlternativeServiceRecentlyBroken(bar_alternative_service));
+  EXPECT_FALSE(
+      impl_.WasAlternativeServiceRecentlyBroken(baz_alternative_service));
+}
+
 typedef HttpServerPropertiesImplTest SpdySettingsServerPropertiesTest;
 
 TEST_F(SpdySettingsServerPropertiesTest, Initialize) {
-- 
2.11.4.GIT