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..c5b1bacce0a3f6970c20ffae5d20463fa2422964
--- /dev/null
+++ b/net/http/broken_alternative_services_unittest.cc
@@ -0,0 +1,253 @@
+// 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/tick_clock.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),

[Ryan Hamilton, 2017/05/27 13:28:16]

Since these are all more than 60 seconds, how about using FromMinutes?

[wangyix1, 2017/05/30 21:18:18]

Done.

+    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 BrokenAlternativeServicesTest
+    : public BrokenAlternativeServices::Delegate,
+      public ::testing::Test {
+ public:
+  BrokenAlternativeServicesTest()
+      : test_task_runner_(new base::TestMockTimeTaskRunner()),
+        broken_services_clock_(test_task_runner_->GetMockTickClock()),
+        broken_services_(this, broken_services_clock_.get()) {}
+
+  // BrokenAlternativeServices::Delegate implementation
+  void OnExpireBrokenAlternativeService(
+      const AlternativeService& expired_alternative_service) override {
+    expired_alt_svcs_.push_back(expired_alternative_service);
+  }
+
+  scoped_refptr<base::TestMockTimeTaskRunner> test_task_runner_;
+
+  std::unique_ptr<base::TickClock> broken_services_clock_;
+  BrokenAlternativeServices broken_services_;
+
+  std::vector<AlternativeService> expired_alt_svcs_;
+};
+
+TEST_F(BrokenAlternativeServicesTest, MarkBroken) {
+  const AlternativeService alternative_service1(kProtoHTTP2, "foo", 443);
+  const AlternativeService alternative_service2(kProtoHTTP2, "foo", 1234);
+
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service2));
+
+  broken_services_.MarkAlternativeServiceBroken(alternative_service1);
+
+  EXPECT_TRUE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service2));
+
+  broken_services_.MarkAlternativeServiceBroken(alternative_service2);
+
+  EXPECT_TRUE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_TRUE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service2));
+
+  broken_services_.ConfirmAlternativeService(alternative_service1);
+
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_TRUE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service2));
+
+  broken_services_.ConfirmAlternativeService(alternative_service2);
+
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service2));
+
+  EXPECT_EQ(0u, expired_alt_svcs_.size());
+}
+
+TEST_F(BrokenAlternativeServicesTest, MarkRecentlyBroken) {
+  const AlternativeService alternative_service(kProtoHTTP2, "foo", 443);
+
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service));
+  EXPECT_FALSE(broken_services_.WasAlternativeServiceRecentlyBroken(
+      alternative_service));
+
+  broken_services_.MarkAlternativeServiceRecentlyBroken(alternative_service);
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service));
+  EXPECT_TRUE(broken_services_.WasAlternativeServiceRecentlyBroken(
+      alternative_service));
+
+  broken_services_.ConfirmAlternativeService(alternative_service);
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service));
+  EXPECT_FALSE(broken_services_.WasAlternativeServiceRecentlyBroken(
+      alternative_service));
+}
+
+TEST_F(BrokenAlternativeServicesTest, ExpireBrokenAlternateProtoclMappings) {
+  AlternativeService alternative_service(kProtoQUIC, "foo", 443);
+
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service));
+  EXPECT_FALSE(broken_services_.WasAlternativeServiceRecentlyBroken(
+      alternative_service));

[Ryan Hamilton, 2017/05/27 13:28:16]

nit: no need to test this since you verified the initial state in earlier tests.

[wangyix1, 2017/05/30 21:18:18]

Done.

+
+  {
+    base::TestMockTimeTaskRunner::ScopedContext scoped_context(
+        test_task_runner_);

[Ryan Hamilton, 2017/05/27 13:28:16]

Can you remove this ScopedContext? (and the nesting)

[wangyix1, 2017/05/30 21:18:18]

Done.

+    broken_services_.MarkAlternativeServiceBroken(alternative_service);
+  }
+  EXPECT_TRUE(broken_services_.IsAlternativeServiceBroken(alternative_service));

