Avoid dropping image requests on the ground from non-script-initiated reflow.

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<Vec<PendingImage>>
+    pub pending_images: Option<Mutex<Vec<PendingImage>>>
 }
 
 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<ImageOrMetadataAvailable> {
+        //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 {
+                //XXXjdm if self.pending_images is not available, we should make sure that
-                    state: PendingImageState::PendingResponse,
+                //       this node gets marked dirty again so it gets a script-initiated
-                    node: node.to_untrusted_node_address(),
+                //       reflow that deals with this properly.
-                    id: id,
+                if let Some(ref pending_images) = self.pending_images {
-                };
+                    let image = PendingImage {
-                self.pending_images.lock().unwrap().push(image);
+                        state: PendingImageState::PendingResponse,
+                        node: node.to_untrusted_node_address(),
+                        id: id,
+                    };
+                    pending_images.lock().unwrap().push(image);
+                }
                 None
             }
         }

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 =
+        let pending_images = match context.pending_images {
-            std_mem::replace(&mut context.pending_images.lock().unwrap(), vec![]);
+            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();
+        assert!(layout_context.pending_images.is_none());
-        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);
-        }
     }
 
     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() {

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.
+/// Result of accessing a cache.
-#[derive(Eq, PartialEq)]
+enum CacheResult<'a> {
-enum CacheResult {
+    /// The value was in the cache.
-    Hit,  // The value was in the cache.
+    Hit(LoadKey, &'a mut PendingLoad),
-    Miss, // The value was not in the cache and needed to be regenerated.
+    /// 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) => {
+            ImageCacheCommand::GetImageOrMetadataIfAvailable(url,
-                let result = self.get_image_or_meta_if_available(url, use_placeholder);
+                                                             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<ImageOrMetadataAvailable, ImageState> {
         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),
                 }
             }
         }

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<Result<ImageOrMetadataAvailable, ImageState>>),
+    GetImageOrMetadataIfAvailable(ServoUrl,
+                                  UsePlaceholder,
+                                  CanRequestImages,
+                                  IpcSender<Result<ImageOrMetadataAvailable, ImageState>>),
 
     /// 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<ImageOrMetadataAvailable, ImageState> {
         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))
     }

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),

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(