Adding a backoff of 60 seconds on network error

When search prefetch service encounters a fetch error (either net or
non-200) the service will back off for 60 seconds (finch controllable).

Bug: 1138641
Change-Id: I9ee868b54b63088755a07a99d242bcc02db5dc1e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2491181
Commit-Queue: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820025}

chrome/browser/prefetch/search_prefetch/field_trial_settings.cc

@@ -41,3 +41,10 @@ size_t SearchPrefetchMaxAttemptsPerCachingDuration() {
       kSearchPrefetchServicePrefetching, "max_attempts_per_caching_duration",
       2);
 }
+
+base::TimeDelta SearchPrefetchErrorBackoffDuration() {
+  return base::TimeDelta::FromMilliseconds(
+      base::GetFieldTrialParamByFeatureAsInt(kSearchPrefetchServicePrefetching,
+                                             "error_backoff_duration_ms",
+                                             60000));
+}

chrome/browser/prefetch/search_prefetch/field_trial_settings.h

@@ -24,4 +24,8 @@ base::TimeDelta SearchPrefetchCachingLimit();
 // period as long as |SearchPrefetchCachingLimit()|
 size_t SearchPrefetchMaxAttemptsPerCachingDuration();
 
+// The amount of time that a network error will cause the search prefetch
+// service to stop prefetching responses.
+base::TimeDelta SearchPrefetchErrorBackoffDuration();
+
 #endif  // CHROME_BROWSER_PREFETCH_SEARCH_PREFETCH_FIELD_TRIAL_SETTINGS_H_

chrome/browser/prefetch/search_prefetch/search_prefetch_service.cc

@@ -25,8 +25,10 @@
 #include "url/origin.h"
 
 SearchPrefetchService::PrefetchRequest::PrefetchRequest(
-    const GURL& prefetch_url)
-    : prefetch_url_(prefetch_url) {}
+    const GURL& prefetch_url,
+    base::OnceClosure report_error_callback)
+    : prefetch_url_(prefetch_url),
+      report_error_callback_(std::move(report_error_callback)) {}
 
 SearchPrefetchService::PrefetchRequest::~PrefetchRequest() = default;
 