[Ryan Hamilton, 2017/05/27 13:28:16]

ditto

[wangyix1, 2017/05/30 21:18:18]

Done.

+
+  // |broken_services_| should have posted task to expire the brokenness of
+  // |alternative_service|.
+  EXPECT_EQ(1u, test_task_runner_->GetPendingTaskCount());
+
+  // Advance time until one time quantum before |alternative_service1|'s
+  // brokenness expires
+  test_task_runner_->FastForwardBy(BROKEN_ALT_SVC_EXPIRE_DELAYS[0] -

[Ryan Hamilton, 2017/05/27 13:28:16]

can you inline FromMinutes(5) instead of using this global variable to make the
test more obvious.

[wangyix1, 2017/05/30 21:18:18]

Done.

+                                   base::TimeDelta::FromInternalValue(1));
+
+  // Ensure |alternative_service| is still marked broken.

[Ryan Hamilton, 2017/05/27 13:28:16]

nit: might want to check that there is still a posted task.

[wangyix1, 2017/05/30 21:18:18]

Done.

+  EXPECT_TRUE(broken_services_.IsAlternativeServiceBroken(alternative_service));
+  EXPECT_EQ(0u, expired_alt_svcs_.size());
+
+  // Advance time by just enough so that |alternative_service|'s brokenness
+  // expires.
+  test_task_runner_->FastForwardBy(base::TimeDelta::FromInternalValue(1));
+
+  // Ensure |alternative_service| brokenness has expired.
+  EXPECT_FALSE(

[Ryan Hamilton, 2017/05/27 13:28:16]

nit: might want to check that there are no posted tasks.

[wangyix1, 2017/05/30 21:18:18]

Done.

+      broken_services_.IsAlternativeServiceBroken(alternative_service));

[Ryan Hamilton, 2017/05/27 13:28:16]

It should now be RecentlyBroken, right? Can you verify this?

[wangyix1, 2017/05/30 21:18:18]

Done.

+  EXPECT_EQ(1u, expired_alt_svcs_.size());
+  EXPECT_EQ(alternative_service, expired_alt_svcs_[0]);
+}

[Ryan Hamilton, 2017/05/27 13:28:16]

Nice test! Very clear. 

Let's add another test which explicitly tests the back off, and the backoff
limit.

MarkBroken, expire, 
MarkBroken, <verify backoff > expire
MarkBroken, <verify backoff > expire
...
MarkBroken, <verify no more backoff > expire

[wangyix1, 2017/05/30 21:18:18]

Done.

[Ryan Hamilton, 2017/05/30 21:32:28]

Thanks for adding the test. Can we unroll the loop and inline the time periods
involved to make the logic simpler?

(See tott #338
https://docs.google.com/document/d/1VbCdizaXK8xDTKD8Z-IP24BeW8-UYklnuG3leU2M4...)

(Can you also inline other uses of BROKEN_ALT_SVC_EXPIRE_DELAYS?)

[wangyix1, 2017/05/30 23:57:49]

Done.

+
+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);
+
+  // Repeately mark |alternative_service1| broken and let brokenness expire.
+  // Do this a few times.
+  for (int i = 0; i < 3; ++i) {
+    {
+      base::TestMockTimeTaskRunner::ScopedContext scoped_context(
+          test_task_runner_);

[Ryan Hamilton, 2017/05/27 13:28:16]

ditto about the scoped context

[wangyix1, 2017/05/30 21:18:18]

Done.

+      broken_services_.MarkAlternativeServiceBroken(alternative_service1);
+    }
+    EXPECT_TRUE(
+        broken_services_.IsAlternativeServiceBroken(alternative_service1));

[Ryan Hamilton, 2017/05/27 13:28:16]

nit: no need to re-verify this since you've done so in previous tests. (here and
in the rest of this loop.) I think you'll end up with:

