From bdbfde9ec0b5e80af0f63777b4cf2b31f5209f92 Mon Sep 17 00:00:00 2001 From: Bastien Orivel Date: Fri, 8 May 2020 15:36:55 +0200 Subject: [PATCH] Don't send a load event when a loaded image is actually the placeholder The image cache returns an `ImageCacheResult::ImageAvailable `the second time you try getting the placeholder. This means that in some cases, the loading of an image would fail, then the same image would get fetched from the cache, the placeholder would be loaded from that but would be seen as a normal image, firing a load event. This made the tests in `fetch/cross-origin-resource-policy/image-loads.html` fail depending on their order. --- components/layout/context.rs | 2 +- components/layout/fragment.rs | 4 ++-- components/net/image_cache.rs | 20 ++++++++++++++------ components/net_traits/image_cache.rs | 7 ++++++- components/script/dom/htmlimageelement.rs | 14 +++++++++++--- components/script/dom/htmlvideoelement.rs | 8 ++++++-- 6 files changed, 40 insertions(+), 15 deletions(-) diff --git a/components/layout/context.rs b/components/layout/context.rs index 536d108ea04..00e0c5a0cc1 100644 --- a/components/layout/context.rs +++ b/components/layout/context.rs @@ -163,7 +163,7 @@ impl<'a> LayoutContext<'a> { } match self.get_or_request_image_or_meta(node, url.clone(), use_placeholder) { - Some(ImageOrMetadataAvailable::ImageAvailable(image, _)) => { + Some(ImageOrMetadataAvailable::ImageAvailable { image, .. }) => { let image_info = WebRenderImageInfo::from_image(&*image); if image_info.key.is_none() { Some(image_info) diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs index 289dc3f3094..062db9c0a7e 100644 --- a/components/layout/fragment.rs +++ b/components/layout/fragment.rs @@ -431,8 +431,8 @@ impl ImageFragmentInfo { layout_context .get_or_request_image_or_meta(node.opaque(), url, UsePlaceholder::Yes) .map(|result| match result { - ImageOrMetadataAvailable::ImageAvailable(i, _) => { - ImageOrMetadata::Image(i) + ImageOrMetadataAvailable::ImageAvailable { image, .. } => { + ImageOrMetadata::Image(image) }, ImageOrMetadataAvailable::MetadataAvailable(m) => { ImageOrMetadata::Metadata(m) diff --git a/components/net/image_cache.rs b/components/net/image_cache.rs index d32061da06a..8a5e6a53749 100644 --- a/components/net/image_cache.rs +++ b/components/net/image_cache.rs @@ -466,9 +466,12 @@ impl ImageCache for ImageCacheImpl { match result { Ok((image, image_url)) => { debug!("{} is available", url); - return ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable( - image, image_url, - )); + let is_placeholder = image_url == store.placeholder_url; + return ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable { + image, + url: image_url, + is_placeholder, + }); }, Err(()) => { debug!("{} is not available", url); @@ -515,9 +518,14 @@ impl ImageCache for ImageCacheImpl { // TODO: make this behaviour configurable according to the caller's needs. store.handle_decoder(decoded); match store.get_completed_image_if_available(url, origin, cors_setting, use_placeholder) { - Some(Ok((image, image_url))) => ImageCacheResult::Available( - ImageOrMetadataAvailable::ImageAvailable(image, image_url), - ), + Some(Ok((image, image_url))) => { + let is_placeholder = image_url == store.placeholder_url; + ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable { + image, + url: image_url, + is_placeholder, + }) + }, _ => ImageCacheResult::LoadError, } } diff --git a/components/net_traits/image_cache.rs b/components/net_traits/image_cache.rs index 39d501a271f..e3c1a565881 100644 --- a/components/net_traits/image_cache.rs +++ b/components/net_traits/image_cache.rs @@ -17,7 +17,12 @@ use std::sync::Arc; /// Indicating either entire image or just metadata availability #[derive(Clone, Debug, Deserialize, MallocSizeOf, Serialize)] pub enum ImageOrMetadataAvailable { - ImageAvailable(#[ignore_malloc_size_of = "Arc"] Arc, ServoUrl), + ImageAvailable { + #[ignore_malloc_size_of = "Arc"] + image: Arc, + url: ServoUrl, + is_placeholder: bool, + }, MetadataAvailable(ImageMetadata), } diff --git a/components/script/dom/htmlimageelement.rs b/components/script/dom/htmlimageelement.rs index 8041fb5685e..2e99e491653 100644 --- a/components/script/dom/htmlimageelement.rs +++ b/components/script/dom/htmlimageelement.rs @@ -329,8 +329,16 @@ impl HTMLImageElement { ); match cache_result { - ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable(image, url)) => { - self.process_image_response(ImageResponse::Loaded(image, url)) + ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable { + image, + url, + is_placeholder, + }) => { + if is_placeholder { + self.process_image_response(ImageResponse::PlaceholderLoaded(image, url)) + } else { + self.process_image_response(ImageResponse::Loaded(image, url)) + } }, ImageCacheResult::Available(ImageOrMetadataAvailable::MetadataAvailable(m)) => { self.process_image_response(ImageResponse::MetadataLoaded(m)) @@ -1109,7 +1117,7 @@ impl HTMLImageElement { ); match cache_result { - ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable(_image, _url)) => { + ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable { .. }) => { // Step 15 self.finish_reacting_to_environment_change( selected_source, diff --git a/components/script/dom/htmlvideoelement.rs b/components/script/dom/htmlvideoelement.rs index c06dc083a22..cea37bc173c 100644 --- a/components/script/dom/htmlvideoelement.rs +++ b/components/script/dom/htmlvideoelement.rs @@ -166,8 +166,12 @@ impl HTMLVideoElement { ); match cache_result { - ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable(img, url)) => { - self.process_image_response(ImageResponse::Loaded(img, url)); + ImageCacheResult::Available(ImageOrMetadataAvailable::ImageAvailable { + image, + url, + .. + }) => { + self.process_image_response(ImageResponse::Loaded(image, url)); }, ImageCacheResult::ReadyForRequest(id) => { self.do_fetch_poster_frame(poster_url, id, cancel_receiver)