Fix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior

net/http/broken_alternative_services_unittest.cc

Index: net/http/broken_alternative_services_unittest.cc
diff --git a/net/http/broken_alternative_services_unittest.cc b/net/http/broken_alternative_services_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..fd0452ee119f32d5b0d8f3784870cc5a470e2490
--- /dev/null
+++ b/net/http/broken_alternative_services_unittest.cc
@@ -0,0 +1,225 @@
+// Copyright (c) 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/http/broken_alternative_services.h"
+
+#include <algorithm>
+#include <vector>
+
+#include "base/test/test_mock_time_task_runner.h"
+#include "base/time/time.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+const base::TimeDelta BROKEN_ALT_SVC_EXPIRE_DELAYS[10] = {
+    base::TimeDelta::FromSeconds(300),   base::TimeDelta::FromSeconds(600),
+    base::TimeDelta::FromSeconds(1200),  base::TimeDelta::FromSeconds(2400),
+    base::TimeDelta::FromSeconds(4800),  base::TimeDelta::FromSeconds(9600),
+    base::TimeDelta::FromSeconds(19200), base::TimeDelta::FromSeconds(38400),
+    base::TimeDelta::FromSeconds(76800), base::TimeDelta::FromSeconds(153600),
+};
+
+class TestClock : public BrokenAlternativeServices::Clock {
+ public:
+  TestClock() : now_(base::TimeTicks::Now()) {}
+  ~TestClock() override {}
+
+  base::TimeTicks Now() const override { return now_; }
+
+  void Set(base::TimeTicks now) { now_ = now; }
+  void Advance(base::TimeDelta delta) { now_ += delta; }
+
+ private:
+  base::TimeTicks now_;
+};
+
+class BrokenAlternativeServicesTest
+    : public BrokenAlternativeServices::Delegate,
+      public ::testing::Test {
+ public:
+  BrokenAlternativeServicesTest()
+      : services_(this, &test_clock_),
+        test_task_runner_(new base::TestMockTimeTaskRunner()) {}
+
+  // BrokenAlternativeServices::Delegate implementation
+  void OnExpireBrokenAlternativeService(
+      const AlternativeService& expired_alternative_service) override {
+    expired_alt_svcs.push_back(expired_alternative_service);
+  }
+
+  ::testing::AssertionResult MarkAlternativeServiceBrokenAndLetExpireNTimes(

[Ryan Hamilton, 2017/05/26 03:52:09]

This method is quite complicated. It seems like it's only called from 2 places.
I think we can just inline in with some simplifications when it's used.

[wangyix1, 2017/05/27 01:20:11]

Done.

+      const AlternativeService& alternative_service,
+      int num_times) {
+    DCHECK(num_times < 4);
+
+    for (int i = 0; i < num_times; ++i) {
+      {
+        base::TestMockTimeTaskRunner::ScopedContext scoped_context(
+            test_task_runner_);

[Ryan Hamilton, 2017/05/26 03:52:09]

Is this required? From reading the class, I don't understand if it's required.

Actually, from reading this more, I realize that TestMockTimeTaskRunner has a 
GetMockTickClock method for vending a clock that's in sync with the runner.
Which pointed out that there is now a base::TicksClock class. yay! This was news
to me. Perhaps we can use this instead of rolling our own?

[wangyix1, 2017/05/27 01:20:11]

Done.

+        services_.MarkAlternativeServiceBroken(alternative_service);
+      }
+      if (!services_.IsAlternativeServiceBroken(alternative_service)) {
+        return ::testing::AssertionFailure() << "Alt svc not showing as broken "
+                                             << "after being marked broken.";
+      }
+
+      // |services_| should have posted task to expire the brokenness of
+      // |alternative_service|.
+      int pending_task_count = test_task_runner_->GetPendingTaskCount();
+      if (pending_task_count != 1u) {
+        return ::testing::AssertionFailure()
+               << "Test task runner has " << pending_task_count
+               << " pending tasks instead of 1 after marking "
+               << "alternative service broken.";
+      }
+
+      // Advance time by just enough so that |alternative_service|'s brokenness
+      // expires.
+      base::TimeDelta delta = BROKEN_ALT_SVC_EXPIRE_DELAYS[std::min(i, 9)];
+      test_clock_.Advance(delta);
+      test_task_runner_->FastForwardBy(delta);
+
+      // Ensure |alternative_service| brokenness has expired
+      if (services_.IsAlternativeServiceBroken(alternative_service)) {
+        return ::testing::AssertionFailure() << "Alt svc showing as broken "
+                                             << " after being confirmed.";
+      }
+    }
+    return ::testing::AssertionSuccess();
+  }
+
+  TestClock test_clock_;
+  std::vector<AlternativeService> expired_alt_svcs;
+  BrokenAlternativeServices services_;

[Ryan Hamilton, 2017/05/26 03:52:09]

nit: how 'bout "broken_services_"

[wangyix1, 2017/05/27 01:20:11]

Done.

+
+  scoped_refptr<base::TestMockTimeTaskRunner> test_task_runner_;
+};
+
+TEST_F(BrokenAlternativeServicesTest, MarkBroken) {
+  const AlternativeService alternative_service1(kProtoHTTP2, "foo", 443);
+  const AlternativeService alternative_service2(kProtoHTTP2, "foo", 1234);
+
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service2));
+
+  services_.MarkAlternativeServiceBroken(alternative_service1);
+
+  EXPECT_TRUE(services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service2));
+
+  services_.MarkAlternativeServiceBroken(alternative_service2);
+
+  EXPECT_TRUE(services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_TRUE(services_.IsAlternativeServiceBroken(alternative_service2));
+
+  services_.ConfirmAlternativeService(alternative_service1);
+
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_TRUE(services_.IsAlternativeServiceBroken(alternative_service2));
+
+  services_.ConfirmAlternativeService(alternative_service2);
+
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service2));
+}
+
+TEST_F(BrokenAlternativeServicesTest, MarkRecentlyBroken) {
+  const AlternativeService alternative_service(kProtoHTTP2, "foo", 443);
+
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service));
+  EXPECT_FALSE(
+      services_.WasAlternativeServiceRecentlyBroken(alternative_service));
+
+  services_.MarkAlternativeServiceRecentlyBroken(alternative_service);
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service));
+  EXPECT_TRUE(
+      services_.WasAlternativeServiceRecentlyBroken(alternative_service));
+
+  services_.ConfirmAlternativeService(alternative_service);
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service));
+  EXPECT_FALSE(
+      services_.WasAlternativeServiceRecentlyBroken(alternative_service));
+}
+
+TEST_F(BrokenAlternativeServicesTest, ExpireBrokenAlternateProtoclMappings) {
+  AlternativeService alternative_service(kProtoQUIC, "foo", 443);
+
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service));
+  EXPECT_FALSE(
+      services_.WasAlternativeServiceRecentlyBroken(alternative_service));
+
+  EXPECT_TRUE(
+      MarkAlternativeServiceBrokenAndLetExpireNTimes(alternative_service, 1));
+}
+
+TEST_F(BrokenAlternativeServicesTest, RemoveExpiredBrokenAltSvc) {
+  // This test will mark broken 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.
+
+  AlternativeService alternative_service1(kProtoQUIC, "foo", 443);
+  AlternativeService alternative_service2(kProtoQUIC, "bar", 443);
+
+  EXPECT_TRUE(
+      MarkAlternativeServiceBrokenAndLetExpireNTimes(alternative_service1, 3));
+
+  {
+    base::TestMockTimeTaskRunner::ScopedContext scoped_context(
+        test_task_runner_);
+
+    services_.MarkAlternativeServiceBroken(alternative_service1);
+    services_.MarkAlternativeServiceBroken(alternative_service2);
+  }
+  EXPECT_TRUE(services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_TRUE(services_.IsAlternativeServiceBroken(alternative_service2));
+
+  // Advance time until one time quantum before |alternative_service2|'s
+  // brokenness expires.
+  base::TimeDelta delta =
+      BROKEN_ALT_SVC_EXPIRE_DELAYS[0] - base::TimeDelta::FromInternalValue(1);
+  test_clock_.Advance(delta);
+  test_task_runner_->FastForwardBy(delta);
+
+  EXPECT_TRUE(services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_TRUE(services_.IsAlternativeServiceBroken(alternative_service2));
+
+  // Advance time by one time quantum. |alternative_service2| should no longer
+  // be broken.
+  delta = base::TimeDelta::FromInternalValue(1);
+  test_clock_.Advance(delta);
+  test_task_runner_->FastForwardBy(delta);
+
+  EXPECT_TRUE(services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service2));
+
+  // Advance time until one time quantum before |alternative_service1|'s
+  // brokenness expires
+  delta = BROKEN_ALT_SVC_EXPIRE_DELAYS[3] - BROKEN_ALT_SVC_EXPIRE_DELAYS[0] -
+          base::TimeDelta::FromInternalValue(1);
+  test_clock_.Advance(delta);
+  test_task_runner_->FastForwardBy(delta);
+
+  EXPECT_TRUE(services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service2));
+
+  // Advance time by one time quantum.  |alternative_service1| should no longer
+  // be broken.
+  delta = base::TimeDelta::FromInternalValue(1);
+  test_clock_.Advance(delta);
+  test_task_runner_->FastForwardBy(delta);
+
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(services_.IsAlternativeServiceBroken(alternative_service2));
+}
+
+}  // namespace
+
+}  // namespace net