layout: Replace AbstractNodes in layout with OpaqueNode, preventing

DOM access during reflow.

src/components/main/layout/box.rs

@@ -40,6 +40,7 @@ use layout::float_context::{ClearType, ClearLeft, ClearRight, ClearBoth};
 use layout::flow::Flow;
 use layout::flow;
 use layout::model::{MaybeAuto, specified};
+use layout::util::OpaqueNode;
 
 /// Boxes (`struct Box`) are the leaves of the layout tree. They cannot position themselves. In
 /// general, boxes do not have a simple correspondence with CSS boxes in the specification:
@@ -63,8 +64,8 @@ use layout::model::{MaybeAuto, specified};
 /// FIXME(pcwalton): This can be slimmed down quite a bit.
 #[deriving(Clone)]
 pub struct Box {
-    /// The DOM node that this `Box` originates from.
+    /// An opaque reference to the DOM node that this `Box` originates from.
-    node: AbstractNode<LayoutView>,
+    node: OpaqueNode,
 
     /// The CSS style of this box.
     style: Arc<ComputedValues>,
@@ -250,7 +251,7 @@ impl Box {
         }
 
         Box {
-            node: node,
+            node: OpaqueNode::from_node(&node),
             style: (*nearest_ancestor_element.style()).clone(),
             position: Slot::init(Au::zero_rect()),
             border: Slot::init(Zero::zero()),
@@ -288,7 +289,7 @@ impl Box {
     /// CSS 2.1.
     fn guess_width(&self) -> Au {
         match self.specific {
-            GenericBox | ImageBox(_) => {}
+            GenericBox | IframeBox(_) | ImageBox(_) => {}
             ScannedTextBox(_) | UnscannedTextBox(_) => return Au(0),
         }
 

src/components/main/layout/display_list_builder.rs

@@ -6,26 +6,20 @@
 
 use layout::box::Box;
 use layout::context::LayoutContext;
-use std::cast::transmute;
+use layout::util::OpaqueNode;
-use script::dom::node::AbstractNode;
 
 use gfx;
 use style;
 
-/// Display list data is usually an AbstractNode with view () to indicate
-/// that nodes in this view shoud not really be touched. The idea is to
-/// store the nodes in the display list and have layout transmute them.
 pub trait ExtraDisplayListData {
     fn new(box: &Box) -> Self;
 }
 
 pub type Nothing = ();
 
-impl ExtraDisplayListData for AbstractNode<()> {
+impl ExtraDisplayListData for OpaqueNode {
-    fn new(box: &Box) -> AbstractNode<()> {
+    fn new(box: &Box) -> OpaqueNode {
-        unsafe {
+        box.node
-            transmute(box.node)
-        }
     }
 }
 

src/components/main/layout/layout_task.rs

@@ -16,7 +16,7 @@ use layout::flow::{Flow, ImmutableFlowUtils, MutableFlowUtils, PreorderFlowTrave
 use layout::flow::{PostorderFlowTraversal};
 use layout::flow;
 use layout::incremental::{RestyleDamage};
-use layout::util::{LayoutData, LayoutDataAccess};
+use layout::util::{LayoutData, LayoutDataAccess, OpaqueNode};
 
 use extra::arc::{Arc, RWArc, MutexArc};
 use geom::point::Point2D;
@@ -69,7 +69,7 @@ struct LayoutTask {
     script_chan: ScriptChan,
 
     /// The channel on which messages can be sent to the painting task.
-    render_chan: RenderChan<AbstractNode<()>>,
+    render_chan: RenderChan<OpaqueNode>,
 
     /// The channel on which messages can be sent to the image cache.
     image_cache_task: ImageCacheTask,
@@ -81,7 +81,7 @@ struct LayoutTask {
     screen_size: Option<Size2D<Au>>,
 
     /// A cached display list.
-    display_list: Option<Arc<DisplayList<AbstractNode<()>>>>,
+    display_list: Option<Arc<DisplayList<OpaqueNode>>>,
 
     stylist: RWArc<Stylist>,
 
@@ -204,7 +204,7 @@ impl LayoutTask {
                   port: Port<Msg>,
                   constellation_chan: ConstellationChan,
                   script_chan: ScriptChan,
-                  render_chan: RenderChan<AbstractNode<()>>,
+                  render_chan: RenderChan<OpaqueNode>,
                   img_cache_task: ImageCacheTask,
                   opts: Opts,
                   profiler_chan: ProfilerChan,
@@ -232,7 +232,7 @@ impl LayoutTask {
            port: Port<Msg>,
            constellation_chan: ConstellationChan,
            script_chan: ScriptChan,
-           render_chan: RenderChan<AbstractNode<()>>, 
+           render_chan: RenderChan<OpaqueNode>, 
            image_cache_task: ImageCacheTask,
            opts: &Opts,
            profiler_chan: ProfilerChan)
@@ -478,34 +478,36 @@ impl LayoutTask {
         if data.goal == ReflowForDisplay {
             do profile(time::LayoutDispListBuildCategory, self.profiler_chan.clone()) {
                 let root_size = flow::base(layout_root).position.size;
-                let display_list= ~Cell::new(DisplayList::<AbstractNode<()>>::new());
+                let display_list = ~Cell::new(DisplayList::<OpaqueNode>::new());
                 let dirty = flow::base(layout_root).position.clone();
-                layout_root.build_display_list(
+                let display_list_builder = DisplayListBuilder {
-                    &DisplayListBuilder {
+                    ctx: &layout_ctx,
-                        ctx: &layout_ctx,
+                };
-                    },
+                layout_root.build_display_list(&display_list_builder, &dirty, display_list);
-                    &dirty,
-                    display_list);
 
                 let display_list = Arc::new(display_list.take());
 
-                    let mut color = color::rgba(255.0, 255.0, 255.0, 255.0);
+                let mut color = color::rgba(255.0, 255.0, 255.0, 255.0);
 
-                    for child in node.traverse_preorder() {
+                for child in node.traverse_preorder() {
-                      if child.type_id() == ElementNodeTypeId(HTMLHtmlElementTypeId) || 
+                    if child.type_id() == ElementNodeTypeId(HTMLHtmlElementTypeId) || 
-                         child.type_id() == ElementNodeTypeId(HTMLBodyElementTypeId) {
+                            child.type_id() == ElementNodeTypeId(HTMLBodyElementTypeId) {
-                             let element_bg_color = child.style().get().resolve_color(
+                        let element_bg_color = child.style()
-                                 child.style().get().Background.background_color
+                                                    .get()
-                             ).to_gfx_color();
+                                                    .resolve_color(child.style()
-                             match element_bg_color {
+                                                                        .get()
-                                 color::rgba(0., 0., 0., 0.) => {}
+                                                                        .Background
-                                 _ => {
+                                                                        .background_color)
-                                   color = element_bg_color;
+                                                    .to_gfx_color();
-                                   break;
+                        match element_bg_color {
-                               }
+                            color::rgba(0., 0., 0., 0.) => {}
-                             }
+                            _ => {
-                      }
+                                color = element_bg_color;
+                                break;
+                           }
+                        }
                     }
+                }
 
                 let render_layer = RenderLayer {
                     display_list: display_list.clone(),
@@ -532,16 +534,15 @@ impl LayoutTask {
     /// `getClientRects()` or `getBoundingClientRect()` ultimately invoke.
     fn handle_query(&self, query: LayoutQuery) {
         match query {
+            // The neat thing here is that in order to answer the following two queries we only
+            // need to compare nodes for equality. Thus we can safely work only with `OpaqueNode`.
             ContentBoxQuery(node, reply_chan) => {
-                // FIXME: Isolate this transmutation into a single "bridge" module.
+                let node = OpaqueNode::from_node(&node);
-                let node: AbstractNode<()> = unsafe {
-                    transmute(node)
-                };
 
                 fn union_boxes_for_node<'a>(
                                         accumulator: &mut Option<Rect<Au>>,
-                                        mut iter: DisplayItemIterator<'a,AbstractNode<()>>,
+                                        mut iter: DisplayItemIterator<'a,OpaqueNode>,
-                                        node: AbstractNode<()>) {
+                                        node: OpaqueNode) {
                     for item in iter {
                         union_boxes_for_node(accumulator, item.children(), node);
                         if item.base().extra == node {
@@ -559,15 +560,12 @@ impl LayoutTask {
                 reply_chan.send(ContentBoxResponse(rect.unwrap_or(Au::zero_rect())))
             }
             ContentBoxesQuery(node, reply_chan) => {
-                // FIXME: Isolate this transmutation into a single "bridge" module.
+                let node = OpaqueNode::from_node(&node);
-                let node: AbstractNode<()> = unsafe {
-                    transmute(node)
-                };
 
                 fn add_boxes_for_node<'a>(
                                       accumulator: &mut ~[Rect<Au>],
-                                      mut iter: DisplayItemIterator<'a,AbstractNode<()>>,
+                                      mut iter: DisplayItemIterator<'a,OpaqueNode>,
-                                      node: AbstractNode<()>) {
+                                      node: OpaqueNode) {
                     for item in iter {
                         add_boxes_for_node(accumulator, item.children(), node);
                         if item.base().extra == node {
@@ -582,7 +580,7 @@ impl LayoutTask {
                 reply_chan.send(ContentBoxesResponse(boxes))
             }
             HitTestQuery(_, point, reply_chan) => {
-                fn hit_test(x: Au, y: Au, list: &[DisplayItem<AbstractNode<()>>])
+                fn hit_test(x: Au, y: Au, list: &[DisplayItem<OpaqueNode>])
                             -> Option<HitTestResponse> {
                     for item in list.rev_iter() {
                         match *item {
@@ -602,13 +600,19 @@ impl LayoutTask {
                             _ => {}
                         }
                         let bounds = item.bounds();
-                        // TODO this check should really be performed by a method of DisplayItem
+
+                        // TODO(tikue): This check should really be performed by a method of
+                        // DisplayItem.
                         if x < bounds.origin.x + bounds.size.width &&
-                            bounds.origin.x <= x &&
+                                bounds.origin.x <= x &&
-                            y < bounds.origin.y + bounds.size.height &&
+                                y < bounds.origin.y + bounds.size.height &&
-                            bounds.origin.y <= y {
+                                bounds.origin.y <= y {
+                            // FIXME(pcwalton): This `unsafe` block is too unsafe, since incorrect
+                            // incremental flow construction could create this. Paranoid validation
+                            // against the set of valid nodes should occur in the script task to
+                            // ensure that this is a valid address instead of transmuting here.
                             let node: AbstractNode<LayoutView> = unsafe {
-                                transmute(item.base().extra)
+                                item.base().extra.to_node()
                             };
                             let resp = Some(HitTestResponse(node));
                             return resp;

src/components/main/layout/util.rs

@@ -12,6 +12,7 @@ use servo_util::slot::{MutSlotRef, SlotRef};
 use servo_util::tree::TreeNodeRef;
 use std::cast;
 use std::iter::Enumerate;
+use std::libc::uintptr_t;
 use std::vec::VecIterator;
 use style::{ComputedValues, PropertyDeclaration};
 
@@ -182,3 +183,35 @@ impl LayoutDataAccess for AbstractNode<LayoutView> {
     }
 }
 
+/// An opaque handle to a node. The only safe operation that can be performed on this node is to
+/// compare it to another opaque handle or to another node.
+///
+/// Because the script task's GC does not trace layout, node data cannot be safely stored in layout
+/// data structures. Also, layout code tends to be faster when the DOM is not being accessed, for
+/// locality reasons. Using `OpaqueNode` enforces this invariant.
+#[deriving(Clone, Eq)]
+pub struct OpaqueNode(uintptr_t);
+
+impl<T> Equiv<AbstractNode<T>> for OpaqueNode {
+    fn equiv(&self, node: &AbstractNode<T>) -> bool {
+        unsafe {
+            **self == cast::transmute_copy::<AbstractNode<T>,uintptr_t>(node)
+        }
+    }
+}
+
+impl OpaqueNode {
+    /// Converts a DOM node to an `OpaqueNode`.
+    pub fn from_node<T>(node: &AbstractNode<T>) -> OpaqueNode {
+        unsafe {
+            OpaqueNode(cast::transmute_copy(node))
+        }
+    }
+
+    /// Unsafely converts an `OpaqueNode` to a DOM node. Use this only if you absolutely know what
+    /// you're doing.
+    pub unsafe fn to_node<T>(&self) -> AbstractNode<T> {
+        cast::transmute(**self)
+    }
+}
+

src/components/main/pipeline.rs

@@ -9,7 +9,7 @@ use extra::url::Url;
 use gfx::opts::Opts;
 use gfx::render_task::{PaintPermissionGranted, PaintPermissionRevoked};
 use gfx::render_task::{RenderChan, RenderTask};
-use script::dom::node::AbstractNode;
+use layout::util::OpaqueNode;
 use script::layout_interface::LayoutChan;
 use script::script_task::LoadMsg;
 use script::script_task::{AttachLayoutMsg, NewLayoutInfo, ScriptTask, ScriptChan};
@@ -27,19 +27,19 @@ pub struct Pipeline {
     subpage_id: Option<SubpageId>,
     script_chan: ScriptChan,
     layout_chan: LayoutChan,
-    render_chan: RenderChan<AbstractNode<()>>,
+    render_chan: RenderChan<OpaqueNode>,
     layout_shutdown_port: Port<()>,
     render_shutdown_port: Port<()>,
     /// The most recently loaded url
     url: Option<Url>,
 }
 
-/// A subset of the Pipeline nthat is eeded for layer composition
+/// The subset of the pipeline that is needed for layer composition.
 #[deriving(Clone)]
 pub struct CompositionPipeline {
     id: PipelineId,
     script_chan: ScriptChan,
-    render_chan: RenderChan<AbstractNode<()>>,
+    render_chan: RenderChan<OpaqueNode>,
 }
 
 impl Pipeline {
@@ -182,7 +182,7 @@ impl Pipeline {
                subpage_id: Option<SubpageId>,
                script_chan: ScriptChan,
                layout_chan: LayoutChan,
-               render_chan: RenderChan<AbstractNode<()>>,
+               render_chan: RenderChan<OpaqueNode>,
                layout_shutdown_port: Port<()>,
                render_shutdown_port: Port<()>)
                -> Pipeline {