Auto merge of #14560 - bholley:fix_crashes, r=heycam

stylo: Fix some crashes on incubator

Corresponding gecko bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1322945

<!-- 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/14560)
<!-- Reviewable:end -->

components/layout_thread/lib.rs

@@ -106,7 +106,7 @@ use std::sync::mpsc::{Receiver, Sender, channel};
 use style::animation::Animation;
 use style::context::{LocalStyleContextCreationInfo, ReflowGoal, SharedStyleContext};
 use style::data::StoredRestyleHint;
-use style::dom::{TElement, TNode};
+use style::dom::{ShowSubtree, ShowSubtreeDataAndPrimaryValues, TElement, TNode};
 use style::error_reporting::{ParseErrorReporter, StdoutErrorReporter};
 use style::logical_geometry::LogicalPoint;
 use style::media_queries::{Device, MediaType};
@@ -1035,9 +1035,7 @@ impl LayoutThread {
 
         debug!("layout: processing reflow request for: {:?} ({}) (query={:?})",
                element, self.url, data.query_type);
-        if log_enabled!(log::LogLevel::Debug) {
+        debug!("{:?}", ShowSubtree(element.as_node()));
-            element.as_node().dump();
-        }
 
         let initial_viewport = data.window_size.initial_viewport;
         let old_viewport_size = self.viewport_size;
@@ -1181,7 +1179,7 @@ impl LayoutThread {
         }
 
         if opts::get().dump_style_tree {
-            element.as_node().dump_style();
+            println!("{:?}", ShowSubtreeDataAndPrimaryValues(element.as_node()));
         }
 
         if opts::get().dump_rule_tree {

components/script/layout_wrapper.rs

@@ -86,7 +86,11 @@ impl<'ln> Debug for ServoLayoutNode<'ln> {
         if let Some(el) = self.as_element() {
             el.fmt(f)
         } else {
-            write!(f, "{:?} ({:#x})", self.type_id(), self.opaque().0)
+            if self.is_text_node() {
+                write!(f, "<text node> ({:#x})", self.opaque().0)
+            } else {
+                write!(f, "<non-text node> ({:#x})", self.opaque().0)
+            }
         }
     }
 }
@@ -156,15 +160,6 @@ impl<'ln> TNode for ServoLayoutNode<'ln> {
         transmute(node)
     }
 
-    fn dump(self) {
-        self.dump_indent(0);
-    }
-
-    fn dump_style(self) {
-        println!("\nDOM with computed styles:");
-        self.dump_style_indent(0);
-    }
-
     fn children(self) -> LayoutIterator<ServoChildrenIterator<'ln>> {
         LayoutIterator(ServoChildrenIterator {
             current: self.first_child(),
@@ -290,54 +285,6 @@ impl<'le> GetLayoutData for ServoThreadSafeLayoutElement<'le> {
 }
 
 impl<'ln> ServoLayoutNode<'ln> {
-    fn dump_indent(self, indent: u32) {
-        let mut s = String::new();
-        for _ in 0..indent {
-            s.push_str("  ");
-        }
-
-        s.push_str(&self.debug_str());
-        println!("{}", s);
-
-        for kid in self.children() {
-            kid.dump_indent(indent + 1);
-        }
-    }
-
-    fn dump_style_indent(self, indent: u32) {
-        if self.is_element() {
-            let mut s = String::new();
-            for _ in 0..indent {
-                s.push_str("  ");
-            }
-            s.push_str(&self.debug_style_str());
-            println!("{}", s);
-        }
-
-        for kid in self.children() {
-            kid.dump_style_indent(indent + 1);
-        }
-    }
-
-    fn debug_str(self) -> String {
-        format!("{:?}: dirty_descendants={}",
-                self.script_type_id(),
-                self.as_element().map_or(false, |el| el.has_dirty_descendants()))
-    }
-
-    fn debug_style_str(self) -> String {
-        let maybe_element = self.as_element();
-        let maybe_data = match maybe_element {
-            Some(ref el) => el.borrow_data(),
-            None => None,
-        };
-        if let Some(data) = maybe_data {
-            format!("{:?}: {:?}", self.script_type_id(), &*data)
-        } else {
-            format!("{:?}: style_data=None", self.script_type_id())
-        }
-    }
-
     /// Returns the interior of this node as a `LayoutJS`. This is highly unsafe for layout to
     /// call and as such is marked `unsafe`.
     pub unsafe fn get_jsmanaged(&self) -> &LayoutJS<Node> {

components/style/build_gecko.rs

@@ -220,7 +220,7 @@ mod bindings {
             "mozilla::ConsumeStyleBehavior",
             "mozilla::LazyComputeBehavior",
             "mozilla::css::SheetParsingMode",
-            "mozilla::SkipRootBehavior",
+            "mozilla::TraversalRootBehavior",
             "mozilla::DisplayItemClip",  // Needed because bindgen generates
                                          // specialization tests for this even
                                          // though it shouldn't.
@@ -444,7 +444,7 @@ mod bindings {
             "ThreadSafePrincipalHolder",
             "ConsumeStyleBehavior",
             "LazyComputeBehavior",
-            "SkipRootBehavior",
+            "TraversalRootBehavior",
             "FontFamilyList",
             "FontFamilyType",
             "ServoElementSnapshot",

components/style/dom.rs

@@ -14,6 +14,7 @@ use parking_lot::RwLock;
 use properties::{ComputedValues, PropertyDeclarationBlock};
 use selector_parser::{ElementExt, PreExistingComputedValues, PseudoElement};
 use sink::Push;
+use std::fmt;
 use std::fmt::Debug;
 use std::sync::Arc;
 use stylist::ApplicableDeclarationBlock;
@@ -67,17 +68,13 @@ impl<T, I> Iterator for LayoutIterator<T> where T: Iterator<Item=I>, I: NodeInfo
     }
 }
 
-pub trait TNode : Sized + Copy + Clone + NodeInfo {
+pub trait TNode : Sized + Copy + Clone + Debug + NodeInfo {
     type ConcreteElement: TElement<ConcreteNode = Self>;
     type ConcreteChildrenIterator: Iterator<Item = Self>;
 
     fn to_unsafe(&self) -> UnsafeNode;
     unsafe fn from_unsafe(n: &UnsafeNode) -> Self;
 
-    fn dump(self);
-
-    fn dump_style(self);
-
     /// Returns an iterator over this node's children.
     fn children(self) -> LayoutIterator<Self::ConcreteChildrenIterator>;
 
@@ -103,6 +100,90 @@ pub trait TNode : Sized + Copy + Clone + NodeInfo {
     fn parent_node(&self) -> Option<Self>;
 }
 
+/// Wrapper to output the ElementData along with the node when formatting for
+/// Debug.
+pub struct ShowData<N: TNode>(pub N);
+impl<N: TNode> Debug for ShowData<N> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        fmt_with_data(f, self.0)
+    }
+}
+
+/// Wrapper to output the primary computed values along with the node when
+/// formatting for Debug. This is very verbose.
+pub struct ShowDataAndPrimaryValues<N: TNode>(pub N);
+impl<N: TNode> Debug for ShowDataAndPrimaryValues<N> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        fmt_with_data_and_primary_values(f, self.0)
+    }
+}
+
+/// Wrapper to output the subtree rather than the single node when formatting
+/// for Debug.
+pub struct ShowSubtree<N: TNode>(pub N);
+impl<N: TNode> Debug for ShowSubtree<N> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        try!(writeln!(f, "DOM Subtree:"));
+        fmt_subtree(f, &|f, n| write!(f, "{:?}", n), self.0, 1)
+    }
+}
+
+/// Wrapper to output the subtree along with the ElementData when formatting
+/// for Debug.
+pub struct ShowSubtreeData<N: TNode>(pub N);
+impl<N: TNode> Debug for ShowSubtreeData<N> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        try!(writeln!(f, "DOM Subtree:"));
+        fmt_subtree(f, &|f, n| fmt_with_data(f, n), self.0, 1)
+    }
+}
+
+/// Wrapper to output the subtree along with the ElementData and primary
+/// ComputedValues when formatting for Debug. This is extremely verbose.
+pub struct ShowSubtreeDataAndPrimaryValues<N: TNode>(pub N);
+impl<N: TNode> Debug for ShowSubtreeDataAndPrimaryValues<N> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        try!(writeln!(f, "DOM Subtree:"));
+        fmt_subtree(f, &|f, n| fmt_with_data_and_primary_values(f, n), self.0, 1)
+    }
+}
+
+fn fmt_with_data<N: TNode>(f: &mut fmt::Formatter, n: N) -> fmt::Result {
+    if let Some(el) = n.as_element() {
+        write!(f, "{:?} dd={} data={:?}", el, el.has_dirty_descendants(), el.borrow_data())
+    } else {
+        write!(f, "{:?}", n)
+    }
+}
+
+fn fmt_with_data_and_primary_values<N: TNode>(f: &mut fmt::Formatter, n: N) -> fmt::Result {
+    if let Some(el) = n.as_element() {
+        let dd = el.has_dirty_descendants();
+        let data = el.borrow_data();
+        let styles = data.as_ref().and_then(|d| d.get_styles());
+        let values = styles.map(|s| &s.primary.values);
+        write!(f, "{:?} dd={} data={:?} values={:?}", el, dd, &data, values)
+    } else {
+        write!(f, "{:?}", n)
+    }
+}
+
+fn fmt_subtree<F, N: TNode>(f: &mut fmt::Formatter, stringify: &F, n: N, indent: u32)
+                            -> fmt::Result
+    where F: Fn(&mut fmt::Formatter, N) -> fmt::Result
+{
+    for _ in 0..indent {
+        try!(write!(f, "  "));
+    }
+    try!(stringify(f, n));
+    for kid in n.children() {
+        try!(writeln!(f, ""));
+        try!(fmt_subtree(f, stringify, kid, indent + 1));
+    }
+
+    Ok(())
+}
+
 pub trait PresentationalHintsSynthetizer {
     fn synthesize_presentational_hints_for_legacy_attributes<V>(&self, hints: &mut V)
         where V: Push<ApplicableDeclarationBlock>;
@@ -163,10 +244,10 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
     /// traversal. Returns the number of children left to process.
     fn did_process_child(&self) -> isize;
 
-    /// Returns true if this element's style is display:none.
+    /// Returns true if this element's style is display:none. Panics if
+    /// the element has no style.
     fn is_display_none(&self) -> bool {
         let data = self.borrow_data().unwrap();
-        debug_assert!(data.has_current_styles());
         data.styles().is_display_none()
     }
 

components/style/gecko/wrapper.rs

@@ -50,6 +50,20 @@ use stylist::ApplicableDeclarationBlock;
 #[derive(Clone, Copy)]
 pub struct GeckoNode<'ln>(pub &'ln RawGeckoNode);
 
