script: Remove note_rendering_opportunity and rendering_opportunity (#34575)

A rendering opportunity is now unconditionally triggered by handling IPC
messages in the `ScriptThread`, unless animations are running in which
case it's driven by the compositor. We can now remove calls to
`note_rendering_opportunity` and `rendering_opportunity`.

There is one tricky case, which is when a promise completion during a
microtask checkpoint dirties the page again. In this case we need to
trigger a new rendering opportunity, unless animations are running.  In
a followup change, when not driven by the compositor, rendering
opportunities will be driven by a timed task, meaning we can remove this
workaround.

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

components/script/dom/document.rs

@@ -2064,6 +2064,12 @@ impl Document {
         }
     }
 
+    /// Whether or not this `Document` has any active requestAnimationFrame callbacks
+    /// registered.
+    pub(crate) fn has_active_request_animation_frame_callbacks(&self) -> bool {
+        !self.animation_frame_list.borrow().is_empty()
+    }
+
     /// <https://html.spec.whatwg.org/multipage/#dom-window-requestanimationframe>
     pub(crate) fn request_animation_frame(&self, callback: AnimationFrameCallback) -> u32 {
         let ident = self.animation_frame_ident.get() + 1;

components/script/dom/node.rs

@@ -2129,8 +2129,6 @@ impl Node {
             MutationObserver::queue_a_mutation_record(parent, mutation);
         }
         node.owner_doc().remove_script_and_layout_blocker();
-
-        ScriptThread::note_rendering_opportunity(window_from_node(parent).pipeline_id());
     }
 
     /// <https://dom.spec.whatwg.org/#concept-node-replace-all>
@@ -2254,7 +2252,6 @@ impl Node {
             MutationObserver::queue_a_mutation_record(parent, mutation);
         }
         parent.owner_doc().remove_script_and_layout_blocker();
-        ScriptThread::note_rendering_opportunity(window_from_node(parent).pipeline_id());
     }
 
     /// <https://dom.spec.whatwg.org/#concept-node-clone>

components/script/dom/resizeobserver.rs

@@ -26,7 +26,6 @@ use crate::dom::resizeobserverentry::ResizeObserverEntry;
 use crate::dom::resizeobserversize::{ResizeObserverSize, ResizeObserverSizeImpl};
 use crate::dom::window::Window;
 use crate::script_runtime::CanGc;
-use crate::script_thread::ScriptThread;
 
 /// <https://drafts.csswg.org/resize-observer/#calculate-depth-for-node>
 #[derive(Debug, Default, PartialEq, PartialOrd)]
@@ -191,10 +190,6 @@ impl ResizeObserverMethods<crate::DomTypeHolder> for ResizeObserver {
         self.observation_targets
             .borrow_mut()
             .push((resize_observation, Dom::from_ref(target)));
-
-        // Note: noting a rendering opportunity here is necessary
-        // to make /resize-observer/iframe-same-origin.html PASS.
-        ScriptThread::note_rendering_opportunity(window_from_node(target).pipeline_id());
     }
 
     /// <https://drafts.csswg.org/resize-observer/#dom-resizeobserver-unobserve>

components/script/dom/window.rs

@@ -1809,6 +1809,10 @@ impl Window {
         ScriptThread::handle_tick_all_animations_for_testing(pipeline_id);
     }
 
+    pub(crate) fn reflows_suppressed(&self) -> bool {
+        self.suppress_reflow.get()
+    }
+
     /// Reflows the page unconditionally if possible and not suppressed. This method will wait for
     /// the layout to complete. If there is no window size yet, the page is presumed invisible and
     /// no reflow is performed. If reflow is suppressed, no reflow will be performed for ForDisplay

components/script/script_thread.rs

