Return IsBlacklisted = true by default if hints aren't loaded

This also moves the ignore blacklist flag one call level higher for use
in browser tests, instead of having to load hints for every test.

Bug: 938621
Change-Id: Ie13f87463dccb9e2a3d57a0cda9b89a80fdb9f55
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504256Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638304}

chrome/browser/previews/previews_lite_page_browsertest.cc

@@ -138,6 +138,7 @@ class PreviewsLitePageServerBrowserTest
     cmd->AppendSwitchASCII("force-variation-ids", "42");
     cmd->AppendSwitchASCII("host-rules", "MAP * 127.0.0.1");
     cmd->AppendSwitch("enable-data-reduction-proxy-force-pingback");
+    cmd->AppendSwitch("ignore-litepage-redirect-optimization-blacklist");
   }
 
   void SetUp() override {
@@ -1475,6 +1476,7 @@ class PreviewsLitePageServerDataSaverBrowserTest
     cmd->AppendSwitch(previews::switches::kIgnorePreviewsBlacklist);
     cmd->AppendSwitchASCII("force-effective-connection-type", "Slow-2G");
     cmd->AppendSwitchASCII("host-rules", "MAP * 127.0.0.1");
+    cmd->AppendSwitch("ignore-litepage-redirect-optimization-blacklist");
   }
 };
 
@@ -1509,6 +1511,7 @@ class PreviewsLitePageServerNoDataSaverHeaderBrowserTest
     cmd->AppendSwitch("enable-spdy-proxy-auth");
     cmd->AppendSwitchASCII("force-effective-connection-type", "Slow-2G");
     cmd->AppendSwitchASCII("host-rules", "MAP * 127.0.0.1");
+    cmd->AppendSwitch("ignore-litepage-redirect-optimization-blacklist");
   }
 };
 
@@ -1612,6 +1615,7 @@ class PreviewsLitePageNotificationDSDisabledBrowserTest
     cmd->AppendSwitch(previews::switches::kIgnorePreviewsBlacklist);
     cmd->AppendSwitchASCII("force-effective-connection-type", "Slow-2G");
     cmd->AppendSwitchASCII("host-rules", "MAP * 127.0.0.1");
+    cmd->AppendSwitch("ignore-litepage-redirect-optimization-blacklist");
   }
 };
 

components/previews/content/previews_hints.cc

@@ -6,7 +6,6 @@
 
 #include <unordered_set>
 
-#include "base/command_line.h"
 #include "base/files/file.h"
 #include "base/files/file_util.h"
 #include "base/metrics/histogram_functions.h"
@@ -484,14 +483,13 @@ bool PreviewsHints::IsBlacklisted(const GURL& url, PreviewsType type) const {
   // Check large scale blacklists received from the server.
   // (At some point, we may have blacklisting to check in HintCache as well.)
   if (type == PreviewsType::LITE_PAGE_REDIRECT) {
-    if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-            switches::kIgnoreLitePageRedirectOptimizationBlacklist)) {
-      return false;
+    // If no bloom filter blacklist is provided by the component update, assume
+    // a server error and return true.
+    if (!lite_page_redirect_blacklist_) {
+      return true;
     }
 
-    if (lite_page_redirect_blacklist_) {
-      return lite_page_redirect_blacklist_->ContainsHostSuffix(url);
-    }
+    return lite_page_redirect_blacklist_->ContainsHostSuffix(url);
   }
 
   return false;

components/previews/content/previews_hints_unittest.cc

@@ -260,32 +260,27 @@ TEST_F(PreviewsHintsTest, LogHintCacheMatch) {
       5 /* EFFECTIVE_CONNECTION_TYPE_4G */, 1);
 }
 
