Reland "Fix flakiness in previews browsertests"

This reverts commit de34f0bd.

Reason for revert: Fixed additional flakiness

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

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

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_;
@@ -244,8 +263,7 @@ class PreviewsNoScriptBrowserTest : public PreviewsBrowserTest {
 // Previews InfoBar (which these tests triggers) does not work on Mac.
 // See https://crbug.com/782322 for detail.
 // Also occasional flakes on win7 (https://crbug.com/789542).
-// Also occasional flakes on chromeos (https://crbug.com/923161).
-#if defined(OS_ANDROID) || defined(OS_LINUX) && !defined(OS_CHROMEOS)
+#if defined(OS_ANDROID) || defined(OS_LINUX)
 #define MAYBE_NoScriptPreviewsEnabled NoScriptPreviewsEnabled
 #define MAYBE_NoScriptPreviewsEnabledHttpRedirectToHttps \
   NoScriptPreviewsEnabledHttpRedirectToHttps
@@ -260,26 +278,35 @@ 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());
   EXPECT_FALSE(noscript_js_requested());
 
   // Verify info bar presented via histogram check.
-  histogram_tester.ExpectUniqueSample("Previews.InfoBarAction.NoScript", 0, 1);
+  EXPECT_FALSE(histogram_tester.GetAllSamples("Previews.InfoBarAction.NoScript")
+                   .empty());
 }
 
 IN_PROC_BROWSER_TEST_F(PreviewsNoScriptBrowserTest,
                        NoScriptPreviewsEnabledButHttpRequest) {
+  GURL url = http_url();
+
   // Whitelist test URL for NoScript.
-  SetUpNoScriptWhitelist({http_url().host()});
+  SetUpNoScriptWhitelist({url.host()});
+
+  LoadUrlHints(url);
 
-  ui_test_utils::NavigateToURL(browser(), http_url());
+  ui_test_utils::NavigateToURL(browser(), url);
 
   // Verify loaded js resource but not css triggered by noscript tag.
   EXPECT_TRUE(noscript_js_requested());
@@ -297,11 +324,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());
@@ -313,18 +344,23 @@ 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());
   EXPECT_FALSE(noscript_js_requested());
 
   // Verify info bar presented via histogram check.
-  histogram_tester.ExpectUniqueSample("Previews.InfoBarAction.NoScript", 0, 1);
+  EXPECT_FALSE(histogram_tester.GetAllSamples("Previews.InfoBarAction.NoScript")
+                   .empty());
 }
 
 // Flaky in all platforms except Android. See https://crbug.com/803626 for
@@ -337,20 +373,24 @@ 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());
+  // Navigate to a NoScript Preview page.
+  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());
+  // Terminate the previous page (non-opt out) and pull up a new NoScript page.
+  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.
+  // Opt out of the NoScript Preview page.
   PreviewsUITabHelper::FromWebContents(
       browser()->tab_strip_model()->GetActiveWebContents())
       ->ReloadWithoutPreviews();
@@ -373,10 +413,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());
@@ -385,10 +429,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());

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_