@@ -457,9 +457,6 @@ unsafe_no_jsmanaged_fields!(TaskQueue<MainThreadScriptMsg>);
 pub struct ScriptThread {
     /// <https://html.spec.whatwg.org/multipage/#last-render-opportunity-time>
     last_render_opportunity_time: DomRefCell<Option<Instant>>,
-    /// Used to batch rendering opportunities
-    #[no_trace]
-    update_the_rendering_task_queued_for_pipeline: DomRefCell<HashSet<PipelineId>>,
     /// The documents for pipelines managed by this thread
     documents: DomRefCell<DocumentCollection>,
     /// The window proxies known by this thread
@@ -806,12 +803,6 @@ impl ScriptThreadFactory for ScriptThread {
 }
 
 impl ScriptThread {
-    pub fn note_rendering_opportunity(pipeline_id: PipelineId) {
-        with_script_thread(|script_thread| {
-            script_thread.rendering_opportunity(pipeline_id);
-        })
-    }
-
     pub fn runtime_handle() -> ParentRuntime {
         with_optional_script_thread(|script_thread| {
             script_thread.unwrap().js_runtime.prepare_for_new_child()
@@ -1200,7 +1191,6 @@ impl ScriptThread {
         ScriptThread {
             documents: DomRefCell::new(DocumentCollection::default()),
             last_render_opportunity_time: Default::default(),
-            update_the_rendering_task_queued_for_pipeline: Default::default(),
             window_proxies: DomRefCell::new(HashMapTracedValues::new()),
             incomplete_loads: DomRefCell::new(vec![]),
             incomplete_parser_contexts: IncompleteParserContexts(RefCell::new(vec![])),
@@ -1509,9 +1499,7 @@ impl ScriptThread {
     /// Attempt to update the rendering and then do a microtask checkpoint if rendering was actually
     /// updated.
     pub(crate) fn update_the_rendering(&self, requested_by_compositor: bool, can_gc: CanGc) {
-        self.update_the_rendering_task_queued_for_pipeline
+        *self.last_render_opportunity_time.borrow_mut() = Some(Instant::now());
-            .borrow_mut()
-            .clear();
 
         if !self.can_continue_running_inner() {
             return;
@@ -1560,11 +1548,11 @@ impl ScriptThread {
 
         // Note: the spec reads: "for doc in docs" at each step
         // whereas this runs all steps per doc in docs.
-        for pipeline_id in documents_in_order {
+        for pipeline_id in documents_in_order.iter() {
             let document = self
                 .documents
                 .borrow()
-                .find_document(pipeline_id)
+                .find_document(*pipeline_id)
                 .expect("Got pipeline for Document not managed by this ScriptThread.");
 
             // TODO(#31581): The steps in the "Revealing the document" section need to be implemente
@@ -1573,7 +1561,7 @@ impl ScriptThread {
 
             // TODO: Should this be broken and to match the specification more closely? For instance see
             // https://html.spec.whatwg.org/multipage/#flush-autofocus-candidates.
-            self.process_pending_compositor_events(pipeline_id, can_gc);
+            self.process_pending_compositor_events(*pipeline_id, can_gc);
 
             // TODO(#31665): Implement the "run the scroll steps" from
             // https://drafts.csswg.org/cssom-view/#document-run-the-scroll-steps.
@@ -1644,48 +1632,50 @@ impl ScriptThread {
             // https://drafts.csswg.org/css-position-4/#process-top-layer-removals.
         }
 
-        // Perform a microtask checkpoint as the specifications says that *update the rendering* should be
+        // Perform a microtask checkpoint as the specifications says that *update the rendering*
-        // run in a task and a microtask checkpoint is always done when running tasks.
+        // should be run in a task and a microtask checkpoint is always done when running tasks.
         self.perform_a_microtask_checkpoint(can_gc);
+
+        // If there are pending reflows, they were probably caused by the execution of
+        // the microtask checkpoint above and we should spin the event loop one more
+        // time to resolve them.
+        self.schedule_rendering_opportunity_if_necessary();
     }
 
-    /// <https://html.spec.whatwg.org/multipage/#event-loop-processing-model:rendering-opportunity>
+    // If there are any pending reflows and we are not having rendering opportunities
-    fn rendering_opportunity(&self, pipeline_id: PipelineId) {
+    // driven by the compositor, then schedule the next rendering opportunity.
-        *self.last_render_opportunity_time.borrow_mut() = Some(Instant::now());
+    //
-
+    // TODO: This is a workaround until rendering opportunities can be triggered from a
-        // Note: the pipeline should be a navigable with a rendering opportunity,
+    // timer in the script thread.
-        // and we should use this opportunity to queue one task for each navigable with
+    fn schedule_rendering_opportunity_if_necessary(&self) {
-        // an opportunity in this script-thread.
+        // If any Document has active animations of rAFs, then we should be receiving
-        let Some(document) = self.documents.borrow().find_document(pipeline_id) else {
+        // regular rendering opportunities from the compositor (or fake animation frame
-            warn!("Trying to update the rendering for closed pipeline {pipeline_id}.");
+        // ticks). In this case, don't schedule an opportunity, just wait for the next
-            return;
+        // one.
-        };
+        if self.documents.borrow().iter().any(|(_, document)| {
-        let window = document.window();
+            document.animations().running_animation_count() != 0 ||
-        let task_manager = window.task_manager();
+                document.has_active_request_animation_frame_callbacks()
-        let rendering_task_source = task_manager.rendering_task_source();
+        }) {
-        let canceller = task_manager.task_canceller(TaskSourceName::Rendering);
-
-        if !self
-            .update_the_rendering_task_queued_for_pipeline
-            .borrow_mut()
-            .insert(pipeline_id)
-        {
             return;
         }
 
+        let Some((_, document)) = self.documents.borrow().iter().find(|(_, document)| {
+            !document.window().reflows_suppressed() && document.needs_reflow().is_some()
+        }) else {
+            return;
+        };
+
         // Queues a task to update the rendering.
         // <https://html.spec.whatwg.org/multipage/#event-loop-processing-model:queue-a-global-task>
         //
         // Note: The specification says to queue a task using the navigable's active
         // window, but then updates the rendering for all documents.
-        let _ = rendering_task_source.queue_with_canceller(
+        //
-            task!(update_the_rendering: move || {
+        // This task is empty because any new IPC messages in the ScriptThread trigger a
-               // This task is empty because any new IPC messages in the ScriptThread trigger a
+        // rendering update when animations are not running.
-               // rendering update, unless animations are running, in which case updates are driven
+        let rendering_task_source = document.window().task_manager().rendering_task_source();
-               // by the compositor.
+        let _ =
-            }),
+            rendering_task_source.queue_unconditionally(task!(update_the_rendering: move || { }));
-            &canceller,
-        );
     }
 
     /// Handle incoming messages from other tasks and the task queue.
@@ -1932,9 +1922,6 @@ impl ScriptThread {
             for document in docs.iter() {
                 let _realm = enter_realm(&**document);
                 document.maybe_queue_document_completion();
-
-                // Document load is a rendering opportunity.
-                ScriptThread::note_rendering_opportunity(document.window().pipeline_id());
             }
             docs.clear();
         }
@@ -2847,7 +2834,6 @@ impl ScriptThread {
         self.profile_event(ScriptThreadEventCategory::Resize, Some(id), || {
             let window = self.documents.borrow().find_window(id);
             if let Some(ref window) = window {
-                self.rendering_opportunity(id);
                 window.add_resize_event(size, size_type);
                 return;
             }
@@ -3390,12 +3376,6 @@ impl ScriptThread {
 
         // TODO: This should only dirty nodes that are waiting for a web font to finish loading!
         document.dirty_all_nodes();
-
-        // This is required because the handlers added to the promise exposed at
-        // `document.fonts.ready` are run by the event loop only when it performs a microtask
-        // checkpoint. Without the call below, this never happens and the promise is 'stuck' waiting
-        // to be resolved until another event forces a microtask checkpoint.
-        self.rendering_opportunity(pipeline_id);
     }
 
     /// Handles a worklet being loaded by triggering a relayout of the page. Does nothing if the
@@ -3872,7 +3852,6 @@ impl ScriptThread {
             warn!("Compositor event sent to closed pipeline {pipeline_id}.");
             return;
         };
-        self.rendering_opportunity(pipeline_id);
         document.note_pending_compositor_event(event);
     }
 

components/script/task_source/rendering.rs

@@ -43,3 +43,19 @@ impl TaskSource for RenderingTaskSource {
         self.0.send(msg_task).map_err(|_| ())
     }
 }
+
+impl RenderingTaskSource {
+    /// This queues a task that will not be cancelled when its associated
+    /// global scope gets destroyed.
+    pub fn queue_unconditionally<T>(&self, task: T) -> Result<(), ()>
+    where
+        T: TaskOnce + 'static,
+    {
+        self.0.send(CommonScriptMsg::Task(
+            ScriptThreadEventCategory::NetworkEvent,
+            Box::new(task),
+            Some(self.1),
+            RenderingTaskSource::NAME,
+        ))
+    }
+}