Ignore single pixel images

Bug: 960513
Change-Id: I4321dec43926b10b4b07a1767eae556f7df5ee72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1745772
Commit-Queue: Sean Harrison <harrisonsean@chromium.org>
Reviewed-by: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689152}

chrome/browser/data_saver/subresource_redirect_browsertest.cc

@@ -5,6 +5,7 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "chrome/browser/metrics/subprocess_metrics_provider.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/test/browser_test_utils.h"
@@ -177,6 +178,20 @@ class SubresourceRedirectBrowserTest : public InProcessBrowserTest {
   DISALLOW_COPY_AND_ASSIGN(SubresourceRedirectBrowserTest);
 };
 
+class DifferentMediaInclusionSubresourceRedirectBrowserTest
+    : public SubresourceRedirectBrowserTest {
+ public:
+  void SetUp() override {
+    feature_list_.InitAndEnableFeatureWithParameters(
+        features::kSubresourceRedirectIncludedMediaSuffixes,
+        {{"included_path_suffixes", ".svg"}});
+    SubresourceRedirectBrowserTest::SetUp();
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
 //  NOTE: It is indirectly verified that correct requests are being sent to
 //  the mock compression server by the counts in the histogram bucket for
 //  HTTP_TEMPORARY_REDIRECTs.
@@ -365,3 +380,23 @@ IN_PROC_BROWSER_TEST_F(SubresourceRedirectBrowserTest,
   EXPECT_EQ(GURL(RunScriptExtractString("imageSrc()")).port(),
             https_url().port());
 }
+
+//  This test loads image.html, but with
+//  SubresourceRedirectIncludedMediaSuffixes set to only allow .svg, so no
+//  internal redirect should occur.
+IN_PROC_BROWSER_TEST_F(DifferentMediaInclusionSubresourceRedirectBrowserTest,
+                       NoTriggerWhenNotIncludedInMeidaSuffixes) {
+  ui_test_utils::NavigateToURL(browser(),
+                               HttpsURLWithPath("/load_image/image.html"));
+
+  content::FetchHistogramsFromChildProcesses();
+  SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
+
+  histogram_tester()->ExpectTotalCount(
+      "SubresourceRedirect.CompressionAttempt.ResponseCode", 0);
+
+  EXPECT_TRUE(RunScriptExtractBool("checkImage()"));
+
+  EXPECT_EQ(GURL(RunScriptExtractString("imageSrc()")).port(),
+            https_url().port());
+}

chrome/common/chrome_features.cc

@@ -656,6 +656,13 @@ const base::Feature kNativeSmb{"NativeSmb", base::FEATURE_ENABLED_BY_DEFAULT};
 const base::Feature kSoundContentSetting{"SoundContentSetting",
                                          base::FEATURE_ENABLED_BY_DEFAULT};
 
+// Enables filtering URLs by suffix to include subresources that look
+// like image resources for compression. For example,
+// http://chromium.org/image.jpg would be included.
+const base::Feature kSubresourceRedirectIncludedMediaSuffixes{
+    "SubresourceRedirectIncludedMediaSuffixes",
+    base::FEATURE_ENABLED_BY_DEFAULT};
+
 #if defined(OS_CHROMEOS)
 // Enables or disables chrome://sys-internals.
 const base::Feature kSysInternals{"SysInternals",

chrome/common/chrome_features.h

@@ -411,6 +411,9 @@ COMPONENT_EXPORT(CHROME_FEATURES) extern const base::Feature kNativeSmb;
 COMPONENT_EXPORT(CHROME_FEATURES)
 extern const base::Feature kSoundContentSetting;
 
+COMPONENT_EXPORT(CHROME_FEATURES)
+extern const base::Feature kSubresourceRedirectIncludedMediaSuffixes;
+
 #if defined(OS_CHROMEOS)
 COMPONENT_EXPORT(CHROME_FEATURES)
 extern const base::Feature kSysInternals;

chrome/renderer/BUILD.gn

@@ -100,6 +100,8 @@ jumbo_static_library("renderer") {
     "sandbox_status_extension_android.h",
     "security_interstitials/security_interstitial_page_controller.cc",
     "security_interstitials/security_interstitial_page_controller.h",
+    "subresource_redirect/subresource_redirect_experiments.cc",
+    "subresource_redirect/subresource_redirect_experiments.h",
     "subresource_redirect/subresource_redirect_params.cc",
     "subresource_redirect/subresource_redirect_params.h",
     "subresource_redirect/subresource_redirect_url_loader_throttle.cc",

chrome/renderer/subresource_redirect/subresource_redirect_experiments.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/renderer/subresource_redirect/subresource_redirect_experiments.h"
+
+#include "base/feature_list.h"
+#include "base/metrics/field_trial.h"
+#include "base/metrics/field_trial_params.h"
+#include "base/strings/string_split.h"
+#include "chrome/common/chrome_features.h"
+
+namespace subresource_redirect {
+
+bool ShouldIncludeMediaSuffix(const GURL& url) {
+  if (!base::FeatureList::IsEnabled(
+          features::kSubresourceRedirectIncludedMediaSuffixes))
+    return true;
+
+  std::vector<std::string> suffixes = {".jpg", ".jpeg", ".png", ".svg",
+                                       ".webp"};
+
+  std::string csv = base::GetFieldTrialParamValueByFeature(
+      features::kSubresourceRedirectIncludedMediaSuffixes,
+      "included_path_suffixes");
+  if (csv != "") {
+    suffixes = base::SplitString(csv, ",", base::TRIM_WHITESPACE,
+                                 base::SPLIT_WANT_NONEMPTY);
+  }
+
+  for (const std::string& suffix : suffixes) {
+    if (base::EndsWith(url.path(), suffix,
+                       base::CompareCase::INSENSITIVE_ASCII)) {
+      return true;
+    }
+  }
+  return false;
+}
+
+}  // namespace subresource_redirect

chrome/renderer/subresource_redirect/subresource_redirect_experiments.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_RENDERER_SUBRESOURCE_REDIRECT_SUBRESOURCE_REDIRECT_EXPERIMENTS_H_
+#define CHROME_RENDERER_SUBRESOURCE_REDIRECT_SUBRESOURCE_REDIRECT_EXPERIMENTS_H_
+
+#include "url/gurl.h"
+
+namespace subresource_redirect {
+
+// Returns true if the given url matches an included media suffix.
+bool ShouldIncludeMediaSuffix(const GURL& url);
+
+}  // namespace subresource_redirect
+
+#endif  // CHROME_RENDERER_SUBRESOURCE_REDIRECT_SUBRESOURCE_REDIRECT_EXPERIMENTS_H_

chrome/renderer/subresource_redirect/subresource_redirect_experiments_unittest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/renderer/subresource_redirect/subresource_redirect_experiments.h"
+
+#include "base/test/scoped_feature_list.h"
+#include "chrome/common/chrome_features.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace subresource_redirect {
+
+namespace {
+
+TEST(SubresourceRedirectExperimentsTest, TestDefaultShouldIncludeMediaSuffix) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      features::kSubresourceRedirectIncludedMediaSuffixes);
+
+  EXPECT_FALSE(ShouldIncludeMediaSuffix(GURL("http://chromium.org/path/")));
+
+  std::vector<std::string> default_suffixes = {".jpg", ".jpeg", ".png", ".svg",
+                                               ".webp"};
+  for (const std::string& suffix : default_suffixes) {
+    GURL url("http://chromium.org/image" + suffix);
+    EXPECT_TRUE(ShouldIncludeMediaSuffix(url));
+  }
+}
+
+TEST(SubresourceRedirectExperimentsTest, TestShouldIncludeMediaSuffix) {
+  struct TestCase {
+    std::string msg;
+    bool enable_feature;
+    std::string varaiation_value;
+    std::vector<std::string> urls;
+    bool want_return;
+  };
+  const TestCase kTestCases[]{
+      {
+          .msg = "Feature disabled, should always return true",
+          .enable_feature = false,
+          .varaiation_value = "",
+          .urls = {"http://chromium.org/image.jpg"},
+          .want_return = true,
+      },
+      {
+          .msg = "Default values are overridden by variations",
+          .enable_feature = true,
+          .varaiation_value = ".html",
+          .urls = {"http://chromium.org/image.jpeg",
+                   "http://chromium.org/image.png",
+                   "http://chromium.org/image.jpg",
+                   "http://chromium.org/image.svg"},
+          .want_return = false,
+      },
+      {
+          .msg = "Variation value whitespace should be trimmed",
+          .enable_feature = true,
+          .varaiation_value = " .svg , \t .png\n",
+          .urls = {"http://chromium.org/image.svg",
+                   "http://chromium.org/image.png"},
+          .want_return = true,
+      },
+      {
+          .msg = "Variation value empty values should be excluded",
+          .enable_feature = true,
+          .varaiation_value = ".svg,,.png,",
+          .urls = {"http://chromium.org/image.svg",
+                   "http://chromium.org/image.png"},
+          .want_return = true,
+      },
+      {
+          .msg = "URLs should be compared case insensitive",
+          .enable_feature = true,
+          .varaiation_value = ".svg,.png,",
+          .urls = {"http://chromium.org/image.SvG",
+                   "http://chromium.org/image.PNG"},
+          .want_return = true,
+      },
+      {
+          .msg = "Query params and fragments don't matter",
+          .enable_feature = true,
+          .varaiation_value = ".svg,.png,",
+          .urls = {"http://chromium.org/image.svg?hello=world",
+                   "http://chromium.org/image.png#test"},
+          .want_return = true,
+      },
+      {
+          .msg = "Query params and fragments shouldn't be considered",
+          .enable_feature = true,
+          .varaiation_value = ".svg,.png,",
+          .urls = {"http://chromium.org/?image=image.svg",
+                   "http://chromium.org/#image.png"},
+          .want_return = false,
+      },
+  };
+  for (const TestCase& test_case : kTestCases) {
+    SCOPED_TRACE(test_case.msg);
+
+    base::test::ScopedFeatureList scoped_feature_list;
+    if (test_case.enable_feature) {
+      scoped_feature_list.InitAndEnableFeatureWithParameters(
+          features::kSubresourceRedirectIncludedMediaSuffixes,
+          {{"included_path_suffixes", test_case.varaiation_value}});
+    } else {
+      scoped_feature_list.InitAndDisableFeature(
+          features::kSubresourceRedirectIncludedMediaSuffixes);
+    }
+
+    for (const std::string& url : test_case.urls) {
+      EXPECT_EQ(test_case.want_return, ShouldIncludeMediaSuffix(GURL(url)));
+    }
+  }
+}
+
+}  // namespace
+}  // namespace subresource_redirect

chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.cc

@@ -5,6 +5,7 @@
 #include "chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.h"
 
 #include "base/metrics/histogram_macros.h"
+#include "chrome/renderer/subresource_redirect/subresource_redirect_experiments.h"
 #include "chrome/renderer/subresource_redirect/subresource_redirect_util.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h"
 #include "content/public/common/resource_type.h"
@@ -27,6 +28,12 @@ void SubresourceRedirectURLLoaderThrottle::WillStartRequest(
 
   DCHECK(request->url.SchemeIs(url::kHttpsScheme));
 
+  // Image subresources that have paths that do not end in one of the
+  // following common formats are commonly single pixel images that will not
+  // benefit from being sent to the compression server.
+  if (!ShouldIncludeMediaSuffix(request->url))
+    return;
+
   request->url = GetSubresourceURLForURL(request->url);
   *defer = false;
 }

chrome/test/BUILD.gn

@@ -3259,6 +3259,7 @@ test("unit_tests") {
     "../renderer/page_load_metrics/page_timing_metrics_sender_unittest.cc",
     "../renderer/plugins/plugin_uma_unittest.cc",
     "../renderer/prerender/prerender_dispatcher_unittest.cc",
+    "../renderer/subresource_redirect/subresource_redirect_experiments_unittest.cc",
     "../renderer/subresource_redirect/subresource_redirect_url_loader_throttle_unittest.cc",
     "../renderer/subresource_redirect/subresource_redirect_util_unittest.cc",
     "../renderer/v8_unwinder_unittest.cc",