Auto merge of #17834 - emilio:animation-fc-crash, r=heycam

stylo: Fix the interaction of frame construction restyles with animation restyles.

Fixes bug 1383001.

5 files changed, 37 insertions, 53 deletions

diff --git a/components/style/data.rs b/components/style/data.rs
index 47cf4c8a8af..21d35252304 100644
--- a/components/style/data.rs
+++ b/components/style/data.rs
@@ -298,22 +298,16 @@ impl ElementData {
         self.styles.primary.is_some()
     }
 
-    /// Returns whether we have any outstanding style invalidation.
-    pub fn has_invalidations(&self) -> bool {
-        self.restyle.hint.has_self_invalidations()
-    }
-
     /// Returns the kind of restyling that we're going to need to do on this
     /// element, based of the stored restyle hint.
-    pub fn restyle_kind(&self,
-                        shared_context: &SharedStyleContext)
-                        -> RestyleKind {
+    pub fn restyle_kind(
+        &self,
+        shared_context: &SharedStyleContext
+    ) -> RestyleKind {
         if shared_context.traversal_flags.for_animation_only() {
             return self.restyle_kind_for_animation(shared_context);
         }
 
-        debug_assert!(!self.has_styles() || self.has_invalidations(),
-                      "Should've stopped earlier");
         if !self.has_styles() {
             return RestyleKind::MatchAndCascade;
         }
@@ -335,9 +329,10 @@ impl ElementData {
     }
 
     /// Returns the kind of restyling for animation-only restyle.
-    pub fn restyle_kind_for_animation(&self,
-                                      shared_context: &SharedStyleContext)
-                                      -> RestyleKind {
+    fn restyle_kind_for_animation(
+        &self,
+        shared_context: &SharedStyleContext,
+    ) -> RestyleKind {
         debug_assert!(shared_context.traversal_flags.for_animation_only());
         debug_assert!(self.has_styles(),
                       "Unstyled element shouldn't be traversed during \
@@ -350,8 +345,8 @@ impl ElementData {
         if hint.has_animation_hint() {
             return RestyleKind::CascadeWithReplacements(hint & RestyleHint::for_animations());
         }
-        return RestyleKind::CascadeOnly;
 
+        return RestyleKind::CascadeOnly;
     }
 
     /// Return true if important rules are different.
@@ -362,9 +357,11 @@ impl ElementData {
     ///       the check which properties do they want.
     ///       If it costs too much, get_properties_overriding_animations() should return a set
     ///       containing only opacity and transform properties.
-    pub fn important_rules_are_different(&self,
-                                         rules: &StrongRuleNode,
-                                         guards: &StylesheetGuards) -> bool {
+    pub fn important_rules_are_different(
+        &self,
+        rules: &StrongRuleNode,
+        guards: &StylesheetGuards
+    ) -> bool {
         debug_assert!(self.has_styles());
         let (important_rules, _custom) =
             self.styles.primary().rules().get_properties_overriding_animations(&guards);
diff --git a/components/style/dom.rs b/components/style/dom.rs
index a7f68a96541..4b69a8e2c8a 100644
--- a/components/style/dom.rs
+++ b/components/style/dom.rs
@@ -470,19 +470,12 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone +
     /// Flags this element as having handled already its snapshot.
     unsafe fn set_handled_snapshot(&self);
 
-    /// Returns whether the element's styles are up-to-date.
-    fn has_current_styles(&self, data: &ElementData) -> bool {
-        if self.has_snapshot() && !self.handled_snapshot() {
-            return false;
-        }
-
-        data.has_styles() && !data.has_invalidations()
-    }
-
     /// Returns whether the element's styles are up-to-date for |traversal_flags|.
-    fn has_current_styles_for_traversal(&self,
-                                        data: &ElementData,
-                                        traversal_flags: TraversalFlags) -> bool {
+    fn has_current_styles_for_traversal(
+        &self,
+        data: &ElementData,
+        traversal_flags: TraversalFlags,
+    ) -> bool {
         if traversal_flags.for_animation_only() {
             // In animation-only restyle we never touch snapshots and don't
             // care about them. But we can't assert '!self.handled_snapshot()'
@@ -499,7 +492,7 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone +
             return false;
         }
 
-        data.has_styles() && !data.has_invalidations()
+        data.has_styles() && !data.restyle.hint.has_non_animation_hint()
     }
 
     /// Flags an element and its ancestors with a given `DescendantsBit`.
diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs
index 204f6021907..306aba45e19 100644
--- a/components/style/style_resolver.rs
+++ b/components/style/style_resolver.rs
@@ -52,17 +52,7 @@ where
 {
     let parent_el = element.inheritance_parent();
     let parent_data = parent_el.as_ref().and_then(|e| e.borrow_data());
-    let parent_style = parent_data.as_ref().map(|d| {
-        // Sometimes Gecko eagerly styles things without processing
-        // pending restyles first. In general we'd like to avoid this,
-        // but there can be good reasons (for example, needing to
-        // construct a frame for some small piece of newly-added
-        // content in order to do something specific with that frame,
-        // but not wanting to flush all of layout).
-        debug_assert!(cfg!(feature = "gecko") ||
-                      parent_el.unwrap().has_current_styles(d));
-        d.styles.primary()
-    });
+    let parent_style = parent_data.as_ref().map(|d| d.styles.primary());
 
     let mut layout_parent_el = parent_el.clone();
     let layout_parent_data;
diff --git a/components/style/traversal.rs b/components/style/traversal.rs
index a3495fac924..1f917aaee2a 100644
--- a/components/style/traversal.rs
+++ b/components/style/traversal.rs
@@ -374,9 +374,8 @@ pub trait DomTraversal<E: TElement> : Sync {
         parent: E,
         parent_data: &ElementData,
     ) -> bool {
-        // See the comment on `cascade_node` for why we allow this on Gecko.
         debug_assert!(cfg!(feature = "gecko") ||
-                      parent.has_current_styles(parent_data));
+                      parent.has_current_styles_for_traversal(parent_data, context.shared.traversal_flags));
 
         // If the parent computed display:none, we don't style the subtree.
         if parent_data.styles.is_display_none() {
diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs
index bdbf856203e..0026c7212e8 100644
--- a/ports/geckolib/glue.rs
+++ b/ports/geckolib/glue.rs
@@ -275,21 +275,26 @@ pub extern "C" fn Servo_TraverseSubtree(root: RawGeckoElementBorrowed,
         (Root::Normal, Restyle::ForThrottledAnimationFlush)
             => TraversalFlags::empty(),
         (Root::UnstyledChildrenOnly, Restyle::Normal) |
-        (Root::UnstyledChildrenOnly, Restyle::ForNewlyBoundElement) |
-        (Root::UnstyledChildrenOnly, Restyle::ForThrottledAnimationFlush)
+        (Root::UnstyledChildrenOnly, Restyle::ForNewlyBoundElement)
             => UNSTYLED_CHILDREN_ONLY,
         (Root::Normal, Restyle::ForCSSRuleChanges) => FOR_CSS_RULE_CHANGES,
         (Root::Normal, Restyle::ForReconstruct) => FOR_RECONSTRUCT,
         _ => panic!("invalid combination of TraversalRootBehavior and TraversalRestyleBehavior"),
     };
 
-    let needs_animation_only_restyle = element.has_animation_only_dirty_descendants() ||
-                                       element.has_animation_restyle_hints();
-    if needs_animation_only_restyle {
-        traverse_subtree(element,
-                         raw_data,
-                         traversal_flags | ANIMATION_ONLY,
-                         unsafe { &*snapshots });
+    // It makes no sense to do an animation restyle when we're restyling
+    // newly-inserted content.
+    if !traversal_flags.contains(UNSTYLED_CHILDREN_ONLY) {
+        let needs_animation_only_restyle =
+            element.has_animation_only_dirty_descendants() ||
+            element.has_animation_restyle_hints();
+
+        if needs_animation_only_restyle {
+            traverse_subtree(element,
+                             raw_data,
+                             traversal_flags | ANIMATION_ONLY,
+                             unsafe { &*snapshots });
+        }
     }
 
     if restyle_behavior == Restyle::ForThrottledAnimationFlush {
@@ -2814,7 +2819,7 @@ pub extern "C" fn Servo_ResolveStyle(element: RawGeckoElementBorrowed,
         TraversalFlags::empty()
     };
     debug_assert!(element.has_current_styles_for_traversal(&*data, flags),
-                  "Resolving style on element without current styles");
+                  "Resolving style on {:?} without current styles: {:?}", element, data);
     data.styles.primary().clone().into()
 }