broken_services_.MarkAlternativeServiceBroken(alternative_service1);
test_task_runner_->FastForwardBy(base::TimeDelta::FromMinutes(5));
broken_services_.MarkAlternativeServiceBroken(alternative_service1);
test_task_runner_->FastForwardBy(base::TimeDelta::FromMinutes(10));
broken_services_.MarkAlternativeServiceBroken(alternative_service1);
test_task_runner_->FastForwardBy(base::TimeDelta::FromMinutes(20));  

[wangyix1, 2017/05/30 21:18:18]

Done.

+
+    // |broken_services_| should have posted task to expire the brokenness of
+    // |alternative_service|.
+    EXPECT_EQ(1u, test_task_runner_->GetPendingTaskCount());
+
+    // Advance time by just enough so that |alternative_service|'s brokenness
+    // expires.
+    test_task_runner_->FastForwardBy(BROKEN_ALT_SVC_EXPIRE_DELAYS[i]);
+
+    // Ensure |alternative_service| brokenness has expired
+    EXPECT_FALSE(
+        broken_services_.IsAlternativeServiceBroken(alternative_service1));
+    EXPECT_EQ(i + 1, (int)expired_alt_svcs_.size());
+    EXPECT_EQ(alternative_service1, expired_alt_svcs_[i]);
+  }
+  expired_alt_svcs_.clear();
+
+  // Mark |alternative_service1| broken (will be given longer expiration delay),
+  // then mark |alternative_service2| broken (will be given shorter expiration
+  // delay).
+  {
+    base::TestMockTimeTaskRunner::ScopedContext scoped_context(
+        test_task_runner_);

[Ryan Hamilton, 2017/05/27 13:28:16]

can you remove this?

[wangyix1, 2017/05/30 21:18:18]

Done.

+
+    broken_services_.MarkAlternativeServiceBroken(alternative_service1);
+    broken_services_.MarkAlternativeServiceBroken(alternative_service2);
+  }
+  EXPECT_TRUE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_TRUE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service2));
+
+  // Advance time until one time quantum before |alternative_service2|'s
+  // brokenness expires.
+  test_task_runner_->FastForwardBy(BROKEN_ALT_SVC_EXPIRE_DELAYS[0] -
+                                   base::TimeDelta::FromInternalValue(1));
+
+  EXPECT_TRUE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_TRUE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service2));
+  EXPECT_EQ(0u, expired_alt_svcs_.size());
+
+  // Advance time by one time quantum. |alternative_service2| should no longer
+  // be broken.
+  test_task_runner_->FastForwardBy(base::TimeDelta::FromInternalValue(1));
+
+  EXPECT_TRUE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service2));
+  EXPECT_EQ(1u, expired_alt_svcs_.size());
+  EXPECT_EQ(alternative_service2, expired_alt_svcs_[0]);
+
+  // Advance time until one time quantum before |alternative_service1|'s
+  // brokenness expires
+  test_task_runner_->FastForwardBy(BROKEN_ALT_SVC_EXPIRE_DELAYS[3] -
+                                   BROKEN_ALT_SVC_EXPIRE_DELAYS[0] -
+                                   base::TimeDelta::FromInternalValue(1));
+
+  EXPECT_TRUE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service2));
+  EXPECT_EQ(1u, expired_alt_svcs_.size());
+  EXPECT_EQ(alternative_service2, expired_alt_svcs_[0]);
+
+  // Advance time by one time quantum.  |alternative_service1| should no longer
+  // be broken.
+  test_task_runner_->FastForwardBy(base::TimeDelta::FromInternalValue(1));
+
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service1));
+  EXPECT_FALSE(
+      broken_services_.IsAlternativeServiceBroken(alternative_service2));
+  EXPECT_EQ(2u, expired_alt_svcs_.size());
+  EXPECT_EQ(alternative_service2, expired_alt_svcs_[0]);
+  EXPECT_EQ(alternative_service1, expired_alt_svcs_[1]);
+}
+
+}  // namespace
+
+}  // namespace net