Auto merge of #18094 - bholley:ancestor_reconstruct, r=emilio

Avoid leaving stale ANCESTOR_WAS_RECONSTRUCTED bits in the tree

https://bugzilla.mozilla.org/show_bug.cgi?id=1390179

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18094)
<!-- Reviewable:end -->

4 files changed, 45 insertions, 24 deletions

diff --git a/components/style/data.rs b/components/style/data.rs
index 096f09f00de..35f75b10b3d 100644
--- a/components/style/data.rs
+++ b/components/style/data.rs
@@ -106,10 +106,14 @@ impl RestyleData {
     }
 
     /// Sets the flag that tells us whether we've reconstructed an ancestor.
-    pub fn set_reconstructed_ancestor(&mut self) {
-        // If it weren't for animation-only traversals, we could assert
-        // `!self.reconstructed_ancestor()` here.
-        self.flags.insert(ANCESTOR_WAS_RECONSTRUCTED);
+    pub fn set_reconstructed_ancestor(&mut self, reconstructed: bool) {
+        if reconstructed {
+            // If it weren't for animation-only traversals, we could assert
+            // `!self.reconstructed_ancestor()` here.
+            self.flags.insert(ANCESTOR_WAS_RECONSTRUCTED);
+        } else {
+            self.flags.remove(ANCESTOR_WAS_RECONSTRUCTED);
+        }
     }
 
     /// Mark this element as restyled, which is useful to know whether we need
diff --git a/components/style/dom.rs b/components/style/dom.rs
index 7df9c850fd4..8673a50e203 100644
--- a/components/style/dom.rs
+++ b/components/style/dom.rs
@@ -513,6 +513,13 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone +
     unsafe fn unset_animation_only_dirty_descendants(&self) {
     }
 
+    /// Clear all bits related to dirty descendant.
+    ///
+    /// In Gecko, this corresponds to the regular dirty descendants bit, the
+    /// animation-only dirty descendants bit, and the lazy frame construction
+    /// descendants bit.
+    unsafe fn clear_descendants_bits(&self) { self.unset_dirty_descendants(); }
+
     /// Returns true if this element is a visited link.
     ///
     /// Servo doesn't support visited styles yet.
diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs
index 80ab02ae518..e4ccac1a2ba 100644
--- a/components/style/gecko/wrapper.rs
+++ b/components/style/gecko/wrapper.rs
@@ -61,6 +61,7 @@ use gecko_bindings::structs::ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SE
 use gecko_bindings::structs::ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO;
 use gecko_bindings::structs::ELEMENT_HAS_SNAPSHOT;
 use gecko_bindings::structs::EffectCompositor_CascadeLevel as CascadeLevel;
+use gecko_bindings::structs::NODE_DESCENDANTS_NEED_FRAMES;
 use gecko_bindings::structs::NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE;
 use gecko_bindings::structs::NODE_IS_NATIVE_ANONYMOUS;
 use gecko_bindings::structs::nsChangeHint;
@@ -1052,6 +1053,12 @@ impl<'le> TElement for GeckoElement<'le> {
         self.unset_flags(ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO as u32)
     }
 
+    unsafe fn clear_descendants_bits(&self) {
+        self.unset_flags(ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32 |
+                         ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO as u32 |
+                         NODE_DESCENDANTS_NEED_FRAMES as u32)
+    }
+
     fn is_visited_link(&self) -> bool {
         use element_state::IN_VISITED_STATE;
         self.get_state().intersects(IN_VISITED_STATE)
diff --git a/components/style/traversal.rs b/components/style/traversal.rs
index 546e254adf4..37abedfc237 100644
--- a/components/style/traversal.rs
+++ b/components/style/traversal.rs
@@ -170,6 +170,12 @@ pub trait DomTraversal<E: TElement> : Sync {
             // Invalidate our style, and the one of our siblings and descendants
             // as needed.
             data.invalidate_style_if_needed(root, shared_context);
+
+            // Make sure we don't have any stale RECONSTRUCTED_ANCESTOR bits from
+            // the last traversal (at a potentially-higher root). From the
+            // perspective of this traversal, the root cannot have reconstructed
+            // ancestors.
+            data.restyle.set_reconstructed_ancestor(false);
         };
 
         let parent = root.traversal_parent();
@@ -591,18 +597,10 @@ where
         data.clear_restyle_flags_and_damage();
     }
 
-    // Optionally clear the descendants bit for the traversal type we're in.
-    if flags.for_animation_only() {
-        if flags.contains(ClearAnimationOnlyDirtyDescendants) {
-            unsafe { element.unset_animation_only_dirty_descendants(); }
-        }
-    } else {
-        // There are two cases when we want to clear the dity descendants bit here
-        // after styling this element. The first case is when we were explicitly
-        // asked to clear the bit by the caller.
-        //
-        // The second case is when this element is the root of a display:none
-        // subtree, even if the style didn't change (since, if the style did change,
+    // Optionally clear the descendants bits.
+    if data.styles.is_display_none() {
+        // When this element is the root of a display:none subtree, we want to clear
+        // the bits even if the style didn't change (since, if the style did change,
         // we'd have already cleared it above).
         //
         // This keeps the tree in a valid state without requiring the DOM to check
@@ -610,10 +608,18 @@ where
         // moderately expensive). Instead, DOM implementations can unconditionally
         // set the dirty descendants bit on any styled parent, and let the traversal
         // sort it out.
-        if flags.contains(ClearDirtyDescendants) ||
-           data.styles.is_display_none() {
-            unsafe { element.unset_dirty_descendants(); }
+        //
+        // Note that the NODE_DESCENDANTS_NEED_FRAMES bit should generally only be set
+        // when appending content beneath an element with a frame (i.e. not
+        // display:none), so clearing it here isn't strictly necessary, but good
+        // belt-and-suspenders.
+        unsafe { element.clear_descendants_bits(); }
+    } else if flags.for_animation_only() {
+        if flags.contains(ClearAnimationOnlyDirtyDescendants) {
+            unsafe { element.unset_animation_only_dirty_descendants(); }
         }
+    } else if flags.contains(ClearDirtyDescendants) {
+        unsafe { element.unset_dirty_descendants(); }
     }
 
     context.thread_local.end_element(element);
@@ -793,9 +799,7 @@ where
         if let Some(ref mut child_data) = child_data {
             // Propagate the parent restyle hint, that may make us restyle the whole
             // subtree.
-            if reconstructed_ancestor {
-                child_data.restyle.set_reconstructed_ancestor();
-            }
+            child_data.restyle.set_reconstructed_ancestor(reconstructed_ancestor);
 
             child_data.restyle.hint.insert(propagated_hint);
 
@@ -845,7 +849,6 @@ where
     }
 
     unsafe {
-        el.unset_dirty_descendants();
-        el.unset_animation_only_dirty_descendants();
+        el.clear_descendants_bits();
     }
 }