-TEST_F(PreviewsHintsTest, IsBlacklisted) {
+TEST_F(PreviewsHintsTest, IsBlacklistedReturnsTrueIfNoBloomFilter) {
   base::test::ScopedFeatureList scoped_list;
   scoped_list.InitAndEnableFeature(features::kLitePageServerPreviews);
 
-  BloomFilter blacklist_bloom_filter(kBlackBlacklistBloomFilterNumHashFunctions,
-                                     kBlackBlacklistBloomFilterNumBits);
-  PopulateBlackBlacklistBloomFilter(&blacklist_bloom_filter);
-
   optimization_guide::proto::Configuration config;
-  AddBlacklistBloomFilterToConfig(blacklist_bloom_filter,
-                                  kBlackBlacklistBloomFilterNumHashFunctions,
-                                  kBlackBlacklistBloomFilterNumBits, &config);
   ParseConfig(config);
 
-  EXPECT_TRUE(HasLitePageRedirectBlacklist());
+  EXPECT_FALSE(HasLitePageRedirectBlacklist());
+
   EXPECT_FALSE(previews_hints()->IsBlacklisted(GURL("https://black.com/path"),
                                                PreviewsType::LOFI));
+
   EXPECT_TRUE(previews_hints()->IsBlacklisted(
       GURL("https://black.com/path"), PreviewsType::LITE_PAGE_REDIRECT));
   EXPECT_TRUE(previews_hints()->IsBlacklisted(
       GURL("https://joe.black.com/path"), PreviewsType::LITE_PAGE_REDIRECT));
-  EXPECT_FALSE(previews_hints()->IsBlacklisted(
+  EXPECT_TRUE(previews_hints()->IsBlacklisted(
       GURL("https://nonblack.com"), PreviewsType::LITE_PAGE_REDIRECT));
 }
 
-TEST_F(PreviewsHintsTest, IgnoreLitePageRedirectBlacklist) {
+TEST_F(PreviewsHintsTest, IsBlacklisted) {
   base::test::ScopedFeatureList scoped_list;
   scoped_list.InitAndEnableFeature(features::kLitePageServerPreviews);
 
@@ -299,14 +294,12 @@ TEST_F(PreviewsHintsTest, IgnoreLitePageRedirectBlacklist) {
                                   kBlackBlacklistBloomFilterNumBits, &config);
   ParseConfig(config);
 
-  base::CommandLine::ForCurrentProcess()->AppendSwitch(
-      switches::kIgnoreLitePageRedirectOptimizationBlacklist);
-
+  EXPECT_TRUE(HasLitePageRedirectBlacklist());
   EXPECT_FALSE(previews_hints()->IsBlacklisted(GURL("https://black.com/path"),
                                                PreviewsType::LOFI));
-  EXPECT_FALSE(previews_hints()->IsBlacklisted(
+  EXPECT_TRUE(previews_hints()->IsBlacklisted(
       GURL("https://black.com/path"), PreviewsType::LITE_PAGE_REDIRECT));
-  EXPECT_FALSE(previews_hints()->IsBlacklisted(
+  EXPECT_TRUE(previews_hints()->IsBlacklisted(
       GURL("https://joe.black.com/path"), PreviewsType::LITE_PAGE_REDIRECT));
   EXPECT_FALSE(previews_hints()->IsBlacklisted(
       GURL("https://nonblack.com"), PreviewsType::LITE_PAGE_REDIRECT));
@@ -338,7 +331,7 @@ TEST_F(PreviewsHintsTest, ParseConfigWithInsufficientConfigDetails) {
       "Previews.OptimizationFilterStatus.LitePageRedirect",
       2 /* FAILED_SERVER_BLACKLIST_BAD_CONFIG */, 1);
 
-  EXPECT_FALSE(previews_hints()->IsBlacklisted(
+  EXPECT_TRUE(previews_hints()->IsBlacklisted(
       GURL("https://black.com/path"), PreviewsType::LITE_PAGE_REDIRECT));
 }
 
@@ -370,7 +363,7 @@ TEST_F(PreviewsHintsTest, ParseConfigWithTooLargeBlacklist) {
       "Previews.OptimizationFilterStatus.LitePageRedirect",
       3 /* FAILED_SERVER_BLACKLIST_TOO_BIG */, 1);
 
-  EXPECT_FALSE(previews_hints()->IsBlacklisted(
+  EXPECT_TRUE(previews_hints()->IsBlacklisted(
       GURL("https://black.com/path"), PreviewsType::LITE_PAGE_REDIRECT));
 }
 

components/previews/content/previews_optimization_guide.cc

@@ -129,11 +129,22 @@ bool PreviewsOptimizationGuide::IsWhitelisted(
 bool PreviewsOptimizationGuide::IsBlacklisted(const GURL& url,
                                               PreviewsType type) const {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
-  if (!hints_) {
-    return false;
+
+  if (type == PreviewsType::LITE_PAGE_REDIRECT) {
+    if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kIgnoreLitePageRedirectOptimizationBlacklist)) {
+      return false;
+    }
+
+    if (!hints_)
+      return true;
+
+    return hints_->IsBlacklisted(url, PreviewsType::LITE_PAGE_REDIRECT);
   }
 
-  return hints_->IsBlacklisted(url, type);
+  // This function is only used by lite page redirect.
+  NOTREACHED();
+  return false;
 }
 
 void PreviewsOptimizationGuide::OnLoadedHint(

components/previews/content/previews_optimization_guide_unittest.cc

@@ -1540,7 +1540,7 @@ TEST_F(PreviewsOptimizationGuideTest, IsBlacklisted) {
   base::test::ScopedFeatureList scoped_list;
   scoped_list.InitAndEnableFeature(features::kLitePageServerPreviews);
 
-  EXPECT_FALSE(
+  EXPECT_TRUE(
       guide()->IsBlacklisted(GURL("https://m.blacklisteddomain.com/path"),
                              PreviewsType::LITE_PAGE_REDIRECT));
 
@@ -1549,7 +1549,7 @@ TEST_F(PreviewsOptimizationGuideTest, IsBlacklisted) {
   EXPECT_TRUE(
       guide()->IsBlacklisted(GURL("https://m.blacklisteddomain.com/path"),
                              PreviewsType::LITE_PAGE_REDIRECT));
-  EXPECT_FALSE(guide()->IsBlacklisted(
+  EXPECT_DCHECK_DEATH(guide()->IsBlacklisted(
       GURL("https://m.blacklisteddomain.com/path"), PreviewsType::NOSCRIPT));
 
   EXPECT_TRUE(guide()->IsBlacklisted(
@@ -1560,6 +1560,22 @@ TEST_F(PreviewsOptimizationGuideTest, IsBlacklisted) {
                                       PreviewsType::LITE_PAGE_REDIRECT));
 }
 
+TEST_F(PreviewsOptimizationGuideTest, LitePageRedirectSkipIsBlacklistedCheck) {
+  base::test::ScopedFeatureList scoped_list;
+  scoped_list.InitAndEnableFeature(features::kLitePageServerPreviews);
+  InitializeWithLitePageRedirectBlacklist();
+
+  EXPECT_TRUE(
+      guide()->IsBlacklisted(GURL("https://m.blacklisteddomain.com/path"),
+                             PreviewsType::LITE_PAGE_REDIRECT));
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kIgnoreLitePageRedirectOptimizationBlacklist);
+
+  EXPECT_FALSE(
+      guide()->IsBlacklisted(GURL("https://m.blacklisteddomain.com/path"),
+                             PreviewsType::LITE_PAGE_REDIRECT));
+}
+
 TEST_F(PreviewsOptimizationGuideTest,
        IsBlacklistedWithLitePageServerPreviewsDisabled) {
   base::test::ScopedFeatureList scoped_list;
@@ -1567,7 +1583,7 @@ TEST_F(PreviewsOptimizationGuideTest,
 
   InitializeWithLitePageRedirectBlacklist();
 
-  EXPECT_FALSE(
+  EXPECT_TRUE(
       guide()->IsBlacklisted(GURL("https://m.blacklisteddomain.com/path"),
                              PreviewsType::LITE_PAGE_REDIRECT));
 }