rule tree: Remove unsound transmute()

RuleTreeDeclarationsIterator would yield &PropertyDeclaration
with the lifetime of the rule tree, but the reference would only
be valid as long as the iterator was holding the corresponding lock.

The lock would be released when the iterator’s `.next()` method
moves to the next rule tree node, or when  the iterator is dropped.

Also change apply_declaration to not require a `Clone` iterator
(since closures unfortunately don’t implement `Clone`).
Instead have it take a callable that returns a fresh iterator.

components/style/animation.rs

@@ -7,7 +7,6 @@
 use Atom;
 use bezier::Bezier;
 use context::SharedStyleContext;
-use declarations_iterators::RawDeclarationsIterator;
 use dom::{OpaqueNode, UnsafeNode};
 use euclid::point::Point2D;
 use keyframes::{KeyframesStep, KeyframesStepValue};
@@ -393,7 +392,9 @@ fn compute_style_for_animation_step(context: &SharedStyleContext,
             debug_assert!(guard.declarations.iter()
                             .all(|&(_, importance)| importance == Importance::Normal));
 
-            let iter = RawDeclarationsIterator::new(&guard.declarations);
+            let iter = || {
+                guard.declarations.iter().rev().map(|&(ref decl, _importance)| decl)
+            };
 
             let computed =
                 properties::apply_declarations(context.viewport_size,

components/style/lib.rs

@@ -99,7 +99,6 @@ pub mod cascade_info;
 pub mod context;
 pub mod custom_properties;
 pub mod data;
-pub mod declarations_iterators;
 pub mod dom;
 pub mod element_state;
 pub mod error_reporting;

components/style/properties/properties.mako.rs

@@ -1434,9 +1434,28 @@ pub fn cascade(viewport_size: Size2D<Au>,
         Some(parent_style) => (false, parent_style),
         None => (true, ComputedValues::initial_values()),
     };
+    // Hold locks until after the apply_declarations() call returns.
+    // Use filter_map because the root node has no style source.
+    let lock_guards = rule_node.self_and_ancestors().filter_map(|node| {
+        node.style_source().map(|source| (source.read(), node.importance()))
+    }).collect::<Vec<_>>();
+    let iter_declarations = || {
+        lock_guards.iter().flat_map(|&(ref source, source_importance)| {
+            source.declarations.iter()
+            // Yield declarations later in source order (with more precedence) first.
+            .rev()
+            .filter_map(move |&(ref declaration, declaration_importance)| {
+                if declaration_importance == source_importance {
+                    Some(declaration)
+                } else {
+                    None
+                }
+            })
+        })
+    };
     apply_declarations(viewport_size,
                        is_root_element,
-                       rule_node.iter_declarations(),
+                       iter_declarations,
                        inherited_style,
                        cascade_info,
                        error_reporter,
@@ -1445,20 +1464,20 @@ pub fn cascade(viewport_size: Size2D<Au>,
 
 /// NOTE: This function expects the declaration with more priority to appear
 /// first.
-pub fn apply_declarations<'a, I>(viewport_size: Size2D<Au>,
-                                 is_root_element: bool,
-                                 declarations_iter: I,
-                                 inherited_style: &ComputedValues,
-                                 mut cascade_info: Option<<&mut CascadeInfo>,
-                                 mut error_reporter: StdBox<ParseErrorReporter + Send>,
-                                 flags: CascadeFlags)
-                                 -> ComputedValues
-    where I: Iterator<Item = &'a PropertyDeclaration> + Clone
+pub fn apply_declarations<'a, F, I>(viewport_size: Size2D<Au>,
+                                    is_root_element: bool,
+                                    iter_declarations: F,
+                                    inherited_style: &ComputedValues,
+                                    mut cascade_info: Option<<&mut CascadeInfo>,
+                                    mut error_reporter: StdBox<ParseErrorReporter + Send>,
+                                    flags: CascadeFlags)
+                                    -> ComputedValues
+    where F: Fn() -> I, I: Iterator<Item = &'a PropertyDeclaration>
 {
     let inherited_custom_properties = inherited_style.custom_properties();
     let mut custom_properties = None;
     let mut seen_custom = HashSet::new();
-    for declaration in declarations_iter.clone() {
+    for declaration in iter_declarations() {
         match *declaration {
             PropertyDeclaration::Custom(ref name, ref value) => {
                 ::custom_properties::cascade(
@@ -1521,12 +1540,7 @@ pub fn apply_declarations<'a, I>(viewport_size: Size2D<Au>,
     // virtual dispatch instead.
     ComputedValues::do_cascade_property(|cascade_property| {
         % for category_to_cascade_now in ["early", "other"]:
-            % if category_to_cascade_now == "early":
-            for declaration in declarations_iter.clone() {
-            % else:
-            for declaration in declarations_iter {
-            % endif
-
+            for declaration in iter_declarations() {
                 if let PropertyDeclaration::Custom(..) = *declaration {
                     continue
                 }

components/style/rule_tree/mod.rs

@@ -10,7 +10,7 @@ use heapsize::HeapSizeOf;
 use owning_handle::OwningHandle;
 use owning_ref::{ArcRef, OwningRef};
 use parking_lot::{RwLock, RwLockReadGuard};
-use properties::{Importance, PropertyDeclaration, PropertyDeclarationBlock};
+use properties::{Importance, PropertyDeclarationBlock};
 use std::io::{self, Write};
 use std::ptr;
 use std::sync::Arc;
@@ -382,13 +382,17 @@ impl StrongRuleNode {
         unsafe { &*self.ptr }
     }
 
-    pub fn iter_declarations<'a>(&'a self) -> RuleTreeDeclarationsIterator<'a> {
-        RuleTreeDeclarationsIterator {
-            current: match self.get().source.as_ref() {
-                Some(ref source) => Some((self, source.read())),
-                None => None,
-            },
-            index: 0,
+    pub fn style_source(&self) -> Option<&StyleSource> {
+        self.get().source.as_ref()
+    }
+
+    pub fn importance(&self) -> Importance {
+        self.get().importance
+    }
+
+    pub fn self_and_ancestors(&self) -> SelfAndAncestors {
+        SelfAndAncestors {
+            current: Some(self)
         }
     }
 
@@ -443,71 +447,19 @@ impl StrongRuleNode {
     }
 }
 
-pub struct RuleTreeDeclarationsIterator<'a> {
-    current: Option<(&'a StrongRuleNode, StyleSourceGuard<'a>)>,
-    index: usize,
-}
-
-impl<'a> Clone for RuleTreeDeclarationsIterator<'a> {
-    fn clone(&self) -> Self {
-        RuleTreeDeclarationsIterator {
-            current: self.current.as_ref().map(|&(node, _)| {
-                (&*node, node.get().source.as_ref().unwrap().read())
-            }),
-            index: self.index,
-        }
-    }
+#[derive(Clone)]
+pub struct SelfAndAncestors<'a> {
+    current: Option<&'a StrongRuleNode>,
 }
 
-impl<'a> Iterator for RuleTreeDeclarationsIterator<'a> {
-    type Item = &'a PropertyDeclaration;
+impl<'a> Iterator for SelfAndAncestors<'a> {
+    type Item = &'a StrongRuleNode;
 
     fn next(&mut self) -> Option<Self::Item> {
-        let (node, guard) = match self.current.take() {
-            Some(node_and_guard) => node_and_guard,
-            None => return None,
-        };
-
-        let declaration_count = guard.declarations.len();
-
-        if self.index == declaration_count {
-            let parent = node.parent().unwrap();
-            let guard = match parent.get().source {
-                Some(ref source) => source.read(),
-                None => {
-                    debug_assert!(parent.parent().is_none());
-                    self.current = None;
-                    return None;
-                }
-            };
-
-            self.current = Some((parent, guard));
-            self.index = 0;
-            // FIXME: Make this a loop and use continue, the borrow checker
-            // isn't too happy about it for some reason.
-            return self.next();
-        }
-
-        let (decl, importance) = {
-            let (ref decl, importance) =
-                guard.declarations[declaration_count - self.index - 1];
-
-            // FIXME: The borrow checker is not smart enough to see that the
-            // guard is moved below and that affects the lifetime of `decl`,
-            // plus `decl` has a stable address (because it's in an Arc), so we
-            // need to transmute here.
-            let decl: &'a PropertyDeclaration = unsafe { ::std::mem::transmute(decl) };
-
-            (decl, importance)
-        };
-
-        self.current = Some((node, guard));
-        self.index += 1;
-        if importance == node.get().importance {
-            return Some(decl);
-        }
-        // FIXME: Same as above, this is crap...
-        return self.next();
+        self.current.map(|node| {
+            self.current = node.parent();
+            node
+        })
     }
 }
 

ports/geckolib/glue.rs

@@ -131,15 +131,14 @@ pub extern "C" fn Servo_RestyleWithAddedDeclaration(declarations: RawServoDeclar
                                                     previous_style: ServoComputedValuesBorrowed)
   -> ServoComputedValuesStrong
 {
-    use style::declarations_iterators::RawDeclarationsIterator;
-
     let previous_style = ComputedValues::as_arc(&previous_style);
     let declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations);
 
     let guard = declarations.read();
 
-    let declarations =
-        RawDeclarationsIterator::new(&guard.declarations);
+    let declarations = || {
+        guard.declarations.iter().rev().map(|&(ref decl, _importance)| decl)
+    };
 
     // FIXME (bug 1303229): Use the actual viewport size here
     let computed = apply_declarations(Size2D::new(Au(0), Au(0)),