servo: Merge #16159 - Centralize note_dirty_descendants implementation, and fully propagate dirty_descendants in resolve_style (from bholley:note_dirty_descendants); r=heycam
authorBobby Holley <bobbyholley@gmail.com>
Wed, 29 Mar 2017 06:52:07 -0500
changeset 553185 6fa1eb4be652ab3aa72cc6bc25c36b156d1182b4
parent 553184 cf506c806cb556cd775bdeb0aab8595ba639b017
child 553186 dd28f22e816ecc8344c461104f989f937a339569
push id51547
push userbmo:emilio+bugs@crisal.io
push dateWed, 29 Mar 2017 14:49:00 +0000
reviewersheycam
milestone55.0a1
servo: Merge #16159 - Centralize note_dirty_descendants implementation, and fully propagate dirty_descendants in resolve_style (from bholley:note_dirty_descendants); r=heycam The current code can leave the tree in an inconsistent state, with the dirty descendants bit not fully propagated. Reviewed in https://bugzilla.mozilla.org/show_bug.cgi?id=1350441 Source-Repo: https://github.com/servo/servo Source-Revision: 8adf1919732a764617ce7c8c26e772a9a7663b98
servo/components/script/layout_wrapper.rs
servo/components/style/dom.rs
servo/components/style/traversal.rs
servo/ports/geckolib/glue.rs
--- a/servo/components/script/layout_wrapper.rs
+++ b/servo/components/script/layout_wrapper.rs
@@ -58,17 +58,18 @@ use std::fmt::Debug;
 use std::marker::PhantomData;
 use std::mem::transmute;
 use std::sync::Arc;
 use std::sync::atomic::Ordering;
 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};
 use style::selector_parser::{NonTSPseudoClass, PseudoElement, SelectorImpl};
 use style::shared_lock::{SharedRwLock as StyleSharedRwLock, Locked as StyleLocked};
 use style::sink::Push;
 use style::str::is_whitespace;
 use style::stylist::ApplicableDeclarationBlock;
@@ -482,43 +483,34 @@ impl<'le> ServoLayoutElement<'le> {
     }
 
     fn get_partial_layout_data(&self) -> Option<&AtomicRefCell<PartialPersistentLayoutData>> {
         unsafe {
             self.get_style_and_layout_data().map(|d| &**d.ptr)
         }
     }
 
+    // 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;
 
         let mut current = Some(*self);
         while let Some(el) = current {
             // FIXME(bholley): Ideally we'd have the invariant that any element
             // with has_dirty_descendants also has the bit set on all its ancestors.
             // However, there are currently some corner-cases where we get that wrong.
             // I have in-flight patches to fix all this stuff up, so we just always
             // propagate this bit for now.
             el.set_dirty_descendants();
             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>());
     }
 }
 
 fn as_element<'le>(node: LayoutJS<Node>) -> Option<ServoLayoutElement<'le>> {
     node.downcast().map(ServoLayoutElement::from_layout_js)
 }
 
 impl<'le> ::selectors::MatchAttrGeneric for ServoLayoutElement<'le> {
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -17,16 +17,17 @@ use selector_parser::{ElementExt, PreExi
 use selectors::matching::ElementSelectorFlags;
 use shared_lock::Locked;
 use sink::Push;
 use std::fmt;
 use std::fmt::Debug;
 use std::ops::Deref;
 use std::sync::Arc;
 use stylist::ApplicableDeclarationBlock;
+use thread_state;
 
 pub use style_traits::UnsafeNode;
 
 /// An opaque handle to a node, which, unlike UnsafeNode, cannot be transformed
 /// back into a non-opaque representation. The only safe operation that can be
 /// performed on this node is to compare it to another opaque handle or to another
 /// OpaqueNode.
 ///
@@ -300,16 +301,46 @@ pub trait TElement : PartialEq + Debug +
     /// the actual restyle traversal.
     fn has_dirty_descendants(&self) -> bool;
 
     /// Flag that this element has a descendant for style processing.
     ///
     /// 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.
     unsafe fn unset_dirty_descendants(&self);
 
     /// Similar to the dirty_descendants but for representing a descendant of
     /// the element needs to be updated in animation-only traversal.
     fn has_animation_only_dirty_descendants(&self) -> bool {
@@ -386,16 +417,38 @@ pub trait TElement : PartialEq + Debug +
             Some(d) => d,
             None => return false,
         };
         return data.get_restyle()
                    .map_or(false, |r| r.hint.has_animation_hint());
     }
 }
 
+/// 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
 /// objects to other threads.
 ///
 /// That's the reason why `SendNode` exists.
 #[derive(Clone, Debug, PartialEq)]
 pub struct SendNode<N: TNode>(N);
--- a/servo/components/style/traversal.rs
+++ b/servo/components/style/traversal.rs
@@ -4,17 +4,17 @@
 
 //! Traversing the DOM tree; the bloom filter.
 
 #![deny(missing_docs)]
 
 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;
 use servo_config::opts;
 use std::borrow::BorrowMut;
 use stylist::Stylist;
 
 /// A per-traversal-level chunk of data. This is sent down by the traversal, and
@@ -382,17 +382,17 @@ fn resolve_style_internal<E, F>(context:
         // Compute our style.
         let match_results = element.match_element(context, &mut data);
         element.cascade_element(context, &mut data,
                                 match_results.primary_is_shareable());
 
         // 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
     // of a display:none subtree.
     if display_none_root.is_none() && data.styles().is_display_none() {
         display_none_root = Some(element);
     }
 
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -13,16 +13,17 @@ use std::borrow::Cow;
 use std::env;
 use std::fmt::Write;
 use std::ptr;
 use std::sync::{Arc, Mutex};
 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};
 use style::gecko::global_style_data::GLOBAL_STYLE_DATA;
 use style::gecko::restyle_damage::GeckoRestyleDamage;
 use style::gecko::selector_parser::{SelectorImpl, PseudoElement};
 use style::gecko::traversal::RecalcStyleOnly;
 use style::gecko::wrapper::DUMMY_BASE_URL;
@@ -1379,27 +1380,22 @@ unsafe fn maybe_restyle<'a>(data: &'a mu
     -> Option<&'a mut RestyleData>
 {
     // Don't generate a useless RestyleData if the element hasn't been styled.
     if !data.has_styles() {
         return None;
     }
 
     // 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.
     Some(data.ensure_restyle())
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_Element_GetSnapshot(element: RawGeckoElementBorrowed) -> *mut structs::ServoElementSnapshot