Marking fetches from search prefetch as viable to serve for 60 seconds

Search pages are considered stale after 60 seconds (finch param that
probably won't be set). This CL adds a OneShot Timer map entry match the
prefetch entry and removes them in sync as well. The Timer triggers
removal if it has not already occurred.

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

chrome/browser/prefetch/search_prefetch/field_trial_settings.cc

@@ -28,3 +28,10 @@ bool SearchPrefetchServicePrefetchingIsEnabled() {
              kSearchPrefetchServiceCommandLineFlag) ||
          base::FeatureList::IsEnabled(kSearchPrefetchServicePrefetching);
 }
+
+base::TimeDelta SearchPrefetchCachingLimit() {
+  return base::TimeDelta::FromMilliseconds(
+      base::GetFieldTrialParamByFeatureAsInt(kSearchPrefetchServicePrefetching,
+                                             "prefetch_caching_limit_ms",
+                                             60000));
+}

chrome/browser/prefetch/search_prefetch/field_trial_settings.h

@@ -6,12 +6,18 @@
 #define CHROME_BROWSER_PREFETCH_SEARCH_PREFETCH_FIELD_TRIAL_SETTINGS_H_
 
 #include "base/feature_list.h"
+#include "base/time/time.h"
 
 extern const base::Feature kSearchPrefetchService;
 extern const base::Feature kSearchPrefetchServicePrefetching;
 
+// Whether the search prefetch service and other objects should be created.
 bool SearchPrefetchServiceIsEnabled();
 
+// Whether the search prefetch service actually initiates prefetches.
 bool SearchPrefetchServicePrefetchingIsEnabled();
 
+// The amount of time a response is considered valid after the prefetch starts.
+base::TimeDelta SearchPrefetchCachingLimit();
+
 #endif  // CHROME_BROWSER_PREFETCH_SEARCH_PREFETCH_FIELD_TRIAL_SETTINGS_H_

chrome/browser/prefetch/search_prefetch/search_prefetch_service.cc

@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/location.h"
 #include "chrome/browser/prefetch/search_prefetch/field_trial_settings.h"
 #include "chrome/browser/prefetch/search_prefetch/prefetched_response_container.h"
 #include "chrome/browser/profiles/profile.h"
@@ -155,8 +156,13 @@ bool SearchPrefetchService::MaybePrefetchURL(const GURL& url) {
 
   prefetches_.emplace(search_terms, std::make_unique<PrefetchRequest>(url));
   prefetches_[search_terms]->StartPrefetchRequest(profile_);
+  prefetch_expiry_timers_.emplace(search_terms,
+                                  std::make_unique<base::OneShotTimer>());
+  prefetch_expiry_timers_[search_terms]->Start(
+      FROM_HERE, SearchPrefetchCachingLimit(),
+      base::BindOnce(&SearchPrefetchService::DeletePrefetch,
+                     base::Unretained(this), search_terms));
   return true;
-  // TODO(ryansturm): Expire entries after 60 seconds. https://crbug.com/1138639
 }
 
 base::Optional<SearchPrefetchStatus>
@@ -209,7 +215,16 @@ SearchPrefetchService::TakePrefetchResponse(const GURL& url) {
   // moved to the correct tab helper object, for now, the object can be deleted
   // entirely. Alternatively, the object can remain here with a new timeout in
   // a set of currently being served requests.
-  prefetches_.erase(iter);
+  DeletePrefetch(search_terms);
 
   return response;
 }
+
+void SearchPrefetchService::DeletePrefetch(base::string16 search_terms) {
+  DCHECK(prefetches_.find(search_terms) != prefetches_.end());
+  DCHECK(prefetch_expiry_timers_.find(search_terms) !=
+         prefetch_expiry_timers_.end());
+
+  prefetches_.erase(search_terms);
+  prefetch_expiry_timers_.erase(search_terms);
+}

chrome/browser/prefetch/search_prefetch/search_prefetch_service.h

@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "base/optional.h"
 #include "base/strings/string16.h"
+#include "base/timer/timer.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "services/network/public/cpp/simple_url_loader.h"
 #include "url/gurl.h"
