Fix flakiness in previews browsertests

The previous logic relied upon the hints requested by the store upon a
navigation to a url being loaded prior to the commit. This was not
guaranteed to happen when cores were not available on a machine for
background threads.

The HintCache has been modified so that it now runs the hint loaded
callback even in the case where no hints are available for the host.

Additionally, a histogram has been added that tracks the loading of
hints from the hint cache store has in PreviewsOptimizationGuide. The
tests now run a priming navigation that waits for the hints to be
loaded. This ensures that the hints are guaranteed to be available
during the tests.

Bug: 923161
Change-Id: I8b09dfc9391164f0a6d424ec0a8ea871334c4c8c
Reviewed-on: https://chromium-review.googlesource.com/c/1423617
Commit-Queue: Jered Gray <jegray@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625056}

chrome/browser/previews/previews_browsertest.cc

@@ -120,6 +120,25 @@ class PreviewsBrowserTest : public InProcessBrowserTest {
   const GURL& https_no_transform_url() const { return https_no_transform_url_; }
   const GURL& http_url() const { return http_url_; }
   const GURL& redirect_url() const { return redirect_url_; }
+
+  // Triggers a navigation to |url| to prime the OptimizationGuide hints for the
+  // url's host and ensure that they have been loaded from the store (via
+  // histogram) prior to the navigation that tests functionality.
+  void LoadUrlHints(const GURL& url) {
+    base::HistogramTester histogram_tester;
+
+    ui_test_utils::NavigateToURL(browser(), url);
+
+    RetryForHistogramUntilCountReached(
+        &histogram_tester,
+        previews::kPreviewsOptimizationGuideOnLoadedHintResultHistogramString,
+        1);
+
+    // Reset state for any subsequent test.
+    noscript_css_requested_ = false;
+    noscript_js_requested_ = false;
+  }
+
   bool noscript_css_requested() const {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     return noscript_css_requested_;
@@ -259,11 +278,15 @@ class PreviewsNoScriptBrowserTest : public PreviewsBrowserTest {
 // script resource is not loaded.
 IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
                        MAYBE_NoScriptPreviewsEnabled) {
+  GURL url = https_url();
+
   // Whitelist test URL for NoScript.
-  SetUpNoScriptWhitelist({https_url().host()});
+  SetUpNoScriptWhitelist({url.host()});
+
+  LoadUrlHints(url);
 
   base::HistogramTester histogram_tester;
-  ui_test_utils::NavigateToURL(browser(), https_url());
+  ui_test_utils::NavigateToURL(browser(), url);
 
   // Verify loaded noscript tag triggered css resource but not js one.
   EXPECT_TRUE(noscript_css_requested());
@@ -275,10 +298,14 @@ IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
                        NoScriptPreviewsEnabledButHttpRequest) {
+  GURL url = http_url();
+
   // Whitelist test URL for NoScript.
-  SetUpNoScriptWhitelist({http_url().host()});
+  SetUpNoScriptWhitelist({url.host()});
 
-  ui_test_utils::NavigateToURL(browser(), http_url());
+  LoadUrlHints(url);
+
+  ui_test_utils::NavigateToURL(browser(), url);
 
   // Verify loaded js resource but not css triggered by noscript tag.
   EXPECT_TRUE(noscript_js_requested());
@@ -296,11 +323,15 @@ IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
 #endif
 IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
                        MAYBE_NoScriptPreviewsEnabledButNoTransformDirective) {
+  GURL url = https_no_transform_url();
+
   // Whitelist test URL for NoScript.
-  SetUpNoScriptWhitelist({https_no_transform_url().host()});
+  SetUpNoScriptWhitelist({url.host()});
+
+  LoadUrlHints(url);
 
   base::HistogramTester histogram_tester;
-  ui_test_utils::NavigateToURL(browser(), https_no_transform_url());
+  ui_test_utils::NavigateToURL(browser(), url);
 
   // Verify loaded js resource but not css triggered by noscript tag.
   EXPECT_TRUE(noscript_js_requested());
@@ -312,11 +343,15 @@ IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
                        MAYBE_NoScriptPreviewsEnabledHttpRedirectToHttps) {
+  GURL url = redirect_url();
+
   // Whitelist test URL for NoScript.
-  SetUpNoScriptWhitelist({redirect_url().host()});
+  SetUpNoScriptWhitelist({url.host()});
+
+  LoadUrlHints(url);
 
   base::HistogramTester histogram_tester;
-  ui_test_utils::NavigateToURL(browser(), redirect_url());
+  ui_test_utils::NavigateToURL(browser(), url);
 
   // Verify loaded noscript tag triggered css resource but not js one.
   EXPECT_TRUE(noscript_css_requested());
@@ -336,18 +371,22 @@ IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
 #endif
 IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
                        MAYBE_NoScriptPreviewsRecordsOptOut) {
+  GURL url = redirect_url();
+
   // Whitelist test URL for NoScript.
-  SetUpNoScriptWhitelist({redirect_url().host()});
+  SetUpNoScriptWhitelist({url.host()});
+
+  LoadUrlHints(url);
 
   base::HistogramTester histogram_tester;
 
   // Navigate to a No Script Preview page.
-  ui_test_utils::NavigateToURL(browser(), redirect_url());
+  ui_test_utils::NavigateToURL(browser(), url);
 
   // Terminate the previous page (non-opt out) and pull up a new No Script page.
-  ui_test_utils::NavigateToURL(browser(), redirect_url());
+  ui_test_utils::NavigateToURL(browser(), url);
   histogram_tester.ExpectUniqueSample("Previews.OptOut.UserOptedOut.NoScript",
-                                      0, 1);
+                                      0, 2);
 
   // Opt out of the No Script Preview page.
   PreviewsUITabHelper::FromWebContents(
@@ -372,10 +411,14 @@ IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
                        MAYBE_NoScriptPreviewsEnabledByWhitelist) {
+  GURL url = https_url();
+
   // Whitelist test URL for NoScript.
-  SetUpNoScriptWhitelist({https_url().host()});
+  SetUpNoScriptWhitelist({url.host()});
 
-  ui_test_utils::NavigateToURL(browser(), https_url());
+  LoadUrlHints(url);
+
+  ui_test_utils::NavigateToURL(browser(), url);
 
   // Verify loaded noscript tag triggered css resource but not js one.
   EXPECT_TRUE(noscript_css_requested());
@@ -384,10 +427,14 @@ IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
                        NoScriptPreviewsNotEnabledByWhitelist) {
+  GURL url = https_url();
+
   // Whitelist random site for NoScript.
   SetUpNoScriptWhitelist({"foo.com"});
 
-  ui_test_utils::NavigateToURL(browser(), https_url());
+  LoadUrlHints(url);
+
+  ui_test_utils::NavigateToURL(browser(), url);
 
   // Verify loaded js resource but not css triggered by noscript tag.
   EXPECT_TRUE(noscript_js_requested());

chrome/browser/previews/resource_loading_hints/resource_loading_hints_browsertest.cc

@@ -119,6 +119,20 @@ class ResourceLoadingNoFeaturesBrowserTest : public InProcessBrowserTest {
     cmd->AppendSwitch(previews::switches::kIgnorePreviewsBlacklist);
   }
 
+  // Triggers a navigation to |url| to prime the OptimizationGuide hints for the
+  // url's host and  ensure that they have been loaded from the store (via
+  // histogram) prior to the navigation that tests functionality.
+  void LoadUrlHints(const GURL& url) {
+    base::HistogramTester histogram_tester;
+
+    ui_test_utils::NavigateToURL(browser(), url);
+
+    RetryForHistogramUntilCountReached(
+        &histogram_tester,
+        previews::kPreviewsOptimizationGuideOnLoadedHintResultHistogramString,
+        1);
+  }
+
   void ProcessHintsComponent(
       const optimization_guide::HintsComponentInfo& component_info) {
     base::HistogramTester histogram_tester;
@@ -314,15 +328,19 @@ class ResourceLoadingHintsBrowserTest
 IN_PROC_BROWSER_TEST_F(
     ResourceLoadingHintsBrowserTest,
     DISABLE_ON_WIN_MAC(ResourceLoadingHintsHttpsWhitelisted)) {
-  SetExpectedFooJpgRequest(false);
-  SetExpectedBarJpgRequest(true);
+  GURL url = https_url();
 
   // Whitelist test URL for resource loading hints.
-  SetDefaultOnlyResourceLoadingHints({https_url().host()});
+  SetDefaultOnlyResourceLoadingHints({url.host()});
+  LoadUrlHints(url);
+
+  SetExpectedFooJpgRequest(false);
+  SetExpectedBarJpgRequest(true);
+  ResetResourceLoadingHintInterventionHeaderSeen();
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), https_url());
+  ui_test_utils::NavigateToURL(browser(), url);
 
   RetryForHistogramUntilCountReached(
       &histogram_tester, "ResourceLoadingHints.CountBlockedSubresourcePatterns",
@@ -347,7 +365,7 @@ IN_PROC_BROWSER_TEST_F(
   SetExpectedBarJpgRequest(true);
   ResetResourceLoadingHintInterventionHeaderSeen();
 
-  ui_test_utils::NavigateToURL(browser(), https_url());
+  ui_test_utils::NavigateToURL(browser(), url);
 
   RetryForHistogramUntilCountReached(
       &histogram_tester, "ResourceLoadingHints.CountBlockedSubresourcePatterns",
@@ -373,15 +391,19 @@ IN_PROC_BROWSER_TEST_F(
 IN_PROC_BROWSER_TEST_F(
     ResourceLoadingHintsBrowserTest,
     DISABLE_ON_WIN_MAC(ExperimentalHints_ExperimentIsNotEnabled)) {
-  SetExpectedFooJpgRequest(true);
-  SetExpectedBarJpgRequest(true);
+  GURL url = https_url();
 
   // Whitelist test URL for resource loading hints.
-  SetExperimentOnlyResourceLoadingHints({https_url().host()});
+  SetExperimentOnlyResourceLoadingHints({url.host()});
+  LoadUrlHints(url);
+
+  SetExpectedFooJpgRequest(true);
+  SetExpectedBarJpgRequest(true);
+  ResetResourceLoadingHintInterventionHeaderSeen();
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), https_url());
+  ui_test_utils::NavigateToURL(browser(), url);
 
   histogram_tester.ExpectBucketCount(
       "Previews.EligibilityReason.ResourceLoadingHints",
@@ -403,15 +425,20 @@ IN_PROC_BROWSER_TEST_F(
       previews::features::kOptimizationHintsExperiments,
       {{previews::features::kOptimizationHintsExperimentNameParam,
         optimization_guide::testing::kFooExperimentName}});
-  SetExpectedFooJpgRequest(false);
-  SetExpectedBarJpgRequest(true);
+
+  GURL url = https_url();
 
   // Whitelist test URL for resource loading hints.
-  SetExperimentOnlyResourceLoadingHints({https_url().host()});
+  SetExperimentOnlyResourceLoadingHints({url.host()});
+  LoadUrlHints(url);
+
+  SetExpectedFooJpgRequest(false);
+  SetExpectedBarJpgRequest(true);
+  ResetResourceLoadingHintInterventionHeaderSeen();
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), https_url());
+  ui_test_utils::NavigateToURL(browser(), url);
 
   RetryForHistogramUntilCountReached(
       &histogram_tester, "ResourceLoadingHints.CountBlockedSubresourcePatterns",
@@ -442,16 +469,21 @@ IN_PROC_BROWSER_TEST_F(
       previews::features::kOptimizationHintsExperiments,
       {{previews::features::kOptimizationHintsExperimentNameParam,
         optimization_guide::testing::kFooExperimentName}});
-  SetExpectedFooJpgRequest(false);
-  SetExpectedBarJpgRequest(true);
+
+  GURL url = https_url();
 
   // Whitelist test URL for resource loading hints. Set both experimental and
   // non-experimental hints.
-  SetMixResourceLoadingHints({https_url().host()});
+  SetMixResourceLoadingHints({url.host()});
+  LoadUrlHints(url);
+
+  SetExpectedFooJpgRequest(false);
+  SetExpectedBarJpgRequest(true);
+  ResetResourceLoadingHintInterventionHeaderSeen();
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), https_url());
+  ui_test_utils::NavigateToURL(browser(), url);
 
   RetryForHistogramUntilCountReached(
       &histogram_tester, "ResourceLoadingHints.CountBlockedSubresourcePatterns",
@@ -482,15 +514,20 @@ IN_PROC_BROWSER_TEST_F(
       previews::features::kOptimizationHintsExperiments,
       {{previews::features::kOptimizationHintsExperimentNameParam,
         "some_other_experiment"}});
-  SetExpectedFooJpgRequest(true);
-  SetExpectedBarJpgRequest(false);
+
+  GURL url = https_url();
 
   // Whitelist test URL for resource loading hints.
-  SetMixResourceLoadingHints({https_url().host()});
+  SetMixResourceLoadingHints({url.host()});
+  LoadUrlHints(url);
+
+  SetExpectedFooJpgRequest(true);
+  SetExpectedBarJpgRequest(false);
+  ResetResourceLoadingHintInterventionHeaderSeen();
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), https_url());
+  ui_test_utils::NavigateToURL(browser(), url);
 
   RetryForHistogramUntilCountReached(
       &histogram_tester, "ResourceLoadingHints.CountBlockedSubresourcePatterns",
@@ -500,22 +537,27 @@ IN_PROC_BROWSER_TEST_F(
       static_cast<int>(previews::PreviewsEligibilityReason::ALLOWED), 1);
   // Infobar would still be shown since there were at least one resource
   // loading hints available, even though none of them matched.
-  histogram_tester.ExpectTotalCount(
-      "Previews.InfoBarAction.ResourceLoadingHints", 1);
+  EXPECT_FALSE(histogram_tester
+                   .GetAllSamples("Previews.InfoBarAction.ResourceLoadingHints")
+                   .empty());
   EXPECT_TRUE(resource_loading_hint_intervention_header_seen());
 }
 
 IN_PROC_BROWSER_TEST_F(
     ResourceLoadingHintsBrowserTest,
     DISABLE_ON_WIN_MAC(ResourceLoadingHintsHttpsWhitelistedRedirectToHttps)) {
-  SetExpectedFooJpgRequest(false);
-  SetExpectedBarJpgRequest(true);
+  GURL url = redirect_url();
 
   SetDefaultOnlyResourceLoadingHints({https_url().host()});
+  LoadUrlHints(url);
+
+  SetExpectedFooJpgRequest(false);
+  SetExpectedBarJpgRequest(true);
+  ResetResourceLoadingHintInterventionHeaderSeen();
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), redirect_url());
+  ui_test_utils::NavigateToURL(browser(), url);
 
   RetryForHistogramUntilCountReached(
       &histogram_tester, "ResourceLoadingHints.CountBlockedSubresourcePatterns",
@@ -528,8 +570,9 @@ IN_PROC_BROWSER_TEST_F(
   histogram_tester.ExpectBucketCount(
       "Previews.EligibilityReason.ResourceLoadingHints",
       static_cast<int>(previews::PreviewsEligibilityReason::ALLOWED), 2);
-  histogram_tester.ExpectTotalCount(
-      "Previews.InfoBarAction.ResourceLoadingHints", 1);
+  EXPECT_FALSE(histogram_tester
+                   .GetAllSamples("Previews.InfoBarAction.ResourceLoadingHints")
+                   .empty());
   // SetDefaultOnlyResourceLoadingHints sets 3 resource loading hints patterns.
   histogram_tester.ExpectBucketCount(
       "ResourceLoadingHints.CountBlockedSubresourcePatterns", 3, 1);
@@ -539,15 +582,19 @@ IN_PROC_BROWSER_TEST_F(
 IN_PROC_BROWSER_TEST_F(
     ResourceLoadingHintsBrowserTest,
     DISABLE_ON_WIN_MAC(ResourceLoadingHintsHttpsNoWhitelisted)) {
-  SetExpectedFooJpgRequest(true);
-  SetExpectedBarJpgRequest(true);
+  GURL url = https_url();
 
   SetDefaultOnlyResourceLoadingHints({});
+  LoadUrlHints(url);
+
+  SetExpectedFooJpgRequest(true);
+  SetExpectedBarJpgRequest(true);
+  ResetResourceLoadingHintInterventionHeaderSeen();
 
   base::HistogramTester histogram_tester;
 
   // The URL is not whitelisted.
-  ui_test_utils::NavigateToURL(browser(), https_url());
+  ui_test_utils::NavigateToURL(browser(), url);
   base::RunLoop().RunUntilIdle();
 
   histogram_tester.ExpectBucketCount(
@@ -564,15 +611,19 @@ IN_PROC_BROWSER_TEST_F(
 
 IN_PROC_BROWSER_TEST_F(ResourceLoadingHintsBrowserTest,
                        DISABLE_ON_WIN_MAC(ResourceLoadingHintsHttp)) {
-  SetExpectedFooJpgRequest(true);
-  SetExpectedBarJpgRequest(true);
+  GURL url = http_url();
 
   // Whitelist test HTTP URL for resource loading hints.
   SetDefaultOnlyResourceLoadingHints({https_url().host()});
+  LoadUrlHints(url);
+
+  SetExpectedFooJpgRequest(true);
+  SetExpectedBarJpgRequest(true);
+  ResetResourceLoadingHintInterventionHeaderSeen();
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), http_url());
+  ui_test_utils::NavigateToURL(browser(), url);
   base::RunLoop().RunUntilIdle();
 
   histogram_tester.ExpectBucketCount(
@@ -588,15 +639,19 @@ IN_PROC_BROWSER_TEST_F(ResourceLoadingHintsBrowserTest,
 IN_PROC_BROWSER_TEST_F(
     ResourceLoadingHintsBrowserTest,
     DISABLE_ON_WIN_MAC(ResourceLoadingHintsHttpsWhitelistedNoTransform)) {
-  SetExpectedFooJpgRequest(true);
-  SetExpectedBarJpgRequest(true);
+  GURL url = https_no_transform_url();
 
   // Whitelist test URL for resource loading hints.
   SetDefaultOnlyResourceLoadingHints({https_url().host()});
+  LoadUrlHints(url);
+
+  SetExpectedFooJpgRequest(true);
+  SetExpectedBarJpgRequest(true);
+  ResetResourceLoadingHintInterventionHeaderSeen();
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), https_no_transform_url());
+  ui_test_utils::NavigateToURL(browser(), url);
   base::RunLoop().RunUntilIdle();
 
   histogram_tester.ExpectBucketCount(

components/previews/content/hint_cache.cc

@@ -74,6 +74,7 @@ void HintCache::LoadHint(const std::string& host, HintLoadedCallback callback) {
 
   HintCacheStore::EntryKey hint_entry_key;
   if (!FindHintEntryKey(host, &hint_entry_key)) {
+    std::move(callback).Run(nullptr);
     return;
   }
 

components/previews/content/hint_cache.h

@@ -63,8 +63,7 @@ class HintCache {
   bool HasHint(const std::string& host) const;
 
   // Requests that hint data for |host| be loaded asynchronously and passed to
-  // |callback| if/when loaded. |callback| will not be called if no hint data
-  // is found for |host|.
+  // |callback| if/when loaded.
   void LoadHint(const std::string& host, HintLoadedCallback callback);
 
   // Returns the hint data for |host| if found in memory, otherwise nullptr.

components/previews/content/previews_optimization_guide.cc

@@ -142,6 +142,11 @@ void PreviewsOptimizationGuide::OnLoadedHint(
     const optimization_guide::proto::Hint* loaded_hint) const {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
 
+  // Record that the hint finished loading. This is used as a signal during
+  // tests.
+  LOCAL_HISTOGRAM_BOOLEAN(
+      kPreviewsOptimizationGuideOnLoadedHintResultHistogramString, loaded_hint);
+
   // Run the callback now that the hint is loaded. This is used as a signal by
   // tests.
   std::move(callback).Run();

components/previews/core/previews_constants.cc

@@ -9,4 +9,7 @@ namespace previews {
 const char kPreviewsOptimizationGuideUpdateHintsResultHistogramString[] =
     "PreviewsOptimizationGuide.UpdateHints.Result";
 
+const char kPreviewsOptimizationGuideOnLoadedHintResultHistogramString[] =
+    "PreviewsOptimizationGuide.OnLoadedHint.Result";
+
 }  // namespace previews

components/previews/core/previews_constants.h

@@ -11,6 +11,10 @@ namespace previews {
 // UpdateHints().
 extern const char kPreviewsOptimizationGuideUpdateHintsResultHistogramString[];
 
+// The local histogram used by PreviewsOptimizationGuide to record that a hint
+// finished loading.
+extern const char kPreviewsOptimizationGuideOnLoadedHintResultHistogramString[];
+
 }  // namespace previews
 
 #endif  // COMPONENTS_PREVIEWS_CORE_PREVIEWS_CONSTANTS_H_