Centralize note_dirty_descendants implementation, and fully propagate dirty_descendants in resolve_style.

The current code can leave the tree in an inconsistent state, with the dirty
descendants bit not fully propagated.

MozReview-Commit-ID: ALI6etmlrDa

4 files changed, 68 insertions, 27 deletions

diff --git a/components/script/layout_wrapper.rs b/components/script/layout_wrapper.rs
index adf47842e82..661d3289671 100644
--- a/components/script/layout_wrapper.rs
+++ b/components/script/layout_wrapper.rs
@@ -63,7 +63,8 @@ use style::attr::AttrValue;
 use style::computed_values::display;
 use style::context::{QuirksMode, SharedStyleContext};
 use style::data::ElementData;
-use style::dom::{LayoutIterator, NodeInfo, OpaqueNode, PresentationalHintsSynthetizer, TElement, TNode};
+use style::dom::{DirtyDescendants, LayoutIterator, NodeInfo, OpaqueNode, PresentationalHintsSynthetizer};
+use style::dom::{TElement, TNode};
 use style::dom::UnsafeNode;
 use style::element_state::*;
 use style::properties::{ComputedValues, PropertyDeclarationBlock};
@@ -487,6 +488,9 @@ impl<'le> ServoLayoutElement<'le> {
         }
     }
 
+    // FIXME(bholley): This should be merged with TElement::note_dirty_descendants,
+    // but that requires re-testing and possibly fixing the broken callers given
+    // the FIXME below, which I don't have time to do right now.
     pub unsafe fn note_dirty_descendant(&self) {
         use ::selectors::Element;
 
@@ -501,19 +505,7 @@ impl<'le> ServoLayoutElement<'le> {
             current = el.parent_element();
         }
 
-        debug_assert!(self.dirty_descendants_bit_is_propagated());
-    }
-
-    fn dirty_descendants_bit_is_propagated(&self) -> bool {
-        use ::selectors::Element;
-
-        let mut current = Some(*self);
-        while let Some(el) = current {
-            if !el.has_dirty_descendants() { return false; }
-            current = el.parent_element();
-        }
-
-        true
+        debug_assert!(self.descendants_bit_is_propagated::<DirtyDescendants>());
     }
 }
 
diff --git a/components/style/dom.rs b/components/style/dom.rs
index 9d25013b15e..ab6ed9aed30 100644
--- a/components/style/dom.rs
+++ b/components/style/dom.rs
@@ -22,6 +22,7 @@ use std::fmt::Debug;
 use std::ops::Deref;
 use std::sync::Arc;
 use stylist::ApplicableDeclarationBlock;
+use thread_state;
 
 pub use style_traits::UnsafeNode;
 
@@ -305,6 +306,36 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
     /// Only safe to call with exclusive access to the element.
     unsafe fn set_dirty_descendants(&self);
 
+    /// Flag that this element has a descendant for style processing, propagating
+    /// the bit up to the root as needed.
+    ///
+    /// This is _not_ safe to call during the parallel traversal.
+    unsafe fn note_descendants<B: DescendantsBit<Self>>(&self) {
+        debug_assert!(!thread_state::get().is_worker());
+        let mut curr = Some(*self);
+        while curr.is_some() && !B::has(curr.unwrap()) {
+            B::set(curr.unwrap());
+            curr = curr.unwrap().parent_element();
+        }
+
+        // Note: We disable this assertion on servo because of bugs. See the
+        // comment around note_dirty_descendant in layout/wrapper.rs.
+        if cfg!(feature = "gecko") {
+            debug_assert!(self.descendants_bit_is_propagated::<B>());
+        }
+    }
+
+    /// Debug helper to be sure the bit is propagated.
+    fn descendants_bit_is_propagated<B: DescendantsBit<Self>>(&self) -> bool {
+        let mut current = Some(*self);
+        while let Some(el) = current {
+            if !B::has(el) { return false; }
+            current = el.parent_element();
+        }
+
+        true
+    }
+
     /// Flag that this element has no descendant for style processing.
     ///
     /// Only safe to call with exclusive access to the element.
@@ -391,6 +422,28 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
     }
 }
 
