From f0f43ad9b0a5e7a99933bbb2192614868ef3250b Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 28 Feb 2017 10:23:23 -0500 Subject: [PATCH 1/7] Remove mutability requirement from fetch_async_background. --- components/script/document_loader.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/script/document_loader.rs b/components/script/document_loader.rs index a4ce36a2f57..611c469fa2d 100644 --- a/components/script/document_loader.rs +++ b/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) { self.resource_threads.sender().send(CoreResourceMsg::Fetch(request, fetch_target)).unwrap(); From 208f97420a737309d93baed75b7706862cac76ed Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 28 Feb 2017 10:25:09 -0500 Subject: [PATCH 2/7] Ignore image cache updates for image requests that have been aborted. --- components/script/dom/htmlimageelement.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/components/script/dom/htmlimageelement.rs b/components/script/dom/htmlimageelement.rs index af98b013410..c826f1e5a48 100644 --- a/components/script/dom/htmlimageelement.rs +++ b/components/script/dom/htmlimageelement.rs @@ -46,6 +46,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}; @@ -75,6 +77,7 @@ pub struct HTMLImageElement { htmlelement: HTMLElement, current_request: DOMRefCell, pending_request: DOMRefCell, + generation: Cell, } impl HTMLImageElement { @@ -86,14 +89,16 @@ impl HTMLImageElement { struct ImageResponseHandlerRunnable { element: Trusted, image: ImageResponse, + generation: u32, } impl ImageResponseHandlerRunnable { - fn new(element: Trusted, image: ImageResponse) + fn new(element: Trusted, image: ImageResponse, generation: u32) -> ImageResponseHandlerRunnable { ImageResponseHandlerRunnable { element: element, image: image, + generation: generation, } } } @@ -103,7 +108,10 @@ impl Runnable for ImageResponseHandlerRunnable { fn handler(self: Box) { 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); + } } } @@ -196,11 +204,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); }); @@ -308,6 +317,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 { @@ -380,6 +392,7 @@ impl HTMLImageElement { metadata: None, blocker: None, }), + generation: Default::default(), } } From 2bd02fe4238e6843e81a2682e7dcd12952401e13 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 28 Feb 2017 10:27:10 -0500 Subject: [PATCH 3/7] Initiate a new image request when an image element's adopting steps are run. Remove the redundant tracked loads for the underlying image request; the load blockers present in the ImageRequest structure perform the same duty with better accuracy. --- components/script/dom/htmlimageelement.rs | 42 +++++++++++------------ 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/components/script/dom/htmlimageelement.rs b/components/script/dom/htmlimageelement.rs index c826f1e5a48..65897f1c91a 100644 --- a/components/script/dom/htmlimageelement.rs +++ b/components/script/dom/htmlimageelement.rs @@ -117,29 +117,20 @@ impl Runnable for ImageResponseHandlerRunnable { /// The context required for asynchronously loading an external image. struct ImageContext { - /// The element that initiated the request. - elem: Trusted, - /// 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) { - self.image_cache().notify_pending_response( + self.image_cache.notify_pending_response( self.id, FetchResponseMsg::ProcessResponse(metadata.clone())); @@ -163,19 +154,16 @@ impl FetchResponseListener for ImageContext { fn process_response_chunk(&mut self, payload: Vec) { 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)); } } @@ -251,8 +239,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, })); @@ -275,7 +262,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) { @@ -634,6 +623,15 @@ impl VirtualMethods for HTMLImageElement { Some(self.upcast::() as &VirtualMethods) } + fn adopting_steps(&self, old_doc: &Document) { + self.super_type().unwrap().adopting_steps(old_doc); + + let elem = self.upcast::(); + 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() { From dc5335a21eb70f5a9751ef2f6929ed3731b4ad5e Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 28 Feb 2017 10:30:13 -0500 Subject: [PATCH 4/7] Move checks for document completion to the end of the event loop. This better reflects the text of the specification - rather than queuing a task to dispatch the load evnet as soon as the document loader is unblocked, we want to "spin the event loop until there is nothing that delays the load event in the Document." Spinning the event loop is a concept that requires running tasks completely, hence we check the condition before returning to the start of the event loop. --- components/script/dom/document.rs | 19 ++++++++++---- components/script/script_thread.rs | 26 ++++++++++++++++++- .../script_plugins/unrooted_must_root.rs | 3 ++- 3 files changed, 41 insertions(+), 7 deletions(-) diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index c836a46f1da..6c79bb38578 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -107,7 +107,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::{ScriptMsg as ConstellationMsg, TouchpadPressurePhase}; @@ -1599,17 +1599,26 @@ impl Document { // Step 5 can be found in asap_script_loaded and // asap_in_order_script_loaded. + let loader = self.loader.borrow(); + if loader.is_blocked() || loader.events_inhibited() { + // Step 6. + return; + } + + 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; } - // The rest will ever run only once per document. - if self.loader.borrow().events_inhibited() { - 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)); diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 192923294a6..e877e904ceb 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -483,6 +483,10 @@ pub struct ScriptThread { /// A handle to the webvr thread, if available webvr_thread: Option>, + + /// 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>>, } /// 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) diff --git a/components/script_plugins/unrooted_must_root.rs b/components/script_plugins/unrooted_must_root.rs index ad74f6c4b8f..c62796b7a90 100644 --- a/components/script_plugins/unrooted_must_root.rs +++ b/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 { From 330e222fe87a1708300ae2612d99219043eeebe6 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 28 Feb 2017 10:50:59 -0500 Subject: [PATCH 5/7] Test adopting images into documents with different base URLs. --- tests/wpt/metadata/MANIFEST.json | 16 ++++++++++++++++ .../the-img-element/document-adopt-base-url.html | 14 ++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 tests/wpt/web-platform-tests/html/semantics/embedded-content/the-img-element/document-adopt-base-url.html diff --git a/tests/wpt/metadata/MANIFEST.json b/tests/wpt/metadata/MANIFEST.json index bca20e4f735..70715054688 100644 --- a/tests/wpt/metadata/MANIFEST.json +++ b/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", @@ -176314,6 +176326,10 @@ "bdbfbe9a5908c6233bd7b9697a0762bd2e0f6ede", "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" diff --git a/tests/wpt/web-platform-tests/html/semantics/embedded-content/the-img-element/document-adopt-base-url.html b/tests/wpt/web-platform-tests/html/semantics/embedded-content/the-img-element/document-adopt-base-url.html new file mode 100644 index 00000000000..ea63114d570 --- /dev/null +++ b/tests/wpt/web-platform-tests/html/semantics/embedded-content/the-img-element/document-adopt-base-url.html @@ -0,0 +1,14 @@ + + +Document base URL adopted img test + + + + + From db79dfb3aaf4554708c5136df6881c76b7ead8fe Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 28 Feb 2017 13:44:00 -0500 Subject: [PATCH 6/7] Verify that aborted image requests do not cause extraneous load events. --- tests/wpt/mozilla/meta/MANIFEST.json | 10 ++++++++ .../tests/mozilla/img_multiple_request.html | 25 +++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 tests/wpt/mozilla/tests/mozilla/img_multiple_request.html diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index 7279d1f9440..e3123f31917 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -12874,6 +12874,12 @@ {} ] ], + "mozilla/img_multiple_request.html": [ + [ + "/_mozilla/mozilla/img_multiple_request.html", + {} + ] + ], "mozilla/img_width_height.html": [ [ "/_mozilla/mozilla/img_width_height.html", @@ -25345,6 +25351,10 @@ "3c4f36abed83367c851d943b1f25b8394de6fe75", "testharness" ], + "mozilla/img_multiple_request.html": [ + "0a6263ad87c9b3307f2dc694747b094a0517b79b", + "testharness" + ], "mozilla/img_width_height.html": [ "37a04735261a6d2b36c3d529ce81eda46ed6967e", "testharness" diff --git a/tests/wpt/mozilla/tests/mozilla/img_multiple_request.html b/tests/wpt/mozilla/tests/mozilla/img_multiple_request.html new file mode 100644 index 00000000000..df625a2bc33 --- /dev/null +++ b/tests/wpt/mozilla/tests/mozilla/img_multiple_request.html @@ -0,0 +1,25 @@ + + + + + From f79850754df974a9f58c235b0742d7e6b9f9559c Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 28 Feb 2017 13:45:32 -0500 Subject: [PATCH 7/7] Avoid marking image element as complete before its image data is available. --- components/script/dom/htmlimageelement.rs | 4 +++- tests/wpt/metadata/MANIFEST.json | 10 ++++++++++ .../the-img-element/delay-load-event.html | 17 +++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 tests/wpt/web-platform-tests/html/semantics/embedded-content/the-img-element/delay-load-event.html diff --git a/components/script/dom/htmlimageelement.rs b/components/script/dom/htmlimageelement.rs index 65897f1c91a..9379ebb48a9 100644 --- a/components/script/dom/htmlimageelement.rs +++ b/components/script/dom/htmlimageelement.rs @@ -296,7 +296,9 @@ impl HTMLImageElement { self.upcast::().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); diff --git a/tests/wpt/metadata/MANIFEST.json b/tests/wpt/metadata/MANIFEST.json index 70715054688..2223d4e2282 100644 --- a/tests/wpt/metadata/MANIFEST.json +++ b/tests/wpt/metadata/MANIFEST.json @@ -93299,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", @@ -176326,6 +176332,10 @@ "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" diff --git a/tests/wpt/web-platform-tests/html/semantics/embedded-content/the-img-element/delay-load-event.html b/tests/wpt/web-platform-tests/html/semantics/embedded-content/the-img-element/delay-load-event.html new file mode 100644 index 00000000000..c67074a40d8 --- /dev/null +++ b/tests/wpt/web-platform-tests/html/semantics/embedded-content/the-img-element/delay-load-event.html @@ -0,0 +1,17 @@ + + +Image element delays window's load event + + + +