Fix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior

net/http/http_server_properties_impl_unittest.cc

Index: net/http/http_server_properties_impl_unittest.cc
diff --git a/net/http/http_server_properties_impl_unittest.cc b/net/http/http_server_properties_impl_unittest.cc
index 57a15bcc37970f2b3559ae4481d828ab81f23e4a..041d08421c39eae16c64e955451fcec0497d5350 100644
--- a/net/http/http_server_properties_impl_unittest.cc
+++ b/net/http/http_server_properties_impl_unittest.cc
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/logging.h"
+#include "base/test/test_mock_time_task_runner.h"
 #include "base/values.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/ip_address.h"
@@ -25,14 +26,19 @@ class HttpServerPropertiesImplPeer {
  public:
   static void AddBrokenAlternativeServiceWithExpirationTime(
       HttpServerPropertiesImpl& impl,
-      AlternativeService alternative_service,
+      const AlternativeService& alternative_service,
       base::TimeTicks when) {
-    impl.broken_alternative_services_.insert(
-        std::make_pair(alternative_service, when));
+    HttpBrokenAlternativeServicesManager::BrokenAlternativeServiceList::iterator
+        unused_it;
+    impl.broken_alternative_services_.AddToBrokenAlternativeServiceListAndMap(
+        alternative_service, when, &unused_it);
     auto it =
-        impl.recently_broken_alternative_services_.Get(alternative_service);
-    if (it == impl.recently_broken_alternative_services_.end()) {
-      impl.recently_broken_alternative_services_.Put(alternative_service, 1);
+        impl.broken_alternative_services_.recently_broken_alternative_services_
+            .Get(alternative_service);
+    if (it == impl.broken_alternative_services_
+                  .recently_broken_alternative_services_.end()) {
+      impl.broken_alternative_services_.recently_broken_alternative_services_
+          .Put(alternative_service, 1);
     } else {
       it->second++;
     }
@@ -40,7 +46,7 @@ class HttpServerPropertiesImplPeer {
 
   static void ExpireBrokenAlternateProtocolMappings(
       HttpServerPropertiesImpl& impl) {
-    impl.ExpireBrokenAlternateProtocolMappings();
+    impl.broken_alternative_services_.ExpireBrokenAlternateProtocolMappings();
   }
 };
 
@@ -48,8 +54,24 @@ namespace {
 
 const int kMaxSupportsSpdyServerHosts = 500;
 
+class TestClock : public HttpBrokenAlternativeServicesManager::Clock {
+ public:
+  TestClock() : now_(base::TimeTicks::Now()) {}
+  ~TestClock() override {}
+
+  base::TimeTicks Now() const override { return now_; }
+
+  void SetNow(base::TimeTicks now) { now_ = now; }
+  void AdvanceNow(base::TimeDelta delta) { now_ += delta; }
+
+ private:
+  base::TimeTicks now_;
+};
+
 class HttpServerPropertiesImplTest : public testing::Test {
  protected:
+  HttpServerPropertiesImplTest() : impl_(&test_clock_) {}
+
   bool HasAlternativeService(const url::SchemeHostPort& origin) {
     const AlternativeServiceVector alternative_service_vector =
         impl_.GetAlternativeServices(origin);
@@ -63,6 +85,11 @@ class HttpServerPropertiesImplTest : public testing::Test {
     return impl_.SetAlternativeService(origin, alternative_service, expiration);
   }
 
+  void MarkBrokenAndLetExpireAlternativeServiceNTimes(
+      const AlternativeService& alternative_service,
+      int num_times) {}
+
+  TestClock test_clock_;
   HttpServerPropertiesImpl impl_;
 };
 
@@ -983,6 +1010,102 @@ TEST_F(AlternateProtocolServerPropertiesTest, RemoveExpiredBrokenAltSvc) {
       impl_.WasAlternativeServiceRecentlyBroken(baz_alternative_service));
 }
 
+// Regression test for https://crbug.com/724302
+TEST_F(AlternateProtocolServerPropertiesTest, RemoveExpiredBrokenAltSvc2) {
+  scoped_refptr<base::TestMockTimeTaskRunner> test_task_runner =
+      new base::TestMockTimeTaskRunner;
+
+  // This test will mark an alternative service A that has already been marked
+  // broken many times, then immediately mark another alternative service B as
+  // broken for the first time. Because A's been marked broken many times
+  // already, its brokenness will be scheduled to expire much further in the
+  // future than B, even though it was marked broken before B. This test makes
+  // sure that even though A was marked broken before B, B's brokenness should
+  // expire before A.
+
+  // This test only makes sense if HttpServerPropertiesImpl's expiration delay
+  // for a broken AlternativeService is an increasing function of how many times
+  // that AlternativeService has been marked broken already.
+  DCHECK(HttpBrokenAlternativeServicesManager::
+             ComputeBrokenAltSvcExpirationDelayForTest(3) >
+         HttpBrokenAlternativeServicesManager::
+             ComputeBrokenAltSvcExpirationDelayForTest(0));
+
+  url::SchemeHostPort server1("https", "foo", 443);
+  AlternativeService alternative_service1(kProtoQUIC, "foo", 443);
+  SetAlternativeService(server1, alternative_service1);
+
+  url::SchemeHostPort server2("https", "bar", 443);
+  AlternativeService alternative_service2(kProtoQUIC, "bar", 443);
+  SetAlternativeService(server2, alternative_service2);
+
+  // Repeatedly mark alt svc 1 broken and wait for its brokenness to expire.
+  // This will increase its time until expiration.
+  for (int i = 0; i < 3; ++i) {
+    {
+      base::TestMockTimeTaskRunner::ScopedContext scoped_context(
+          test_task_runner);
+      impl_.MarkAlternativeServiceBroken(alternative_service1);
+    }
+    // |impl_| should have posted task to expire the brokenness of
+    // |alternative_service1|
+    EXPECT_EQ(1u, test_task_runner->GetPendingTaskCount());
+    EXPECT_TRUE(impl_.IsAlternativeServiceBroken(alternative_service1));
+
+    // Advance time by just enough so that |alternative_service1|'s brokenness
+    // expires.
+    base::TimeDelta delta = HttpBrokenAlternativeServicesManager::
+                                ComputeBrokenAltSvcExpirationDelayForTest(i) +
+                            base::TimeDelta::FromInternalValue(1);
+    test_clock_.AdvanceNow(delta);
+    test_task_runner->FastForwardBy(delta);
+
+    // Ensure brokenness of |alternative_service1| has expired.
+    EXPECT_FALSE(test_task_runner->HasPendingTask());
+    EXPECT_FALSE(impl_.IsAlternativeServiceBroken(alternative_service1));
+  }
+
+  {
+    base::TestMockTimeTaskRunner::ScopedContext scoped_context(
+        test_task_runner);
+    impl_.MarkAlternativeServiceBroken(alternative_service1);
+  }
+
+  // Advance time by one quantum
+  base::TimeDelta delta = base::TimeDelta::FromInternalValue(1);
+  test_clock_.AdvanceNow(delta);
+  test_task_runner->FastForwardBy(delta);
+
+  {
+    base::TestMockTimeTaskRunner::ScopedContext scoped_context(
+        test_task_runner);
+    impl_.MarkAlternativeServiceBroken(alternative_service2);
+  }
+
+  EXPECT_TRUE(impl_.IsAlternativeServiceBroken(alternative_service2));
+
+  // Advance time by just enough so that |alternative_service2|'s brokennness
+  // expires.
+  delta = HttpBrokenAlternativeServicesManager::
+              ComputeBrokenAltSvcExpirationDelayForTest(0) +
+          base::TimeDelta::FromInternalValue(1);
+  test_clock_.AdvanceNow(delta);
+  test_task_runner->FastForwardBy(delta);
+
+  EXPECT_TRUE(impl_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(impl_.IsAlternativeServiceBroken(alternative_service2));
+
+  // Advance time by enough so that |alternative_service1|'s brokenness expires.
+  delta = HttpBrokenAlternativeServicesManager ::
+              ComputeBrokenAltSvcExpirationDelayForTest(3) +
+          base::TimeDelta::FromInternalValue(1);
+  test_clock_.AdvanceNow(delta);
+  test_task_runner->FastForwardBy(delta);
+
+  EXPECT_FALSE(impl_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(impl_.IsAlternativeServiceBroken(alternative_service2));
+}
+
 typedef HttpServerPropertiesImplTest SupportsQuicServerPropertiesTest;
 
 TEST_F(SupportsQuicServerPropertiesTest, Set) {