@@ -103,12 +105,14 @@ void SearchPrefetchService::PrefetchRequest::LoadDone(
   // error. https://crbug.com/1138641
   if (!success || response_body->empty()) {
     current_status_ = SearchPrefetchStatus::kRequestFailed;
+    std::move(report_error_callback_).Run();
     return;
   }
   if (simple_loader_->ResponseInfo() && simple_loader_->ResponseInfo()->headers)
     response_code = simple_loader_->ResponseInfo()->headers->response_code();
   if (response_code != net::HTTP_OK) {
     current_status_ = SearchPrefetchStatus::kRequestFailed;
+    std::move(report_error_callback_).Run();
     return;
   }
   current_status_ = SearchPrefetchStatus::kSuccessfullyCompleted;
@@ -149,6 +153,11 @@ bool SearchPrefetchService::MaybePrefetchURL(const GURL& url) {
   if (search_terms.size() == 0)
     return false;
 
+  if (last_error_time_ticks_ + SearchPrefetchErrorBackoffDuration() >
+      base::TimeTicks::Now()) {
+    return false;
+  }
+
   if (prefetches_.size() >= SearchPrefetchMaxAttemptsPerCachingDuration())
     return false;
 
@@ -157,7 +166,10 @@ bool SearchPrefetchService::MaybePrefetchURL(const GURL& url) {
     return false;
   }
 
-  prefetches_.emplace(search_terms, std::make_unique<PrefetchRequest>(url));
+  prefetches_.emplace(
+      search_terms, std::make_unique<PrefetchRequest>(
+                        url, base::BindOnce(&SearchPrefetchService::ReportError,
+                                            base::Unretained(this))));
   prefetches_[search_terms]->StartPrefetchRequest(profile_);
   prefetch_expiry_timers_.emplace(search_terms,
                                   std::make_unique<base::OneShotTimer>());
@@ -231,3 +243,7 @@ void SearchPrefetchService::DeletePrefetch(base::string16 search_terms) {
   prefetches_.erase(search_terms);
   prefetch_expiry_timers_.erase(search_terms);
 }
+
+void SearchPrefetchService::ReportError() {
+  last_error_time_ticks_ = base::TimeTicks::Now();
+}

chrome/browser/prefetch/search_prefetch/search_prefetch_service.h

@@ -51,12 +51,16 @@ class SearchPrefetchService : public KeyedService {
   // Removes the prefetch and prefetch timers associated with |search_terms|.
   void DeletePrefetch(base::string16 search_terms);
 
+  // Records the current time to prevent prefetches for a set duration.
+  void ReportError();
+
   // Internal class to represent an ongoing or completed prefetch.
   class PrefetchRequest {
    public:
     // |service| must outlive this class and be able to manage this class's
     // lifetime.
-    explicit PrefetchRequest(const GURL& prefetch_url);
+    PrefetchRequest(const GURL& prefetch_url,
+                    base::OnceClosure report_error_callback);
     ~PrefetchRequest();
 
     PrefetchRequest(const PrefetchRequest&) = delete;
@@ -88,6 +92,9 @@ class SearchPrefetchService : public KeyedService {
     // Once a prefetch is completed successfully, the associated prefetch data
     // and metadata about the request.
     std::unique_ptr<PrefetchedResponseContainer> prefetch_response_container_;
+
+    // Called when there is a network/server error on the prefetch request.
+    base::OnceClosure report_error_callback_;
   };
 
   // Prefetches that are started are stored using search terms as a key. Only
@@ -99,6 +106,9 @@ class SearchPrefetchService : public KeyedService {
   std::map<base::string16, std::unique_ptr<base::OneShotTimer>>
       prefetch_expiry_timers_;
 
+  // The time of the last prefetch network/server error.
+  base::TimeTicks last_error_time_ticks_;
+
   Profile* profile_;
 };
 

chrome/browser/prefetch/search_prefetch/search_prefetch_service_browsertest.cc

@@ -116,6 +116,13 @@ class SearchPrefetchBaseBrowserTest : public InProcessBrowserTest {
     should_hang_requests_ = should_hang_requests;
   }
 
+  void WaitForDuration(base::TimeDelta duration) {
+    base::RunLoop run_loop;
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE, run_loop.QuitClosure(), duration);
+    run_loop.Run();
+  }
+
  private:
   std::unique_ptr<net::test_server::HttpResponse> HandleSearchRequest(
       const net::test_server::HttpRequest& request) {
@@ -462,6 +469,31 @@ IN_PROC_BROWSER_TEST_F(SearchPrefetchServiceEnabledBrowserTest,
   EXPECT_FALSE(base::Contains(inner_html, "prefetch"));
 }
 
+IN_PROC_BROWSER_TEST_F(SearchPrefetchServiceEnabledBrowserTest,
+                       ErrorCausesNoFetch) {
+  auto* search_prefetch_service =
+      SearchPrefetchServiceFactory::GetForProfile(browser()->profile());
+  EXPECT_NE(nullptr, search_prefetch_service);
+
+  std::string search_terms = "502_on_prefetch";
+
+  GURL prefetch_url = GetSearchServerQueryURL(search_terms);
+
+  EXPECT_TRUE(search_prefetch_service->MaybePrefetchURL(prefetch_url));
+  auto prefetch_status =
+      search_prefetch_service->GetSearchPrefetchStatusForTesting(
+          base::ASCIIToUTF16(search_terms));
+  WaitUntilStatusChanges(base::ASCIIToUTF16(search_terms));
+
+  prefetch_status = search_prefetch_service->GetSearchPrefetchStatusForTesting(
+      base::ASCIIToUTF16(search_terms));
+  ASSERT_TRUE(prefetch_status.has_value());
+  EXPECT_EQ(SearchPrefetchStatus::kRequestFailed, prefetch_status.value());
+
+  EXPECT_FALSE(search_prefetch_service->MaybePrefetchURL(
+      GetSearchServerQueryURL("other_query")));
+}
+
 class SearchPrefetchServiceZeroCacheTimeBrowserTest
     : public SearchPrefetchBaseBrowserTest {
  public:
@@ -528,3 +560,45 @@ IN_PROC_BROWSER_TEST_F(SearchPrefetchServiceZeroCacheTimeBrowserTest,
   EXPECT_TRUE(search_prefetch_service->MaybePrefetchURL(
       GetSearchServerQueryURL("prefetch_4")));
 }
+
+class SearchPrefetchServiceZeroErrorTimeBrowserTest
+    : public SearchPrefetchBaseBrowserTest {
+ public:
+  SearchPrefetchServiceZeroErrorTimeBrowserTest() {
+    feature_list_.InitWithFeaturesAndParameters(
+        {{kSearchPrefetchServicePrefetching,
+          {{"error_backoff_duration_ms", "10"}}},
+         {{kSearchPrefetchService}, {}}},
+        {});
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(SearchPrefetchServiceZeroErrorTimeBrowserTest,
+                       ErrorClearedAfterDuration) {
+  auto* search_prefetch_service =
+      SearchPrefetchServiceFactory::GetForProfile(browser()->profile());
+  EXPECT_NE(nullptr, search_prefetch_service);
+
+  std::string search_terms = "502_on_prefetch";
+
+  GURL prefetch_url = GetSearchServerQueryURL(search_terms);
+
+  EXPECT_TRUE(search_prefetch_service->MaybePrefetchURL(prefetch_url));
+  auto prefetch_status =
+      search_prefetch_service->GetSearchPrefetchStatusForTesting(
+          base::ASCIIToUTF16(search_terms));
+  WaitUntilStatusChanges(base::ASCIIToUTF16(search_terms));
+
+  prefetch_status = search_prefetch_service->GetSearchPrefetchStatusForTesting(
+      base::ASCIIToUTF16(search_terms));
+  ASSERT_TRUE(prefetch_status.has_value());
+  EXPECT_EQ(SearchPrefetchStatus::kRequestFailed, prefetch_status.value());
+
+  WaitForDuration(base::TimeDelta::FromMilliseconds(30));
+
+  EXPECT_TRUE(search_prefetch_service->MaybePrefetchURL(
+      GetSearchServerQueryURL("other_query")));
+}