Allow for prefetches to happen even if local preconnect predictions exist

Bug: 1108610
Change-Id: I427b51c39b8b62a33ad262d8b8e21b864709bff5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2324995
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792825}

chrome/browser/predictors/loading_predictor.cc

@@ -93,7 +93,7 @@ bool LoadingPredictor::PrepareForPageLoad(
                                                                &prediction);
   }
   if (active_hints_.find(url) != active_hints_.end() &&
-      has_local_preconnect_prediction) {
+      has_local_preconnect_prediction && !preconnect_prediction) {
     // We are currently preconnecting using the local preconnect prediction. Do
     // not proceed further.
     return true;

chrome/browser/predictors/loading_predictor_browsertest.cc

@@ -1575,7 +1575,8 @@ class LoadingPredictorBrowserTestWithOptimizationGuide
     feature_list_.InitWithFeaturesAndParameters(
         {{features::kLoadingPredictorUseOptimizationGuide,
           {{"use_predictions",
-            ShouldUseOptimizationGuidePredictions() ? "true" : "false"}}},
+            ShouldUseOptimizationGuidePredictions() ? "true" : "false"},
+           {"always_prefetch", "true"}}},
          {optimization_guide::features::kOptimizationHints, {}}},
         {});
     if (IsLocalPredictionEnabled()) {
@@ -1995,9 +1996,10 @@ class LoadingPredictorPrefetchBrowserTest
 };
 
 // Tests that the LoadingPredictor performs prefetching
-// for a navigation which it has a prediction for.
+// for a navigation which it has a prediction for and there isn't a local
+// prediction available.
 IN_PROC_BROWSER_TEST_P(LoadingPredictorPrefetchBrowserTest,
-                       PrepareForPageLoadWithPredictionForPrefetch) {
+                       PrepareForPageLoadWithPredictionForPrefetchNoLocalHint) {
   GURL url = embedded_test_server()->GetURL(
       "test.com", GetPathWithPortReplacement(kHtmlSubresourcesPath,
                                              embedded_test_server()->port()));
@@ -2044,14 +2046,82 @@ IN_PROC_BROWSER_TEST_P(LoadingPredictorPrefetchBrowserTest,
   EXPECT_TRUE(preconnect_manager_observer()->HostFound("preconnect.com",
                                                        network_isolation_key));
   EXPECT_TRUE(preconnect_manager_observer()->HasOriginAttemptedToPreconnect(
-      origin.GetURL()));
+      embedded_test_server()->GetURL("preconnect.com", "/")));
+}
+
+// Tests that the LoadingPredictor performs prefetching
+// for a navigation which it has a prediction for and there is a local
+// prediction available.
+IN_PROC_BROWSER_TEST_P(
+    LoadingPredictorPrefetchBrowserTest,
+    PrepareForPageLoadWithPredictionForPrefetchHasLocalHint) {
+  // Navigate the first time to fill the predictor's database and the HTTP
+  // cache.
+  GURL url = embedded_test_server()->GetURL(
+      "test.com", GetPathWithPortReplacement(kHtmlSubresourcesPath,
+                                             embedded_test_server()->port()));
+  ui_test_utils::NavigateToURL(browser(), url);
+  ResetNetworkState();
+
+  // Set up optimization hints.
+  std::vector<Subresource> hints = {
+      {"skipsoverinvalidurl/////",
+       optimization_guide::proto::RESOURCE_TYPE_CSS},
+      {embedded_test_server()->GetURL("subresource.com", "/css").spec(),
+       optimization_guide::proto::RESOURCE_TYPE_CSS},
+      {embedded_test_server()->GetURL("subresource.com", "/image").spec(),
+       optimization_guide::proto::RESOURCE_TYPE_UNKNOWN},
+      {embedded_test_server()->GetURL("otherresource.com", "/js").spec(),
+       optimization_guide::proto::RESOURCE_TYPE_SCRIPT},
+      {embedded_test_server()->GetURL("preconnect.com", "/other").spec(),
+       optimization_guide::proto::RESOURCE_TYPE_UNKNOWN, true},
+  };
+  SetUpOptimizationHint(url, hints);
+
+  // Expect these prefetches.
+  std::vector<GURL> requests;
+  if (GetSubresourceTypeParam() == "all") {
+    requests = {embedded_test_server()->GetURL("subresource.com", "/css"),
+                embedded_test_server()->GetURL("subresource.com", "/image"),
+                embedded_test_server()->GetURL("otherresource.com", "/js")};
+  } else if (GetSubresourceTypeParam() == "css") {
+    requests = {embedded_test_server()->GetURL("subresource.com", "/css")};
+  } else if (GetSubresourceTypeParam() == "js_css") {
+    requests = {embedded_test_server()->GetURL("subresource.com", "/css"),
+                embedded_test_server()->GetURL("otherresource.com", "/js")};
+  }
+  SetExpectedRequests(std::move(requests));
+
+  // Start a navigation and observe these prefetches.
+  auto observer = NavigateToURLAsync(url);
+  EXPECT_TRUE(observer->WaitForRequestStart());
+  WaitForRequests();
+
+  std::vector<std::string> expected_subresource_hosts;
+  if (IsLocalPredictionEnabled()) {
+    // Should use subresources that were learned.
+    expected_subresource_hosts = {"baz.com", "foo.com"};
+  } else {
+    // Should use subresources from optimization hint.
+    expected_subresource_hosts = {"preconnect.com"};
+  }
+  url::Origin origin = url::Origin::Create(url);
+  net::NetworkIsolationKey network_isolation_key(origin, origin);
+  for (const auto& host : expected_subresource_hosts) {
+    preconnect_manager_observer()->WaitUntilHostLookedUp(host,
+                                                         network_isolation_key);
+    EXPECT_TRUE(
+        preconnect_manager_observer()->HostFound(host, network_isolation_key));
+    EXPECT_TRUE(preconnect_manager_observer()->HasOriginAttemptedToPreconnect(
+        embedded_test_server()->GetURL(host, "/")));
+  }
 }
 
 INSTANTIATE_TEST_SUITE_P(
     ,
     LoadingPredictorPrefetchBrowserTest,
     testing::Combine(
-        /*IsLocalPredictionEnabled()=*/testing::Values(false),
+        /*IsLocalPredictionEnabled()=*/testing::Values(true, false),
         /*ShouldUseOptimizationGuidePredictions()=*/
         testing::Values(true),
         /*GetSubresourceType()=*/testing::Values("all", "css", "js_css")));

