Clear the subresource redirect hints when navigation is started

RenderFrames could be reused for same-origin navigations. This causes
the hints from previous navigations to be used in next navigation
until the new hints fetch finishes and updates the hints. This CL clears
the hints.

Unfortunately this causes race conditions between the image fetch and
the hints update, which causes images to not redirected to compressed
versions. So the image fetches are delayed a bit to circumvent this.

Bug: 1051283
Change-Id: Ie9e5351e68c0081ab9dbfe23faed2c3c5e8e4d42
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2056207
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742326}

chrome/browser/subresource_redirect/subresource_redirect_observer.cc

@@ -114,7 +114,7 @@ SubresourceRedirectObserver::SubresourceRedirectObserver(
 
 SubresourceRedirectObserver::~SubresourceRedirectObserver() = default;
 
-void SubresourceRedirectObserver::ReadyToCommitNavigation(
+void SubresourceRedirectObserver::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   DCHECK(navigation_handle);
   if (!navigation_handle->IsInMainFrame() ||

chrome/browser/subresource_redirect/subresource_redirect_observer.h

@@ -34,7 +34,7 @@ class SubresourceRedirectObserver
   explicit SubresourceRedirectObserver(content::WebContents* web_contents);
 
   // content::WebContentsObserver.
-  void ReadyToCommitNavigation(
+  void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
 
   WEB_CONTENTS_USER_DATA_KEY_DECL();

chrome/renderer/previews/resource_loading_hints_agent.cc

@@ -48,15 +48,16 @@ GURL ResourceLoadingHintsAgent::GetDocumentURL() const {
   return render_frame()->GetWebFrame()->GetDocument().Url();
 }
 
+void ResourceLoadingHintsAgent::DidStartNavigation(
+    const GURL& url,
+    base::Optional<blink::WebNavigationType> navigation_type) {
+  subresource_redirect_hints_agent_.DidStartNavigation();
+}
+
 void ResourceLoadingHintsAgent::DidCreateNewDocument() {
   DCHECK(IsMainFrame());
-  did_create_new_document_ = true;
   if (!GetDocumentURL().SchemeIsHTTPOrHTTPS())
     return;
-  if (images_hints_) {
-    subresource_redirect_hints_agent_.SetCompressPublicImagesHints(
-        std::move(images_hints_));
-  }
 
   if (subresource_patterns_to_block_.empty())
     return;
@@ -110,13 +111,8 @@ void ResourceLoadingHintsAgent::SetResourceLoadingHints(
 
 void ResourceLoadingHintsAgent::SetCompressPublicImagesHints(
     blink::mojom::CompressPublicImagesHintsPtr images_hints) {
-  if (did_create_new_document_) {
-    subresource_redirect_hints_agent_.SetCompressPublicImagesHints(
-        std::move(images_hints));
-  } else {
-    // Let the hints be sent in DidCreateNewDocument.
-    images_hints_ = std::move(images_hints);
-  }
+  subresource_redirect_hints_agent_.SetCompressPublicImagesHints(
+      std::move(images_hints));
 }
 
 }  // namespace previews

chrome/renderer/previews/resource_loading_hints_agent.h

@@ -46,6 +46,9 @@ class ResourceLoadingHintsAgent
 
  private:
   // content::RenderFrameObserver:
+  void DidStartNavigation(
+      const GURL& url,
+      base::Optional<blink::WebNavigationType> navigation_type) override;
   void DidCreateNewDocument() override;
   void OnDestruct() override;
 
@@ -63,8 +66,6 @@ class ResourceLoadingHintsAgent
 
   bool IsMainFrame() const;
 
-  bool did_create_new_document_ = false;
-
   std::vector<std::string> subresource_patterns_to_block_;
   base::Optional<int64_t> ukm_source_id_;
 
@@ -73,7 +74,6 @@ class ResourceLoadingHintsAgent
 
   subresource_redirect::SubresourceRedirectHintsAgent
       subresource_redirect_hints_agent_;
-  blink::mojom::CompressPublicImagesHintsPtr images_hints_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourceLoadingHintsAgent);
 };

chrome/renderer/subresource_redirect/subresource_redirect_hints_agent.cc

@@ -16,8 +16,17 @@ namespace subresource_redirect {
 SubresourceRedirectHintsAgent::SubresourceRedirectHintsAgent() = default;
 SubresourceRedirectHintsAgent::~SubresourceRedirectHintsAgent() = default;
 
+void SubresourceRedirectHintsAgent::DidStartNavigation() {
+  // Clear the hints when a navigation starts, so that hints from previous
+  // navigation do not apply in case the same renderframe is reused.
+  public_image_urls_.clear();
+  public_image_urls_received_ = false;
+}
+
 void SubresourceRedirectHintsAgent::SetCompressPublicImagesHints(
     blink::mojom::CompressPublicImagesHintsPtr images_hints) {
+  DCHECK(public_image_urls_.empty());
+  DCHECK(!public_image_urls_received_);
   public_image_urls_ = images_hints->image_urls;
   public_image_urls_received_ = true;
 }

chrome/renderer/subresource_redirect/subresource_redirect_hints_agent.h

@@ -40,6 +40,10 @@ class SubresourceRedirectHintsAgent {
   SubresourceRedirectHintsAgent& operator=(
       const SubresourceRedirectHintsAgent&) = delete;
 
+  // Called when a navigation starts to clear the state from previous
+  // navigation.
+  void DidStartNavigation();
+
   void SetCompressPublicImagesHints(
       blink::mojom::CompressPublicImagesHintsPtr images_hints);
 

chrome/test/data/load_image/common.js

+// Copyright (c) 2020 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.
+
+function checkImage() {
+  return imageLoadedPromise(document.images[0]);
+}
+
+function imageLoadedPromise(img_element) {
+  return new Promise((resolve, reject) => {
+    if (img_element.complete && img_element.src) {
+      console.log("image loaded callback", img_element.src);
+      resolve(true);
+    } else {
+      img_element.addEventListener('load', () => {
+        console.log("image loaded callback", img_element.src);
+        resolve(true);
+      });
+    }
+  });
+}
+
+function imageSrc() {
+  sendValueToTest(document.images[0].src);
+}
+
+function sendValueToTest(value) {
+  window.domAutomationController.send(value);
+}

chrome/test/data/load_image/fail_image.html

 <html>
 <head></head>
-<img alt="long_placeholder_text" src="fail_image.png" />
+<img alt="long_placeholder_text"/>
+<script src="common.js"></script>
 <script>
-function checkImage() {
-	sendValueToTest(document.images[0].complete);
-}
-
-function imageSrc() {
-	sendValueToTest(document.images[0].src);
-}
-
-function sendValueToTest(value) {
-  window.domAutomationController.send(value);
+window.onload = () => {
+  setTimeout(() => {
+    document.images[0].src = "fail_image.png"
+  }, 1000);
 }
 </script>
 </html>

chrome/test/data/load_image/image_delayed_load.html

+<html>
+<head></head>
+<img alt="long_placeholder_text"/>
+<script src="common.js"></script>
+<script>
+window.onload = () => {
+  setTimeout(() => {
+    document.images[0].src = "image.png"
+  }, 1000);
+}
+</script>
+</html>

chrome/test/data/load_image/image_js.html

 <html>
 <body>
 <div> div tag </div>
+<script src="common.js"></script>
 <script>
 	var img = document.createElement("img");
 	img.src="image.png";
 	document.getElementsByTagName('div')[0].appendChild(img);
-
-	function checkImage() {
-		sendValueToTest(document.images[0].complete);
-	}
-
-	function imageSrc() {
-		sendValueToTest(document.images[0].src);
-	}
-
-	function sendValueToTest(value) {
-	  window.domAutomationController.send(value);
-	}
 </script>
 </body>
-</html>
+</html>
\ No newline at end of file

chrome/test/data/load_image/image_with_fragment.html

 <html>
 <head></head>
-<img alt="long_placeholder_text" src="image.png#fragment" />
+<img alt="long_placeholder_text"/>
+<script src="common.js"></script>
 <script>
-function checkImage() {
-	sendValueToTest(document.images[0].complete);
-}
-
-function imageSrc() {
-	sendValueToTest(document.images[0].src);
-}
-
-function sendValueToTest(value) {
-  window.domAutomationController.send(value);
+window.onload = () => {
+  setTimeout(() => {
+    document.images[0].src = "image.png#fragment"
+  }, 1000)
 }
 </script>
 </html>

chrome/test/data/load_image/private_url_image.html

 <html>
 <body>
-<img alt="long_placeholder_text" src="private_url_image.png" />
+<img alt="long_placeholder_text"/>
+<script src="common.js"></script>
 <script>
-function checkImage() {
-	sendValueToTest((document.images[0].complete && (document.images[0].naturalWidth > 0)));
-}
-
-function imageSrc() {
-	sendValueToTest(document.images[0].src);
-}
-
-function sendValueToTest(value) {
-  window.domAutomationController.send(value);
+window.onload = () => {
+  setTimeout(() => {
+    document.images[0].src = "private_url_image.png"
+  }, 1000)
 }
 </script>
 </body>
-</html>
+</html>
\ No newline at end of file

chrome/test/data/load_image/two_images.html

 <html>
 <head></head>
-<img src="image.png" />
-<img src="image.png?foo" />
+<img/>
+<img/>
+<script src="common.js"></script>
 <script>
 function checkBothImagesLoaded() {
-	sendValueToTest(document.images[0].complete && document.images[1].complete);
+  return new Promise((resolve, reject) => {
+    Promise.all([imageLoadedPromise(document.images[0]),
+      imageLoadedPromise(document.images[1])]).then(() => {
+        resolve(true);
+      });
+   });
 }
 
-function imageSrc() {
-	sendValueToTest(document.images[0].src);
-}
-
-function sendValueToTest(value) {
-  window.domAutomationController.send(value);
+window.onload = () => {
+  setTimeout(() => {
+    document.images[0].src = "image.png"
+    document.images[1].src = "image.png?foo"
+  }, 1000)
 }
 </script>
 </html>