Auto merge of #15771 - jdm:img-panic, r=nox

Improve behaviour of image elements that perform multiple requests

This addresses cases where image elements end up making multiple requests, as well as makes the element respond to additional relevant mutations that trigger updating the image data.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #15709 (github issue number if applicable).
- [X] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15771)
<!-- Reviewable:end -->

components/script/document_loader.rs

@@ -120,7 +120,7 @@ impl DocumentLoader {
     }
 
     /// Initiate a new fetch that does not block the document load event.
-    pub fn fetch_async_background(&mut self,
+    pub fn fetch_async_background(&self,
                                   request: RequestInit,
                                   fetch_target: IpcSender<FetchResponseMsg>) {
         self.resource_threads.sender().send(CoreResourceMsg::Fetch(request, fetch_target)).unwrap();

components/script/dom/document.rs

@@ -108,7 +108,7 @@ use net_traits::response::HttpsState;
 use num_traits::ToPrimitive;
 use script_layout_interface::message::{Msg, ReflowQueryType};
 use script_runtime::{CommonScriptMsg, ScriptThreadEventCategory};
-use script_thread::{MainThreadScriptMsg, Runnable};
+use script_thread::{MainThreadScriptMsg, Runnable, ScriptThread};
 use script_traits::{AnimationState, CompositorEvent, DocumentActivity};
 use script_traits::{MouseButton, MouseEventType, MozBrowserEvent};
 use script_traits::{MsDuration, ScriptMsg as ConstellationMsg, TouchpadPressurePhase};
@@ -1641,17 +1641,26 @@ impl Document {
         // Step 5 can be found in asap_script_loaded and
         // asap_in_order_script_loaded.
 
-        if self.loader.borrow().is_blocked() {
+        let loader = self.loader.borrow();
+        if loader.is_blocked() || loader.events_inhibited() {
             // Step 6.
             return;
         }
 
-        // The rest will ever run only once per document.
-        if self.loader.borrow().events_inhibited() {
+        ScriptThread::mark_document_with_no_blocked_loads(self);
+    }
+
+    // https://html.spec.whatwg.org/multipage/#the-end
+    pub fn maybe_queue_document_completion(&self) {
+        if self.loader.borrow().is_blocked() {
+            // Step 6.
             return;
         }
+
+        assert!(!self.loader.borrow().events_inhibited());
         self.loader.borrow_mut().inhibit_events();
 
+        // The rest will ever run only once per document.
         // Step 7.
         debug!("Document loads are complete.");
         let handler = box DocumentProgressHandler::new(Trusted::new(self));

components/script/dom/htmlimageelement.rs

@@ -47,6 +47,8 @@ use network_listener::{NetworkListener, PreInvoke};
 use num_traits::ToPrimitive;
 use script_thread::Runnable;
 use servo_url::ServoUrl;
+use std::cell::Cell;
+use std::default::Default;
 use std::i32;
 use std::sync::{Arc, Mutex};
 use style::attr::{AttrValue, LengthOrPercentageOrAuto};
@@ -76,6 +78,7 @@ pub struct HTMLImageElement {
     htmlelement: HTMLElement,
     current_request: DOMRefCell<ImageRequest>,
     pending_request: DOMRefCell<ImageRequest>,
+    generation: Cell<u32>,
 }
 
 impl HTMLImageElement {
@@ -87,14 +90,16 @@ impl HTMLImageElement {
 struct ImageResponseHandlerRunnable {
     element: Trusted<HTMLImageElement>,
     image: ImageResponse,
+    generation: u32,
 }
 
 impl ImageResponseHandlerRunnable {
-    fn new(element: Trusted<HTMLImageElement>, image: ImageResponse)
+    fn new(element: Trusted<HTMLImageElement>, image: ImageResponse, generation: u32)
            -> ImageResponseHandlerRunnable {
         ImageResponseHandlerRunnable {
             element: element,
             image: image,
+            generation: generation,
         }
     }
 }
@@ -104,35 +109,29 @@ impl Runnable for ImageResponseHandlerRunnable {
 
     fn handler(self: Box<Self>) {
         let element = self.element.root();
-        element.process_image_response(self.image);
+        // Ignore any image response for a previous request that has been discarded.
+        if element.generation.get() == self.generation {
+            element.process_image_response(self.image);
+        }
     }
 }
 
 /// The context required for asynchronously loading an external image.
 struct ImageContext {
-    /// The element that initiated the request.
-    elem: Trusted<HTMLImageElement>,
-    /// The initial URL requested.
-    url: ServoUrl,
+    /// A handle with which to communicate with the image cache.
+    image_cache: ImageCacheThread,
     /// Indicates whether the request failed, and why
     status: Result<(), NetworkError>,
     /// The cache ID for this request.
     id: PendingImageId,
 }
 
-impl ImageContext {
-    fn image_cache(&self) -> ImageCacheThread {
-        let elem = self.elem.root();
-        window_from_node(&*elem).image_cache_thread().clone()
-    }
-}
-
 impl FetchResponseListener for ImageContext {
     fn process_request_body(&mut self) {}
     fn process_request_eof(&mut self) {}
 
     fn process_response(&mut self, metadata: Result<FetchMetadata, NetworkError>) {
-        self.image_cache().notify_pending_response(
+        self.image_cache.notify_pending_response(
             self.id,
             FetchResponseMsg::ProcessResponse(metadata.clone()));
 
@@ -156,19 +155,16 @@ impl FetchResponseListener for ImageContext {
 
     fn process_response_chunk(&mut self, payload: Vec<u8>) {
         if self.status.is_ok() {
-            self.image_cache().notify_pending_response(
+            self.image_cache.notify_pending_response(
                 self.id,
                 FetchResponseMsg::ProcessResponseChunk(payload));
         }
     }
 
     fn process_response_eof(&mut self, response: Result<(), NetworkError>) {
-        let elem = self.elem.root();
-        let document = document_from_node(&*elem);
-        let image_cache = self.image_cache();
-        image_cache.notify_pending_response(self.id,
-                                            FetchResponseMsg::ProcessResponseEOF(response));
-        document.finish_load(LoadType::Image(self.url.clone()));
+        self.image_cache.notify_pending_response(
+            self.id,
+            FetchResponseMsg::ProcessResponseEOF(response));
     }
 }
 
@@ -197,11 +193,12 @@ impl HTMLImageElement {
             let window = window_from_node(elem);
             let task_source = window.networking_task_source();
             let wrapper = window.get_runnable_wrapper();
+            let generation = elem.generation.get();
             ROUTER.add_route(responder_receiver.to_opaque(), box move |message| {
                 // Return the image via a message to the script thread, which marks the element
                 // as dirty and triggers a reflow.
                 let runnable = ImageResponseHandlerRunnable::new(
-                    trusted_node.clone(), message.to().unwrap());
+                    trusted_node.clone(), message.to().unwrap(), generation);
                 let _ = task_source.queue_with_wrapper(box runnable, &wrapper);
             });
 
@@ -243,8 +240,7 @@ impl HTMLImageElement {
         let window = window_from_node(self);
 
         let context = Arc::new(Mutex::new(ImageContext {
-            elem: Trusted::new(self),
-            url: img_url.clone(),
+            image_cache: window.image_cache_thread().clone(),
             status: Ok(()),
             id: id,
         }));
@@ -267,7 +263,9 @@ impl HTMLImageElement {
             .. RequestInit::default()
         };
 
-        document.fetch_async(LoadType::Image(img_url), request, action_sender);
+        // This is a background load because the load blocker already fulfills the
+        // purpose of delaying the document's load event.
+        document.loader().fetch_async_background(request, action_sender);
     }
 
     fn process_image_response(&self, image: ImageResponse) {
@@ -299,7 +297,9 @@ impl HTMLImageElement {
             self.upcast::<EventTarget>().fire_event(atom!("error"));
         }
 
-        LoadBlocker::terminate(&mut self.current_request.borrow_mut().blocker);
+        if trigger_image_load || trigger_image_error {
+            LoadBlocker::terminate(&mut self.current_request.borrow_mut().blocker);
+        }
 
         // Trigger reflow
         let window = window_from_node(self);
@@ -309,6 +309,9 @@ impl HTMLImageElement {
     /// Makes the local `image` member match the status of the `src` attribute and starts
     /// prefetching the image. This method must be called after `src` is changed.
     fn update_image(&self, value: Option<(DOMString, ServoUrl)>) {
+        // Force any in-progress request to be ignored.
+        self.generation.set(self.generation.get() + 1);
+
         let document = document_from_node(self);
         let window = document.window();
         match value {
@@ -381,6 +384,7 @@ impl HTMLImageElement {
                 metadata: None,
                 blocker: None,
             }),
+            generation: Default::default(),
         }
     }
 
@@ -622,6 +626,15 @@ impl VirtualMethods for HTMLImageElement {
         Some(self.upcast::<HTMLElement>() as &VirtualMethods)
     }
 
+    fn adopting_steps(&self, old_doc: &Document) {
+        self.super_type().unwrap().adopting_steps(old_doc);
+
+        let elem = self.upcast::<Element>();
+        let document = document_from_node(self);
+        self.update_image(Some((elem.get_string_attribute(&local_name!("src")),
+                                document.base_url())));
+    }
+
     fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation) {
         self.super_type().unwrap().attribute_mutated(attr, mutation);
         match attr.local_name() {

components/script/script_thread.rs

@@ -483,6 +483,10 @@ pub struct ScriptThread {
 
     /// A handle to the webvr thread, if available
     webvr_thread: Option<IpcSender<WebVRMsg>>,
+
+    /// A list of pipelines containing documents that finished loading all their blocking
+    /// resources during a turn of the event loop.
+    docs_with_no_blocking_loads: DOMRefCell<HashSet<JS<Document>>>,
 }
 
 /// In the event of thread panic, all data on the stack runs its destructor. However, there
@@ -567,6 +571,15 @@ impl ScriptThreadFactory for ScriptThread {
 }
 
 impl ScriptThread {
+    pub fn mark_document_with_no_blocked_loads(doc: &Document) {
+        SCRIPT_THREAD_ROOT.with(|root| {
+            let script_thread = unsafe { &*root.get().unwrap() };
+            script_thread.docs_with_no_blocking_loads
+                .borrow_mut()
+                .insert(JS::from_ref(doc));
+        })
+    }
+
     pub fn invoke_perform_a_microtask_checkpoint() {
         SCRIPT_THREAD_ROOT.with(|root| {
             let script_thread = unsafe { &*root.get().unwrap() };
@@ -704,7 +717,9 @@ impl ScriptThread {
 
             layout_to_constellation_chan: state.layout_to_constellation_chan,
 
-            webvr_thread: state.webvr_thread
+            webvr_thread: state.webvr_thread,
+
+            docs_with_no_blocking_loads: Default::default(),
         }
     }
 
@@ -885,6 +900,15 @@ impl ScriptThread {
             }
         }
 
+        {
+            // https://html.spec.whatwg.org/multipage/#the-end step 6
+            let mut docs = self.docs_with_no_blocking_loads.borrow_mut();
+            for document in docs.iter() {
+                document.maybe_queue_document_completion();
+            }
+            docs.clear();
+        }
+
         // https://html.spec.whatwg.org/multipage/#event-loop-processing-model step 7.12
 
         // Issue batched reflows on any pages that require it (e.g. if images loaded)

components/script_plugins/unrooted_must_root.rs

@@ -54,7 +54,8 @@ fn is_unrooted_ty(cx: &LateContext, ty: &ty::TyS, in_new_function: bool) -> bool
                         || match_def_path(cx, did.did, &["core", "slice", "Iter"])
                         || match_def_path(cx, did.did, &["std", "collections", "hash", "map", "Entry"])
                         || match_def_path(cx, did.did, &["std", "collections", "hash", "map", "OccupiedEntry"])
-                        || match_def_path(cx, did.did, &["std", "collections", "hash", "map", "VacantEntry"]) {
+                        || match_def_path(cx, did.did, &["std", "collections", "hash", "map", "VacantEntry"])
+                        || match_def_path(cx, did.did, &["std", "collections", "hash", "set", "Iter"]) {
                     // Structures which are semantically similar to an &ptr.
                     false
                 } else if did.is_box() && in_new_function {

tests/wpt/metadata/MANIFEST.json

@@ -8337,6 +8337,18 @@
      {}
     ]
    ],
+   "html/semantics/embedded-content/the-img-element/document-adopt-base-url.html": [
+    [
+     "/html/semantics/embedded-content/the-img-element/document-adopt-base-url.html",
+     [
+      [
+       "/html/semantics/embedded-content/the-img-element/document-base-url-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "html/semantics/embedded-content/the-img-element/document-base-url.html": [
     [
      "/html/semantics/embedded-content/the-img-element/document-base-url.html",
@@ -93287,6 +93299,12 @@
      {}
     ]
    ],
+   "html/semantics/embedded-content/the-img-element/delay-load-event.html": [
+    [
+     "/html/semantics/embedded-content/the-img-element/delay-load-event.html",
+     {}
+    ]
+   ],
    "html/semantics/embedded-content/the-img-element/environment-changes/viewport-change.html": [
     [
      "/html/semantics/embedded-content/the-img-element/environment-changes/viewport-change.html",
@@ -176314,6 +176332,14 @@
    "bdbfbe9a5908c6233bd7b9697a0762bd2e0f6ede",
    "testharness"
   ],
+  "html/semantics/embedded-content/the-img-element/delay-load-event.html": [
+   "e4782535af755b29864fd3de67bbdd0de13f19d7",
+   "testharness"
+  ],
+  "html/semantics/embedded-content/the-img-element/document-adopt-base-url.html": [
+   "a4b542eb344cca6bdcceceb3aa7006e900f5400f",
+   "reftest"
+  ],
   "html/semantics/embedded-content/the-img-element/document-base-url-ref.html": [
    "add78257076d22891334b93c8072d098ace9b6eb",
    "support"

tests/wpt/mozilla/meta/MANIFEST.json

@@ -12892,6 +12892,12 @@
      {}
     ]
    ],
+   "mozilla/img_multiple_request.html": [
+    [
+     "/_mozilla/mozilla/img_multiple_request.html",
+     {}
+    ]
+   ],
    "mozilla/img_width_height.html": [
     [
      "/_mozilla/mozilla/img_width_height.html",
@@ -25375,6 +25381,10 @@
    "3c4f36abed83367c851d943b1f25b8394de6fe75",
    "testharness"
   ],
+  "mozilla/img_multiple_request.html": [
+   "0a6263ad87c9b3307f2dc694747b094a0517b79b",
+   "testharness"
+  ],
   "mozilla/img_width_height.html": [
    "37a04735261a6d2b36c3d529ce81eda46ed6967e",
    "testharness"

tests/wpt/mozilla/tests/mozilla/img_multiple_request.html

@@ -0,0 +1,25 @@
+<html>
+<head>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script>
+    async_test(function(t) {
+        var i = new Image();
+        i.src = "2x2.png";
+        i.src = "2x2.png";
+        i.onload = t.step_func(function() {
+            i.onload = this.unreached_func("Load event for aborted request.");
+            t.step_timeout(t.step_func_done(), 100);
+        });
+    }, "Multiple requests for the same URL do not trigger multiple load events.");
+
+    async_test(function(t) {
+        var i = new Image();
+        i.src = "test.png";
+        i.src = "2x2.png";
+        i.onload = t.step_func(function() {
+            i.onload = this.unreached_func("Load event for aborted request.");
+            t.step_timeout(t.step_func_done(), 100);
+        });
+    }, "Multiple requests for different URL do not trigger multiple load events.");
+</script>