ImageLoader: Stop storing request KURL in Task

This CL removes |ImageLoader::Task::request_url_| in favor of lazily
parsing the image request's absolute URL when ImageLoader::Task is run.

Currently ImageLoader::Task stores a KURL |request_url_|, which is a
snapshot of the image's absolute URL, parsed relative to the image
element's node document at the time ImageLoader::Task is queued.
We shouldn't have to store a snapshot of the parsed URL, because the
source URL at the time an ImageLoader::Task is run is the same as when
the task is queued.

The reason we stored a snapshot of the parsed URL is because what *can*
change in between when an ImageLoader::Task is queued and run, is the
document's base URL, via a `<base>` element. In https://crbug.com/569760
a CL was landed that takes this snapshot, so that if a `<base>` element
was dynamically-inserted before the ImageLoader::Task was run, the URL
would be unaffected by the addition of the `<base>`. However, the spec
is very clear that the parsing of an image request's URL [1] happens
after the microtask is run, meaning our snapshot is unnecessary, and
incorrect.

This CL:
 - Removes the snapshot
 - Fixes the associated test
 - Exports the test to external/wpt so other impls can benefit

[1]: https://html.spec.whatwg.org/multipage/images.html#updating-the-image-data:parse-a-url-2

Bug: 569760,1061685
Change-Id: I8d9e3dbefd27a26626bb930acc809196753c7506
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2103987
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#756376}

third_party/blink/renderer/core/loader/image_loader.cc

