Mirrors/servo
1
0
Fork 0
mirror of https://github.com/servo/servo.git synced 2025-08-06 22:15:33 +01:00
Code Issues Projects Releases Packages Wiki Activity

rule tree: Remove unsound transmute()

Browse source 
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.
This commit is contained in:
Simon Sapin 2016-11-04 15:47:10 +01:00
parent de4fe6e2b6
commit 98bd99b74c
6 changed files with 58 additions and 176 deletions
Download patch file Download diff file Expand all files Collapse all files

5
components/style/animation.rs
View file

@ -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,

83
components/style/declarations_iterators.rs
View file

@ -1,83 +0,0 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
//! A set of simple iterators to be reused for doing the cascade.
use properties::{Importance, PropertyDeclaration};
/// An iterator that applies the declarations in a list that matches the given
/// importance.
#[derive(Clone)]
pub struct SimpleDeclarationsIterator<'a> {
declarations: &'a [(PropertyDeclaration, Importance)],
importance: Importance,
index: usize,
}
impl<'a> SimpleDeclarationsIterator<'a> {
pub fn new(declarations: &'a [(PropertyDeclaration, Importance)],
importance: Importance) -> Self {
SimpleDeclarationsIterator {
declarations: declarations,
importance: importance,
index: 0,
}
}
}
impl<'a> Iterator for SimpleDeclarationsIterator<'a> {
type Item = &'a PropertyDeclaration;
fn next(&mut self) -> Option<Self::Item> {
loop {
if self.index == self.declarations.len() {
return None;
}
let (ref decl, importance) =
self.declarations[self.declarations.len() - self.index - 1];
self.index += 1;
if importance == self.importance {
return Some(decl)
}
}
}
}
/// An iterator that applies the declarations in a list without checking any
/// order.
#[derive(Clone)]
pub struct RawDeclarationsIterator<'a> {
declarations: &'a [(PropertyDeclaration, Importance)],
index: usize,
}
impl<'a> RawDeclarationsIterator<'a> {
pub fn new(declarations: &'a [(PropertyDeclaration, Importance)]) -> Self {
RawDeclarationsIterator {
declarations: declarations,
index: 0,
}
}
}
impl<'a> Iterator for RawDeclarationsIterator<'a> {
type Item = &'a PropertyDeclaration;
fn next(&mut self) -> Option<Self::Item> {
if self.index == self.declarations.len() {
return None;
}
let (ref decl, _) =
self.declarations[self.declarations.len() - self.index - 1];
self.index += 1;
return Some(decl)
}
}

1
components/style/lib.rs
View file

@ -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;

48
components/style/properties/properties.mako.rs
View file

@ -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
}

90
components/style/rule_tree/mod.rs
View file

@ -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,
#[derive(Clone)]
pub struct SelfAndAncestors<'a> {
current: Option<&'a StrongRuleNode>,
}
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,
}
}
}
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
})
}
}

7
ports/geckolib/glue.rs
View file

@ -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)),