+/// Trait abstracting over different kinds of dirty-descendants bits.
+pub trait DescendantsBit<E: TElement> {
+    /// Returns true if the Element has the bit.
+    fn has(el: E) -> bool;
+    /// Sets the bit on the Element.
+    unsafe fn set(el: E);
+}
+
+/// Implementation of DescendantsBit for the regular dirty descendants bit.
+pub struct DirtyDescendants;
+impl<E: TElement> DescendantsBit<E> for DirtyDescendants {
+    fn has(el: E) -> bool { el.has_dirty_descendants() }
+    unsafe fn set(el: E) { el.set_dirty_descendants(); }
+}
+
+/// Implementation of DescendantsBit for the animation-only dirty descendants bit.
+pub struct AnimationOnlyDirtyDescendants;
+impl<E: TElement> DescendantsBit<E> for AnimationOnlyDirtyDescendants {
+    fn has(el: E) -> bool { el.has_animation_only_dirty_descendants() }
+    unsafe fn set(el: E) { el.set_animation_only_dirty_descendants(); }
+}
+
 /// TNode and TElement aren't Send because we want to be careful and explicit
 /// about our parallel traversal. However, there are certain situations
 /// (including but not limited to the traversal) where we need to send DOM
diff --git a/components/style/traversal.rs b/components/style/traversal.rs
index df70c29664c..b37b10ef94f 100644
--- a/components/style/traversal.rs
+++ b/components/style/traversal.rs
@@ -9,7 +9,7 @@
 use atomic_refcell::{AtomicRefCell, AtomicRefMut};
 use context::{SharedStyleContext, StyleContext, ThreadLocalStyleContext};
 use data::{ElementData, ElementStyles, StoredRestyleHint};
-use dom::{NodeInfo, TElement, TNode};
+use dom::{DirtyDescendants, NodeInfo, TElement, TNode};
 use matching::{MatchMethods, MatchResults};
 use restyle_hints::{RESTYLE_DESCENDANTS, RESTYLE_SELF};
 use selector_parser::RestyleDamage;
@@ -388,7 +388,7 @@ fn resolve_style_internal<E, F>(context: &mut StyleContext<E>,
         // Conservatively mark us as having dirty descendants, since there might
         // be other unstyled siblings we miss when walking straight up the parent
         // chain.
-        unsafe { element.set_dirty_descendants() };
+        unsafe { element.note_descendants::<DirtyDescendants>() };
     }
 
     // If we're display:none and none of our ancestors are, we're the root
diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs
index 3ac1fdb8690..e639070b28b 100644
--- a/ports/geckolib/glue.rs
+++ b/ports/geckolib/glue.rs
@@ -18,6 +18,7 @@ use style::arc_ptr_eq;
 use style::context::{QuirksMode, SharedStyleContext, StyleContext};
 use style::context::{ThreadLocalStyleContext, ThreadLocalStyleContextCreationInfo};
 use style::data::{ElementData, ElementStyles, RestyleData};
+use style::dom::{AnimationOnlyDirtyDescendants, DirtyDescendants};
 use style::dom::{ShowSubtreeData, TElement, TNode};
 use style::error_reporting::StdoutErrorReporter;
 use style::gecko::data::{PerDocumentStyleData, PerDocumentStyleDataImpl};
@@ -1384,17 +1385,12 @@ unsafe fn maybe_restyle<'a>(data: &'a mut AtomicRefMut<ElementData>,
     }
 
     // Propagate the bit up the chain.
-    let mut curr = element;
-    while let Some(parent) = curr.parent_element() {
-        curr = parent;
-        if animation_only {
-            if curr.has_animation_only_dirty_descendants() { break; }
-            curr.set_animation_only_dirty_descendants();
-        } else {
-            if curr.has_dirty_descendants() { break; }
-            curr.set_dirty_descendants();
-        }
-    }
+    if animation_only {
+        element.parent_element().map(|p| p.note_descendants::<AnimationOnlyDirtyDescendants>());
+    } else  {
+        element.parent_element().map(|p| p.note_descendants::<DirtyDescendants>());
+    };
+
     bindings::Gecko_SetOwnerDocumentNeedsStyleFlush(element.0);
 
     // Ensure and return the RestyleData.