Revert "Fix flakiness in previews browsertests"

This reverts commit ddd4f721.

Reason for revert: Looks like some tests become flaky (or even flakier?)

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-dbg/10417
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-dbg/10418
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-dbg/10419
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/31176

Original change's description:
> 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}

TBR=tbansal@chromium.org,dougarnett@chromium.org,jegray@chromium.org

Change-Id: I7d7c3393765e2d24d02c49fd5f8be95dd8eebc86
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 923161
Reviewed-on: https://chromium-review.googlesource.com/c/1429879Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625136}

chrome/browser/previews/previews_browsertest.cc

@@ -120,25 +120,6 @@ 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_;
@@ -278,15 +259,11 @@ 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({url.host()});
-
-  LoadUrlHints(url);
+  SetUpNoScriptWhitelist({https_url().host()});
 
   base::HistogramTester histogram_tester;
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), https_url());
 
   // Verify loaded noscript tag triggered css resource but not js one.
   EXPECT_TRUE(noscript_css_requested());
@@ -298,14 +275,10 @@ IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
                        NoScriptPreviewsEnabledButHttpRequest) {
-  GURL url = http_url();
-
   // Whitelist test URL for NoScript.
-  SetUpNoScriptWhitelist({url.host()});
+  SetUpNoScriptWhitelist({http_url().host()});
 
-  LoadUrlHints(url);
-
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), http_url());
 
   // Verify loaded js resource but not css triggered by noscript tag.
   EXPECT_TRUE(noscript_js_requested());
@@ -323,15 +296,11 @@ 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({url.host()});
-
-  LoadUrlHints(url);
+  SetUpNoScriptWhitelist({https_no_transform_url().host()});
 
   base::HistogramTester histogram_tester;
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), https_no_transform_url());
 
   // Verify loaded js resource but not css triggered by noscript tag.
   EXPECT_TRUE(noscript_js_requested());
@@ -343,15 +312,11 @@ IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
                        MAYBE_NoScriptPreviewsEnabledHttpRedirectToHttps) {
-  GURL url = redirect_url();
-
   // Whitelist test URL for NoScript.
-  SetUpNoScriptWhitelist({url.host()});
-
-  LoadUrlHints(url);
+  SetUpNoScriptWhitelist({redirect_url().host()});
 
   base::HistogramTester histogram_tester;
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), redirect_url());
 
   // Verify loaded noscript tag triggered css resource but not js one.
   EXPECT_TRUE(noscript_css_requested());
@@ -371,22 +336,18 @@ 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({url.host()});
-
-  LoadUrlHints(url);
+  SetUpNoScriptWhitelist({redirect_url().host()});
 
   base::HistogramTester histogram_tester;
 
   // Navigate to a No Script Preview page.
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), redirect_url());
 
   // Terminate the previous page (non-opt out) and pull up a new No Script page.
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), redirect_url());
   histogram_tester.ExpectUniqueSample("Previews.OptOut.UserOptedOut.NoScript",
-                                      0, 2);
+                                      0, 1);
 
   // Opt out of the No Script Preview page.
   PreviewsUITabHelper::FromWebContents(
@@ -411,14 +372,10 @@ IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
                        MAYBE_NoScriptPreviewsEnabledByWhitelist) {
-  GURL url = https_url();
-
   // Whitelist test URL for NoScript.
-  SetUpNoScriptWhitelist({url.host()});
-
-  LoadUrlHints(url);
+  SetUpNoScriptWhitelist({https_url().host()});
 
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), https_url());
 
   // Verify loaded noscript tag triggered css resource but not js one.
   EXPECT_TRUE(noscript_css_requested());
@@ -427,14 +384,10 @@ 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"});
 
