servo: Merge #12185 - style: Do not re-expire animations (from emilio:dont-reexpire-animations); r=nox
authorEmilio Cobos Álvarez <me@emiliocobos.me>
Sun, 03 Jul 2016 12:51:15 -0700
changeset 339193 8fd2dbdc95ce5aa51937164f3e37e4f6d8dfa917
parent 339192 832f74e40511cfa776bb9b0b8e4ce269cc2b48c8
child 339194 46615d92583f67738218607fa0e0292211605dd3
push id31307
push usergszorc@mozilla.com
push dateSat, 04 Feb 2017 00:59:06 +0000
treeherdermozilla-central@94079d43835f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnox
servo: Merge #12185 - style: Do not re-expire animations (from emilio:dont-reexpire-animations); r=nox <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #12171 (github issue number if applicable). <!-- Either: --> - [x] These changes do not have tests because they depend on weird timing conditions. <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> If we're restyling a page with animations and layout takes too long, we might still have the expired animations from the last restyle, without these being cleared out by layout on `tick_animations`. Unfortunately it's hard (near to impossible?) to make a reduced test case for this, since it heavily depends on the speed of the build and conditions that only happen under heavy loads. Mainly, it depends on how accurately the `TickAllAnimations` message is sent from the constellation. Thus, we can't just re-expire an animation, and we should re-check for it as a previous animation. Fixes #12171 Source-Repo: https://github.com/servo/servo Source-Revision: 212aa4437e06af72ed3a215a1b49a4b1121f6398
servo/components/style/animation.rs
servo/components/style/matching.rs
--- a/servo/components/style/animation.rs
+++ b/servo/components/style/animation.rs
@@ -78,16 +78,17 @@ pub struct KeyframesAnimationState<Impl:
 impl<Impl: SelectorImplExt> KeyframesAnimationState<Impl> {
     /// Performs a tick in the animation state, i.e., increments the counter of
     /// the current iteration count, updates times and then toggles the
     /// direction if appropriate.
     ///
     /// Returns true if the animation should keep running.
     pub fn tick(&mut self) -> bool {
         debug!("KeyframesAnimationState::tick");
+        debug_assert!(!self.expired);
 
         self.started_at += self.duration + self.delay;
         match self.running_state {
             // If it's paused, don't update direction or iteration count.
             KeyframesRunningState::Paused(_) => return true,
             KeyframesRunningState::Running => {},
         }
 
@@ -490,35 +491,34 @@ pub fn update_style_for_animation_frame<
 /// If `damage` is provided, inserts the appropriate restyle damage.
 pub fn update_style_for_animation<Damage, Impl>(context: &SharedStyleContext<Impl>,
                                                 animation: &Animation<Impl>,
                                                 style: &mut Arc<Damage::ConcreteComputedValues>,
                                                 damage: Option<&mut Damage>)
 where Impl: SelectorImplExt,
       Damage: TRestyleDamage<ConcreteComputedValues = Impl::ComputedValues> {
     debug!("update_style_for_animation: entering");
+    debug_assert!(!animation.is_expired());
     match *animation {
         Animation::Transition(_, start_time, ref frame, expired) => {
-            debug_assert!(!expired);
             debug!("update_style_for_animation: transition found");
             let now = time::precise_time_s();
             let mut new_style = (*style).clone();
             let updated_style = update_style_for_animation_frame(&mut new_style,
                                                                  now, start_time,
                                                                  frame);
             if updated_style {
                 if let Some(damage) = damage {
                     *damage = *damage | Damage::compute(Some(style), &new_style);
                 }
 
                 *style = new_style
             }
         }
         Animation::Keyframes(_, ref name, ref state) => {
-            debug_assert!(!state.expired);
             debug!("update_style_for_animation: animation found: \"{}\", {:?}", name, state);
             let duration = state.duration;
             let started_at = state.started_at;
 
             let now = match state.running_state {
                 KeyframesRunningState::Running => time::precise_time_s(),
                 KeyframesRunningState::Paused(progress) => started_at + duration * progress,
             };
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -493,19 +493,32 @@ trait PrivateMatchMethods: TNode
         let had_running_animations = context.running_animations
                                             .read()
                                             .unwrap()
                                             .get(&this_opaque)
                                             .is_some();
         if had_running_animations {
             let mut all_running_animations = context.running_animations.write().unwrap();
             for mut running_animation in all_running_animations.get_mut(&this_opaque).unwrap() {
-                animation::update_style_for_animation::<Self::ConcreteRestyleDamage,
-                    <Self::ConcreteElement as Element>::Impl>(context, running_animation, style, None);
-                running_animation.mark_as_expired();
+                // This shouldn't happen frequently, but under some
+                // circumstances mainly huge load or debug builds, the
+                // constellation might be delayed in sending the
+                // `TickAllAnimations` message to layout.
+                //
+                // Thus, we can't assume all the animations have been already
+                // updated by layout, because other restyle due to script might
+                // be triggered by layout before the animation tick.
+                //
+                // See #12171 and the associated PR for an example where this
+                // happened while debugging other release panic.
+                if !running_animation.is_expired() {
+                    animation::update_style_for_animation::<Self::ConcreteRestyleDamage,
+                        <Self::ConcreteElement as Element>::Impl>(context, running_animation, style, None);
+                    running_animation.mark_as_expired();
+                }
             }
         }
 
         had_animations_to_expire || had_running_animations
     }
 }
 
 impl<N: TNode> PrivateMatchMethods for N