@@ -109,14 +109,12 @@ static ImageLoader::BypassMainWorldBehavior ShouldBypassMainWorldCSP(
 class ImageLoader::Task {
  public:
   Task(ImageLoader* loader,
-       const KURL& request_url,
        UpdateFromElementBehavior update_behavior,
        network::mojom::ReferrerPolicy referrer_policy)
       : loader_(loader),
         should_bypass_main_world_csp_(ShouldBypassMainWorldCSP(loader)),
         update_behavior_(update_behavior),
-        referrer_policy_(referrer_policy),
-        request_url_(request_url) {
+        referrer_policy_(referrer_policy) {
     ExecutionContext* context =
         loader_->GetElement()->GetDocument().ToExecutionContext();
     probe::AsyncTaskScheduled(context, "Image", &async_task_id_);
@@ -142,13 +140,11 @@ class ImageLoader::Task {
     if (script_state_ && script_state_->ContextIsValid()) {
       ScriptState::Scope scope(script_state_);
       loader_->DoUpdateFromElement(should_bypass_main_world_csp_,
-                                   update_behavior_, request_url_,
-                                   referrer_policy_);
+                                   update_behavior_, referrer_policy_);
     } else {
       // This call does not access v8::Context internally.
       loader_->DoUpdateFromElement(should_bypass_main_world_csp_,
-                                   update_behavior_, request_url_,
-                                   referrer_policy_);
+                                   update_behavior_, referrer_policy_);
     }
   }
 
@@ -165,7 +161,7 @@ class ImageLoader::Task {
   UpdateFromElementBehavior update_behavior_;
   WeakPersistent<ScriptState> script_state_;
   network::mojom::ReferrerPolicy referrer_policy_;
-  KURL request_url_;
+
   probe::AsyncTaskId async_task_id_;
   base::WeakPtrFactory<Task> weak_factory_{this};
 };
@@ -416,11 +412,9 @@ inline void ImageLoader::ClearFailedLoadURL() {
 }
 
 inline void ImageLoader::EnqueueImageLoadingMicroTask(
-    const KURL& request_url,
     UpdateFromElementBehavior update_behavior,
     network::mojom::ReferrerPolicy referrer_policy) {
-  auto task = std::make_unique<Task>(this, request_url, update_behavior,
-                                     referrer_policy);
+  auto task = std::make_unique<Task>(this, update_behavior, referrer_policy);
   pending_task_ = task->GetWeakPtr();
   Microtask::EnqueueMicrotask(
       WTF::Bind(&Task::Run, WTF::Passed(std::move(task))));
@@ -448,7 +442,6 @@ void ImageLoader::UpdateImageState(ImageResourceContent* new_image_content) {
 void ImageLoader::DoUpdateFromElement(
     BypassMainWorldBehavior bypass_behavior,
     UpdateFromElementBehavior update_behavior,
-    const KURL& url,
     network::mojom::ReferrerPolicy referrer_policy,
     UpdateType update_type) {
   // FIXME: According to
@@ -469,6 +462,7 @@ void ImageLoader::DoUpdateFromElement(
     return;
 
   AtomicString image_source_url = element_->ImageSourceURL();
+  const KURL url = ImageSourceToKURL(image_source_url);
   ImageResourceContent* new_image_content = nullptr;
   if (!url.IsNull() && !url.IsEmpty()) {
     // Unlike raw <img>, we block mixed content inside of <picture> or
@@ -677,10 +671,8 @@ void ImageLoader::UpdateFromElement(
     delay_until_do_update_from_element_ = nullptr;
   }
 
-  KURL url = ImageSourceToKURL(image_source_url);
-
-  if (ShouldLoadImmediately(url)) {
-    DoUpdateFromElement(kDoNotBypassMainWorldCSP, update_behavior, url,
+  if (ShouldLoadImmediately(ImageSourceToKURL(image_source_url))) {
+    DoUpdateFromElement(kDoNotBypassMainWorldCSP, update_behavior,
                         referrer_policy, UpdateType::kSync);
     return;
   }
@@ -705,7 +697,7 @@ void ImageLoader::UpdateFromElement(
   // images we don't intend to display.
   Document& document = element_->GetDocument();
   if (!document.IsContextDestroyed() && document.IsActive())
-    EnqueueImageLoadingMicroTask(url, update_behavior, referrer_policy);
+    EnqueueImageLoadingMicroTask(update_behavior, referrer_policy);
 }
 
 KURL ImageLoader::ImageSourceToKURL(AtomicString image_source_url) const {

third_party/blink/renderer/core/loader/image_loader.h

@@ -173,7 +173,6 @@ class CORE_EXPORT ImageLoader : public GarbageCollected<ImageLoader>,
   void DoUpdateFromElement(
       BypassMainWorldBehavior,
       UpdateFromElementBehavior,
-      const KURL&,
       network::mojom::ReferrerPolicy = network::mojom::ReferrerPolicy::kDefault,
       UpdateType = UpdateType::kAsync);
 
@@ -197,8 +196,7 @@ class CORE_EXPORT ImageLoader : public GarbageCollected<ImageLoader>,
   void ClearFailedLoadURL();
   void DispatchErrorEvent();
   void CrossSiteOrCSPViolationOccurred(AtomicString);
-  void EnqueueImageLoadingMicroTask(const KURL&,
-                                    UpdateFromElementBehavior,
+  void EnqueueImageLoadingMicroTask(UpdateFromElementBehavior,
                                     network::mojom::ReferrerPolicy);
 
   KURL ImageSourceToKURL(AtomicString) const;

third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-img-element/image-parse-url-base.html

@@ -0,0 +1,30 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <title>Image load parses URL after microtask runs</title>
+  <script src="/resources/testharness.js"></script>
+  <script src="/resources/testharnessreport.js"></script>
+</head>
+
+<body>
+<img id="img">
+<script>
+const t = async_test("An image request's parsed URL should be affected by a " +
+                     "dynamically-inserted <base>, if it was inserted before " +
+                     "the image request microtask runs");
+
+t.step(() => {
+  const elm = document.getElementById('img');
+  elm.src = 'resources/image.png';
+  elm.onload = t.unreached_func("The image should have failed to load, as " +
+                                "the request URL should be affected by the " +
+                                "<base> element");
+  elm.onerror = t.step_func_done();
+
+  const base = document.createElement("base");
+  base.setAttribute("href", "bogus/");
+  document.head.appendChild(base);
+});
+</script>
+</body>
+</html>