Ensure to reschedule an image update if base url changed.

We're careful not to update the image loader when it is inserted into but nothing has changed. However, if we're inserted into a subtree such that the base element is now different, we do need to schedule a new run since the final URL may be different. R=fs@opera.com, chrishtr@chromium.org Bug: 897545 Change-Id: Id84894049c52e8f28a7ae581c3a626da5550207f Reviewed-on: https://chromium-review.googlesource.com/c/1296506 Commit-Queue: vmpstr <vmpstr@chromium.org> Reviewed-by: Fredrik Söderquist <fs@opera.com> Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Cr-Commit-Position: refs/heads/master@{#602119}

Ensure to reschedule an image update if base url changed.
We're careful not to update the image loader when it is inserted into but nothing has changed. However, if we're inserted into a subtree such that the base element is now different, we do need to schedule a new run since the final URL may be different. R=fs@opera.com, chrishtr@chromium.org Bug: 897545 Change-Id: Id84894049c52e8f28a7ae581c3a626da5550207f Reviewed-on: https://chromium-review.googlesource.com/c/1296506 Commit-Queue: vmpstr <vmpstr@chromium.org> Reviewed-by: Fredrik Söderquist <fs@opera.com> Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Cr-Commit-Position: refs/heads/master@{#602119}
81414243 · vmpstr · Commit Bot · eb193717 · 81414243 · 81414243
Commit 81414243 authored Oct 23, 2018 by vmpstr Committed by Commit Bot Oct 23, 2018
9 changed files
--- a/third_party/WebKit/LayoutTests/images/image-load-reset-on-new-base-expected.html
+++ b/third_party/WebKit/LayoutTests/images/image-load-reset-on-new-base-expected.html
+<!doctype HTML>
+
+<iframe srcdoc="<img src='resources/base-test-resources/success.png'>"></iframe>
--- a/third_party/WebKit/LayoutTests/images/image-load-reset-on-new-base.html
+++ b/third_party/WebKit/LayoutTests/images/image-load-reset-on-new-base.html
+<!doctype HTML>
+
+<iframe id="frame"></iframe>
+<script>
+if (window.testRunner)
+  window.testRunner.waitUntilDone();
+function test() {
+  var content = '<head><base href="resources/"></head><body><img src="base-test-resources/success.png"></body>';
+  var doc = document.getElementById("frame").contentDocument;
+  doc.firstChild.innerHTML = content;
+  if (window.testRunner)
+    window.testRunner.notifyDone();
+}
+window.onload = test;
+</script>
+
--- a/third_party/WebKit/LayoutTests/images/resources/base-test-resources/success.png
+++ b/third_party/WebKit/LayoutTests/images/resources/base-test-resources/success.png
--- a/third_party/WebKit/LayoutTests/images/svg-image-load-reset-on-new-base-expected.html
+++ b/third_party/WebKit/LayoutTests/images/svg-image-load-reset-on-new-base-expected.html
+<!doctype HTML>
+
+<iframe srcdoc="<img src='resources/base-test-resources/success.png'>"></iframe>
--- a/third_party/WebKit/LayoutTests/images/svg-image-load-reset-on-new-base.html
+++ b/third_party/WebKit/LayoutTests/images/svg-image-load-reset-on-new-base.html
+<!doctype HTML>
+
+<iframe id="frame"></iframe>
+<script>
+if (window.testRunner)
+  window.testRunner.waitUntilDone();
+function test() {
+  var content = '<head><base href="resources/"></head><body>' +
+    '<svg width="100" height="100">' +
+    '  <image xlink:href="base-test-resources/success.png" />' +
+    '</svg></body>';
+  var doc = document.getElementById("frame").contentDocument;
+  doc.firstChild.innerHTML = content;
+  if (window.testRunner)
+    window.testRunner.notifyDone();
+}
+window.onload = test;
+</script>
+
--- a/third_party/blink/renderer/core/html/html_image_element.cc
+++ b/third_party/blink/renderer/core/html/html_image_element.cc
@@ -446,15 +446,11 @@ Node::InsertionNotificationRequest HTMLImageElement::InsertedInto(
    }
  }

