Allow for server to specify for some resources to be preconnect only even in prefetch mode

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

chrome/browser/predictors/loading_predictor.cc

@@ -256,9 +256,9 @@ void LoadingPredictor::MaybeAddPreconnect(const GURL& url,
   if (!prediction.prefetch_requests.empty()) {
     DCHECK(base::FeatureList::IsEnabled(features::kLoadingPredictorPrefetch));
     prefetch_manager()->Start(url, std::move(prediction.prefetch_requests));
-    return;
   }
-  preconnect_manager()->Start(url, std::move(prediction.requests));
+  if (!prediction.requests.empty())
+    preconnect_manager()->Start(url, std::move(prediction.requests));
 }
 
 void LoadingPredictor::MaybeRemovePreconnect(const GURL& url) {

chrome/browser/predictors/loading_predictor_browsertest.cc

@@ -1573,9 +1573,8 @@ class LoadingPredictorBrowserTestWithOptimizationGuide
   LoadingPredictorBrowserTestWithOptimizationGuide() {
     feature_list_.InitWithFeaturesAndParameters(
         {{features::kLoadingPredictorUseOptimizationGuide,
-          {{"use_predictions_for_preconnect",
-            ShouldPreconnectUsingOptimizationGuidePredictions() ? "true"
-                                                                : "false"}}},
+          {{"use_predictions",
+            ShouldUseOptimizationGuidePredictions() ? "true" : "false"}}},
          {optimization_guide::features::kOptimizationHints, {}}},
         {});
     if (IsLocalPredictionEnabled()) {
@@ -1589,7 +1588,7 @@ class LoadingPredictorBrowserTestWithOptimizationGuide
 
   bool IsLocalPredictionEnabled() const { return std::get<0>(GetParam()); }
 
-  bool ShouldPreconnectUsingOptimizationGuidePredictions() const {
+  bool ShouldUseOptimizationGuidePredictions() const {
     return std::get<1>(GetParam());
   }
 
@@ -1600,12 +1599,19 @@ class LoadingPredictorBrowserTestWithOptimizationGuide
   // A predicted subresource.
   struct Subresource {
     explicit Subresource(std::string url)
-        : url(url), type(optimization_guide::proto::RESOURCE_TYPE_UNKNOWN) {}
+        : url(url),
+          type(optimization_guide::proto::RESOURCE_TYPE_UNKNOWN),
+          preconnect_only(false) {}
     Subresource(std::string url, optimization_guide::proto::ResourceType type)
-        : url(url), type(type) {}
+        : url(url), type(type), preconnect_only(false) {}
+    Subresource(std::string url,
+                optimization_guide::proto::ResourceType type,
+                bool preconnect_only)
+        : url(url), type(type), preconnect_only(preconnect_only) {}
 
     std::string url;
     optimization_guide::proto::ResourceType type;
+    bool preconnect_only;
   };
 
   void SetUpOptimizationHint(
@@ -1620,6 +1626,7 @@ class LoadingPredictorBrowserTestWithOptimizationGuide
       auto* added = loading_predictor_metadata.add_subresources();
       added->set_url(subresource.url);
       added->set_resource_type(subresource.type);
+      added->set_preconnect_only(subresource.preconnect_only);
     }
 
     optimization_guide::OptimizationMetadata optimization_metadata;
@@ -1714,7 +1721,7 @@ IN_PROC_BROWSER_TEST_P(LoadingPredictorBrowserTestWithOptimizationGuide,
   if (IsLocalPredictionEnabled()) {
     // Should use subresources that were learned.
     expected_subresource_hosts = {"baz.com", "foo.com"};
-  } else if (ShouldPreconnectUsingOptimizationGuidePredictions()) {
+  } else if (ShouldUseOptimizationGuidePredictions()) {
     // Should use subresources from optimization hint.
     expected_subresource_hosts = {"subresource.com", "otherresource.com"};
   }
@@ -1761,18 +1768,18 @@ IN_PROC_BROWSER_TEST_P(LoadingPredictorBrowserTestWithOptimizationGuide,
       origin.GetURL()));
 
   for (auto* const host : {"subresource.com", "otherresource.com"}) {
-    if (ShouldPreconnectUsingOptimizationGuidePredictions()) {
+    if (ShouldUseOptimizationGuidePredictions()) {
       // Both subresource hosts should be preconnected to.
       preconnect_manager_observer()->WaitUntilHostLookedUp(
           host, network_isolation_key);
     }
     EXPECT_EQ(
         preconnect_manager_observer()->HostFound(host, network_isolation_key),
-        ShouldPreconnectUsingOptimizationGuidePredictions());
+        ShouldUseOptimizationGuidePredictions());
 
     EXPECT_EQ(preconnect_manager_observer()->HasOriginAttemptedToPreconnect(
                   GURL(base::StringPrintf("http://%s/", host))),
-              ShouldPreconnectUsingOptimizationGuidePredictions());
+              ShouldUseOptimizationGuidePredictions());
   }
 
   EXPECT_TRUE(observer->WaitForResponse());
@@ -1852,8 +1859,7 @@ IN_PROC_BROWSER_TEST_P(LoadingPredictorBrowserTestWithOptimizationGuide,
 
   std::vector<std::string> expected_opt_guide_subresource_hosts = {
       "subresource.com", "otherresource.com"};
-  if (!IsLocalPredictionEnabled() &&
-      ShouldPreconnectUsingOptimizationGuidePredictions()) {
+  if (!IsLocalPredictionEnabled() && ShouldUseOptimizationGuidePredictions()) {
     // Should use subresources from optimization hint.
     for (const auto& host : expected_opt_guide_subresource_hosts) {
       preconnect_manager_observer()->WaitUntilHostLookedUp(
@@ -2004,7 +2010,10 @@ IN_PROC_BROWSER_TEST_P(LoadingPredictorPrefetchBrowserTest,
       {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}};
+       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.
@@ -2025,6 +2034,16 @@ IN_PROC_BROWSER_TEST_P(LoadingPredictorPrefetchBrowserTest,
   auto observer = NavigateToURLAsync(url);
   EXPECT_TRUE(observer->WaitForRequestStart());
   WaitForRequests();
+
+  // preconnect.com should be preconnected to.
+  url::Origin origin = url::Origin::Create(url);
+  net::NetworkIsolationKey network_isolation_key(origin, origin);
+  preconnect_manager_observer()->WaitUntilHostLookedUp("preconnect.com",
+                                                       network_isolation_key);
+  EXPECT_TRUE(preconnect_manager_observer()->HostFound("preconnect.com",
+                                                       network_isolation_key));
+  EXPECT_TRUE(preconnect_manager_observer()->HasOriginAttemptedToPreconnect(
+      origin.GetURL()));
 }
 
 INSTANTIATE_TEST_SUITE_P(
@@ -2032,7 +2051,7 @@ INSTANTIATE_TEST_SUITE_P(
     LoadingPredictorPrefetchBrowserTest,
     testing::Combine(
         /*IsLocalPredictionEnabled()=*/testing::Values(false),
-        /*ShouldPreconnectUsingOptimizationGuidePredictions()=*/
+        /*ShouldUseOptimizationGuidePredictions()=*/
         testing::Values(true),
         /*GetSubresourceType()=*/testing::Values("all", "css", "js_css")));
 

chrome/browser/predictors/loading_predictor_tab_helper.cc

@@ -383,7 +383,8 @@ void LoadingPredictorTabHelper::OnOptimizationGuideDecision(
     if (!subresource_url.is_valid())
       continue;
     predicted_subresources.push_back(subresource_url);
-    if (base::FeatureList::IsEnabled(features::kLoadingPredictorPrefetch)) {
+    if (!subresource.preconnect_only() &&
+        base::FeatureList::IsEnabled(features::kLoadingPredictorPrefetch)) {
       network::mojom::RequestDestination destination =
           GetDestination(subresource.resource_type());
       if (ShouldPrefetchDestination(destination)) {
@@ -410,10 +411,10 @@ void LoadingPredictorTabHelper::OnOptimizationGuideDecision(
   last_optimization_guide_prediction_->predicted_subresources =
       predicted_subresources;
 
-  // Only preconnect if the navigation is still pending and we want to use the
-  // predictions to preconnect to subresource origins.
+  // Only prepare page load if the navigation is still pending and we want to
+  // use the predictions to pre* subresources.
   if (current_navigation_id_.is_valid() &&
-      features::ShouldUseOptimizationGuidePredictionsToPreconnect()) {
+      features::ShouldUseOptimizationGuidePredictions()) {
     predictor_->PrepareForPageLoad(navigation_id.main_frame_url,
                                    HintOrigin::OPTIMIZATION_GUIDE,
                                    /*preconnectable=*/false, prediction);

chrome/browser/predictors/loading_predictor_tab_helper_unittest.cc

@@ -694,6 +694,86 @@ TEST_F(
       OptimizationHintsReceiveStatus::kBeforeNavigationFinish, 1);
 }
 
+class LoadingPredictorTabHelperOptimizationGuideDeciderWithPrefetchTest
+    : public LoadingPredictorTabHelperOptimizationGuideDeciderTest {
+ public:
+  LoadingPredictorTabHelperOptimizationGuideDeciderWithPrefetchTest() {
+    scoped_feature_list_.InitWithFeatures(
+        {features::kLoadingPredictorUseOptimizationGuide,
+         features::kLoadingPredictorPrefetch,
+         // Need to add otherwise GetForProfile() returns null.
+         optimization_guide::features::kOptimizationHints},
+        {});
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+// Tests that document on load completed is recorded with correct navigation
+// id and optimization guide prediction.
+TEST_F(LoadingPredictorTabHelperOptimizationGuideDeciderWithPrefetchTest,
+       DocumentOnLoadCompletedOptimizationGuide) {
+  base::HistogramTester histogram_tester;
+
+  optimization_guide::OptimizationMetadata optimization_metadata;
+  optimization_guide::proto::LoadingPredictorMetadata lp_metadata;
+  lp_metadata.add_subresources()->set_url("http://test.org/resource1");
+  lp_metadata.add_subresources()->set_url("http://other.org/resource2");
+  lp_metadata.add_subresources()->set_url("http://other.org/resource3");
+  auto* preconnect_only_resource = lp_metadata.add_subresources();
+  preconnect_only_resource->set_url("http://preconnectonly.com/");
+  preconnect_only_resource->set_preconnect_only(true);
+  optimization_metadata.set_loading_predictor_metadata(lp_metadata);
+  EXPECT_CALL(
+      *mock_optimization_guide_keyed_service_,
+      CanApplyOptimizationAsync(_, optimization_guide::proto::LOADING_PREDICTOR,
+                                base::test::IsNotNullCallback()))
+      .WillOnce(base::test::RunOnceCallback<2>(
+          optimization_guide::OptimizationGuideDecision::kTrue,
+          ByRef(optimization_metadata)));
+  NavigateAndCommitInMainFrameAndVerifyMetrics("http://test.org");
+  auto navigation_id =
+      CreateNavigationID(GetTabID(), "http://test.org",
+                         web_contents()->GetMainFrame()->GetPageUkmSourceId());
+
+  // Adding subframe navigation to ensure that the committed main frame url will
+  // be used.
+  auto* subframe =
+      content::RenderFrameHostTester::For(main_rfh())->AppendChild("subframe");
+  NavigateAndCommitInFrame("http://sub.test.org", subframe);
+
+  base::Optional<OptimizationGuidePrediction> prediction =
+      OptimizationGuidePrediction();
+  prediction->decision = optimization_guide::OptimizationGuideDecision::kTrue;
+  url::Origin main_frame_origin = url::Origin::Create(GURL("http://test.org"));
+  net::NetworkIsolationKey network_isolation_key(main_frame_origin,
+                                                 main_frame_origin);
+  network::mojom::RequestDestination destination =
+      network::mojom::RequestDestination::kEmpty;
+  PreconnectPrediction preconnect_prediction = CreatePreconnectPrediction(
+      "", false,
+      {{url::Origin::Create(GURL("http://preconnectonly.com/")), 1,
+        network_isolation_key}});
+  preconnect_prediction.prefetch_requests.emplace_back(
+      GURL("http://test.org/resource1"), network_isolation_key, destination);
+  preconnect_prediction.prefetch_requests.emplace_back(
+      GURL("http://other.org/resource1"), network_isolation_key, destination);
+  preconnect_prediction.prefetch_requests.emplace_back(
+      GURL("http://other.org/resource2"), network_isolation_key, destination);
+  prediction->preconnect_prediction = preconnect_prediction;
+  prediction->predicted_subresources = {
+      GURL("http://test.org/resource1"), GURL("http://other.org/resource2"),
+      GURL("http://other.org/resource3"), GURL("http://preconnectonly.com/")};
+  EXPECT_CALL(*mock_collector_,
+              RecordMainFrameLoadComplete(navigation_id, prediction));
+  tab_helper_->DocumentOnLoadCompletedInMainFrame();
+
+  histogram_tester.ExpectUniqueSample(
+      "LoadingPredictor.OptimizationHintsReceiveStatus",
+      OptimizationHintsReceiveStatus::kBeforeNavigationFinish, 1);
+}
+
 class TestLoadingDataCollector : public LoadingDataCollector {
  public:
   explicit TestLoadingDataCollector(const LoadingPredictorConfig& config);

chrome/browser/predictors/predictors_features.cc

@@ -57,13 +57,12 @@ bool ShouldUseLocalPredictions() {
   return base::FeatureList::IsEnabled(kLoadingPredictorUseLocalPredictions);
 }
 
-bool ShouldUseOptimizationGuidePredictionsToPreconnect() {
+bool ShouldUseOptimizationGuidePredictions() {
   if (!base::FeatureList::IsEnabled(kLoadingPredictorUseOptimizationGuide))
     return false;
 
   return base::GetFieldTrialParamByFeatureAsBool(
-      kLoadingPredictorUseOptimizationGuide, "use_predictions_for_preconnect",
-      true);
+      kLoadingPredictorUseOptimizationGuide, "use_predictions", true);
 }
 
 }  // namespace features

chrome/browser/predictors/predictors_features.h

@@ -36,12 +36,12 @@ extern const base::FeatureParam<PrefetchSubresourceType>
 bool ShouldUseLocalPredictions();
 
 // Returns whether optimization guide predictions should be used to make
-// preconnect predictions.
+// loading predictions, such as preconnect or prefetch.
 //
 // In addition to checking whether the feature is enabled, this will
 // additionally check a feature parameter is specified to dictate if the
-// predictions should be used to preconnect to subresource origins.
-bool ShouldUseOptimizationGuidePredictionsToPreconnect();
+// predictions should actually be used.
+bool ShouldUseOptimizationGuidePredictions();
 
 }  // namespace features
 

chrome/browser/predictors/prefetch_manager.cc

@@ -285,6 +285,9 @@ void PrefetchManager::OnPrefetchFinished(
 void PrefetchManager::TryToLaunchPrefetchJobs() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  if (queued_jobs_.empty() || inflight_jobs_count_ >= kMaxInflightJobs)
+    return;
+
   // TODO(falken): Is it ok to assume the default partition? Try to plumb the
   // partition here, e.g., from WebContentsObserver. And make a similar change
   // in PreconnectManager.

chrome/browser/predictors/resource_prefetch_predictor.h

@@ -93,8 +93,9 @@ struct PrefetchRequest {
   network::mojom::RequestDestination destination;
 };
 
-// Stores a result of preconnect prediction. The |requests| vector is the main
-// result of prediction and other fields are used for histograms reporting.
+// Stores a result of pre* prediction. The |requests| vector is the main
+// result for preconnects, while the |prefetch_requests| vector is the main
+// result for prefetches. Other fields are used for metrics reporting.
 struct PreconnectPrediction {
   PreconnectPrediction();
   PreconnectPrediction(const PreconnectPrediction& other);
@@ -107,9 +108,6 @@ struct PreconnectPrediction {
   bool is_redirected = false;
   std::string host;
   std::vector<PreconnectRequest> requests;
-
-  // For LoadingPredictorPrefetch. When |prefetch_requests| is non-empty, it is
-  // used instead of |requests|.
   std::vector<PrefetchRequest> prefetch_requests;
 };
 

components/optimization_guide/proto/loading_predictor_metadata.proto

@@ -27,6 +27,10 @@ message Resource {
   optional string url = 1;
   // The resource type of the resource.
   optional ResourceType resource_type = 2;
+  // Specifies that this resource should only be preconnected to and not
+  // prefetched due to low confidence that this resource will actually be
+  // fetched for the page load.
+  optional bool preconnect_only = 3;
 }
 
 // Optimization metadata associated with improving the loading predictor.