+impl<'ln> fmt::Debug for GeckoNode<'ln> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        if let Some(el) = self.as_element() {
+            el.fmt(f)
+        } else {
+            if self.is_text_node() {
+                write!(f, "<text node> ({:#x})", self.opaque().0)
+            } else {
+                write!(f, "<non-text node> ({:#x})", self.opaque().0)
+            }
+        }
+    }
+}
+
 impl<'ln> GeckoNode<'ln> {
     fn from_content(content: &'ln nsIContent) -> Self {
         GeckoNode(&content._base)
@@ -102,19 +116,6 @@ impl<'ln> TNode for GeckoNode<'ln> {
         GeckoNode(&*(n.0 as *mut RawGeckoNode))
     }
 
-    fn dump(self) {
-        if self.is_text_node() {
-            println!("Text ({:?})", &self.0 as *const _);
-        } else {
-            let el = self.as_element().unwrap();
-            println!("Element {} ({:?})", el.get_local_name(), &el.0 as *const _);
-        }
-    }
-
-    fn dump_style(self) {
-        unimplemented!()
-    }
-
     fn children(self) -> LayoutIterator<GeckoChildrenIterator<'ln>> {
         let maybe_iter = unsafe { Gecko_MaybeCreateStyleChildrenIterator(self.0) };
         if let Some(iter) = maybe_iter.into_owned_opt() {
@@ -211,7 +212,7 @@ impl<'le> fmt::Debug for GeckoElement<'le> {
         if let Some(id) = self.get_id() {
             try!(write!(f, " id={}", id));
         }
-        write!(f, "> ({:?})", self.0 as *const _)
+        write!(f, "> ({:#x})", self.as_node().opaque().0)
     }
 }
 

components/style/gecko_bindings/bindings.rs

@@ -70,7 +70,7 @@ use gecko_bindings::structs::ThreadSafeURIHolder;
 use gecko_bindings::structs::ThreadSafePrincipalHolder;
 use gecko_bindings::structs::ConsumeStyleBehavior;
 use gecko_bindings::structs::LazyComputeBehavior;
-use gecko_bindings::structs::SkipRootBehavior;
+use gecko_bindings::structs::TraversalRootBehavior;
 use gecko_bindings::structs::FontFamilyList;
 use gecko_bindings::structs::FontFamilyType;
 use gecko_bindings::structs::ServoElementSnapshot;
@@ -1197,7 +1197,7 @@ extern "C" {
 extern "C" {
     pub fn Servo_TraverseSubtree(root: RawGeckoElementBorrowed,
                                  set: RawServoStyleSetBorrowed,
-                                 skip_root: SkipRootBehavior);
+                                 behavior: TraversalRootBehavior);
 }
 extern "C" {
     pub fn Servo_AssertTreeIsClean(root: RawGeckoElementBorrowed);

components/style/gecko_bindings/structs_debug.rs

@@ -2493,7 +2493,7 @@ pub mod root {
         }
         #[repr(i32)]
         #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
-        pub enum SkipRootBehavior { Skip = 0, DontSkip = 1, }
+        pub enum TraversalRootBehavior { Normal = 0, UnstyledChildrenOnly = 1, }
         pub mod a11y {
             #[allow(unused_imports)]
             use self::super::super::super::root;

components/style/gecko_bindings/structs_release.rs

@@ -2475,7 +2475,7 @@ pub mod root {
         }
         #[repr(i32)]
         #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
-        pub enum SkipRootBehavior { Skip = 0, DontSkip = 1, }
+        pub enum TraversalRootBehavior { Normal = 0, UnstyledChildrenOnly = 1, }
         pub mod a11y {
             #[allow(unused_imports)]
             use self::super::super::super::root;

components/style/matching.rs

@@ -733,7 +733,6 @@ pub trait MatchMethods : TElement {
         // Get our parent's style.
         let parent_data = parent.as_ref().map(|x| x.borrow_data().unwrap());
         let parent_style = parent_data.as_ref().map(|d| {
-            debug_assert!(d.has_current_styles());
             &d.styles().primary.values
         });
 

components/style/parallel.rs

@@ -28,13 +28,17 @@ pub fn traverse_dom<N, C>(root: N::ConcreteElement,
         STYLE_SHARING_CACHE_MISSES.store(0, Ordering::SeqCst);
     }
 
-    // Handle root skipping. We don't currently support it in conjunction with
+    // Handle Gecko's eager initial styling. We don't currently support it
-    // bottom-up traversal. If we did, we'd need to put it on the context to make
+    // in conjunction with bottom-up traversal. If we did, we'd need to put
-    // it available to the bottom-up phase.
+    // it on the context to make it available to the bottom-up phase.
-    debug_assert!(!token.should_skip_root() || !C::needs_postorder_traversal());
+    let (nodes, depth) = if token.traverse_unstyled_children_only() {
-    let (nodes, depth) = if token.should_skip_root() {
+        debug_assert!(!C::needs_postorder_traversal());
         let mut children = vec![];
-        C::traverse_children(root, |kid| children.push(kid.to_unsafe()));
+        for kid in root.as_node().children() {
+            if kid.as_element().map_or(false, |el| el.get_data().is_none()) {
+                children.push(kid.to_unsafe());
+            }
+        }
         (children, known_root_dom_depth.map(|x| x + 1))
     } else {
         (vec![root.as_node().to_unsafe()], known_root_dom_depth)

components/style/sequential.rs

@@ -42,8 +42,12 @@ pub fn traverse_dom<N, C>(root: N::ConcreteElement,
     };
     let context = C::new(shared, root.as_node().opaque());
 
-    if token.should_skip_root() {
+    if token.traverse_unstyled_children_only() {
-        C::traverse_children(root, |kid| doit::<N, C>(&context, kid, &mut data));
+        for kid in root.as_node().children() {
+            if kid.as_element().map_or(false, |el| el.get_data().is_none()) {
+                doit::<N, C>(&context, kid, &mut data);
+            }
+        }
     } else {
         doit::<N, C>(&context, root.as_node(), &mut data);
     }

components/style/traversal.rs

@@ -104,7 +104,7 @@ pub struct PerLevelTraversalData {
 /// to pass information from the pre-traversal into the primary traversal.
 pub struct PreTraverseToken {
     traverse: bool,
-    skip_root: bool,
+    unstyled_children_only: bool,
 }
 
 impl PreTraverseToken {
@@ -112,8 +112,8 @@ impl PreTraverseToken {
         self.traverse
     }
 
-    pub fn should_skip_root(&self) -> bool {
+    pub fn traverse_unstyled_children_only(&self) -> bool {
-        self.skip_root
+        self.unstyled_children_only
     }
 }
 
@@ -140,16 +140,16 @@ pub trait DomTraversalContext<N: TNode> {
     /// a traversal is needed. Returns a token that allows the caller to prove
     /// that the call happened.
     ///
-    /// The skip_root parameter is used in Gecko to style newly-appended children
+    /// The unstyled_children_only parameter is used in Gecko to style newly-
-    /// without restyling the parent.
+    /// appended children without restyling the parent.
-    fn pre_traverse(root: N::ConcreteElement, stylist: &Stylist, skip_root: bool)
+    fn pre_traverse(root: N::ConcreteElement, stylist: &Stylist,
+                    unstyled_children_only: bool)
                     -> PreTraverseToken
     {
-        // If we should skip the root, traverse unconditionally.
+        if unstyled_children_only {
-        if skip_root {
             return PreTraverseToken {
                 traverse: true,
-                skip_root: true,
+                unstyled_children_only: true,
             };
         }
 
@@ -157,18 +157,18 @@ pub trait DomTraversalContext<N: TNode> {
         // we need a special case for the root.
         //
         // Expanding snapshots here may create a LATER_SIBLINGS restyle hint, which
-        // we will drop on the floor. This is fine, because we don't traverse roots
+        // we will drop on the floor. To prevent missed restyles, we assert against
-        // with siblings.
+        // restyling a root with later siblings.
-        debug_assert!(root.next_sibling_element().is_none());
         if let Some(mut data) = root.mutate_data() {
             if let Some(r) = data.as_restyle_mut() {
+                debug_assert!(root.next_sibling_element().is_none());
                 let _later_siblings = r.expand_snapshot(root, stylist);
             }
         }
 
         PreTraverseToken {
             traverse: Self::node_needs_traversal(root.as_node()),
-            skip_root: false,
+            unstyled_children_only: false,
         }
     }
 

ports/geckolib/glue.rs

@@ -18,7 +18,7 @@ use style::arc_ptr_eq;
 use style::atomic_refcell::AtomicRefMut;
 use style::context::{LocalStyleContextCreationInfo, ReflowGoal, SharedStyleContext};
 use style::data::{ElementData, RestyleData};
-use style::dom::{TElement, TNode};
+use style::dom::{ShowSubtreeData, TElement, TNode};
 use style::error_reporting::StdoutErrorReporter;
 use style::gecko::context::StandaloneStyleContext;
 use style::gecko::context::clear_local_context;
@@ -118,7 +118,7 @@ fn create_shared_context(mut per_doc_data: &mut AtomicRefMut<PerDocumentStyleDat
 }
 
 fn traverse_subtree(element: GeckoElement, raw_data: RawServoStyleSetBorrowed,
-                    skip_root: bool) {
+                    unstyled_children_only: bool) {
     // Force the creation of our lazily-constructed initial computed values on
     // the main thread, since it's not safe to call elsewhere.
     //
@@ -139,12 +139,15 @@ fn traverse_subtree(element: GeckoElement, raw_data: RawServoStyleSetBorrowed,
 
     let mut per_doc_data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
 
-    let token = RecalcStyleOnly::pre_traverse(element, &per_doc_data.stylist, skip_root);
+    let token = RecalcStyleOnly::pre_traverse(element, &per_doc_data.stylist, unstyled_children_only);
     if !token.should_traverse() {
         error!("Unnecessary call to traverse_subtree");
         return;
     }
 
+    debug!("Traversing subtree:");
+    debug!("{:?}", ShowSubtreeData(element.as_node()));
+
     let shared_style_context = create_shared_context(&mut per_doc_data);
     let known_depth = None;
 
@@ -160,10 +163,11 @@ fn traverse_subtree(element: GeckoElement, raw_data: RawServoStyleSetBorrowed,
 #[no_mangle]
 pub extern "C" fn Servo_TraverseSubtree(root: RawGeckoElementBorrowed,
                                         raw_data: RawServoStyleSetBorrowed,
-                                        skip_root: structs::SkipRootBehavior) -> () {
+                                        behavior: structs::TraversalRootBehavior) -> () {
     let element = GeckoElement(root);
     debug!("Servo_TraverseSubtree: {:?}", element);
-    traverse_subtree(element, raw_data, skip_root == structs::SkipRootBehavior::Skip);
+    traverse_subtree(element, raw_data,
+                     behavior == structs::TraversalRootBehavior::UnstyledChildrenOnly);
 }
 
 #[no_mangle]