@@ -46,6 +47,10 @@ class SearchPrefetchService : public KeyedService {
   base::Optional<SearchPrefetchStatus> GetSearchPrefetchStatusForTesting(
       base::string16 search_terms);
 
+ private:
+  // Removes the prefetch and prefetch timers associated with |search_terms|.
+  void DeletePrefetch(base::string16 search_terms);
+
   // Internal class to represent an ongoing or completed prefetch.
   class PrefetchRequest {
    public:
@@ -90,6 +95,10 @@ class SearchPrefetchService : public KeyedService {
   // prefetch expires.
   std::map<base::string16, std::unique_ptr<PrefetchRequest>> prefetches_;
 
+  // A group of timers to expire |prefetches_| based on the same key.
+  std::map<base::string16, std::unique_ptr<base::OneShotTimer>>
+      prefetch_expiry_timers_;
+
   Profile* profile_;
 };
 

chrome/browser/prefetch/search_prefetch/search_prefetch_service_browsertest.cc

@@ -5,6 +5,7 @@
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/time/time.h"
 #include "chrome/browser/prefetch/search_prefetch/field_trial_settings.h"
 #include "chrome/browser/prefetch/search_prefetch/search_prefetch_service.h"
 #include "chrome/browser/prefetch/search_prefetch/search_prefetch_service_factory.h"
@@ -111,12 +112,19 @@ class SearchPrefetchBaseBrowserTest : public InProcessBrowserTest {
         .ExtractString();
   }
 
+  void set_should_hang_requests(bool should_hang_requests) {
+    should_hang_requests_ = should_hang_requests;
+  }
+
  private:
   std::unique_ptr<net::test_server::HttpResponse> HandleSearchRequest(
       const net::test_server::HttpRequest& request) {
     if (request.GetURL().spec().find("favicon") != std::string::npos)
       return nullptr;
 
+    if (should_hang_requests_)
+      return std::make_unique<net::test_server::HungResponse>();
+
     bool is_prefetch =
         request.headers.find("Purpose") != request.headers.end() &&
         request.headers.find("Purpose")->second == "prefetch";
@@ -158,9 +166,10 @@ class SearchPrefetchBaseBrowserTest : public InProcessBrowserTest {
     }
   }
 
+  std::vector<net::test_server::HttpRequest> search_server_requests_;
   std::unique_ptr<net::EmbeddedTestServer> search_server_;
 
-  std::vector<net::test_server::HttpRequest> search_server_requests_;
+  bool should_hang_requests_ = false;
 
   size_t search_server_request_count_ = 0;
   size_t search_server_prefetch_request_count_ = 0;
@@ -423,3 +432,51 @@ IN_PROC_BROWSER_TEST_F(SearchPrefetchServiceEnabledBrowserTest,
   EXPECT_TRUE(base::Contains(inner_html, "regular"));
   EXPECT_FALSE(base::Contains(inner_html, "prefetch"));
 }
+
+class SearchPrefetchServiceZeroCacheTimeBrowserTest
+    : public SearchPrefetchBaseBrowserTest {
+ public:
+  SearchPrefetchServiceZeroCacheTimeBrowserTest() {
+    feature_list_.InitWithFeaturesAndParameters(
+        {{kSearchPrefetchServicePrefetching,
+          {{"prefetch_caching_limit_ms", "10"}}},
+         {{kSearchPrefetchService}, {}}},
+        {});
+
+    // Hang responses so the status will stay as InFlight until the entry is
+    // removed.
+    set_should_hang_requests(true);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(SearchPrefetchServiceZeroCacheTimeBrowserTest,
+                       ExpireAfterDuration) {
+  auto* search_prefetch_service =
+      SearchPrefetchServiceFactory::GetForProfile(browser()->profile());
+  EXPECT_NE(nullptr, search_prefetch_service);
+
+  std::string search_terms = "prefetch_content";
+
+  GURL prefetch_url = GetSearchServerQueryURL(search_terms);
+
+  EXPECT_TRUE(search_prefetch_service->MaybePrefetchURL(prefetch_url));
+
+  // Make sure a new fetch doesn't happen before expiry.
+  EXPECT_FALSE(search_prefetch_service->MaybePrefetchURL(prefetch_url));
+
+  auto prefetch_status =
+      search_prefetch_service->GetSearchPrefetchStatusForTesting(
+          base::ASCIIToUTF16(search_terms));
+  EXPECT_TRUE(prefetch_status.has_value());
+
+  WaitUntilStatusChanges(base::ASCIIToUTF16(search_terms));
+  prefetch_status = search_prefetch_service->GetSearchPrefetchStatusForTesting(
+      base::ASCIIToUTF16(search_terms));
+
+  // Prefetch should be gone now.
+  EXPECT_FALSE(prefetch_status.has_value());
+  EXPECT_TRUE(search_prefetch_service->MaybePrefetchURL(prefetch_url));
+}