-  // If we have been inserted from a layoutObject-less document,
-  // our loader may have not fetched the image, so do it now.
-  if ((insertion_point.isConnected() && !GetImageLoader().GetContent() &&
-       !GetImageLoader().HasPendingActivity()) ||
-      image_was_modified) {
+  if (image_was_modified ||
+      GetImageLoader().ShouldUpdateOnInsertedInto(insertion_point)) {
    GetImageLoader().UpdateFromElement(ImageLoader::kUpdateNormal,
                                       referrer_policy_);
  }
-
  return HTMLElement::InsertedInto(insertion_point);
 }


--- a/third_party/blink/renderer/core/loader/image_loader.cc
+++ b/third_party/blink/renderer/core/loader/image_loader.cc
@@ -299,6 +299,25 @@ void ImageLoader::SetImageForTest(ImageResourceContent* new_image) {
  SetImageWithoutConsideringPendingLoadEvent(new_image);
 }

+bool ImageLoader::ShouldUpdateOnInsertedInto(
+    ContainerNode& insertion_point) const {
+  // If we're being inserted into a disconnected tree, we don't need to update.
+  if (!insertion_point.isConnected())
+    return false;
+
+  // If the base element URL changed, it means that we might be in the process
+  // of fetching a wrong image. We should update to ensure we fetch the correct
+  // image. This can happen when inserting content into an iframe which has a
+  // base element. See crbug.com/897545 for more details.
+  if (element_->GetDocument().ValidBaseElementURL() != last_base_element_url_)
+    return true;
+
+  // Finally, try to update if we're idle (that is, we have neither the image
+  // contents nor any activity). This could be an indication that we skipped a
+  // previous load when inserted into an inactive document.
+  return !image_content_ && !HasPendingActivity();
+}
+
 void ImageLoader::ClearImage() {
  SetImageWithoutConsideringPendingLoadEvent(nullptr);
 }
@@ -563,6 +582,8 @@ void ImageLoader::UpdateFromElement(UpdateFromElementBehavior update_behavior,
                                    ReferrerPolicy referrer_policy) {
  AtomicString image_source_url = element_->ImageSourceURL();
  suppress_error_events_ = (update_behavior == kUpdateSizeChanged);
+  last_base_element_url_ =
+      element_->GetDocument().ValidBaseElementURL().GetString();

  if (update_behavior == kUpdateIgnorePreviousError)
    ClearFailedLoadURL();

--- a/third_party/blink/renderer/core/loader/image_loader.h
+++ b/third_party/blink/renderer/core/loader/image_loader.h
@@ -87,6 +87,10 @@ class CORE_EXPORT ImageLoader : public GarbageCollectedFinalized<ImageLoader>,

  ImageResourceContent* GetContent() const { return image_content_.Get(); }

+  // Returns true if this loader should be updated via UpdateFromElement() when
+  // being inserted into a new parent; returns false otherwise.
+  bool ShouldUpdateOnInsertedInto(ContainerNode& insertion_point) const;
+
  // Cancels pending load events, and doesn't dispatch new ones.
  // Note: ClearImage/SetImage.*() are not a simple setter.
  // Check the implementation to see what they do.
@@ -200,6 +204,7 @@ class CORE_EXPORT ImageLoader : public GarbageCollectedFinalized<ImageLoader>,
  Member<ImageResourceContent> image_content_;
  Member<ImageResource> image_resource_for_image_document_;

+  String last_base_element_url_;
  AtomicString failed_load_url_;
  base::WeakPtr<Task> pending_task_;  // owned by Microtask
  std::unique_ptr<IncrementLoadEventDelayCount>

--- a/third_party/blink/renderer/core/svg/svg_image_element.cc
+++ b/third_party/blink/renderer/core/svg/svg_image_element.cc
@@ -223,8 +223,7 @@ Node::InsertionNotificationRequest SVGImageElement::InsertedInto(
  // A previous loader update may have failed to actually fetch the image if
  // the document was inactive. In that case, force a re-update (but don't
  // clear previous errors).
-  if (root_parent.isConnected() && !GetImageLoader().GetContent() &&
-      !GetImageLoader().HasPendingActivity())
+  if (GetImageLoader().ShouldUpdateOnInsertedInto(root_parent))
    GetImageLoader().UpdateFromElement(ImageLoader::kUpdateNormal);

  return SVGGraphicsElement::InsertedInto(root_parent);