Handle failed redirects for public image compression

Handles the following cases:
1. When the litepages server and the origin server both fail the
response, it should not go into a infinite loop.
2. When the initial image itself is a litepages URL, no redirect should
happen and no metrics recorded.

TBR=tbansal@chromium.org

Bug: 1059606
Change-Id: I477839ab09493445a085d646a4da4fb22c5981c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2128944
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#755960}

chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.cc

@@ -27,13 +27,23 @@ namespace subresource_redirect {
 
 namespace {
 
-bool IsSubresourceRedirectOrigin(const GURL& url) {
+// Whether the url points to compressed server origin.
+bool IsCompressionServerOrigin(const GURL& url) {
   auto compression_server = GetSubresourceRedirectOrigin();
   return url.DomainIs(compression_server.host()) &&
          (url.EffectiveIntPort() == compression_server.port()) &&
          (url.scheme() == compression_server.scheme());
 }
 
+// Should the subresource be redirected to its compressed version. This returns
+// false if only coverage metrics need to be recorded and actual redirection
+// should not happen.
+bool ShouldCompressionServerRedirectSubresource() {
+  return base::GetFieldTrialParamByFeatureAsBool(
+      blink::features::kSubresourceRedirect,
+      "enable_subresource_server_redirect", false);
+}
+
 }  // namespace
 
 // static
@@ -82,6 +92,10 @@ void SubresourceRedirectURLLoaderThrottle::WillStartRequest(
           SubresourceRedirectHintsAgent::RedirectResult::kIneligibleOtherImage);
   DCHECK(request->url.SchemeIs(url::kHttpsScheme));
 
+  // Do not redirect if its already a litepage subresource.
+  if (IsCompressionServerOrigin(request->url))
+    return;
+
   if (redirect_result_ !=
       SubresourceRedirectHintsAgent::RedirectResult::kRedirectable) {
     return;
@@ -97,13 +111,11 @@ void SubresourceRedirectURLLoaderThrottle::WillStartRequest(
       SubresourceRedirectHintsAgent::RedirectResult::kRedirectable) {
     return;
   }
-  if (!base::GetFieldTrialParamByFeatureAsBool(
-          blink::features::kSubresourceRedirect, "enable_lite_page_redirect",
-          false)) {
+  if (!ShouldCompressionServerRedirectSubresource())
     return;
-  }
 
   request->url = GetSubresourceURLForURL(request->url);
+  did_redirect_compressed_origin_ = true;
   *defer = false;
 }
 
@@ -135,13 +147,13 @@ void SubresourceRedirectURLLoaderThrottle::BeforeWillProcessResponse(
     const GURL& response_url,
     const network::mojom::URLResponseHead& response_head,
     bool* defer) {
+  if (!did_redirect_compressed_origin_)
+    return;
+  DCHECK(ShouldCompressionServerRedirectSubresource());
   // If response was not from the compression server, don't restart it.
   if (!response_url.is_valid())
     return;
 
-  if (!IsSubresourceRedirectOrigin(response_url))
-    return;
-
   // Log all response codes, from the compression server.
   UMA_HISTOGRAM_ENUMERATION(
       "SubresourceRedirect.CompressionAttempt.ResponseCode",
@@ -157,14 +169,10 @@ void SubresourceRedirectURLLoaderThrottle::BeforeWillProcessResponse(
   }
   redirect_result_ =
       SubresourceRedirectHintsAgent::RedirectResult::kIneligibleOtherImage;
-  if (!base::GetFieldTrialParamByFeatureAsBool(
-          blink::features::kSubresourceRedirect, "enable_lite_page_redirect",
-          false)) {
-    return;
-  }
 
   // Non 2XX responses from the compression server need to have unaltered
   // requests sent to the original resource.
+  did_redirect_compressed_origin_ = false;
   delegate_->RestartWithURLResetAndFlags(net::LOAD_NORMAL);
 }
 
@@ -187,8 +195,9 @@ void SubresourceRedirectURLLoaderThrottle::WillProcessResponse(
         response_url, content_length, redirect_result_);
   }
 
-  if (!IsSubresourceRedirectOrigin(response_url))
+  if (!did_redirect_compressed_origin_)
     return;
+  DCHECK(ShouldCompressionServerRedirectSubresource());
 
   // Record that the server responded.
   UMA_HISTOGRAM_BOOLEAN(
@@ -217,16 +226,15 @@ void SubresourceRedirectURLLoaderThrottle::WillProcessResponse(
 void SubresourceRedirectURLLoaderThrottle::WillOnCompleteWithError(
     const network::URLLoaderCompletionStatus& status,
     bool* defer) {
+  if (!did_redirect_compressed_origin_)
+    return;
+  DCHECK(ShouldCompressionServerRedirectSubresource());
   redirect_result_ =
       SubresourceRedirectHintsAgent::RedirectResult::kIneligibleOtherImage;
-  if (!base::GetFieldTrialParamByFeatureAsBool(
-          blink::features::kSubresourceRedirect, "enable_lite_page_redirect",
-          false)) {
-    return;
-  }
 
   // If the server fails, restart the request to the original resource, and
   // record it.
+  did_redirect_compressed_origin_ = false;
   delegate_->RestartWithURLResetAndFlags(net::LOAD_NORMAL);
   UMA_HISTOGRAM_BOOLEAN(
       "SubresourceRedirect.CompressionAttempt.ServerResponded", false);

chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.h

@@ -64,6 +64,13 @@ class SubresourceRedirectURLLoaderThrottle : public blink::URLLoaderThrottle {
   // its not eligible.
   SubresourceRedirectHintsAgent::RedirectResult redirect_result_;
 
+  // Whether this resource was actually redirected to compressed server origin.
+  // This will be true when the redirect was attempted. Will be false when
+  // redirect failed due to neterrors, or redirect was not attempted (but
+  // coverage metrics recorded), or redirect was not needed when the initial URL
+  // itself is compressed origin.
+  bool did_redirect_compressed_origin_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(SubresourceRedirectURLLoaderThrottle);
 };
 

chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle_unittest.cc

@@ -101,7 +101,7 @@ TEST(SubresourceRedirectURLLoaderThrottleTest, TestMaybeCreateThrottle) {
     if (test_case.is_subresource_redirect_feature_enabled) {
       scoped_feature_list.InitWithFeaturesAndParameters(
           {{blink::features::kSubresourceRedirect,
-            {{"enable_lite_page_redirect", "true"}}}},
+            {{"enable_subresource_server_redirect", "true"}}}},
           {});
     } else {
       scoped_feature_list.InitAndDisableFeature(
@@ -167,7 +167,7 @@ TEST(SubresourceRedirectURLLoaderThrottleTest, TestGetSubresourceURL) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitWithFeaturesAndParameters(
       {{blink::features::kSubresourceRedirect,
-        {{"enable_lite_page_redirect", "true"}}}},
+        {{"enable_subresource_server_redirect", "true"}}}},
       {});
 
   for (const TestCase& test_case : kTestCases) {
@@ -198,7 +198,7 @@ TEST(SubresourceRedirectURLLoaderThrottleTest, DeferOverridenToFalse) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitWithFeaturesAndParameters(
       {{blink::features::kSubresourceRedirect,
-        {{"enable_lite_page_redirect", "true"}}}},
+        {{"enable_subresource_server_redirect", "true"}}}},
       {});
 
   auto throttle = CreateSubresourceRedirectURLLoaderThrottle(

chrome/test/data/load_image/common.js

@@ -9,11 +9,15 @@ function checkImage() {
 function imageLoadedPromise(img_element) {
   return new Promise((resolve, reject) => {
     if (img_element.complete && img_element.src) {
-      resolve(true);
+      // Treat the image as failed based on its dimension.
+      resolve(img_element.naturalHeight > 0);
     } else {
       img_element.addEventListener('load', () => {
         resolve(true);
       });
+      img_element.addEventListener('error', () => {
+        resolve(false);
+      });
     }
   });
 }

testing/variations/fieldtrial_testing_config.json

@@ -5497,7 +5497,7 @@
                 {
                     "name": "Enabled",
                     "params": {
-                        "enable_lite_page_redirect": "false"
+                        "enable_subresource_server_redirect": "false"
                     },
                     "enable_features": [
                         "SubresourceRedirect"

tools/chrome_proxy/webdriver/subresource_redirect.py

@@ -17,7 +17,7 @@ class SubresourceRedirect(IntegrationTest):
     test_driver.AddChromeArg('--force-fieldtrials=SubresourceRedirect/Enabled')
     test_driver.AddChromeArg(
         '--force-fieldtrial-params='
-        'SubresourceRedirect.Enabled:enable_lite_page_redirect/true')
+        'SubresourceRedirect.Enabled:enable_subresource_server_redirect/true')
     test_driver.EnableChromeFeature('OptimizationHints')
     test_driver.EnableChromeFeature('OptimizationHintsFetching')
     test_driver.EnableChromeFeature(