From af1b0b0f14d951b8e712b71a220e308d7b2f0c2e Mon Sep 17 00:00:00 2001 From: yvt Date: Fri, 28 Oct 2022 12:49:47 +0900 Subject: [PATCH 1/2] fix(script): update animation timeline before processing pending events This commit reverses the order of the `send_pending_events` and `update_animation_timeline` calls in `ScriptThread::update_animations_ send_events` so that animation-related events pended by the latter are processed by the former. The new calling order is more compliant with the "update animations and send events" algorithm steps from [the Web Animations specification][1]. The old implementation was prone to blocking for an indefinite period while holding pending events. Due to complex interaction with other events and timing behavior, I was only able to reproduce the problem under the following conditions: - *The `maybe_observe_paint_time` call in `LayoutThread::compute_abs_ pos_and_build_display_list` is removed from the code*. While performance events may seem irrelevant to the issue, they would bombard the script thread with events. *Any* extra messages received would unblock the event loop and prevent the manifestation of the issue. (But, of course, we aren't supposed to count on that to avoid the issue.) - Servo is running in a headless mode, which somehow makes it less likely for the script thread to receive a `TickAllAnimations` event after sending `AnimationState::NoAnimationsPresent`. With the above conditions met and the stars aligned, you can reproduce the problem by running the WPT test `/css/css-transitions/events-001. html`. [1]: https://drafts.csswg.org/web-animations-1/#update-animations-and-send-events --- components/script/script_thread.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 506cd5d6bf2..a82edd61cc3 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -1705,11 +1705,11 @@ impl ScriptThread { // Perform step 11.10 from https://html.spec.whatwg.org/multipage/#event-loops. fn update_animations_and_send_events(&self) { for (_, document) in self.documents.borrow().iter() { - document.animations().send_pending_events(); + document.update_animation_timeline(); } for (_, document) in self.documents.borrow().iter() { - document.update_animation_timeline(); + document.animations().send_pending_events(); } } From effd5a3107081d6efb2bf6e50a67bbb0b282f82a Mon Sep 17 00:00:00 2001 From: yvt Date: Fri, 28 Oct 2022 01:31:19 +0900 Subject: [PATCH 2/2] fix(script): request animation ticks if `Animations::pending_events` is not empty Fixes the test case `/_mozilla/css/css-transition-cancel-event .html`, which was failing under a specific circumstance. The observed sequence of events during the failing test run looks like this: 1. Transitions start in `div1` and `div2`. 2. `div1` generates a `transitionend` event. 3. The `transitionend` event handler removes `div2` from DOM, cancelling its ongoing transition. 4. `div2` is supposed to generate a `transitioncancel` event in a timely manner, which it does not. The test fails as a result. What is going on here? Here's a possible explaination: 1. During one invocation of `ScriptThread::handle_msgs`... 2. In step 2, `ScriptThread::update_animations_send_events` -> `Document ::update_for_new_timeline_value` detects the completion of the transition, and in response, pends the `transitionend` event. 3. In step 3, `ScriptThread::update_animations_send_events` -> `Animations::send_pending_events` calls the `transitionend` handler. 4. The `transitionend` event handler removes `div2`, thereby cancelling its ongoing transition and triggering a reflow. 5. Reflow takes place. During this, `Animations::do_post_reflow_update` -> `Animations::handle_canceled_animations` pends the `transitioncancel` event (precursor to step 4). 6. Having discovering that there was no running animation, `Animations:: do_post_reflow_update` calls `self.update_running_animation_presence (_, false)`, which sends `AnimationState::NoAnimationsPresent`. 7. The invocation of `ScriptThread::handle_msgs` ends, and another starts. It blocks waiting for events. 8. Meanwhile, the compositor receives `AnimationState:: NoAnimationsPresent` and stops further generation of animation ticks. 9. With no events to wake it up, the script thread is stuck waiting despite having the pending `transitioncancel` event (step 4). The HTML specification [says][1] that "an event loop must continually run [...] as long as it exists" and does not say it can block if there is nothing to do. Blocking is merely optimization in a user agent implementation. Pending animation-related events must be processed every time a "rendering opportunity" arises unless the user agent has a reason to believe that it "would have no visible effect". Skipping the processing of animation-related events would have visible effect if such events are indeed present. The correct implementation in Servo, therefore, would be to request more animation ticks so that such events are processed in a subsequent tick. [1]: https://html.spec.whatwg.org/multipage/#event-loop-processing-model --- components/script/animations.rs | 32 ++++++--- components/script/script_thread.rs | 2 +- tests/wpt/mozilla/meta/MANIFEST.json | 7 ++ .../css/css-transition-cancel-event.html | 68 +++++++++++++++++++ 4 files changed, 100 insertions(+), 9 deletions(-) create mode 100644 tests/wpt/mozilla/tests/css/css-transition-cancel-event.html diff --git a/components/script/animations.rs b/components/script/animations.rs index cd581ed3cb3..73131763430 100644 --- a/components/script/animations.rs +++ b/components/script/animations.rs @@ -40,7 +40,7 @@ pub(crate) struct Animations { pub sets: DocumentAnimationSet, /// Whether or not we have animations that are running. - have_running_animations: Cell, + has_running_animations: Cell, /// A list of nodes with in-progress CSS transitions or pending events. rooted_nodes: DomRefCell>>, @@ -53,7 +53,7 @@ impl Animations { pub(crate) fn new() -> Self { Animations { sets: Default::default(), - have_running_animations: Cell::new(false), + has_running_animations: Cell::new(false), rooted_nodes: Default::default(), pending_events: Default::default(), } @@ -97,6 +97,7 @@ impl Animations { } self.unroot_unused_nodes(&sets); + //self.update_running_animations_presence(window); } /// Cancel animations for the given node, if any exist. @@ -142,17 +143,25 @@ impl Animations { } fn update_running_animations_presence(&self, window: &Window, new_value: bool) { - let have_running_animations = self.have_running_animations.get(); - if new_value == have_running_animations { + let had_running_animations = self.has_running_animations.get(); + if new_value == had_running_animations { return; } - self.have_running_animations.set(new_value); - let state = match new_value { + self.has_running_animations.set(new_value); + self.handle_animation_presence_or_pending_events_change(window); + } + + fn handle_animation_presence_or_pending_events_change(&self, window: &Window) { + let has_running_animations = self.has_running_animations.get(); + let has_pending_events = !self.pending_events.borrow().is_empty(); + + // Do not send the NoAnimationCallbacksPresent state until all pending + // animation events are delivered. + let state = match has_running_animations || has_pending_events { true => AnimationsPresentState::AnimationsPresent, false => AnimationsPresentState::NoAnimationsPresent, }; - window.send_to_constellation(ScriptMsg::ChangeRunningAnimationsState(state)); } @@ -425,10 +434,13 @@ impl Animations { }); } - pub(crate) fn send_pending_events(&self) { + pub(crate) fn send_pending_events(&self, window: &Window) { // Take all of the events here, in case sending one of these events // triggers adding new events by forcing a layout. let events = std::mem::replace(&mut *self.pending_events.borrow_mut(), Vec::new()); + if events.is_empty() { + return; + } for event in events.into_iter() { // We root the node here to ensure that sending this event doesn't @@ -488,6 +500,10 @@ impl Animations { .fire(node.upcast()); } } + + if self.pending_events.borrow().is_empty() { + self.handle_animation_presence_or_pending_events_change(window); + } } } diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index a82edd61cc3..b417f453cdf 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -1709,7 +1709,7 @@ impl ScriptThread { } for (_, document) in self.documents.borrow().iter() { - document.animations().send_pending_events(); + document.animations().send_pending_events(document.window()); } } diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index cbb7a753297..8eb3d7dd00f 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -12980,6 +12980,13 @@ ] ] }, + "css-transition-cancel-event.html": [ + "23400c556d58bb21b78a9cbed3b56028c7d299c3", + [ + null, + {} + ] + ], "empty-keyframes.html": [ "9f8935fb7f51219bb3ee07335e208a63c9edde81", [ diff --git a/tests/wpt/mozilla/tests/css/css-transition-cancel-event.html b/tests/wpt/mozilla/tests/css/css-transition-cancel-event.html new file mode 100644 index 00000000000..23400c556d5 --- /dev/null +++ b/tests/wpt/mozilla/tests/css/css-transition-cancel-event.html @@ -0,0 +1,68 @@ + + + + + + + + + + + + + +