From 37695c8c8cd588ea103cb92ea44c549acc869706 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Sat, 25 Jan 2025 10:39:04 +0100 Subject: [PATCH] script: Do not set up an IPC route for every image load (#35041) Instead of setting up a route for every image load in the DOM / Layout, route all incoming image cache responses through the `ScriptThread`. This avoids creating a set of file descriptor for every image that is loaded. This change requires having the `ImageCache` track the `PipelineId` of the original the listener so that the `ScriptThread` can route it properly to the correct `Window`. Signed-off-by: Martin Robinson --- Cargo.lock | 1 + components/layout/context.rs | 2 +- components/layout/fragment.rs | 4 +- components/layout_2020/context.rs | 2 +- components/layout_2020/replaced.rs | 2 +- components/malloc_size_of/Cargo.toml | 1 + components/malloc_size_of/lib.rs | 6 + components/net/image_cache.rs | 53 +------ components/script/dom/globalscope.rs | 1 - components/script/dom/htmlimageelement.rs | 164 +++++++++++----------- components/script/dom/htmlvideoelement.rs | 78 ++++++---- components/script/dom/window.rs | 95 +++++++++---- components/script/image_listener.rs | 53 ------- components/script/lib.rs | 1 - components/script/messaging.rs | 14 +- components/script/script_thread.rs | 16 ++- components/shared/net/image_cache.rs | 39 +++-- 17 files changed, 260 insertions(+), 272 deletions(-) delete mode 100644 components/script/image_listener.rs diff --git a/Cargo.lock b/Cargo.lock index a7bea7667f1..c387bef5e0a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6675,6 +6675,7 @@ dependencies = [ "euclid", "http 1.2.0", "indexmap", + "ipc-channel", "keyboard-types", "malloc_size_of", "markup5ever", diff --git a/components/layout/context.rs b/components/layout/context.rs index 39b3f2b88aa..6c1d2e4e228 100644 --- a/components/layout/context.rs +++ b/components/layout/context.rs @@ -137,7 +137,7 @@ impl LayoutContext<'_> { Some(image_info) } }, - None | Some(ImageOrMetadataAvailable::MetadataAvailable(_)) => None, + None | Some(ImageOrMetadataAvailable::MetadataAvailable(..)) => None, } } } diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs index b50add41e98..b697c7da395 100644 --- a/components/layout/fragment.rs +++ b/components/layout/fragment.rs @@ -440,8 +440,8 @@ impl ImageFragmentInfo { ImageOrMetadataAvailable::ImageAvailable { image, .. } => { ImageOrMetadata::Image(image) }, - ImageOrMetadataAvailable::MetadataAvailable(m) => { - ImageOrMetadata::Metadata(m) + ImageOrMetadataAvailable::MetadataAvailable(metadata, _id) => { + ImageOrMetadata::Metadata(metadata) }, }) }) diff --git a/components/layout_2020/context.rs b/components/layout_2020/context.rs index ad3d3ade32a..2dbfb7eabb8 100644 --- a/components/layout_2020/context.rs +++ b/components/layout_2020/context.rs @@ -129,7 +129,7 @@ impl LayoutContext<'_> { Some(image_info) } }, - None | Some(ImageOrMetadataAvailable::MetadataAvailable(_)) => None, + None | Some(ImageOrMetadataAvailable::MetadataAvailable(..)) => None, } } } diff --git a/components/layout_2020/replaced.rs b/components/layout_2020/replaced.rs index 5d982dc4ee3..8739428758d 100644 --- a/components/layout_2020/replaced.rs +++ b/components/layout_2020/replaced.rs @@ -217,7 +217,7 @@ impl ReplacedContents { Some(ImageOrMetadataAvailable::ImageAvailable { image, .. }) => { (Some(image.clone()), image.width as f32, image.height as f32) }, - Some(ImageOrMetadataAvailable::MetadataAvailable(metadata)) => { + Some(ImageOrMetadataAvailable::MetadataAvailable(metadata, _id)) => { (None, metadata.width as f32, metadata.height as f32) }, None => return None, diff --git a/components/malloc_size_of/Cargo.toml b/components/malloc_size_of/Cargo.toml index a41ddd06978..69c6d4a870b 100644 --- a/components/malloc_size_of/Cargo.toml +++ b/components/malloc_size_of/Cargo.toml @@ -18,6 +18,7 @@ crossbeam-channel = { workspace = true } euclid = { workspace = true } http = { workspace = true } indexmap = { workspace = true } +ipc-channel = { workspace = true } keyboard-types = { workspace = true } markup5ever = { workspace = true } serde = { workspace = true } diff --git a/components/malloc_size_of/lib.rs b/components/malloc_size_of/lib.rs index d53aacf27f2..ec633a49f9b 100644 --- a/components/malloc_size_of/lib.rs +++ b/components/malloc_size_of/lib.rs @@ -674,6 +674,12 @@ impl MallocSizeOf for tokio::sync::mpsc::UnboundedSender { } } +impl MallocSizeOf for ipc_channel::ipc::IpcSender { + fn size_of(&self, _ops: &mut MallocSizeOfOps) -> usize { + 0 + } +} + impl MallocSizeOf for accountable_refcell::RefCell { fn size_of(&self, ops: &mut MallocSizeOfOps) -> usize { self.borrow().size_of(ops) diff --git a/components/net/image_cache.rs b/components/net/image_cache.rs index a9a6e558919..28ca66dea21 100644 --- a/components/net/image_cache.rs +++ b/components/net/image_cache.rs @@ -8,11 +8,11 @@ use std::sync::{Arc, Mutex}; use std::{mem, thread}; use imsz::imsz_from_reader; -use ipc_channel::ipc::{IpcSender, IpcSharedMemory}; +use ipc_channel::ipc::IpcSharedMemory; use log::{debug, warn}; use net_traits::image_cache::{ ImageCache, ImageCacheResult, ImageOrMetadataAvailable, ImageResponder, ImageResponse, - PendingImageId, PendingImageResponse, UsePlaceholder, + PendingImageId, UsePlaceholder, }; use net_traits::request::CorsSettings; use net_traits::{FetchMetadata, FetchResponseMsg, FilteredMetadata, NetworkError}; @@ -497,7 +497,7 @@ impl ImageCache for ImageCacheImpl { (&None, Some(meta)) => { debug!("Metadata available for {} ({:?})", url, key); return ImageCacheResult::Available( - ImageOrMetadataAvailable::MetadataAvailable(meta.clone()), + ImageOrMetadataAvailable::MetadataAvailable(meta.clone(), key), ); }, (&Some(Err(_)), _) | (&None, &None) => { @@ -534,46 +534,11 @@ impl ImageCache for ImageCacheImpl { } } - fn track_image( - &self, - url: ServoUrl, - origin: ImmutableOrigin, - cors_setting: Option, - sender: IpcSender, - use_placeholder: UsePlaceholder, - ) -> ImageCacheResult { - debug!("Track image for {} ({:?})", url, origin); - let cache_result = self.get_cached_image_status( - url.clone(), - origin.clone(), - cors_setting, - use_placeholder, - ); - - match cache_result { - ImageCacheResult::Available(ImageOrMetadataAvailable::MetadataAvailable(_)) => { - let mut store = self.store.lock().unwrap(); - let id = *store - .pending_loads - .url_to_load_key - .get(&(url, origin, cors_setting)) - .unwrap(); - self.add_listener_with_store(&mut store, id, ImageResponder::new(sender, id)); - }, - ImageCacheResult::Pending(id) | ImageCacheResult::ReadyForRequest(id) => { - self.add_listener(id, ImageResponder::new(sender, id)); - }, - _ => {}, - } - - cache_result - } - /// Add a new listener for the given pending image id. If the image is already present, /// the responder will still receive the expected response. - fn add_listener(&self, id: PendingImageId, listener: ImageResponder) { + fn add_listener(&self, listener: ImageResponder) { let mut store = self.store.lock().unwrap(); - self.add_listener_with_store(&mut store, id, listener); + self.add_listener_with_store(&mut store, listener); } /// Inform the image cache about a response for a pending request. @@ -658,12 +623,8 @@ impl ImageCache for ImageCacheImpl { impl ImageCacheImpl { /// Require self.store.lock() before calling. - fn add_listener_with_store( - &self, - store: &mut ImageCacheStore, - id: PendingImageId, - listener: ImageResponder, - ) { + fn add_listener_with_store(&self, store: &mut ImageCacheStore, listener: ImageResponder) { + let id = listener.id; if let Some(load) = store.pending_loads.get_by_key_mut(&id) { if let Some(ref metadata) = load.metadata { listener.respond(ImageResponse::MetadataLoaded(metadata.clone())); diff --git a/components/script/dom/globalscope.rs b/components/script/dom/globalscope.rs index 119d8c239ab..ba5cbc5b720 100644 --- a/components/script/dom/globalscope.rs +++ b/components/script/dom/globalscope.rs @@ -233,7 +233,6 @@ pub(crate) struct GlobalScope { inline_module_map: DomRefCell>>, /// For providing instructions to an optional devtools server. - #[ignore_malloc_size_of = "channels are hard"] #[no_trace] devtools_chan: Option>, diff --git a/components/script/dom/htmlimageelement.rs b/components/script/dom/htmlimageelement.rs index f8b4ba5d354..36fa7b9ee67 100644 --- a/components/script/dom/htmlimageelement.rs +++ b/components/script/dom/htmlimageelement.rs @@ -15,16 +15,13 @@ use cssparser::{Parser, ParserInput}; use dom_struct::dom_struct; use euclid::Point2D; use html5ever::{local_name, namespace_url, ns, LocalName, Prefix, QualName}; -use ipc_channel::ipc; -use ipc_channel::ipc::IpcSender; -use ipc_channel::router::ROUTER; use js::jsapi::JSAutoRealm; use js::rust::HandleObject; use mime::{self, Mime}; use net_traits::http_status::HttpStatus; use net_traits::image_cache::{ - ImageCache, ImageCacheResult, ImageOrMetadataAvailable, ImageResponse, PendingImageId, - PendingImageResponse, UsePlaceholder, + ImageCache, ImageCacheResult, ImageOrMetadataAvailable, ImageResponder, ImageResponse, + PendingImageId, UsePlaceholder, }; use net_traits::request::{ CorsSettings, Destination, Initiator, Referrer, RequestBuilder, RequestId, @@ -89,7 +86,6 @@ use crate::dom::values::UNSIGNED_LONG_MAX; use crate::dom::virtualmethods::VirtualMethods; use crate::dom::window::Window; use crate::fetch::create_a_potential_cors_request; -use crate::image_listener::{generate_cache_listener_for_element, ImageCacheListener}; use crate::microtask::{Microtask, MicrotaskRunnable}; use crate::network_listener::{self, PreInvoke, ResourceTimingListener}; use crate::realms::enter_realm; @@ -355,13 +351,11 @@ impl HTMLImageElement { /// Update the current image with a valid URL. fn fetch_image(&self, img_url: &ServoUrl, can_gc: CanGc) { let window = self.owner_window(); - let image_cache = window.image_cache(); - let sender = generate_cache_listener_for_element(self); - let cache_result = image_cache.track_image( + + let cache_result = window.image_cache().get_cached_image_status( img_url.clone(), window.origin().immutable().clone(), cors_setting_for_element(self.upcast()), - sender, UsePlaceholder::Yes, ); @@ -380,15 +374,63 @@ impl HTMLImageElement { self.process_image_response(ImageResponse::Loaded(image, url), can_gc) } }, - ImageCacheResult::Available(ImageOrMetadataAvailable::MetadataAvailable(m)) => { - self.process_image_response(ImageResponse::MetadataLoaded(m), can_gc) + ImageCacheResult::Available(ImageOrMetadataAvailable::MetadataAvailable( + metadata, + id, + )) => { + self.process_image_response(ImageResponse::MetadataLoaded(metadata), can_gc); + self.register_image_cache_callback(id, ChangeType::Element); + }, + ImageCacheResult::Pending(id) => { + self.register_image_cache_callback(id, ChangeType::Element); + }, + ImageCacheResult::ReadyForRequest(id) => { + self.fetch_request(img_url, id); + self.register_image_cache_callback(id, ChangeType::Element); }, - ImageCacheResult::Pending(_) => (), - ImageCacheResult::ReadyForRequest(id) => self.fetch_request(img_url, id), ImageCacheResult::LoadError => self.process_image_response(ImageResponse::None, can_gc), }; } + fn register_image_cache_callback(&self, id: PendingImageId, change_type: ChangeType) { + let trusted_node = Trusted::new(self); + let generation = self.generation_id(); + let window = self.owner_window(); + let sender = window.register_image_cache_listener(id, move |response| { + let trusted_node = trusted_node.clone(); + let window = trusted_node.root().owner_window(); + let callback_type = change_type.clone(); + + window + .as_global_scope() + .task_manager() + .networking_task_source() + .queue(task!(process_image_response: move || { + let element = trusted_node.root(); + + // Ignore any image response for a previous request that has been discarded. + if generation != element.generation_id() { + return; + } + + match callback_type { + ChangeType::Element => { + element.process_image_response(response.response, CanGc::note()); + } + ChangeType::Environment { selected_source, selected_pixel_density } => { + element.process_image_response_for_environment_change( + response.response, selected_source, generation, selected_pixel_density, CanGc::note() + ); + } + } + })); + }); + + window + .image_cache() + .add_listener(ImageResponder::new(sender, window.pipeline_id(), id)); + } + fn fetch_request(&self, img_url: &ServoUrl, id: PendingImageId) { let document = self.owner_document(); let window = self.owner_window(); @@ -1054,48 +1096,6 @@ impl HTMLImageElement { /// Step 2-12 of fn react_to_environment_changes_sync_steps(&self, generation: u32, can_gc: CanGc) { - // TODO reduce duplicacy of this code - - fn generate_cache_listener_for_element( - elem: &HTMLImageElement, - selected_source: String, - selected_pixel_density: f64, - ) -> IpcSender { - let trusted_node = Trusted::new(elem); - let (responder_sender, responder_receiver) = ipc::channel().unwrap(); - let task_source = elem - .owner_global() - .task_manager() - .networking_task_source() - .to_sendable(); - let generation = elem.generation.get(); - - ROUTER.add_typed_route( - responder_receiver, - Box::new(move |message| { - debug!("Got image {:?}", message); - // Return the image via a message to the script thread, which marks - // the element as dirty and triggers a reflow. - let element = trusted_node.clone(); - let image: PendingImageResponse = message.unwrap(); - let selected_source_clone = selected_source.clone(); - task_source.queue( - task!(process_image_response_for_environment_change: move || { - let element = element.root(); - // Ignore any image response for a previous request that has been discarded. - if generation == element.generation.get() { - element.process_image_response_for_environment_change(image.response, - USVString::from(selected_source_clone), generation, - selected_pixel_density, CanGc::note()); - } - }) - ); - }), - ); - - responder_sender - } - let elem = self.upcast::(); let document = elem.owner_document(); let has_pending_request = matches!(self.image_request.get(), ImageRequestPhase::Pending); @@ -1143,23 +1143,20 @@ impl HTMLImageElement { can_gc, ); - let window = self.owner_window(); - let image_cache = window.image_cache(); - // Step 14 - let sender = generate_cache_listener_for_element( - self, - selected_source.0.clone(), - selected_pixel_density, - ); - let cache_result = image_cache.track_image( + let window = self.owner_window(); + let cache_result = window.image_cache().get_cached_image_status( img_url.clone(), window.origin().immutable().clone(), cors_setting_for_element(self.upcast()), - sender, UsePlaceholder::No, ); + let change_type = ChangeType::Environment { + selected_source: selected_source.clone(), + selected_pixel_density, + }; + match cache_result { ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable { .. }) => { // Step 15 @@ -1169,7 +1166,7 @@ impl HTMLImageElement { selected_pixel_density, ) }, - ImageCacheResult::Available(ImageOrMetadataAvailable::MetadataAvailable(m)) => { + ImageCacheResult::Available(ImageOrMetadataAvailable::MetadataAvailable(m, id)) => { self.process_image_response_for_environment_change( ImageResponse::MetadataLoaded(m), selected_source, @@ -1177,6 +1174,7 @@ impl HTMLImageElement { selected_pixel_density, can_gc, ); + self.register_image_cache_callback(id, change_type); }, ImageCacheResult::LoadError => { self.process_image_response_for_environment_change( @@ -1187,8 +1185,13 @@ impl HTMLImageElement { can_gc, ); }, - ImageCacheResult::ReadyForRequest(id) => self.fetch_request(&img_url, id), - ImageCacheResult::Pending(_) => (), + ImageCacheResult::ReadyForRequest(id) => { + self.fetch_request(&img_url, id); + self.register_image_cache_callback(id, change_type); + }, + ImageCacheResult::Pending(id) => { + self.register_image_cache_callback(id, change_type); + }, } } @@ -1383,6 +1386,10 @@ impl HTMLImageElement { .as_ref() .is_some_and(|url| url.scheme() == "data" || url.origin().same_origin(origin)) } + + fn generation_id(&self) -> u32 { + self.generation.get() + } } #[derive(JSTraceable, MallocSizeOf)] @@ -1882,16 +1889,6 @@ impl FormControl for HTMLImageElement { } } -impl ImageCacheListener for HTMLImageElement { - fn generation_id(&self) -> u32 { - self.generation.get() - } - - fn process_image_response(&self, response: ImageResponse, can_gc: CanGc) { - self.process_image_response(response, can_gc); - } -} - fn image_dimension_setter(element: &Element, attr: LocalName, value: u32, can_gc: CanGc) { // This setter is a bit weird: the IDL type is unsigned long, but it's parsed as // a dimension for rendering. @@ -2186,3 +2183,12 @@ pub fn parse_a_srcset_attribute(input: &str) -> Vec { } candidates } + +#[derive(Clone)] +enum ChangeType { + Environment { + selected_source: USVString, + selected_pixel_density: f64, + }, + Element, +} diff --git a/components/script/dom/htmlvideoelement.rs b/components/script/dom/htmlvideoelement.rs index 2147580928b..974b50a3949 100644 --- a/components/script/dom/htmlvideoelement.rs +++ b/components/script/dom/htmlvideoelement.rs @@ -11,8 +11,8 @@ use html5ever::{local_name, namespace_url, ns, LocalName, Prefix}; use ipc_channel::ipc; use js::rust::HandleObject; use net_traits::image_cache::{ - ImageCache, ImageCacheResult, ImageOrMetadataAvailable, ImageResponse, PendingImageId, - UsePlaceholder, + ImageCache, ImageCacheResult, ImageOrMetadataAvailable, ImageResponder, ImageResponse, + PendingImageId, UsePlaceholder, }; use net_traits::request::{CredentialsMode, Destination, RequestBuilder, RequestId}; use net_traits::{ @@ -41,7 +41,6 @@ use crate::dom::node::{Node, NodeTraits}; use crate::dom::performanceresourcetiming::InitiatorType; use crate::dom::virtualmethods::VirtualMethods; use crate::fetch::FetchCanceller; -use crate::image_listener::{generate_cache_listener_for_element, ImageCacheListener}; use crate::network_listener::{self, PreInvoke, ResourceTimingListener}; use crate::script_runtime::CanGc; @@ -177,28 +176,47 @@ impl HTMLVideoElement { // network activity as possible. let window = self.owner_window(); let image_cache = window.image_cache(); - let sender = generate_cache_listener_for_element(self); - let cache_result = image_cache.track_image( + let cache_result = image_cache.get_cached_image_status( poster_url.clone(), window.origin().immutable().clone(), None, - sender, UsePlaceholder::No, ); - match cache_result { + let id = match cache_result { ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable { image, url, .. }) => { self.process_image_response(ImageResponse::Loaded(image, url), can_gc); + return; }, + ImageCacheResult::Available(ImageOrMetadataAvailable::MetadataAvailable(_, id)) => id, ImageCacheResult::ReadyForRequest(id) => { self.do_fetch_poster_frame(poster_url, id, can_gc); + id }, - _ => (), - } + ImageCacheResult::LoadError => { + self.process_image_response(ImageResponse::None, can_gc); + return; + }, + ImageCacheResult::Pending(id) => id, + }; + + let trusted_node = Trusted::new(self); + let generation = self.generation_id(); + let sender = window.register_image_cache_listener(id, move |response| { + let element = trusted_node.root(); + + // Ignore any image response for a previous request that has been discarded. + if generation != element.generation_id() { + return; + } + element.process_image_response(response.response, CanGc::note()); + }); + + image_cache.add_listener(ImageResponder::new(sender, window.pipeline_id(), id)); } /// @@ -228,6 +246,26 @@ impl HTMLVideoElement { let context = PosterFrameFetchContext::new(self, poster_url, id, request.id); self.owner_document().fetch_background(request, context); } + + fn generation_id(&self) -> u32 { + self.generation_id.get() + } + + fn process_image_response(&self, response: ImageResponse, can_gc: CanGc) { + match response { + ImageResponse::Loaded(image, url) => { + debug!("Loaded poster image for video element: {:?}", url); + self.htmlmediaelement.process_poster_image_loaded(image); + LoadBlocker::terminate(&self.load_blocker, can_gc); + }, + ImageResponse::MetadataLoaded(..) => {}, + // The image cache may have loaded a placeholder for an invalid poster url + ImageResponse::PlaceholderLoaded(..) | ImageResponse::None => { + // A failed load should unblock the document load. + LoadBlocker::terminate(&self.load_blocker, can_gc); + }, + } + } } impl HTMLVideoElementMethods for HTMLVideoElement { @@ -289,28 +327,6 @@ impl VirtualMethods for HTMLVideoElement { } } -impl ImageCacheListener for HTMLVideoElement { - fn generation_id(&self) -> u32 { - self.generation_id.get() - } - - fn process_image_response(&self, response: ImageResponse, can_gc: CanGc) { - match response { - ImageResponse::Loaded(image, url) => { - debug!("Loaded poster image for video element: {:?}", url); - self.htmlmediaelement.process_poster_image_loaded(image); - LoadBlocker::terminate(&self.load_blocker, can_gc); - }, - ImageResponse::MetadataLoaded(..) => {}, - // The image cache may have loaded a placeholder for an invalid poster url - ImageResponse::PlaceholderLoaded(..) | ImageResponse::None => { - // A failed load should unblock the document load. - LoadBlocker::terminate(&self.load_blocker, can_gc); - }, - } - } -} - struct PosterFrameFetchContext { /// Reference to the script thread image cache. image_cache: Arc, diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 41fd03f3996..f8725e3fb4a 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -30,7 +30,6 @@ use euclid::default::{Point2D as UntypedPoint2D, Rect as UntypedRect}; use euclid::{Point2D, Rect, Scale, Size2D, Vector2D}; use fonts::FontContext; use ipc_channel::ipc::{self, IpcSender}; -use ipc_channel::router::ROUTER; use js::conversions::ToJSValConvertible; use js::glue::DumpJSStack; use js::jsapi::{ @@ -148,9 +147,7 @@ use crate::dom::windowproxy::{WindowProxy, WindowProxyHandler}; use crate::dom::worklet::Worklet; use crate::dom::workletglobalscope::WorkletGlobalScopeType; use crate::layout_image::fetch_image_for_layout; -use crate::messaging::{ - ImageCacheMsg, MainThreadScriptMsg, ScriptEventLoopReceiver, ScriptEventLoopSender, -}; +use crate::messaging::{MainThreadScriptMsg, ScriptEventLoopReceiver, ScriptEventLoopSender}; use crate::microtask::MicrotaskQueue; use crate::realms::{enter_realm, InRealm}; use crate::script_runtime::{CanGc, JSContext, Runtime}; @@ -160,6 +157,16 @@ use crate::unminify::unminified_path; use crate::webdriver_handlers::jsval_to_webdriver; use crate::{fetch, window_named_properties}; +/// A callback to call when a response comes back from the `ImageCache`. +/// +/// This is wrapped in a struct so that we can implement `MallocSizeOf` +/// for this type. +#[derive(MallocSizeOf)] +pub struct PendingImageCallback( + #[ignore_malloc_size_of = "dyn Fn is currently impossible to measure"] + Box, +); + /// Current state of the window object #[derive(Clone, Copy, Debug, JSTraceable, MallocSizeOf, PartialEq)] enum WindowState { @@ -216,7 +223,7 @@ pub(crate) struct Window { #[no_trace] image_cache: Arc, #[no_trace] - image_cache_chan: Sender, + image_cache_sender: IpcSender, window_proxy: MutNullableDom, document: MutNullableDom, location: MutNullableDom, @@ -234,7 +241,6 @@ pub(crate) struct Window { /// no devtools server #[no_trace] devtools_markers: DomRefCell>, - #[ignore_malloc_size_of = "channels are hard"] #[no_trace] devtools_marker_sender: DomRefCell>>>, @@ -262,7 +268,6 @@ pub(crate) struct Window { window_size: Cell, /// A handle for communicating messages to the bluetooth thread. - #[ignore_malloc_size_of = "channels are hard"] #[no_trace] bluetooth_thread: IpcSender, @@ -280,7 +285,6 @@ pub(crate) struct Window { layout_blocker: Cell, /// A channel for communicating results of async scripts back to the webdriver server - #[ignore_malloc_size_of = "channels are hard"] #[no_trace] webdriver_script_chan: DomRefCell>>, @@ -311,6 +315,12 @@ pub(crate) struct Window { #[cfg(feature = "webxr")] webxr_registry: Option, + /// When an element triggers an image load or starts watching an image load from the + /// `ImageCache` it adds an entry to this list. When those loads are triggered from + /// layout, they also add an etry to [`Self::pending_layout_images`]. + #[no_trace] + pending_image_callbacks: DomRefCell>>, + /// All of the elements that have an outstanding image request that was /// initiated by layout during a reflow. They are stored in the script thread /// to ensure that the element can be marked dirty when the image data becomes @@ -532,10 +542,20 @@ impl Window { Worklet::new(self, WorkletGlobalScopeType::Paint) } - pub(crate) fn pending_image_notification(&self, response: PendingImageResponse) { - //XXXjdm could be more efficient to send the responses to layout, - // rather than making layout talk to the image cache to - // obtain the same data. + pub(crate) fn register_image_cache_listener( + &self, + id: PendingImageId, + callback: impl Fn(PendingImageResponse) + 'static, + ) -> IpcSender { + self.pending_image_callbacks + .borrow_mut() + .entry(id) + .or_default() + .push(PendingImageCallback(Box::new(callback))); + self.image_cache_sender.clone() + } + + fn pending_layout_image_notification(&self, response: PendingImageResponse) { let mut images = self.pending_layout_images.borrow_mut(); let nodes = images.entry(response.id); let nodes = match nodes { @@ -555,6 +575,33 @@ impl Window { } } + pub(crate) fn pending_image_notification(&self, response: PendingImageResponse) { + // We take the images here, in order to prevent maintaining a mutable borrow when + // image callbacks are called. These, in turn, can trigger garbage collection. + // Normally this shouldn't trigger more pending image notifications, but just in + // case we do not want to cause a double borrow here. + let mut images = std::mem::take(&mut *self.pending_image_callbacks.borrow_mut()); + let Entry::Occupied(callbacks) = images.entry(response.id) else { + let _ = std::mem::replace(&mut *self.pending_image_callbacks.borrow_mut(), images); + return; + }; + + for callback in callbacks.get() { + callback.0(response.clone()); + } + + match response.response { + ImageResponse::MetadataLoaded(_) => {}, + ImageResponse::Loaded(_, _) | + ImageResponse::PlaceholderLoaded(_, _) | + ImageResponse::None => { + callbacks.remove(); + }, + } + + let _ = std::mem::replace(&mut *self.pending_image_callbacks.borrow_mut(), images); + } + pub(crate) fn compositor_api(&self) -> &CrossProcessCompositorApi { &self.compositor_api } @@ -1924,17 +1971,16 @@ impl Window { let mut images = self.pending_layout_images.borrow_mut(); let nodes = images.entry(id).or_default(); if !nodes.iter().any(|n| std::ptr::eq(&**n, &*node)) { - let (responder, responder_listener) = - ProfiledIpc::channel(self.global().time_profiler_chan().clone()).unwrap(); - let image_cache_chan = self.image_cache_chan.clone(); - ROUTER.add_typed_route( - responder_listener.to_ipc_receiver(), - Box::new(move |message| { - let _ = image_cache_chan.send((pipeline_id, message.unwrap())); - }), - ); + let trusted_node = Trusted::new(&*node); + let sender = self.register_image_cache_listener(id, move |response| { + trusted_node + .root() + .owner_window() + .pending_layout_image_notification(response); + }); + self.image_cache - .add_listener(id, ImageResponder::new(responder, id)); + .add_listener(ImageResponder::new(sender, self.pipeline_id(), id)); nodes.push(Dom::from_ref(&*node)); } } @@ -2696,7 +2742,7 @@ impl Window { script_chan: Sender, layout: Box, font_context: Arc, - image_cache_chan: Sender, + image_cache_sender: IpcSender, image_cache: Arc, resource_threads: ResourceThreads, bluetooth_thread: IpcSender, @@ -2760,7 +2806,7 @@ impl Window { script_chan, layout: RefCell::new(layout), font_context, - image_cache_chan, + image_cache_sender, image_cache, navigator: Default::default(), location: Default::default(), @@ -2795,6 +2841,7 @@ impl Window { webgl_chan, #[cfg(feature = "webxr")] webxr_registry, + pending_image_callbacks: Default::default(), pending_layout_images: Default::default(), unminified_css_dir: Default::default(), local_script_source, diff --git a/components/script/image_listener.rs b/components/script/image_listener.rs deleted file mode 100644 index 84b09048466..00000000000 --- a/components/script/image_listener.rs +++ /dev/null @@ -1,53 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ - -use ipc_channel::ipc; -use ipc_channel::ipc::IpcSender; -use ipc_channel::router::ROUTER; -use net_traits::image_cache::{ImageResponse, PendingImageResponse}; - -use crate::dom::bindings::conversions::DerivedFrom; -use crate::dom::bindings::refcounted::Trusted; -use crate::dom::bindings::reflector::DomObject; -use crate::dom::node::{Node, NodeTraits}; -use crate::script_runtime::CanGc; - -pub(crate) trait ImageCacheListener { - fn generation_id(&self) -> u32; - fn process_image_response(&self, response: ImageResponse, can_gc: CanGc); -} - -pub(crate) fn generate_cache_listener_for_element< - T: ImageCacheListener + DerivedFrom + DomObject, ->( - elem: &T, -) -> IpcSender { - let trusted_node = Trusted::new(elem); - let (responder_sender, responder_receiver) = ipc::channel().unwrap(); - - let task_source = elem - .owner_global() - .task_manager() - .networking_task_source() - .to_sendable(); - let generation = elem.generation_id(); - - ROUTER.add_typed_route( - responder_receiver, - Box::new(move |message| { - let element = trusted_node.clone(); - let image: PendingImageResponse = message.unwrap(); - debug!("Got image {:?}", image); - task_source.queue(task!(process_image_response: move || { - let element = element.root(); - // Ignore any image response for a previous request that has been discarded. - if generation == element.generation_id() { - element.process_image_response(image.response, CanGc::note()); - } - })); - }), - ); - - responder_sender -} diff --git a/components/script/lib.rs b/components/script/lib.rs index c005f56c168..0c7afcce98a 100644 --- a/components/script/lib.rs +++ b/components/script/lib.rs @@ -37,7 +37,6 @@ pub(crate) mod document_loader; mod dom; mod canvas_state; pub(crate) mod fetch; -mod image_listener; mod init; mod layout_image; diff --git a/components/script/messaging.rs b/components/script/messaging.rs index 65429622b69..c6f0ccd745c 100644 --- a/components/script/messaging.rs +++ b/components/script/messaging.rs @@ -32,14 +32,12 @@ use crate::task::TaskBox; use crate::task_queue::{QueuedTask, QueuedTaskConversion, TaskQueue}; use crate::task_source::TaskSourceName; -pub(crate) type ImageCacheMsg = (PipelineId, PendingImageResponse); - #[derive(Debug)] pub(crate) enum MixedMessage { FromConstellation(ConstellationControlMsg), FromScript(MainThreadScriptMsg), FromDevtools(DevtoolScriptControlMsg), - FromImageCache((PipelineId, PendingImageResponse)), + FromImageCache(PendingImageResponse), #[cfg(feature = "webgpu")] FromWebGPUServer(WebGPUMsg), TimerFired, @@ -101,7 +99,7 @@ impl MixedMessage { MainThreadScriptMsg::Inactive => None, MainThreadScriptMsg::WakeUp => None, }, - MixedMessage::FromImageCache((pipeline_id, _)) => Some(*pipeline_id), + MixedMessage::FromImageCache(response) => Some(response.pipeline_id), MixedMessage::FromDevtools(_) | MixedMessage::TimerFired => None, #[cfg(feature = "webgpu")] MixedMessage::FromWebGPUServer(..) => None, @@ -321,9 +319,11 @@ pub(crate) struct ScriptThreadSenders { #[no_trace] pub(crate) layout_to_constellation_ipc_sender: IpcSender, - /// The [`Sender`] on which messages can be sent to the `ImageCache`. + /// The shared [`IpcSender`] which is sent to the `ImageCache` when requesting an image. The + /// messages on this channel are routed to crossbeam [`Sender`] on the router thread, which + /// in turn sends messages to [`ScriptThreadReceivers::image_cache_receiver`]. #[no_trace] - pub(crate) image_cache_sender: Sender, + pub(crate) image_cache_sender: IpcSender, /// For providing contact with the time profiler. #[no_trace] @@ -352,7 +352,7 @@ pub(crate) struct ScriptThreadReceivers { /// The [`Receiver`] which receives incoming messages from the `ImageCache`. #[no_trace] - pub(crate) image_cache_receiver: Receiver, + pub(crate) image_cache_receiver: Receiver, /// For receiving commands from an optional devtools server. Will be ignored if no such server /// exists. When devtools are not active this will be [`crossbeam_channel::never()`]. diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 5224332f330..13558b81832 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -847,7 +847,6 @@ impl ScriptThread { .map(|_| ROUTER.route_ipc_receiver_to_new_crossbeam_receiver(ipc_devtools_receiver)) .unwrap_or_else(crossbeam_channel::never); - let (image_cache_sender, image_cache_receiver) = unbounded(); let task_queue = TaskQueue::new(self_receiver, self_sender.clone()); let closing = Arc::new(AtomicBool::new(false)); @@ -863,6 +862,15 @@ impl ScriptThread { Some(Box::new(background_hang_monitor_exit_signal)), ); + let (image_cache_sender, image_cache_receiver) = unbounded(); + let (ipc_image_cache_sender, ipc_image_cache_receiver) = ipc::channel().unwrap(); + ROUTER.add_typed_route( + ipc_image_cache_receiver, + Box::new(move |message| { + let _ = image_cache_sender.send(message.unwrap()); + }), + ); + let receivers = ScriptThreadReceivers { constellation_receiver, image_cache_receiver, @@ -878,7 +886,7 @@ impl ScriptThread { constellation_sender: state.constellation_sender, pipeline_to_constellation_sender: state.pipeline_to_constellation_sender.sender.clone(), layout_to_constellation_ipc_sender: state.layout_to_constellation_ipc_sender, - image_cache_sender, + image_cache_sender: ipc_image_cache_sender, time_profiler_sender: state.time_profiler_sender, memory_profiler_sender: state.memory_profiler_sender, devtools_server_sender, @@ -2078,8 +2086,8 @@ impl ScriptThread { } } - fn handle_msg_from_image_cache(&self, (id, response): (PipelineId, PendingImageResponse)) { - let window = self.documents.borrow().find_window(id); + fn handle_msg_from_image_cache(&self, response: PendingImageResponse) { + let window = self.documents.borrow().find_window(response.pipeline_id); if let Some(ref window) = window { window.pending_image_notification(response); } diff --git a/components/shared/net/image_cache.rs b/components/shared/net/image_cache.rs index cbe1e3503c8..c98b52c20d4 100644 --- a/components/shared/net/image_cache.rs +++ b/components/shared/net/image_cache.rs @@ -4,6 +4,7 @@ use std::sync::Arc; +use base::id::PipelineId; use ipc_channel::ipc::IpcSender; use log::debug; use malloc_size_of_derive::MallocSizeOf; @@ -28,7 +29,7 @@ pub enum ImageOrMetadataAvailable { url: ServoUrl, is_placeholder: bool, }, - MetadataAvailable(ImageMetadata), + MetadataAvailable(ImageMetadata, PendingImageId), } /// This is optionally passed to the image cache when requesting @@ -37,13 +38,22 @@ pub enum ImageOrMetadataAvailable { /// and/or repaint. #[derive(Clone, Debug, Deserialize, Serialize)] pub struct ImageResponder { - id: PendingImageId, + pipeline_id: PipelineId, + pub id: PendingImageId, sender: IpcSender, } impl ImageResponder { - pub fn new(sender: IpcSender, id: PendingImageId) -> ImageResponder { - ImageResponder { sender, id } + pub fn new( + sender: IpcSender, + pipeline_id: PipelineId, + id: PendingImageId, + ) -> ImageResponder { + ImageResponder { + pipeline_id, + sender, + id, + } } pub fn respond(&self, response: ImageResponse) { @@ -52,6 +62,7 @@ impl ImageResponder { // That's not a case that's worth warning about. // TODO(#15501): are there cases in which we should perform cleanup? let _ = self.sender.send(PendingImageResponse { + pipeline_id: self.pipeline_id, response, id: self.id, }); @@ -75,8 +86,9 @@ pub enum ImageResponse { #[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, MallocSizeOf, PartialEq, Serialize)] pub struct PendingImageId(pub u64); -#[derive(Debug, Deserialize, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] pub struct PendingImageResponse { + pub pipeline_id: PipelineId, pub response: ImageResponse, pub id: PendingImageId, } @@ -119,24 +131,9 @@ pub trait ImageCache: Sync + Send { use_placeholder: UsePlaceholder, ) -> ImageCacheResult; - /// Add a listener for the provided pending image id, eventually called by - /// ImageCacheStore::complete_load. - /// If only metadata is available, Available(ImageOrMetadataAvailable) will - /// be returned. - /// If Available(ImageOrMetadataAvailable::Image) or LoadError is the final value, - /// the provided listener will be dropped (consumed & not added to PendingLoad). - fn track_image( - &self, - url: ServoUrl, - origin: ImmutableOrigin, - cors_setting: Option, - sender: IpcSender, - use_placeholder: UsePlaceholder, - ) -> ImageCacheResult; - /// Add a new listener for the given pending image id. If the image is already present, /// the responder will still receive the expected response. - fn add_listener(&self, id: PendingImageId, listener: ImageResponder); + fn add_listener(&self, listener: ImageResponder); /// Inform the image cache about a response for a pending request. fn notify_pending_response(&self, id: PendingImageId, action: FetchResponseMsg);