Reland "Clear the subresource redirect hints when navigation is started"

This is a reland of 653f075d
(After deflaking the test)

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

TBR=mcrouse@chromium.org

Bug: 1051283
Change-Id: I0336295bc6120c16e02dd28f787f2edcfe37e9fe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2065984Reviewed-by: rajendrant <rajendrant@chromium.org>
Commit-Queue: rajendrant <rajendrant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#743275}

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>