From 980eb5ac33b16d7310d0b5b20d8c3f94b4b2c241 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Thu, 29 Dec 2016 14:07:57 -0500 Subject: [PATCH] Avoid dropping image requests on the ground from non-script-initiated reflow. --- components/layout/context.rs | 39 +++++++++++++------ components/layout_thread/lib.rs | 29 +++++++------- components/net/image_cache_thread.rs | 42 +++++++++++++-------- components/net_traits/image_cache_thread.rs | 22 +++++++++-- components/script/dom/htmlcanvaselement.rs | 5 ++- components/script/dom/htmlimageelement.rs | 6 ++- 6 files changed, 96 insertions(+), 47 deletions(-) diff --git a/components/layout/context.rs b/components/layout/context.rs index 13029dd80bf..1dff9f6ea5e 100644 --- a/components/layout/context.rs +++ b/components/layout/context.rs @@ -9,7 +9,7 @@ use gfx::display_list::{WebRenderImageInfo, OpaqueNode}; use gfx::font_cache_thread::FontCacheThread; use gfx::font_context::FontContext; use heapsize::HeapSizeOf; -use net_traits::image_cache_thread::{ImageCacheThread, ImageState}; +use net_traits::image_cache_thread::{ImageCacheThread, ImageState, CanRequestImages}; use net_traits::image_cache_thread::{ImageOrMetadataAvailable, UsePlaceholder}; use opaque_node::OpaqueNodeMethods; use parking_lot::RwLock; @@ -92,13 +92,15 @@ pub struct LayoutContext { /// A list of in-progress image loads to be shared with the script thread. /// A None value means that this layout was not initiated by the script thread. - pub pending_images: Mutex> + pub pending_images: Option>> } impl Drop for LayoutContext { fn drop(&mut self) { if !thread::panicking() { - assert!(self.pending_images.lock().unwrap().is_empty()); + if let Some(ref pending_images) = self.pending_images { + assert!(pending_images.lock().unwrap().is_empty()); + } } } } @@ -114,10 +116,20 @@ impl LayoutContext { url: ServoUrl, use_placeholder: UsePlaceholder) -> Option { + //XXXjdm For cases where we do not request an image, we still need to + // ensure the node gets another script-initiated reflow or it + // won't be requested at all. + let can_request = if self.pending_images.is_some() { + CanRequestImages::Yes + } else { + CanRequestImages::No + }; + // See if the image is already available let result = self.image_cache_thread.lock().unwrap() .find_image_or_metadata(url.clone(), - use_placeholder); + use_placeholder, + can_request); match result { Ok(image_or_metadata) => Some(image_or_metadata), // Image failed to load, so just return nothing @@ -129,18 +141,23 @@ impl LayoutContext { node: node.to_untrusted_node_address(), id: id, }; - self.pending_images.lock().unwrap().push(image); + self.pending_images.as_ref().unwrap().lock().unwrap().push(image); None } // Image has been requested, is still pending. Return no image for this paint loop. // When the image loads it will trigger a reflow and/or repaint. Err(ImageState::Pending(id)) => { - let image = PendingImage { - state: PendingImageState::PendingResponse, - node: node.to_untrusted_node_address(), - id: id, - }; - self.pending_images.lock().unwrap().push(image); + //XXXjdm if self.pending_images is not available, we should make sure that + // this node gets marked dirty again so it gets a script-initiated + // reflow that deals with this properly. + if let Some(ref pending_images) = self.pending_images { + let image = PendingImage { + state: PendingImageState::PendingResponse, + node: node.to_untrusted_node_address(), + id: id, + }; + pending_images.lock().unwrap().push(image); + } None } } diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index af56555e399..8d6f2377130 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -495,7 +495,8 @@ impl LayoutThread { fn build_layout_context(&self, rw_data: &LayoutThreadData, screen_size_changed: bool, - goal: ReflowGoal) + goal: ReflowGoal, + request_images: bool) -> LayoutContext { let thread_local_style_context_creation_data = ThreadLocalStyleContextCreationInfo::new(self.new_animations_sender.clone()); @@ -521,7 +522,7 @@ impl LayoutThread { image_cache_thread: Mutex::new(self.image_cache_thread.clone()), font_cache_thread: Mutex::new(self.font_cache_thread.clone()), webrender_image_cache: self.webrender_image_cache.clone(), - pending_images: Mutex::new(vec![]), + pending_images: if request_images { Some(Mutex::new(vec![])) } else { None }, } } @@ -1115,7 +1116,8 @@ impl LayoutThread { // Create a layout context for use throughout the following passes. let mut layout_context = self.build_layout_context(&*rw_data, viewport_size_changed, - data.reflow_info.goal); + data.reflow_info.goal, + true); // NB: Type inference falls apart here for some reason, so we need to be very verbose. :-( let traversal_driver = if self.parallel_flag && self.parallel_traversal.is_some() { @@ -1196,8 +1198,11 @@ impl LayoutThread { query_type: &ReflowQueryType, rw_data: &mut LayoutThreadData, context: &mut LayoutContext) { - rw_data.pending_images = - std_mem::replace(&mut context.pending_images.lock().unwrap(), vec![]); + let pending_images = match context.pending_images { + Some(ref pending) => std_mem::replace(&mut *pending.lock().unwrap(), vec![]), + None => vec![], + }; + rw_data.pending_images = pending_images; let mut root_flow = match self.root_flow.clone() { Some(root_flow) => root_flow, @@ -1319,7 +1324,8 @@ impl LayoutThread { let mut layout_context = self.build_layout_context(&*rw_data, false, - reflow_info.goal); + reflow_info.goal, + false); if let Some(mut root_flow) = self.root_flow.clone() { // Perform an abbreviated style recalc that operates without access to the DOM. @@ -1340,13 +1346,7 @@ impl LayoutThread { &mut *rw_data, &mut layout_context); - let mut pending_images = layout_context.pending_images.lock().unwrap(); - if pending_images.len() > 0 { - //XXXjdm we drop all the images on the floor, but there's no guarantee that - // the node references are valid since the script thread isn't paused. - // need to figure out what to do here! - pending_images.truncate(0); - } + assert!(layout_context.pending_images.is_none()); } fn reflow_with_newly_loaded_web_font<'a, 'b>(&mut self, possibly_locked_rw_data: &mut RwData<'a, 'b>) { @@ -1360,7 +1360,8 @@ impl LayoutThread { let mut layout_context = self.build_layout_context(&*rw_data, false, - reflow_info.goal); + reflow_info.goal, + false); // No need to do a style recalc here. if self.root_flow.is_none() { diff --git a/components/net/image_cache_thread.rs b/components/net/image_cache_thread.rs index 17a852833c2..6c2da2c7c08 100644 --- a/components/net/image_cache_thread.rs +++ b/components/net/image_cache_thread.rs @@ -9,7 +9,7 @@ use net_traits::{NetworkError, FetchResponseMsg}; use net_traits::image::base::{Image, ImageMetadata, PixelFormat, load_from_memory}; use net_traits::image_cache_thread::{ImageCacheCommand, ImageCacheThread, ImageState}; use net_traits::image_cache_thread::{ImageOrMetadataAvailable, ImageResponse, UsePlaceholder}; -use net_traits::image_cache_thread::{ImageResponder, PendingImageId}; +use net_traits::image_cache_thread::{ImageResponder, PendingImageId, CanRequestImages}; use servo_config::resource_files::resources_dir_path; use servo_url::ServoUrl; use std::borrow::ToOwned; @@ -108,11 +108,12 @@ struct AllPendingLoads { keygen: LoadKeyGenerator, } -// Result of accessing a cache. -#[derive(Eq, PartialEq)] -enum CacheResult { - Hit, // The value was in the cache. - Miss, // The value was not in the cache and needed to be regenerated. +/// Result of accessing a cache. +enum CacheResult<'a> { + /// The value was in the cache. + Hit(LoadKey, &'a mut PendingLoad), + /// The value was not in the cache and needed to be regenerated. + Miss(Option<(LoadKey, &'a mut PendingLoad)>), } impl AllPendingLoads { @@ -143,13 +144,18 @@ impl AllPendingLoads { }) } - fn get_cached(&mut self, url: ServoUrl) -> (CacheResult, LoadKey, &mut PendingLoad) { + fn get_cached<'a>(&'a mut self, url: ServoUrl, can_request: CanRequestImages) + -> CacheResult<'a> { match self.url_to_load_key.entry(url.clone()) { Occupied(url_entry) => { let load_key = url_entry.get(); - (CacheResult::Hit, *load_key, self.loads.get_mut(load_key).unwrap()) + CacheResult::Hit(*load_key, self.loads.get_mut(load_key).unwrap()) } Vacant(url_entry) => { + if can_request == CanRequestImages::No { + return CacheResult::Miss(None); + } + let load_key = self.keygen.next(); url_entry.insert(load_key); @@ -158,7 +164,7 @@ impl AllPendingLoads { Occupied(_) => unreachable!(), Vacant(load_entry) => { let mut_load = load_entry.insert(pending_load); - (CacheResult::Miss, load_key, mut_load) + CacheResult::Miss(Some((load_key, mut_load))) } } } @@ -362,8 +368,12 @@ impl ImageCache { ImageCacheCommand::AddListener(id, responder) => { self.add_listener(id, responder); } - ImageCacheCommand::GetImageOrMetadataIfAvailable(url, use_placeholder, consumer) => { - let result = self.get_image_or_meta_if_available(url, use_placeholder); + ImageCacheCommand::GetImageOrMetadataIfAvailable(url, + use_placeholder, + can_request, + consumer) => { + let result = self.get_image_or_meta_if_available(url, use_placeholder, can_request); + // TODO(#15501): look for opportunities to clean up cache if this send fails. let _ = consumer.send(result); } ImageCacheCommand::StoreDecodeImage(id, image_vector) => { @@ -495,7 +505,8 @@ impl ImageCache { /// of the complete load is not fully decoded or is unavailable. fn get_image_or_meta_if_available(&mut self, url: ServoUrl, - placeholder: UsePlaceholder) + placeholder: UsePlaceholder, + can_request: CanRequestImages) -> Result { match self.completed_loads.get(&url) { Some(completed_load) => { @@ -512,15 +523,16 @@ impl ImageCache { } } None => { - let (result, key, pl) = self.pending_loads.get_cached(url); + let result = self.pending_loads.get_cached(url, can_request); match result { - CacheResult::Hit => match pl.metadata { + CacheResult::Hit(key, pl) => match pl.metadata { Some(ref meta) => Ok(ImageOrMetadataAvailable::MetadataAvailable(meta.clone())), None => Err(ImageState::Pending(key)), }, - CacheResult::Miss => Err(ImageState::NotRequested(key)), + CacheResult::Miss(Some((key, _pl))) => Err(ImageState::NotRequested(key)), + CacheResult::Miss(None) => Err(ImageState::LoadError), } } } diff --git a/components/net_traits/image_cache_thread.rs b/components/net_traits/image_cache_thread.rs index a000bd8b6bc..481a4f551ea 100644 --- a/components/net_traits/image_cache_thread.rs +++ b/components/net_traits/image_cache_thread.rs @@ -79,7 +79,10 @@ pub enum ImageOrMetadataAvailable { pub enum ImageCacheCommand { /// Synchronously check the state of an image in the cache. If the image is in a loading /// state and but its metadata has been made available, it will be sent as a response. - GetImageOrMetadataIfAvailable(ServoUrl, UsePlaceholder, IpcSender>), + GetImageOrMetadataIfAvailable(ServoUrl, + UsePlaceholder, + CanRequestImages, + IpcSender>), /// Add a new listener for the given pending image. AddListener(PendingImageId, ImageResponder), @@ -98,6 +101,15 @@ pub enum UsePlaceholder { Yes, } +/// Whether a consumer is in a position to request images or not. This can occur when +/// animations are being processed by the layout thread while the script thread is executing +/// in parallel. +#[derive(Copy, Clone, PartialEq, Deserialize, Serialize)] +pub enum CanRequestImages { + No, + Yes, +} + /// The client side of the image cache thread. This can be safely cloned /// and passed to different threads. #[derive(Clone, Deserialize, Serialize)] @@ -120,10 +132,14 @@ impl ImageCacheThread { /// FIXME: We shouldn't do IPC for data uris! pub fn find_image_or_metadata(&self, url: ServoUrl, - use_placeholder: UsePlaceholder) + use_placeholder: UsePlaceholder, + can_request: CanRequestImages) -> Result { let (sender, receiver) = ipc::channel().unwrap(); - let msg = ImageCacheCommand::GetImageOrMetadataIfAvailable(url, use_placeholder, sender); + let msg = ImageCacheCommand::GetImageOrMetadataIfAvailable(url, + use_placeholder, + can_request, + sender); let _ = self.chan.send(msg); try!(receiver.recv().map_err(|_| ImageState::LoadError)) } diff --git a/components/script/dom/htmlcanvaselement.rs b/components/script/dom/htmlcanvaselement.rs index 9f77f8b9ce1..a51463f4674 100644 --- a/components/script/dom/htmlcanvaselement.rs +++ b/components/script/dom/htmlcanvaselement.rs @@ -343,9 +343,10 @@ pub mod utils { pub fn request_image_from_cache(window: &Window, url: ServoUrl) -> ImageResponse { let image_cache = window.image_cache_thread(); - //XXXjdm add a image cache mode that doesn't store anything for NotRequested? let response = - image_cache.find_image_or_metadata(url.into(), UsePlaceholder::No); + image_cache.find_image_or_metadata(url.into(), + UsePlaceholder::No, + CanRequestImages::No); match response { Ok(ImageOrMetadataAvailable::ImageAvailable(image)) => ImageResponse::Loaded(image), diff --git a/components/script/dom/htmlimageelement.rs b/components/script/dom/htmlimageelement.rs index e762675bc70..f104330c741 100644 --- a/components/script/dom/htmlimageelement.rs +++ b/components/script/dom/htmlimageelement.rs @@ -39,7 +39,7 @@ use ipc_channel::router::ROUTER; use net_traits::{FetchResponseListener, FetchMetadata, Metadata, NetworkError}; use net_traits::image::base::{Image, ImageMetadata}; use net_traits::image_cache_thread::{ImageResponder, ImageResponse, PendingImageId, ImageState}; -use net_traits::image_cache_thread::{UsePlaceholder, ImageOrMetadataAvailable}; +use net_traits::image_cache_thread::{UsePlaceholder, ImageOrMetadataAvailable, CanRequestImages}; use net_traits::request::{RequestInit, Type as RequestType}; use network_listener::{NetworkListener, PreInvoke}; use num_traits::ToPrimitive; @@ -293,7 +293,9 @@ impl HTMLImageElement { let image_cache = window.image_cache_thread(); let response = - image_cache.find_image_or_metadata(img_url_cloned.into(), UsePlaceholder::Yes); + image_cache.find_image_or_metadata(img_url_cloned.into(), + UsePlaceholder::Yes, + CanRequestImages::Yes); match response { Ok(ImageOrMetadataAvailable::ImageAvailable(image)) => { let event = box ImageResponseHandlerRunnable::new(