script: Properly root nodes with animating images (#37689)

This change fixes an issue and makes a few more minor improvements to
the `ImageAnimationState`:

1. Image rooting and unrooted now happens in one step from
   `Window::update_animations_post_reflow`.
2. The `node_to_animating_image_map` is now stored as a shared `RwLock`
   so that it doesn't need to be taken and then replaced in the
`ImageAnimationState` during reflow. This should prevent a hypothetical
issue
   where image animations are restarted during empty reflows.
3. General naming and idiomatic Rust usage improvements.

Testing: This doesn't really have any obvious behavioral changes,
because all
reflows currently trigger a restyle. It becomes a serious problem with
#37677
and this change fixes the failing test there.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>

Cargo.lock

@@ -1358,7 +1358,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "117725a109d387c937a1533ce01b450cbde6b88abceea8473c4d7a85853cda3c"
 dependencies = [
  "lazy_static",
- "windows-sys 0.48.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -2384,7 +2384,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "778e2ac28f6c47af28e4907f13ffd1e1ddbd400980a9abd7c8df189bf578a5ad"
 dependencies = [
  "libc",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -3003,7 +3003,7 @@ dependencies = [
  "gobject-sys",
  "libc",
  "system-deps",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -4507,7 +4507,7 @@ checksum = "e04d7f318608d35d4b61ddd75cbdaee86b023ebe2bd5a66ee0915f0bf93095a9"
 dependencies = [
  "hermit-abi 0.5.0",
  "libc",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -4735,6 +4735,7 @@ dependencies = [
  "libc",
  "malloc_size_of_derive",
  "net_traits",
+ "parking_lot",
  "pixels",
  "profile_traits",
  "range",
@@ -4803,7 +4804,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "07033963ba89ebaf1584d767badaa2e8fcec21aedea6b8c0346d487d49c28667"
 dependencies = [
  "cfg-if",
- "windows-targets 0.48.5",
+ "windows-targets 0.52.6",
 ]
 
 [[package]]
@@ -6898,7 +6899,7 @@ dependencies = [
  "errno",
  "libc",
  "linux-raw-sys 0.4.15",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -7078,6 +7079,7 @@ dependencies = [
  "nom",
  "num-traits",
  "num_cpus",
+ "parking_lot",
  "percent-encoding",
  "phf",
  "pixels",
@@ -8289,7 +8291,7 @@ dependencies = [
  "getrandom 0.2.16",
  "once_cell",
  "rustix 0.38.44",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -9732,7 +9734,7 @@ version = "0.1.9"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb"
 dependencies = [
- "windows-sys 0.48.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]

components/layout/context.rs

@@ -55,7 +55,10 @@ pub struct LayoutContext<'a> {
     pub resolved_images_cache:
         Arc<RwLock<FnvHashMap<(ServoUrl, UsePlaceholder), CachedImageOrError>>>,
 
-    pub node_image_animation_map: Arc<RwLock<FxHashMap<OpaqueNode, ImageAnimationState>>>,
+    /// A shared reference to script's map of DOM nodes with animated images. This is used
+    /// to manage image animations in script and inform the script about newly animating
+    /// nodes.
+    pub node_to_animating_image_map: Arc<RwLock<FxHashMap<OpaqueNode, ImageAnimationState>>>,
 
     /// The DOM node that is highlighted by the devtools inspector, if any
     pub highlighted_dom_node: Option<OpaqueNode>,
@@ -153,31 +156,24 @@ impl LayoutContext<'_> {
     }
 
     pub fn handle_animated_image(&self, node: OpaqueNode, image: Arc<RasterImage>) {
-        let mut store = self.node_image_animation_map.write();
+        let mut map = self.node_to_animating_image_map.write();
+        if !image.should_animate() {
+            map.remove(&node);
+            return;
+        }
+        let new_image_animation_state = || {
+            ImageAnimationState::new(
+                image.clone(),
+                self.shared_context().current_time_for_animations,
+            )
+        };
 
-        // 1. first check whether node previously being track for animated image.
+        let entry = map.entry(node).or_insert_with(new_image_animation_state);
-        if let Some(image_state) = store.get(&node) {
+
-            // a. if the node is not containing the same image as before.
+        // If the entry exists, but it is for a different image id, replace it as the image
-            if image_state.image_key() != image.id {
+        // has changed during this layout.
-                if image.should_animate() {
+        if entry.image.id != image.id {
-                    // i. Register/Replace tracking item in image_animation_manager.
+            *entry = new_image_animation_state();
-                    store.insert(
-                        node,
-                        ImageAnimationState::new(
-                            image,
-                            self.shared_context().current_time_for_animations,
-                        ),
-                    );
-                } else {
-                    // ii. Cancel Action if the node's image is no longer animated.
-                    store.remove(&node);
-                }
-            }
-        } else if image.should_animate() {
-            store.insert(
-                node,
-                ImageAnimationState::new(image, self.shared_context().current_time_for_animations),
-            );
         }
     }
 

components/layout/fragment_tree/fragment_tree.rs

@@ -58,7 +58,10 @@ impl FragmentTree {
         // them. Create a set of all elements that used to be animating.
         let mut animations = layout_context.style_context.animations.sets.write();
         let mut invalid_animating_nodes: FxHashSet<_> = animations.keys().cloned().collect();
-        let mut image_animations = layout_context.node_image_animation_map.write().to_owned();
+        let mut image_animations = layout_context
+            .node_to_animating_image_map
+            .write()
+            .to_owned();
         let mut invalid_image_animating_nodes: FxHashSet<_> = image_animations
             .keys()
             .cloned()

components/layout/layout_impl.rs

@@ -644,9 +644,7 @@ impl LayoutThread {
             resolved_images_cache: self.resolved_images_cache.clone(),
             pending_images: Mutex::default(),
             pending_rasterization_images: Mutex::default(),
-            node_image_animation_map: Arc::new(RwLock::new(std::mem::take(
+            node_to_animating_image_map: reflow_request.node_to_animating_image_map.clone(),
-                &mut reflow_request.node_to_image_animation_map,
-            ))),
             iframe_sizes: Mutex::default(),
             use_rayon: rayon_pool.is_some(),
             highlighted_dom_node: reflow_request.highlighted_dom_node,
@@ -687,15 +685,12 @@ impl LayoutThread {
         let pending_rasterization_images =
             std::mem::take(&mut *layout_context.pending_rasterization_images.lock());
         let iframe_sizes = std::mem::take(&mut *layout_context.iframe_sizes.lock());
-        let node_to_image_animation_map =
-            std::mem::take(&mut *layout_context.node_image_animation_map.write());
 
         Some(ReflowResult {
             built_display_list,
             pending_images,
             pending_rasterization_images,
             iframe_sizes,
-            node_to_image_animation_map,
         })
     }
 

components/script/Cargo.toml

@@ -95,6 +95,7 @@ net_traits = { workspace = true }
 nom = "7.1.3"
 num-traits = { workspace = true }
 num_cpus = { workspace = true }
+parking_lot = { workspace = true }
 percent-encoding = { workspace = true }
 phf = "0.11"
 pixels = { path = "../pixels" }

components/script/dom/document.rs

@@ -4189,7 +4189,7 @@ impl Document {
                 DomRefCell::new(AnimationTimeline::new())
             },
             animations: DomRefCell::new(Animations::new()),
-            image_animation_manager: DomRefCell::new(ImageAnimationManager::new()),
+            image_animation_manager: DomRefCell::new(ImageAnimationManager::default()),
             dirty_root: Default::default(),
             declarative_refresh: Default::default(),
             pending_input_events: Default::default(),
@@ -4960,6 +4960,7 @@ impl Document {
         self.animations
             .borrow()
             .do_post_reflow_update(&self.window, self.current_animation_timeline_value());
+        self.image_animation_manager().update_rooted_dom_nodes();
     }
 
     pub(crate) fn cancel_animations_for_node(&self, node: &Node) {
@@ -4998,12 +4999,9 @@ impl Document {
     pub(crate) fn image_animation_manager(&self) -> Ref<ImageAnimationManager> {
         self.image_animation_manager.borrow()
     }
-    pub(crate) fn image_animation_manager_mut(&self) -> RefMut<ImageAnimationManager> {
-        self.image_animation_manager.borrow_mut()
-    }
 
     pub(crate) fn update_animating_images(&self) {
-        let mut image_animation_manager = self.image_animation_manager.borrow_mut();
+        let image_animation_manager = self.image_animation_manager.borrow();
         if !image_animation_manager.image_animations_present() {
             return;
         }
@@ -5011,8 +5009,8 @@ impl Document {
             .update_active_frames(&self.window, self.current_animation_timeline_value());
 
         if !self.animations().animations_present() {
-            let next_scheduled_time =
+            let next_scheduled_time = image_animation_manager
-                image_animation_manager.next_schedule_time(self.current_animation_timeline_value());
+                .next_scheduled_time(self.current_animation_timeline_value());
             // TODO: Once we have refresh signal from the compositor,
             // we should get rid of timer for animated image update.
             if let Some(next_scheduled_time) = next_scheduled_time {

components/script/dom/window.rs

@@ -2218,9 +2218,7 @@ impl Window {
             pending_restyles,
             animation_timeline_value: document.current_animation_timeline_value(),
             animations: document.animations().sets.clone(),
-            node_to_image_animation_map: document
+            node_to_animating_image_map: document.image_animation_manager().node_to_image_map(),
-                .image_animation_manager_mut()
-                .take_image_animate_set(),
             theme: self.theme.get(),
             highlighted_dom_node,
         };
@@ -2297,9 +2295,6 @@ impl Window {
         if !size_messages.is_empty() {
             self.send_to_constellation(ScriptToConstellationMessage::IFrameSizes(size_messages));
         }
-        document
-            .image_animation_manager_mut()
-            .restore_image_animate_set(results.node_to_image_animation_map);
         document.update_animations_post_reflow();
         self.update_constellation_epoch();
 

components/script/image_animation.rs

@@ -2,12 +2,16 @@
  * 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 std::sync::Arc;
+
 use compositing_traits::{ImageUpdate, SerializableImageData};
 use embedder_traits::UntrustedNodeAddress;
-use fxhash::{FxHashMap, FxHashSet};
+use fxhash::FxHashMap;
 use ipc_channel::ipc::IpcSharedMemory;
 use layout_api::ImageAnimationState;
 use libc::c_void;
+use malloc_size_of::MallocSizeOf;
+use parking_lot::RwLock;
 use script_bindings::root::Dom;
 use style::dom::OpaqueNode;
 use webrender_api::units::DeviceIntSize;
@@ -18,97 +22,90 @@ use crate::dom::bindings::trace::NoTrace;
 use crate::dom::node::{Node, from_untrusted_node_address};
 use crate::dom::window::Window;
 
-#[derive(Clone, Debug, Default, JSTraceable, MallocSizeOf)]
+#[derive(Clone, Debug, Default, JSTraceable)]
 #[cfg_attr(crown, crown::unrooted_must_root_lint::must_root)]
 pub struct ImageAnimationManager {
     #[no_trace]
-    pub node_to_image_map: FxHashMap<OpaqueNode, ImageAnimationState>,
+    node_to_image_map: Arc<RwLock<FxHashMap<OpaqueNode, ImageAnimationState>>>,
 
     /// A list of nodes with in-progress image animations.
+    ///
+    /// TODO(mrobinson): This does not properly handle animating images that are in pseudo-elements.
     rooted_nodes: DomRefCell<FxHashMap<NoTrace<OpaqueNode>, Dom<Node>>>,
 }
 
+impl MallocSizeOf for ImageAnimationManager {
+    fn size_of(&self, ops: &mut malloc_size_of::MallocSizeOfOps) -> usize {
+        (*self.node_to_image_map.read()).size_of(ops) + self.rooted_nodes.size_of(ops)
+    }
+}
+
 impl ImageAnimationManager {
-    pub fn new() -> Self {
+    pub(crate) fn node_to_image_map(
-        ImageAnimationManager {
+        &self,
-            node_to_image_map: Default::default(),
+    ) -> Arc<RwLock<FxHashMap<OpaqueNode, ImageAnimationState>>> {
-            rooted_nodes: DomRefCell::new(FxHashMap::default()),
+        self.node_to_image_map.clone()
-        }
     }
 
-    pub fn take_image_animate_set(&mut self) -> FxHashMap<OpaqueNode, ImageAnimationState> {
+    pub(crate) fn next_scheduled_time(&self, now: f64) -> Option<f64> {
-        std::mem::take(&mut self.node_to_image_map)
-    }
-
-    pub fn restore_image_animate_set(&mut self, map: FxHashMap<OpaqueNode, ImageAnimationState>) {
-        let _ = std::mem::replace(&mut self.node_to_image_map, map);
-        self.root_newly_animating_dom_nodes();
-        self.unroot_unused_nodes();
-    }
-
-    pub fn next_schedule_time(&self, now: f64) -> Option<f64> {
         self.node_to_image_map
+            .read()
             .values()
             .map(|state| state.time_to_next_frame(now))
             .min_by(|a, b| a.partial_cmp(b).unwrap_or(std::cmp::Ordering::Equal))
     }
 
-    pub fn image_animations_present(&self) -> bool {
+    pub(crate) fn image_animations_present(&self) -> bool {
-        !self.node_to_image_map.is_empty()
+        !self.node_to_image_map.read().is_empty()
     }
 
-    pub fn update_active_frames(&mut self, window: &Window, now: f64) {
+    pub(crate) fn update_active_frames(&self, window: &Window, now: f64) {
         let rooted_nodes = self.rooted_nodes.borrow();
-        let updates: Vec<ImageUpdate> = self
+        let updates = self
             .node_to_image_map
+            .write()
             .iter_mut()
             .filter_map(|(node, state)| {
-                if state.update_frame_for_animation_timeline_value(now) {
+                if !state.update_frame_for_animation_timeline_value(now) {
-                    let image = &state.image;
+                    return None;
-                    let frame = image
-                        .frames()
-                        .nth(state.active_frame)
-                        .expect("active_frame should within range of frames");
-
-                    if let Some(node) = rooted_nodes.get(&NoTrace(*node)) {
-                        node.dirty(crate::dom::node::NodeDamage::Other);
-                    }
-                    Some(ImageUpdate::UpdateImage(
-                        image.id.unwrap(),
-                        ImageDescriptor {
-                            format: ImageFormat::BGRA8,
-                            size: DeviceIntSize::new(
-                                image.metadata.width as i32,
-                                image.metadata.height as i32,
-                            ),
-                            stride: None,
-                            offset: 0,
-                            flags: ImageDescriptorFlags::ALLOW_MIPMAPS,
-                        },
-                        SerializableImageData::Raw(IpcSharedMemory::from_bytes(frame.bytes)),
-                    ))
-                } else {
-                    None
                 }
+
+                let image = &state.image;
+                let frame = image
+                    .frames()
+                    .nth(state.active_frame)
+                    .expect("active_frame should within range of frames");
+
+                if let Some(node) = rooted_nodes.get(&NoTrace(*node)) {
+                    node.dirty(crate::dom::node::NodeDamage::Other);
+                }
+                Some(ImageUpdate::UpdateImage(
+                    image.id.unwrap(),
+                    ImageDescriptor {
+                        format: ImageFormat::BGRA8,
+                        size: DeviceIntSize::new(
+                            image.metadata.width as i32,
+                            image.metadata.height as i32,
+                        ),
+                        stride: None,
+                        offset: 0,
+                        flags: ImageDescriptorFlags::ALLOW_MIPMAPS,
+                    },
+                    SerializableImageData::Raw(IpcSharedMemory::from_bytes(frame.bytes)),
+                ))
             })
             .collect();
         window.compositor_api().update_images(updates);
     }
-
+    /// Ensure that all nodes with animating images are rooted and unroots any nodes that
-    // Unroot any nodes that we have rooted but no longer have animating images.
+    /// no longer have an animating image. This should be called immediately after a
-    fn unroot_unused_nodes(&self) {
+    /// restyle, to ensure that these addresses are still valid.
-        let nodes: FxHashSet<&OpaqueNode> = self.node_to_image_map.keys().collect();
-        self.rooted_nodes
-            .borrow_mut()
-            .retain(|node, _| nodes.contains(&node.0));
-    }
-
-    /// Ensure that all nodes with Image animations are rooted. This should be called
-    /// immediately after a restyle, to ensure that these addresses are still valid.
     #[allow(unsafe_code)]
-    fn root_newly_animating_dom_nodes(&self) {
+    pub(crate) fn update_rooted_dom_nodes(&self) {
         let mut rooted_nodes = self.rooted_nodes.borrow_mut();
-        for opaque_node in self.node_to_image_map.keys() {
+        let node_to_image_map = self.node_to_image_map.read();
+
+        for opaque_node in node_to_image_map.keys() {
             let opaque_node = *opaque_node;
             if rooted_nodes.contains_key(&NoTrace(opaque_node)) {
                 continue;
@@ -121,5 +118,7 @@ impl ImageAnimationManager {
                 )
             };
         }
+
+        rooted_nodes.retain(|node, _| node_to_image_map.contains_key(&node.0));
     }
 }

components/shared/layout/Cargo.toml

@@ -29,6 +29,7 @@ libc = { workspace = true }
 malloc_size_of = { workspace = true }
 malloc_size_of_derive = { workspace = true }
 net_traits = { workspace = true }
+parking_lot = { workspace = true }
 pixels = { path = "../../pixels" }
 profile_traits = { workspace = true }
 range = { path = "../../range" }

components/shared/layout/lib.rs

@@ -31,6 +31,7 @@ use libc::c_void;
 use malloc_size_of::{MallocSizeOf as MallocSizeOfTrait, MallocSizeOfOps};
 use malloc_size_of_derive::MallocSizeOf;
 use net_traits::image_cache::{ImageCache, PendingImageId};
+use parking_lot::RwLock;
 use pixels::RasterImage;
 use profile_traits::mem::Report;
 use profile_traits::time;
@@ -412,8 +413,6 @@ pub struct ReflowResult {
     /// to communicate them with the Constellation and also the `Window`
     /// element of their content pages.
     pub iframe_sizes: IFrameSizes,
-    /// The mapping of node to animated image, need to be returned to ImageAnimationManager
-    pub node_to_image_animation_map: FxHashMap<OpaqueNode, ImageAnimationState>,
 }
 
 /// Information needed for a script-initiated reflow.
@@ -440,7 +439,7 @@ pub struct ReflowRequest {
     /// The set of animations for this document.
     pub animations: DocumentAnimationSet,
     /// The set of image animations.
-    pub node_to_image_animation_map: FxHashMap<OpaqueNode, ImageAnimationState>,
+    pub node_to_animating_image_map: Arc<RwLock<FxHashMap<OpaqueNode, ImageAnimationState>>>,
     /// The theme for the window
     pub theme: Theme,
     /// The node highlighted by the devtools, if any