Add TimeoutPolicy to PreviewsProber

Timeouts count as a failed attempt. I'm not using the SimpleURLLoader
timeout because I can't set it's TickClock for testing without calling
SetTickClockForTesting in the production code.

Bug: 977603
Change-Id: I6c4c6d0ebbc10f95942ca0e83011270735216c6e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1679222
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672707}

chrome/browser/previews/previews_prober.cc

@@ -47,18 +47,24 @@ PreviewsProber::RetryPolicy::RetryPolicy() = default;
 PreviewsProber::RetryPolicy::~RetryPolicy() = default;
 PreviewsProber::RetryPolicy::RetryPolicy(PreviewsProber::RetryPolicy const&) =
     default;
+PreviewsProber::TimeoutPolicy::TimeoutPolicy() = default;
+PreviewsProber::TimeoutPolicy::~TimeoutPolicy() = default;
+PreviewsProber::TimeoutPolicy::TimeoutPolicy(
+    PreviewsProber::TimeoutPolicy const&) = default;
 
 PreviewsProber::PreviewsProber(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     const std::string& name,
     const GURL& url,
     const HttpMethod http_method,
-    const RetryPolicy& retry_policy)
+    const RetryPolicy& retry_policy,
+    const TimeoutPolicy& timeout_policy)
     : PreviewsProber(url_loader_factory,
                      name,
                      url,
                      http_method,
                      retry_policy,
+                     timeout_policy,
                      base::DefaultTickClock::GetInstance()) {}
 
 PreviewsProber::PreviewsProber(
@@ -67,13 +73,15 @@ PreviewsProber::PreviewsProber(
     const GURL& url,
     const HttpMethod http_method,
     const RetryPolicy& retry_policy,
+    const TimeoutPolicy& timeout_policy,
     const base::TickClock* tick_clock)
     : name_(name),
       url_(url),
       http_method_(http_method),
       retry_policy_(retry_policy),
+      timeout_policy_(timeout_policy),
       successive_retry_count_(0),
-      retry_timer_(nullptr),
+      successive_timeout_count_(0),
       tick_clock_(tick_clock),
       is_active_(false),
       last_probe_status_(base::nullopt),
@@ -88,6 +96,7 @@ void PreviewsProber::SendNowIfInactive() {
     return;
 
   successive_retry_count_ = 0;
+  successive_timeout_count_ = 0;
   CreateAndStartURLLoader();
 }
 
@@ -135,7 +144,6 @@ void PreviewsProber::CreateAndStartURLLoader() {
   request->load_flags = net::LOAD_DISABLE_CACHE;
   request->allow_credentials = false;
 
-  // TODO(crbug/977603): Set retry options.
   url_loader_ =
       network::SimpleURLLoader::Create(std::move(request), traffic_annotation);
   url_loader_->SetAllowHttpErrorResults(true);
@@ -145,6 +153,17 @@ void PreviewsProber::CreateAndStartURLLoader() {
       base::BindOnce(&PreviewsProber::OnURLLoadComplete,
                      base::Unretained(this)),
       1024);
+
+  // We don't use SimpleURLLoader's timeout functionality because it is not
+  // possible to test by PreviewsProberTest.
+  base::TimeDelta ttl = ComputeNextTimeDeltaForBackoff(
+      timeout_policy_.backoff, timeout_policy_.base_timeout,
+      successive_timeout_count_);
+  timeout_timer_ = std::make_unique<base::OneShotTimer>(tick_clock_);
+  // base::Unretained is safe because |timeout_timer_| is owned by this.
+  timeout_timer_->Start(FROM_HERE, ttl,
+                        base::BindOnce(&PreviewsProber::ProcessProbeTimeout,
+                                       base::Unretained(this)));
 }
 
 void PreviewsProber::OnURLLoadComplete(
@@ -160,7 +179,9 @@ void PreviewsProber::OnURLLoadComplete(
   bool was_successful =
       url_loader_->NetError() == net::OK && response_code == net::HTTP_OK;
 
+  timeout_timer_.reset();
   url_loader_.reset();
+  successive_timeout_count_ = 0;
 
   if (was_successful) {
     ProcessProbeSuccess();
@@ -169,9 +190,20 @@ void PreviewsProber::OnURLLoadComplete(
   ProcessProbeFailure();
 }
 
+void PreviewsProber::ProcessProbeTimeout() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(url_loader_);
+
+  url_loader_.reset();
+  successive_timeout_count_++;
+  ProcessProbeFailure();
+}
+
 void PreviewsProber::ProcessProbeFailure() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(!retry_timer_ || !retry_timer_->IsRunning());
+  DCHECK(!timeout_timer_ || !timeout_timer_->IsRunning());
+  DCHECK(!url_loader_);
   DCHECK(is_active_);
 
   last_probe_status_ = false;
@@ -197,11 +229,14 @@ void PreviewsProber::ProcessProbeFailure() {
 void PreviewsProber::ProcessProbeSuccess() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(!retry_timer_ || !retry_timer_->IsRunning());
+  DCHECK(!timeout_timer_ || !timeout_timer_->IsRunning());
+  DCHECK(!url_loader_);
   DCHECK(is_active_);
 
   is_active_ = false;
   last_probe_status_ = true;
   successive_retry_count_ = 0;
+  successive_timeout_count_ = 0;
 }
 
 base::Optional<bool> PreviewsProber::LastProbeWasSuccessful() const {

chrome/browser/previews/previews_prober.h

@@ -60,6 +60,21 @@ class PreviewsProber {
     bool use_random_urls = false;
   };
 
+  struct TimeoutPolicy {
+    TimeoutPolicy();
+    TimeoutPolicy(const TimeoutPolicy& other);
+    ~TimeoutPolicy();
+
+    // How to compute the TTL of probes.
+    Backoff backoff = Backoff::kLinear;
+
+    // The TTL base value. For example,
+    //   LINEAR: Each probe times out in |base_timeout|.
+    //   EXPONENTIAL: Each probe times out in
+    //                (|base_timeout| * 2 ^ |successive_timeout_count_|).
+    base::TimeDelta base_timeout = base::TimeDelta::FromSeconds(60);
+  };
+
   enum class HttpMethod {
     kGet,
     kHead,
@@ -70,7 +85,8 @@ class PreviewsProber {
       const std::string& name,
       const GURL& url,
       HttpMethod http_method,
-      const RetryPolicy& retry_policy);
+      const RetryPolicy& retry_policy,
+      const TimeoutPolicy& timeout_policy);
   ~PreviewsProber();
 
   // Sends a probe now if the prober is currently inactive. If the probe is
@@ -91,11 +107,13 @@ class PreviewsProber {
       const GURL& url,
       HttpMethod http_method,
       const RetryPolicy& retry_policy,
+      const TimeoutPolicy& timeout_policy,
       const base::TickClock* tick_clock);
 
  private:
   void CreateAndStartURLLoader();
   void OnURLLoadComplete(std::unique_ptr<std::string> response_body);
+  void ProcessProbeTimeout();
   void ProcessProbeFailure();
   void ProcessProbeSuccess();
 
@@ -112,14 +130,23 @@ class PreviewsProber {
   // The retry policy to use in this prober.
   const RetryPolicy retry_policy_;
 
+  // The timeout policy to use in this prober.
+  const TimeoutPolicy timeout_policy_;
+
   // The number of retries that have been attempted. This count does not include
   // the original probe.
   size_t successive_retry_count_;
 
+  // The number of timeouts that have occurred.
+  size_t successive_timeout_count_;
+
   // If a retry is being attempted, this will be running until the next attempt.
   std::unique_ptr<base::OneShotTimer> retry_timer_;
 
-  // The tick clock used for |retry_timer_|.
+  // If a probe is being attempted, this will be running until the TTL.
+  std::unique_ptr<base::OneShotTimer> timeout_timer_;
+
+  // The tick clock used within this class.
   const base::TickClock* tick_clock_;
 
   // Whether the prober is currently sending probes.

chrome/browser/previews/previews_prober_unittest.cc

@@ -28,12 +28,14 @@ class TestPreviewsProber : public PreviewsProber {
       const GURL& url,
       const HttpMethod http_method,
       const RetryPolicy& retry_policy,
+      const TimeoutPolicy& timeout_policy,
       const base::TickClock* tick_clock)
       : PreviewsProber(url_loader_factory,
                        name,
                        url,
                        http_method,
                        retry_policy,
+                       timeout_policy,
                        tick_clock) {}
 };
 
@@ -47,17 +49,21 @@ class PreviewsProberTest : public testing::Test {
                 &test_url_loader_factory_)) {}
 
   std::unique_ptr<PreviewsProber> NewProber() {
-    return std::make_unique<TestPreviewsProber>(
-        test_shared_loader_factory_, kName, kTestUrl,
-        PreviewsProber::HttpMethod::kGet, PreviewsProber::RetryPolicy(),
-        scoped_task_environment_.GetMockTickClock());
+    return NewProberWithPolicies(PreviewsProber::RetryPolicy(),
+                                 PreviewsProber::TimeoutPolicy());
   }
 
   std::unique_ptr<PreviewsProber> NewProberWithRetryPolicy(
       const PreviewsProber::RetryPolicy& retry_policy) {
+    return NewProberWithPolicies(retry_policy, PreviewsProber::TimeoutPolicy());
+  }
+
+  std::unique_ptr<PreviewsProber> NewProberWithPolicies(
+      const PreviewsProber::RetryPolicy& retry_policy,
+      const PreviewsProber::TimeoutPolicy& timeout_policy) {
     return std::make_unique<TestPreviewsProber>(
         test_shared_loader_factory_, kName, kTestUrl,
-        PreviewsProber::HttpMethod::kGet, retry_policy,
+        PreviewsProber::HttpMethod::kGet, retry_policy, timeout_policy,
         scoped_task_environment_.GetMockTickClock());
   }
 
@@ -253,3 +259,75 @@ TEST_F(PreviewsProberTest, RetryExponential) {
   EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
   EXPECT_FALSE(prober->is_active());
 }
+
+TEST_F(PreviewsProberTest, TimeoutLinear) {
+  PreviewsProber::RetryPolicy retry_policy;
+  retry_policy.max_retries = 1;
+  retry_policy.base_interval = base::TimeDelta::FromMilliseconds(10);
+
+  PreviewsProber::TimeoutPolicy timeout_policy;
+  timeout_policy.backoff = PreviewsProber::Backoff::kLinear;
+  timeout_policy.base_timeout = base::TimeDelta::FromMilliseconds(1000);
+
+  std::unique_ptr<PreviewsProber> prober =
+      NewProberWithPolicies(retry_policy, timeout_policy);
+  EXPECT_EQ(prober->LastProbeWasSuccessful(), base::nullopt);
+
+  // First attempt.
+  prober->SendNowIfInactive();
+  VerifyRequest();
+  FastForward(base::TimeDelta::FromMilliseconds(999));
+  VerifyRequest();
+  FastForward(base::TimeDelta::FromMilliseconds(1));
+  VerifyNoRequests();
+  EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_TRUE(prober->is_active());
+
+  // Fast forward to the start of the next attempt.
+  FastForward(base::TimeDelta::FromMilliseconds(10));
+
+  // Second attempt should have the same timeout.
+  VerifyRequest();
+  FastForward(base::TimeDelta::FromMilliseconds(999));
+  VerifyRequest();
+  FastForward(base::TimeDelta::FromMilliseconds(1));
+  VerifyNoRequests();
+  EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_FALSE(prober->is_active());
+}
+
+TEST_F(PreviewsProberTest, TimeoutExponential) {
+  PreviewsProber::RetryPolicy retry_policy;
+  retry_policy.max_retries = 1;
+  retry_policy.base_interval = base::TimeDelta::FromMilliseconds(10);
+
+  PreviewsProber::TimeoutPolicy timeout_policy;
+  timeout_policy.backoff = PreviewsProber::Backoff::kExponential;
+  timeout_policy.base_timeout = base::TimeDelta::FromMilliseconds(1000);
+
+  std::unique_ptr<PreviewsProber> prober =
+      NewProberWithPolicies(retry_policy, timeout_policy);
+  EXPECT_EQ(prober->LastProbeWasSuccessful(), base::nullopt);
+
+  // First attempt.
+  prober->SendNowIfInactive();
+  VerifyRequest();
+  FastForward(base::TimeDelta::FromMilliseconds(999));
+  VerifyRequest();
+  FastForward(base::TimeDelta::FromMilliseconds(1));
+  VerifyNoRequests();
+  EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_TRUE(prober->is_active());
+
+  // Fast forward to the start of the next attempt.
+  FastForward(base::TimeDelta::FromMilliseconds(10));
+
+  // Second attempt should have a 2s timeout.
+  VerifyRequest();
+  FastForward(base::TimeDelta::FromMilliseconds(1999));
+  VerifyRequest();
+  FastForward(base::TimeDelta::FromMilliseconds(1));
+  VerifyNoRequests();
+  EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_FALSE(prober->is_active());
+}