-  LoadUrlHints(url);
-
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), https_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,20 +119,6 @@ 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;
@@ -328,19 +314,15 @@ class ResourceLoadingHintsBrowserTest
 IN_PROC_BROWSER_TEST_F(
     ResourceLoadingHintsBrowserTest,
     DISABLE_ON_WIN_MAC(ResourceLoadingHintsHttpsWhitelisted)) {
-  GURL url = https_url();
-
-  // Whitelist test URL for resource loading hints.
-  SetDefaultOnlyResourceLoadingHints({url.host()});
-  LoadUrlHints(url);
-
   SetExpectedFooJpgRequest(false);
   SetExpectedBarJpgRequest(true);
-  ResetResourceLoadingHintInterventionHeaderSeen();
+
+  // Whitelist test URL for resource loading hints.
+  SetDefaultOnlyResourceLoadingHints({https_url().host()});
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), https_url());
 
   RetryForHistogramUntilCountReached(
       &histogram_tester, "ResourceLoadingHints.CountBlockedSubresourcePatterns",
@@ -365,7 +347,7 @@ IN_PROC_BROWSER_TEST_F(
   SetExpectedBarJpgRequest(true);
   ResetResourceLoadingHintInterventionHeaderSeen();
 
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), https_url());
 
   RetryForHistogramUntilCountReached(
       &histogram_tester, "ResourceLoadingHints.CountBlockedSubresourcePatterns",
@@ -391,19 +373,15 @@ IN_PROC_BROWSER_TEST_F(
 IN_PROC_BROWSER_TEST_F(
     ResourceLoadingHintsBrowserTest,
     DISABLE_ON_WIN_MAC(ExperimentalHints_ExperimentIsNotEnabled)) {
-  GURL url = https_url();
-
-  // Whitelist test URL for resource loading hints.
-  SetExperimentOnlyResourceLoadingHints({url.host()});
-  LoadUrlHints(url);
-
   SetExpectedFooJpgRequest(true);
   SetExpectedBarJpgRequest(true);
-  ResetResourceLoadingHintInterventionHeaderSeen();
+
+  // Whitelist test URL for resource loading hints.
+  SetExperimentOnlyResourceLoadingHints({https_url().host()});
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), https_url());
 
   histogram_tester.ExpectBucketCount(
       "Previews.EligibilityReason.ResourceLoadingHints",
@@ -425,20 +403,15 @@ IN_PROC_BROWSER_TEST_F(
       previews::features::kOptimizationHintsExperiments,
       {{previews::features::kOptimizationHintsExperimentNameParam,
         optimization_guide::testing::kFooExperimentName}});
-
-  GURL url = https_url();
-
-  // Whitelist test URL for resource loading hints.
-  SetExperimentOnlyResourceLoadingHints({url.host()});
-  LoadUrlHints(url);
-
   SetExpectedFooJpgRequest(false);
   SetExpectedBarJpgRequest(true);
-  ResetResourceLoadingHintInterventionHeaderSeen();
+
+  // Whitelist test URL for resource loading hints.
+  SetExperimentOnlyResourceLoadingHints({https_url().host()});
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), https_url());
 
   RetryForHistogramUntilCountReached(
       &histogram_tester, "ResourceLoadingHints.CountBlockedSubresourcePatterns",
@@ -469,21 +442,16 @@ IN_PROC_BROWSER_TEST_F(
       previews::features::kOptimizationHintsExperiments,
       {{previews::features::kOptimizationHintsExperimentNameParam,
         optimization_guide::testing::kFooExperimentName}});
-
-  GURL url = https_url();
+  SetExpectedFooJpgRequest(false);
+  SetExpectedBarJpgRequest(true);
 
   // Whitelist test URL for resource loading hints. Set both experimental and
   // non-experimental hints.
-  SetMixResourceLoadingHints({url.host()});
-  LoadUrlHints(url);
-
-  SetExpectedFooJpgRequest(false);
-  SetExpectedBarJpgRequest(true);
-  ResetResourceLoadingHintInterventionHeaderSeen();
+  SetMixResourceLoadingHints({https_url().host()});
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), https_url());
 
   RetryForHistogramUntilCountReached(
       &histogram_tester, "ResourceLoadingHints.CountBlockedSubresourcePatterns",
@@ -514,20 +482,15 @@ IN_PROC_BROWSER_TEST_F(
       previews::features::kOptimizationHintsExperiments,
       {{previews::features::kOptimizationHintsExperimentNameParam,
         "some_other_experiment"}});
-
-  GURL url = https_url();
-
-  // Whitelist test URL for resource loading hints.
-  SetMixResourceLoadingHints({url.host()});
-  LoadUrlHints(url);
-
   SetExpectedFooJpgRequest(true);
   SetExpectedBarJpgRequest(false);
-  ResetResourceLoadingHintInterventionHeaderSeen();
+
+  // Whitelist test URL for resource loading hints.
+  SetMixResourceLoadingHints({https_url().host()});
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), https_url());
 
   RetryForHistogramUntilCountReached(
       &histogram_tester, "ResourceLoadingHints.CountBlockedSubresourcePatterns",
@@ -537,27 +500,22 @@ 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.
-  EXPECT_FALSE(histogram_tester
-                   .GetAllSamples("Previews.InfoBarAction.ResourceLoadingHints")
-                   .empty());
+  histogram_tester.ExpectTotalCount(
+      "Previews.InfoBarAction.ResourceLoadingHints", 1);
   EXPECT_TRUE(resource_loading_hint_intervention_header_seen());
 }
 
 IN_PROC_BROWSER_TEST_F(
     ResourceLoadingHintsBrowserTest,
     DISABLE_ON_WIN_MAC(ResourceLoadingHintsHttpsWhitelistedRedirectToHttps)) {
-  GURL url = redirect_url();
-
-  SetDefaultOnlyResourceLoadingHints({https_url().host()});
-  LoadUrlHints(url);
-
   SetExpectedFooJpgRequest(false);
   SetExpectedBarJpgRequest(true);
-  ResetResourceLoadingHintInterventionHeaderSeen();
+
+  SetDefaultOnlyResourceLoadingHints({https_url().host()});
 
   base::HistogramTester histogram_tester;
 
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), redirect_url());
 
   RetryForHistogramUntilCountReached(
       &histogram_tester, "ResourceLoadingHints.CountBlockedSubresourcePatterns",
@@ -570,9 +528,8 @@ IN_PROC_BROWSER_TEST_F(
   histogram_tester.ExpectBucketCount(
       "Previews.EligibilityReason.ResourceLoadingHints",
       static_cast<int>(previews::PreviewsEligibilityReason::ALLOWED), 2);
-  EXPECT_FALSE(histogram_tester
-                   .GetAllSamples("Previews.InfoBarAction.ResourceLoadingHints")
-                   .empty());
+  histogram_tester.ExpectTotalCount(
+      "Previews.InfoBarAction.ResourceLoadingHints", 1);
   // SetDefaultOnlyResourceLoadingHints sets 3 resource loading hints patterns.
   histogram_tester.ExpectBucketCount(
       "ResourceLoadingHints.CountBlockedSubresourcePatterns", 3, 1);
@@ -582,19 +539,15 @@ IN_PROC_BROWSER_TEST_F(
 IN_PROC_BROWSER_TEST_F(
     ResourceLoadingHintsBrowserTest,
     DISABLE_ON_WIN_MAC(ResourceLoadingHintsHttpsNoWhitelisted)) {
-  GURL url = https_url();
-
-  SetDefaultOnlyResourceLoadingHints({});
-  LoadUrlHints(url);
-
   SetExpectedFooJpgRequest(true);
   SetExpectedBarJpgRequest(true);
-  ResetResourceLoadingHintInterventionHeaderSeen();
+
+  SetDefaultOnlyResourceLoadingHints({});
 
   base::HistogramTester histogram_tester;
 
   // The URL is not whitelisted.
-  ui_test_utils::NavigateToURL(browser(), url);
+  ui_test_utils::NavigateToURL(browser(), https_url());
   base::RunLoop().RunUntilIdle();
 
   histogram_tester.ExpectBucketCount(
@@ -611,19 +564,15 @@ IN_PROC_BROWSER_TEST_F(
 
 IN_PROC_BROWSER_TEST_F(ResourceLoadingHintsBrowserTest,
                        DISABLE_ON_WIN_MAC(ResourceLoadingHintsHttp)) {
-  GURL url = http_url();
+  SetExpectedFooJpgRequest(true);
+  SetExpectedBarJpgRequest(true);
 
   // 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(), url);
+  ui_test_utils::NavigateToURL(browser(), http_url());
   base::RunLoop().RunUntilIdle();
 
   histogram_tester.ExpectBucketCount(
@@ -639,19 +588,15 @@ IN_PROC_BROWSER_TEST_F(ResourceLoadingHintsBrowserTest,
 IN_PROC_BROWSER_TEST_F(
     ResourceLoadingHintsBrowserTest,
     DISABLE_ON_WIN_MAC(ResourceLoadingHintsHttpsWhitelistedNoTransform)) {
-  GURL url = https_no_transform_url();
+  SetExpectedFooJpgRequest(true);
+  SetExpectedBarJpgRequest(true);
 
   // 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(), url);
+  ui_test_utils::NavigateToURL(browser(), https_no_transform_url());
   base::RunLoop().RunUntilIdle();
 
   histogram_tester.ExpectBucketCount(

components/previews/content/hint_cache.cc

@@ -74,7 +74,6 @@ 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,7 +63,8 @@ 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| if/when loaded. |callback| will not be called if no hint data
+  // is found for |host|.
   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,11 +142,6 @@ 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,7 +9,4 @@ namespace previews {
 const char kPreviewsOptimizationGuideUpdateHintsResultHistogramString[] =
     "PreviewsOptimizationGuide.UpdateHints.Result";
 
-const char kPreviewsOptimizationGuideOnLoadedHintResultHistogramString[] =
-    "PreviewsOptimizationGuide.OnLoadedHint.Result";
-
 }  // namespace previews

components/previews/core/previews_constants.h

@@ -11,10 +11,6 @@ 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_