chrome/browser/predictors/loading_predictor_tab_helper.cc

@@ -170,8 +170,12 @@ void LoadingPredictorTabHelper::DidStartNavigation(
     return;
   current_navigation_id_ = navigation_id;
 
-  if (predictor_->OnNavigationStarted(navigation_id))
+  has_local_preconnect_predictions_for_current_navigation_ =
+      predictor_->OnNavigationStarted(navigation_id);
+  if (has_local_preconnect_predictions_for_current_navigation_ &&
+      !features::ShouldAlwaysPrefetchUsingOptimizationGuidePredictions()) {
     return;
+  }
 
   if (!optimization_guide_decider_)
     return;
@@ -180,12 +184,12 @@ void LoadingPredictorTabHelper::DidStartNavigation(
   last_optimization_guide_prediction_->decision =
       optimization_guide::OptimizationGuideDecision::kUnknown;
 
-  // We do not have any predictions on device, so consult the optimization
-  // guide.
   optimization_guide_decider_->CanApplyOptimizationAsync(
       navigation_handle, optimization_guide::proto::LOADING_PREDICTOR,
-      base::BindOnce(&LoadingPredictorTabHelper::OnOptimizationGuideDecision,
-                     weak_ptr_factory_.GetWeakPtr(), navigation_id));
+      base::BindOnce(
+          &LoadingPredictorTabHelper::OnOptimizationGuideDecision,
+          weak_ptr_factory_.GetWeakPtr(), navigation_id,
+          !has_local_preconnect_predictions_for_current_navigation_));
 }
 
 void LoadingPredictorTabHelper::DidRedirectNavigation(
@@ -204,6 +208,9 @@ void LoadingPredictorTabHelper::DidRedirectNavigation(
       navigation_handle->GetURL(), navigation_handle->NavigationStart());
   if (!navigation_id.is_valid())
     return;
+  bool is_same_origin_redirect =
+      url::Origin::Create(current_navigation_id_.main_frame_url) ==
+      url::Origin::Create(navigation_handle->GetURL());
   current_navigation_id_ = navigation_id;
 
   // If we are not trying to get an optimization guide prediction for this page
@@ -214,8 +221,11 @@ void LoadingPredictorTabHelper::DidRedirectNavigation(
   // Get an updated prediction for the navigation.
   optimization_guide_decider_->CanApplyOptimizationAsync(
       navigation_handle, optimization_guide::proto::LOADING_PREDICTOR,
-      base::BindOnce(&LoadingPredictorTabHelper::OnOptimizationGuideDecision,
-                     weak_ptr_factory_.GetWeakPtr(), navigation_id));
+      base::BindOnce(
+          &LoadingPredictorTabHelper::OnOptimizationGuideDecision,
+          weak_ptr_factory_.GetWeakPtr(), navigation_id,
+          !(has_local_preconnect_predictions_for_current_navigation_ &&
+            is_same_origin_redirect)));
 }
 
 void LoadingPredictorTabHelper::DidFinishNavigation(
@@ -227,8 +237,10 @@ void LoadingPredictorTabHelper::DidFinishNavigation(
   if (!IsHandledNavigation(navigation_handle))
     return;
 
-  // Clear out the current navigation since there is not one in flight anymore.
+  // Clear state for the current navigation since there is not one in flight
+  // anymore.
   current_navigation_id_ = NavigationID();
+  has_local_preconnect_predictions_for_current_navigation_ = false;
 
   auto old_navigation_id =
       NavigationID(web_contents(),
@@ -312,6 +324,7 @@ void LoadingPredictorTabHelper::DocumentOnLoadCompletedInMainFrame() {
 
 void LoadingPredictorTabHelper::OnOptimizationGuideDecision(
     const NavigationID& navigation_id,
+    bool should_add_preconnects_to_prediction,
     optimization_guide::OptimizationGuideDecision decision,
     const optimization_guide::OptimizationMetadata& metadata) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -392,7 +405,7 @@ void LoadingPredictorTabHelper::OnOptimizationGuideDecision(
         prediction.prefetch_requests.emplace_back(
             subresource_url, network_isolation_key, destination);
       }
-    } else {
+    } else if (should_add_preconnects_to_prediction) {
       url::Origin subresource_origin = url::Origin::Create(subresource_url);
       if (subresource_origin == main_frame_origin) {
         // We are already connecting to the main frame origin by default, so

chrome/browser/predictors/loading_predictor_tab_helper.h

@@ -67,6 +67,7 @@ class LoadingPredictorTabHelper
   // required to decide if it has remote predictions for the page load.
   void OnOptimizationGuideDecision(
       const NavigationID& navigation_id,
+      bool should_add_preconnects_to_prediction,
       optimization_guide::OptimizationGuideDecision decision,
       const optimization_guide::OptimizationMetadata& metadata);
 
@@ -75,6 +76,8 @@ class LoadingPredictorTabHelper
 
   NavigationID current_navigation_id_;
 
+  bool has_local_preconnect_predictions_for_current_navigation_ = false;
+
   // The optimization guide decider to consult for remote predictions.
   optimization_guide::OptimizationGuideDecider* optimization_guide_decider_ =
       nullptr;

chrome/browser/predictors/loading_predictor_unittest.cc

@@ -533,8 +533,8 @@ TEST_F(LoadingPredictorPreconnectTest,
       main_frame_url, HintOrigin::OPTIMIZATION_GUIDE, false, prediction));
 }
 
-// Checks that the predictor doesn't use the provided prediction if there is
-// already in flight and there was a local preconnect prediction.
+// Checks that the predictor uses a prediction even if there is already a local
+// initiated one in flight.
 TEST_F(
     LoadingPredictorPreconnectTest,
     TestPrepareForPageLoadPredictionProvidedButHasLocalPreconnectPrediction) {
@@ -574,7 +574,16 @@ TEST_F(
         network_isolation_key},
        {url::Origin::Create(GURL("http://cdn3.search.com")), 1,
         network_isolation_key}});
-  EXPECT_CALL(*mock_preconnect_manager_, StartProxy(_, _)).Times(0);
+  EXPECT_CALL(
+      *mock_preconnect_manager_,
+      StartProxy(main_frame_url,
+                 std::vector<PreconnectRequest>(
+                     {{url::Origin::Create(GURL("http://cdn1.search.com")), 1,
+                       network_isolation_key},
+                      {url::Origin::Create(GURL("http://cdn2.search.com")), 1,
+                       network_isolation_key},
+                      {url::Origin::Create(GURL("http://cdn3.search.com")), 1,
+                       network_isolation_key}})));
   EXPECT_TRUE(predictor_->PrepareForPageLoad(
       main_frame_url, HintOrigin::OPTIMIZATION_GUIDE, false, prediction));
 }

chrome/browser/predictors/predictors_features.cc

@@ -69,6 +69,14 @@ bool ShouldUseOptimizationGuidePredictions() {
       kLoadingPredictorUseOptimizationGuide, "use_predictions", true);
 }
 
+bool ShouldAlwaysPrefetchUsingOptimizationGuidePredictions() {
+  if (!base::FeatureList::IsEnabled(kLoadingPredictorPrefetch))
+    return false;
+
+  return base::GetFieldTrialParamByFeatureAsBool(
+      kLoadingPredictorUseOptimizationGuide, "always_prefetch", false);
+}
+
 size_t GetMaxInflightPreresolves() {
   return static_cast<size_t>(base::GetFieldTrialParamByFeatureAsInt(
       kLoadingPredictorInflightPredictiveActions, "max_inflight_preresolves",

chrome/browser/predictors/predictors_features.h

@@ -45,6 +45,11 @@ bool ShouldUseLocalPredictions();
 // predictions should actually be used.
 bool ShouldUseOptimizationGuidePredictions();
 
+// Returns whether optimization guide predictions should be used to prefetch
+// resources, even if local predictions are available for preconnect
+// predictions.
+bool ShouldAlwaysPrefetchUsingOptimizationGuidePredictions();
+
 // Returns the maximum number of preresolves that can be inflight at any given
 // time.
 size_t GetMaxInflightPreresolves();