Allow for counterfactual logging the predictions coming from opt guide

This adds a feature parameter within the
LoadingPredictorUseOptimizationGuide feature to dictate whether to
actually use the predictions for preconnect or anything else later.

I tested via browser tests by removing the DISABLED_ locally. I'm not
remove the DISABLED_ in real code since I still haven't been able to
repro the flake locally and don't know the root cause of the flake on
bots.

Bug: 1064121
Change-Id: I6a47645662b04760520c8eff4dd52ec7f55ca064
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2119372Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754111}

chrome/browser/predictors/loading_predictor_browsertest.cc

@@ -1521,15 +1521,18 @@ IN_PROC_BROWSER_TEST_F(LoadingPredictorBrowserTestWithProxy,
 }
 
 class LoadingPredictorBrowserTestWithOptimizationGuide
-    : public ::testing::WithParamInterface<bool>,
+    : public ::testing::WithParamInterface<std::tuple<bool, bool>>,
       public LoadingPredictorBrowserTest {
  public:
   LoadingPredictorBrowserTestWithOptimizationGuide() {
-    feature_list_.InitWithFeatures(
-        {features::kLoadingPredictorUseOptimizationGuide,
-         optimization_guide::features::kOptimizationHints},
+    feature_list_.InitWithFeaturesAndParameters(
+        {{features::kLoadingPredictorUseOptimizationGuide,
+          {{"use_predictions_for_preconnect",
+            ShouldPreconnectUsingOptimizationGuidePredictions() ? "true"
+                                                                : "false"}}},
+         {optimization_guide::features::kOptimizationHints, {}}},
         {});
-    if (GetParam()) {
+    if (IsLocalPredictionEnabled()) {
       local_predictions_feature_list_.InitAndEnableFeature(
           features::kLoadingPredictorUseLocalPredictions);
     } else {
@@ -1575,16 +1578,20 @@ class LoadingPredictorBrowserTestWithOptimizationGuide
         optimization_guide::switches::kHintsProtoOverride, config_string);
   }
 
-  bool IsLocalPredictionEnabled() const { return GetParam(); }
+  bool IsLocalPredictionEnabled() const { return std::get<0>(GetParam()); }
+
+  bool ShouldPreconnectUsingOptimizationGuidePredictions() const {
+    return std::get<1>(GetParam());
+  }
 
  private:
   base::test::ScopedFeatureList feature_list_;
   base::test::ScopedFeatureList local_predictions_feature_list_;
 };
 
-INSTANTIATE_TEST_SUITE_P(UseLocalPrediction,
+INSTANTIATE_TEST_SUITE_P(,
                          LoadingPredictorBrowserTestWithOptimizationGuide,
-                         ::testing::Bool());
+                         testing::Combine(testing::Bool(), testing::Bool()));
 
 // https://crbug.com/1056693
 #if (defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS))
@@ -1677,7 +1684,7 @@ IN_PROC_BROWSER_TEST_P(
   if (IsLocalPredictionEnabled()) {
     // Should use subresources that were learned.
     expected_subresource_hosts = {"baz.com", "foo.com"};
-  } else {
+  } else if (ShouldPreconnectUsingOptimizationGuidePredictions()) {
     // Should use subresources from optimization hint.
     expected_subresource_hosts = {"subresource.com", "otherresource.com"};
   }
@@ -1760,15 +1767,19 @@ IN_PROC_BROWSER_TEST_P(
     EXPECT_TRUE(preconnect_manager_observer()->HasOriginAttemptedToPreconnect(
         origin.GetURL()));
 
-    // Both subresource hosts should be preconnected to.
     for (auto* const host : {"subresource.com", "otherresource.com"}) {
-      preconnect_manager_observer()->WaitUntilHostLookedUp(
-          host, network_isolation_key);
-      EXPECT_TRUE(preconnect_manager_observer()->HostFound(
-          host, network_isolation_key));
+      if (ShouldPreconnectUsingOptimizationGuidePredictions()) {
+        // 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());
 
-      EXPECT_TRUE(preconnect_manager_observer()->HasOriginAttemptedToPreconnect(
-          GURL(base::StringPrintf("http://%s/", host))));
+      EXPECT_EQ(preconnect_manager_observer()->HasOriginAttemptedToPreconnect(
+                    GURL(base::StringPrintf("http://%s/", host))),
+                ShouldPreconnectUsingOptimizationGuidePredictions());
     }
 
     EXPECT_TRUE(observer->WaitForResponse());

chrome/browser/predictors/loading_predictor_tab_helper.cc

@@ -116,6 +116,10 @@ LoadingPredictorTabHelper::~LoadingPredictorTabHelper() = default;
 void LoadingPredictorTabHelper::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  // Clear out prediction from previous navigation.
+  last_optimization_guide_preconnect_prediction_ = base::nullopt;
+
   if (!predictor_)
     return;
 
@@ -240,7 +244,6 @@ void LoadingPredictorTabHelper::DocumentOnLoadCompletedInMainFrame() {
 
   predictor_->loading_data_collector()->RecordMainFrameLoadComplete(
       navigation_id, last_optimization_guide_preconnect_prediction_);
-  last_optimization_guide_preconnect_prediction_ = base::nullopt;
 }
 
 void LoadingPredictorTabHelper::OnOptimizationGuideDecision(
@@ -255,20 +258,33 @@ void LoadingPredictorTabHelper::OnOptimizationGuideDecision(
 
   ScopedOptimizationHintsReceiveStatusRecorder recorder;
 
-  if (!current_navigation_id_.is_valid()) {
-    // There is not a pending navigation, so return.
-    recorder.set_status(OptimizationHintsReceiveStatus::kAfterNavigationFinish);
-    return;
-  }
-  if (current_navigation_id_ != navigation_id) {
+  if (current_navigation_id_.is_valid() &&
+      current_navigation_id_ != navigation_id) {
     // The current navigation has either redirected or a new one has started, so
     // return.
     recorder.set_status(
         OptimizationHintsReceiveStatus::kAfterRedirectOrNextNavigationStart);
     return;
   }
+  if (!current_navigation_id_.is_valid()) {
+    // There is not a pending navigation.
+    recorder.set_status(OptimizationHintsReceiveStatus::kAfterNavigationFinish);
+
+    auto last_committed_navigation_id = NavigationID(web_contents());
+    if (!last_committed_navigation_id.is_valid() ||
+        last_committed_navigation_id != navigation_id) {
+      // This hint is no longer relevant, so return.
+      return;
+    }
 
-  recorder.set_status(OptimizationHintsReceiveStatus::kBeforeNavigationFinish);
+    // If we get here, we have not navigated away from the navigation we
+    // received hints for. Proceed to get the preconnect prediction so we can
+    // see how the predicted resources match what was actually fetched for
+    // counterfactual logging purposes.
+  } else {
+    recorder.set_status(
+        OptimizationHintsReceiveStatus::kBeforeNavigationFinish);
+  }
 
   if (decision != optimization_guide::OptimizationGuideDecision::kTrue)
     return;
@@ -295,9 +311,15 @@ void LoadingPredictorTabHelper::OnOptimizationGuideDecision(
                                      network_isolation_key);
   }
   last_optimization_guide_preconnect_prediction_ = prediction;
-  predictor_->PrepareForPageLoad(navigation_id.main_frame_url,
-                                 HintOrigin::OPTIMIZATION_GUIDE,
-                                 /*preconnectable=*/false, prediction);
+
+  // Only preconnect if the navigation is still pending and we want to use the
+  // predictions to preconnect to subresource origins.
+  if (current_navigation_id_.is_valid() &&
+      features::ShouldUseOptimizationGuidePredictionsToPreconnect()) {
+    predictor_->PrepareForPageLoad(navigation_id.main_frame_url,
+                                   HintOrigin::OPTIMIZATION_GUIDE,
+                                   /*preconnectable=*/false, prediction);
+  }
 }
 
 WEB_CONTENTS_USER_DATA_KEY_IMPL(LoadingPredictorTabHelper)

chrome/browser/predictors/predictors_features.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/predictors/predictors_features.h"
 
+#include "base/metrics/field_trial_params.h"
+
 namespace features {
 
 // Whether local predictions should be used to make preconnect predictions.
@@ -39,4 +41,13 @@ bool ShouldUseLocalPredictions() {
   return base::FeatureList::IsEnabled(kLoadingPredictorUseLocalPredictions);
 }
 
+bool ShouldUseOptimizationGuidePredictionsToPreconnect() {
+  if (!base::FeatureList::IsEnabled(kLoadingPredictorUseOptimizationGuide))
+    return false;
+
+  return base::GetFieldTrialParamByFeatureAsBool(
+      kLoadingPredictorUseOptimizationGuide, "use_predictions_for_preconnect",
+      true);
+}
+
 }  // namespace features

chrome/browser/predictors/predictors_features.h

@@ -28,6 +28,14 @@ extern const base::Feature kLoadingPredictorUseOptimizationGuide;
 // predictions.
 bool ShouldUseLocalPredictions();
 
+// Returns whether optimization guide predictions should be used to make
+// preconnect predictions.
+//
+// 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();
+
 }  // namespace features
 
 #endif  // CHROME_BROWSER_PREDICTORS_PREDICTORS_FEATURES_H_