Implement throttling logic for fetching affiliation information.

components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc

+// Copyright 2015 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 "components/password_manager/core/browser/affiliation_fetch_throttler.h"
+
+#include <cmath>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/numerics/safe_math.h"
+#include "base/run_loop.h"
+#include "base/test/test_mock_time_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "base/time/tick_clock.h"
+#include "base/time/time.h"
+#include "components/password_manager/core/browser/affiliation_fetch_throttler_delegate.h"
+#include "net/base/network_change_notifier.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace password_manager {
+namespace {
+
+class MockAffiliationFetchThrottlerDelegate
+    : public AffiliationFetchThrottlerDelegate {
+ public:
+  MockAffiliationFetchThrottlerDelegate(scoped_ptr<base::TickClock> tick_clock)
+      : tick_clock_(tick_clock.Pass()),
+        emulated_return_value_(true),
+        release_count_(0u) {}
+  ~MockAffiliationFetchThrottlerDelegate() { EXPECT_EQ(0u, release_count_); }
+
+  void set_emulated_return_value(bool value) { emulated_return_value_ = value; }
+  void reset_release_count() { release_count_ = 0u; }
+  size_t release_count() const { return release_count_; }
+  base::TimeTicks last_release_time() const { return last_release_time_; }
+
+  // AffiliationFetchThrottlerDelegate:
+  bool OnCanSendNetworkRequest() override {
+    ++release_count_;
+    last_release_time_ = tick_clock_->NowTicks();
+    return emulated_return_value_;
+  }
+
+ private:
+  scoped_ptr<base::TickClock> tick_clock_;
+  bool emulated_return_value_;
+  size_t release_count_;
+  base::TimeTicks last_release_time_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockAffiliationFetchThrottlerDelegate);
+};
+
+class MockNetworkChangeNotifier : public net::NetworkChangeNotifier {

[mmenke, 2015/01/26 18:49:13]

This class isn't needed.  See NetworkChangeNotifier::CreateMock.  If you need it
to track connection types, which the current mock does not do, I think it's
better to have the mock class in net/ track the changes, rather than having
everyone who wants to do this kind of test creating their own solution.

[engedy, 2015/01/26 19:39:35]

Done.

+ public:
+  MockNetworkChangeNotifier() : connection_type_(CONNECTION_UNKNOWN) {}
+
+  void set_connection_type(ConnectionType connection_type) {
+    connection_type_ = connection_type;
+    NotifyObserversOfConnectionTypeChangeForTests(connection_type_);
+  }
+
+  ConnectionType GetCurrentConnectionType() const override {
+    return connection_type_;
+  }
+
+ private:
+  ConnectionType connection_type_;
+};
+
+// Rounds the provided time delta in milliseconds (potentially having fractional
+// part) to the nearest microsecond and returns it.
+base::TimeDelta RoundToNearestMicrosecond(double delta_ms) {
+  base::internal::CheckedNumeric<int64_t> delta_us(
+      delta_ms * base::Time::kMicrosecondsPerMillisecond + .5);
+  return base::TimeDelta::FromMicroseconds(delta_us.ValueOrDie());
+}
+
+}  // namespace
+
+class AffiliationFetchThrottlerTest : public testing::Test {
+ public:
+  AffiliationFetchThrottlerTest()
+      : task_runner_(new base::TestMockTimeTaskRunner),
+        mock_delegate_(task_runner_->GetMockTickClock()) {}
+  ~AffiliationFetchThrottlerTest() override {}
+
+  scoped_ptr<AffiliationFetchThrottler> CreateThrottler() {
+    return make_scoped_ptr(new AffiliationFetchThrottler(
+        &mock_delegate_, task_runner_, task_runner_->GetMockTickClock()));
+  }
+
+  void SimulateHasNetworkConnectivity(bool has_connectivity) {
+    network_change_notifier_.set_connection_type(
+        has_connectivity ? net::NetworkChangeNotifier::CONNECTION_UNKNOWN
+                         : net::NetworkChangeNotifier::CONNECTION_NONE);
+    base::RunLoop().RunUntilIdle();
+  }
+
+  // Asserts that OnCanSendNetworkRequest() will have been called once by the
+  // time no tasks remain in the task runner, with an expected delay of at least
+  // |min_delay_ms| but no more than |max_delay_ms|. When called, the mock will
+  // return |emulated_return_value|, a boolean value normally indicating whether
+  // or not a request was actually issued in response to the call.
+  void AssertReleaseInBetween(bool emulated_return_value,
+                              double min_delay_ms,
+                              double max_delay_ms) {
+    ASSERT_EQ(0u, mock_delegate_.release_count());
+    base::TimeTicks ticks_at_start = task_runner_->GetCurrentMockTime();
+    mock_delegate_.set_emulated_return_value(emulated_return_value);
+    task_runner_->FastForwardUntilNoTasksRemain();
+    base::TimeDelta delay = mock_delegate_.last_release_time() - ticks_at_start;
+    ASSERT_EQ(1u, mock_delegate_.release_count());
+    EXPECT_LE(RoundToNearestMicrosecond(min_delay_ms), delay);
+    EXPECT_GE(RoundToNearestMicrosecond(max_delay_ms), delay);

[mmenke, 2015/01/26 18:49:13]

Per comment, it may theoretically be possible for delay to be less than
min_delay_ms, due to floating point error.  While if you go through all the
math, it *may* be possible that the behavior of the pow method, and how floating
point numbers are represented, and your choice of values, this never occurs, the
test should not depend on all that.

[engedy, 2015/01/26 19:39:34]

The reasons that it should never occur is because the result of the floating
point calculation is rounder (not truncated) to the nearest integer microsecond.
Precision of this calculations should always be sufficient to make sure this is
the correct microsecond.

But you are right that the test should not expect this. I have changed to tests
so that they allow +-1 millisecond slack.

[mmenke, 2015/01/30 19:50:41]

Hrm...I thought I checked and it was truncated...May have missed something,
though.  I'm happy with the +/- 1's, though.

+    mock_delegate_.reset_release_count();
+  }
+
+  // Runs the task runner for |ms| and asserts that OnCanSendNetworkRequest()
+  // will not have been called by the end of this period.
+  void AssertNoReleaseForMs(double ms) {
+    task_runner_->FastForwardBy(RoundToNearestMicrosecond(ms));
+    ASSERT_EQ(0u, mock_delegate_.release_count());
+  }
+
+  // Runs the task runner until no tasks remain, and asserts that
+  // OnCanSendNetworkRequest() will not have been called.
+  void AssertNoReleaseUntilNoTasksRemain() {
+    task_runner_->FastForwardUntilNoTasksRemain();
+    ASSERT_EQ(0u, mock_delegate_.release_count());
+  }
+
+  size_t GetPendingTaskCount() const {
+    return task_runner_->GetPendingTaskCount();
+  }
+
+ private:
+  // Needed because NetworkChangeNotifier uses ObserverList, which notifies
+  // observers on the MessageLoop that belongs to the thread from which they
+  // have registered.
+  base::MessageLoop message_loop_;
+  MockNetworkChangeNotifier network_change_notifier_;
+  scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
+  MockAffiliationFetchThrottlerDelegate mock_delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(AffiliationFetchThrottlerTest);
+};
+
+TEST_F(AffiliationFetchThrottlerTest, SuccessfulRequests) {
+  scoped_ptr<AffiliationFetchThrottler> throttler(CreateThrottler());
+
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+
+  // Signal while request is in flight should be ignored.
+  throttler->SignalNetworkRequestNeeded();
+  AssertNoReleaseUntilNoTasksRemain();
+  throttler->InformOfNetworkRequestComplete(true);
+  AssertNoReleaseUntilNoTasksRemain();
+
+  // Duplicate the second signal 3 times: still only 1 callback should arrive.
+  throttler->SignalNetworkRequestNeeded();
+  throttler->SignalNetworkRequestNeeded();
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+}
+
+TEST_F(AffiliationFetchThrottlerTest, FailedRequests) {
+  scoped_ptr<AffiliationFetchThrottler> throttler(CreateThrottler());
+
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+  throttler->InformOfNetworkRequestComplete(false);
+
+  // The request after the first failure should be delayed by |initial_delay_ms|
+  // spread out over Uniform(1 - |jitter_factor|, 1).
+  throttler->SignalNetworkRequestNeeded();
+  const auto& kP = AffiliationFetchThrottler::kBackoffPolicy;

[mmenke, 2015/01/26 18:49:13]

Per Google style guide, uncommon abbreviations should not be used in variable
names.  I suggest just kPolicy.

[engedy, 2015/01/26 19:39:35]

Done.

+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(
+      true, kP.initial_delay_ms * (1 - kP.jitter_factor), kP.initial_delay_ms));
+  throttler->InformOfNetworkRequestComplete(true);
+
+  // After a successful request, the next one should be released immediately.
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+  throttler->InformOfNetworkRequestComplete(false);
+
+  // In general, the request after the n-th failure should be delayed by
+  //   |multiply_factor| ^ (n-1) * |initial_delay_ms|,
+  // spread out over Uniform(1 - |jitter_factor|, 1), up until
+  //   |maximum_backoff_ms|
+  // is reached.
+  for (size_t num_failures = 1; num_failures < 100; ++num_failures) {
+    throttler->SignalNetworkRequestNeeded();
+    double max_delay_ms =
+        kP.initial_delay_ms * pow(kP.multiply_factor, num_failures - 1);
+    double min_delay_ms = max_delay_ms * (1 - kP.jitter_factor);
+    if (max_delay_ms > kP.maximum_backoff_ms)
+      max_delay_ms = kP.maximum_backoff_ms;
+    if (min_delay_ms > kP.maximum_backoff_ms)
+      min_delay_ms = kP.maximum_backoff_ms;
+    ASSERT_NO_FATAL_FAILURE(
+        AssertReleaseInBetween(true, min_delay_ms, max_delay_ms));
+    throttler->InformOfNetworkRequestComplete(false);
+  }
+}
+
+TEST_F(AffiliationFetchThrottlerTest, OnCanSendNetworkRequestReturnsFalse) {
+  scoped_ptr<AffiliationFetchThrottler> throttler(CreateThrottler());
+
+  // A need for a network request is signaled, but as OnCanSendNetworkRequest()
+  // is called, the implementation returns false to indicate that the request
+  // will not be needed after all. InformOfNetworkRequestComplete() must not be
+  // called in this case.
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(false, 0, 0));
+
+  // A subsequent signaling, however, should result in OnCanSendNetworkRequest()
+  // being called immediately.
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+}
+
+TEST_F(AffiliationFetchThrottlerTest, GracePeriodAfterConnectivityIsRestored) {
+  SimulateHasNetworkConnectivity(false);
+  scoped_ptr<AffiliationFetchThrottler> throttler(CreateThrottler());
+
+  // After connectivity is restored, the first request should be delayed by the
+  // grace period, spread out over Uniform(1 - |jitter_factor|, 1).
+  throttler->SignalNetworkRequestNeeded();
+  AssertNoReleaseUntilNoTasksRemain();
+
+  SimulateHasNetworkConnectivity(true);
+  const auto& kP = AffiliationFetchThrottler::kBackoffPolicy;
+  const int64_t& kGraceMs =
+      AffiliationFetchThrottler::kGracePeriodAfterReconnectMs;
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(
+      true, kGraceMs * (1 - kP.jitter_factor), kGraceMs));
+  throttler->InformOfNetworkRequestComplete(true);
+
+  // The next request should not be delayed.
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+}
+
+// Same as GracePeriodAfterConnectivityIsRestored, but the network comes back
+// just before SignalNetworkRequestNeeded() is called.
+TEST_F(AffiliationFetchThrottlerTest, GracePeriodAfterConnectivityIsRestored2) {
+  SimulateHasNetworkConnectivity(false);
+  scoped_ptr<AffiliationFetchThrottler> throttler(CreateThrottler());
+
+  SimulateHasNetworkConnectivity(true);
+  throttler->SignalNetworkRequestNeeded();
+  const auto& kP = AffiliationFetchThrottler::kBackoffPolicy;
+  const int64_t& kGraceMs =
+      AffiliationFetchThrottler::kGracePeriodAfterReconnectMs;
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(
+      true, kGraceMs * (1 - kP.jitter_factor), kGraceMs));
+  throttler->InformOfNetworkRequestComplete(true);
+
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+}
+
+// Same as GracePeriodAfterConnectivityIsRestored, but the network goes down
+// just as SignalNetworkRequestNeeded() is constructed.
+TEST_F(AffiliationFetchThrottlerTest, GracePeriodAfterConnectivityIsRestored3) {
+  scoped_ptr<AffiliationFetchThrottler> throttler(CreateThrottler());
+  SimulateHasNetworkConnectivity(false);
+
+  throttler->SignalNetworkRequestNeeded();
+  AssertNoReleaseUntilNoTasksRemain();
+
+  SimulateHasNetworkConnectivity(true);
+  const auto& kP = AffiliationFetchThrottler::kBackoffPolicy;
+  const int64_t& kGraceMs =
+      AffiliationFetchThrottler::kGracePeriodAfterReconnectMs;
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(
+      true, kGraceMs * (1 - kP.jitter_factor), kGraceMs));
+  throttler->InformOfNetworkRequestComplete(true);
+
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+}
+
+TEST_F(AffiliationFetchThrottlerTest, ConnectivityLostDuringBackoff) {
+  scoped_ptr<AffiliationFetchThrottler> throttler(CreateThrottler());
+
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+  throttler->InformOfNetworkRequestComplete(false);
+
+  throttler->SignalNetworkRequestNeeded();
+  SimulateHasNetworkConnectivity(false);
+
+  // Let the exponential backoff delay expire, and verify nothing happens.
+  AssertNoReleaseUntilNoTasksRemain();
+
+  // Verify that the request is, however, sent after the normal grace period
+  // once connectivity is restored.
+  SimulateHasNetworkConnectivity(true);
+  const auto& kP = AffiliationFetchThrottler::kBackoffPolicy;
+  const int64_t& kGraceMs =
+      AffiliationFetchThrottler::kGracePeriodAfterReconnectMs;
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(
+      true, kGraceMs * (1 - kP.jitter_factor), kGraceMs));
+  throttler->InformOfNetworkRequestComplete(true);
+}
+
+TEST_F(AffiliationFetchThrottlerTest,
+       ConnectivityLostAndRestoredDuringBackoff) {
+  scoped_ptr<AffiliationFetchThrottler> throttler(CreateThrottler());
+
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+  throttler->InformOfNetworkRequestComplete(false);
+
+  // Simulate 10 flakes, and run for a total of 5 grace periods. This verifies
+  // that a flaky connection will not flood the task queue with lots of of tasks
+  // and also that release will not happen while the connection is flaky even
+  // once the first grace period has expired.
+  throttler->SignalNetworkRequestNeeded();
+  const auto& kP = AffiliationFetchThrottler::kBackoffPolicy;
+  const int64_t& kGraceMs =
+      AffiliationFetchThrottler::kGracePeriodAfterReconnectMs;
+  for (size_t t = 1; t <= 10; ++t) {
+    SimulateHasNetworkConnectivity(false);
+    AssertNoReleaseForMs(kGraceMs * (1 - kP.jitter_factor) / 2);
+    SimulateHasNetworkConnectivity(true);
+  }
+  EXPECT_EQ(1u, GetPendingTaskCount());
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(
+      true, kGraceMs * (1 - kP.jitter_factor), kGraceMs));
+}
+
+TEST_F(AffiliationFetchThrottlerTest, ConnectivityLostDuringRequest) {
+  scoped_ptr<AffiliationFetchThrottler> throttler(CreateThrottler());
+
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+
+  SimulateHasNetworkConnectivity(false);
+  AssertNoReleaseUntilNoTasksRemain();
+
+  throttler->InformOfNetworkRequestComplete(false);
+  AssertNoReleaseUntilNoTasksRemain();
+  throttler->SignalNetworkRequestNeeded();
+  AssertNoReleaseUntilNoTasksRemain();
+
+  SimulateHasNetworkConnectivity(true);
+
+  // Verify that the next request is released after the grace period once
+  // connectivity is restored.
+  const auto& kP = AffiliationFetchThrottler::kBackoffPolicy;
+  const int64_t& kGraceMs =
+      AffiliationFetchThrottler::kGracePeriodAfterReconnectMs;
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(
+      true, kGraceMs * (1 - kP.jitter_factor), kGraceMs));
+  throttler->InformOfNetworkRequestComplete(true);
+}
+
+TEST_F(AffiliationFetchThrottlerTest,
+       ConnectivityLostAndRestoredDuringRequest) {
+  scoped_ptr<AffiliationFetchThrottler> throttler(CreateThrottler());
+
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+
+  SimulateHasNetworkConnectivity(false);
+  AssertNoReleaseUntilNoTasksRemain();
+  SimulateHasNetworkConnectivity(true);
+  throttler->InformOfNetworkRequestComplete(true);
+  AssertNoReleaseUntilNoTasksRemain();
+
+  // Even though the previous request succeeded, the next request should still
+  // be held back for the normal grace period after connection is restored.
+  throttler->SignalNetworkRequestNeeded();
+  const auto& kP = AffiliationFetchThrottler::kBackoffPolicy;
+  const int64_t& kGraceMs =
+      AffiliationFetchThrottler::kGracePeriodAfterReconnectMs;
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(
+      true, kGraceMs * (1 - kP.jitter_factor), kGraceMs));
+  throttler->InformOfNetworkRequestComplete(true);
+}
+
+TEST_F(AffiliationFetchThrottlerTest, InstanceDestroyedWhileInBackoff) {
+  scoped_ptr<AffiliationFetchThrottler> throttler(CreateThrottler());
+
+  throttler->SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseInBetween(true, 0, 0));
+  throttler->InformOfNetworkRequestComplete(false);
+
+  throttler->SignalNetworkRequestNeeded();
+  throttler.reset();
+  EXPECT_EQ(1u, GetPendingTaskCount());
+  AssertNoReleaseUntilNoTasksRemain();
+}
+
+}  // namespace password_manager