Embed layout data directly in Node

This eliminates

    layout_data: Option<@mut ()>

and the unsafe casting around it, which was causing crashes on exit.

Fixes #762.

src/components/main/css/node_util.rs

@@ -2,10 +2,10 @@
  * 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/. */
 
-use layout::aux::LayoutAuxMethods;
 use layout::incremental::RestyleDamage;
 
-use std::cast::transmute;
+use std::cast;
+use std::cell::Cell;
 use newcss::complete::CompleteSelectResults;
 use script::dom::node::{AbstractNode, LayoutView};
 
@@ -31,15 +31,20 @@ impl<'self> NodeUtil<'self> for AbstractNode<LayoutView> {
             fail!(~"style() called on a node without aux data!");
         }
 
-        match self.layout_data().style {
-            None => fail!(~"style() called on node without a style!"),
-            Some(ref style) => unsafe { transmute(style) }
+        do self.read_layout_data |layout_data| {
+            match layout_data.style {
+                None => fail!(~"style() called on node without a style!"),
+                Some(ref style) => unsafe { cast::transmute_region(style) }
+            }
         }
     }
 
     /// Does this node have a computed style yet?
     fn have_css_select_results(self) -> bool {
-        self.has_layout_data() && self.layout_data().style.is_some()
+        if !self.has_layout_data() {
+            return false;
+        }
+        self.read_layout_data(|data| data.style.is_some())
     }
 
     /// Update the computed style of an HTML element with a style specified by CSS.
@@ -48,7 +53,8 @@ impl<'self> NodeUtil<'self> for AbstractNode<LayoutView> {
             fail!(~"set_css_select_results() called on a node without aux data!");
         }
 
-        self.layout_data().style = Some(decl);
+        let cell = Cell::new(decl);
+        self.write_layout_data(|data| data.style = Some(cell.take()));
     }
 
     /// Get the description of how to account for recent style changes.
@@ -65,7 +71,11 @@ impl<'self> NodeUtil<'self> for AbstractNode<LayoutView> {
         if !self.has_layout_data() {
             return default;
         }
-        self.layout_data().restyle_damage.unwrap_or_default(default)
+        do self.read_layout_data |layout_data| {
+            layout_data.restyle_damage
+                .map(|&x| RestyleDamage::from_int(x))
+                .unwrap_or_default(default)
+        }
     }
 
     /// Set the restyle damage field.
@@ -74,6 +84,6 @@ impl<'self> NodeUtil<'self> for AbstractNode<LayoutView> {
             fail!(~"set_restyle_damage() called on a node without aux data!");
         }
 
-        self.layout_data().restyle_damage = Some(damage);
+        self.write_layout_data(|data| data.restyle_damage = Some(damage.to_int()));
     }
 }

src/components/main/layout/aux.rs

@@ -4,94 +4,34 @@
 
 //! Code for managing the layout data in the DOM.
 
-use layout::incremental::RestyleDamage;
-use gfx::display_list::DisplayList;
-use servo_util::range::Range;
-
-use extra::arc::Arc;
-use newcss::complete::CompleteSelectResults;
-use script::dom::node::{AbstractNode, LayoutView};
+use script::dom::node::{AbstractNode, LayoutView, LayoutData};
 use servo_util::tree::TreeNodeRef;
 
-pub struct DisplayBoxes {
-    display_list: Option<Arc<DisplayList<AbstractNode<()>>>>,
-    range: Option<Range>,
-}
-
-/// Data that layout associates with a node.
-pub struct LayoutData {
-    /// The results of CSS styling for this node.
-    style: Option<CompleteSelectResults>,
-
-    /// Description of how to account for recent style changes.
-    restyle_damage: Option<RestyleDamage>,
-
-    /// The boxes assosiated with this flow.
-    /// Used for getBoundingClientRect and friends.
-    boxes: DisplayBoxes,
-}
-
-impl LayoutData {
-    /// Creates new layout data.
-    pub fn new() -> LayoutData {
-        LayoutData {
-            style: None,
-            restyle_damage: None,
-            boxes: DisplayBoxes { display_list: None, range: None },
-        }
-    }
-}
-
 /// Functionality useful for querying the layout-specific data on DOM nodes.
 pub trait LayoutAuxMethods {
-    fn layout_data(self) -> @mut LayoutData;
-    fn has_layout_data(self) -> bool;
-    fn set_layout_data(self, data: @mut LayoutData);
-
-    fn initialize_layout_data(self) -> Option<@mut LayoutData>;
-    fn initialize_style_for_subtree(self, refs: &mut ~[@mut LayoutData]);
+    fn initialize_layout_data(self);
+    fn initialize_style_for_subtree(self);
 }
 
 impl LayoutAuxMethods for AbstractNode<LayoutView> {
-    fn layout_data(self) -> @mut LayoutData {
-        unsafe {
-            self.unsafe_layout_data()
-        }
-    }
-    fn has_layout_data(self) -> bool {
-       unsafe {
-            self.unsafe_has_layout_data()
-        }
-    }
-    fn set_layout_data(self, data: @mut LayoutData) {
-        unsafe {
-            self.unsafe_set_layout_data(data)
-        }
-    }
-
     /// If none exists, creates empty layout data for the node (the reader-auxiliary
     /// box in the COW model) and populates it with an empty style object.
-    fn initialize_layout_data(self) -> Option<@mut LayoutData> {
+    fn initialize_layout_data(self) {
         if self.has_layout_data() {
-            self.layout_data().boxes.display_list = None;
-            self.layout_data().boxes.range = None;
-            None
+            do self.write_layout_data |data| {
+                data.boxes.display_list = None;
+                data.boxes.range = None;
+            }
         } else {
-            let data = @mut LayoutData::new();
-            self.set_layout_data(data);
-            Some(data)
+            self.set_layout_data(LayoutData::new());
         }
     }
 
     /// Initializes layout data and styles for a Node tree, if any nodes do not have
-    /// this data already. Append created layout data to the task's GC roots.
-    fn initialize_style_for_subtree(self, refs: &mut ~[@mut LayoutData]) {
-        let _ = for n in self.traverse_preorder() {
-            match n.initialize_layout_data() {
-                Some(r) => refs.push(r),
-                None => {}
-            }
-        };
+    /// this data already.
+    fn initialize_style_for_subtree(self) {
+        for n in self.traverse_preorder() {
+            n.initialize_layout_data();
+        }
     }
 }
-

src/components/main/layout/incremental.rs

@@ -54,6 +54,17 @@ impl RestyleDamage {
         RestyleDamage::all()
     }
 
+    /// Create a RestyleDamage from the underlying bit field.
+    /// We would rather not allow this, but some types in script
+    /// need to store RestyleDamage without depending on this crate.
+    pub fn from_int(n: int) -> RestyleDamage {
+        RestyleDamage { bits: n }
+    }
+
+    pub fn to_int(self) -> int {
+        self.bits
+    }
+
     pub fn is_empty(self) -> bool {
         self.bits == 0
     }

src/components/main/layout/layout_task.rs

@@ -7,7 +7,7 @@
 
 use css::matching::MatchMethods;
 use css::select::new_css_select_ctx;
-use layout::aux::{LayoutData, LayoutAuxMethods};
+use layout::aux::LayoutAuxMethods;
 use layout::box_builder::LayoutTreeBuilder;
 use layout::context::LayoutContext;
 use layout::display_list_builder::{DisplayListBuilder};
@@ -59,9 +59,6 @@ struct LayoutTask {
     doc_url: Option<Url>,
     screen_size: Option<Size2D<Au>>,
 
-    /// This is used to root reader data.
-    layout_refs: ~[@mut LayoutData],
-
     display_list: Option<Arc<DisplayList<AbstractNode<()>>>>,
 
     css_select_ctx: @mut SelectCtx,
@@ -123,7 +120,6 @@ impl LayoutTask {
 
             display_list: None,
             
-            layout_refs: ~[],
             css_select_ctx: @mut new_css_select_ctx(),
             profiler_chan: profiler_chan,
         }
@@ -210,7 +206,7 @@ impl LayoutTask {
         //
         // FIXME: This is inefficient. We don't need an entire traversal to do this!
         do profile(time::LayoutAuxInitCategory, self.profiler_chan.clone()) {
-            node.initialize_style_for_subtree(&mut self.layout_refs);
+            node.initialize_style_for_subtree();
         }
 
         // Perform CSS selector matching if necessary.
@@ -328,13 +324,13 @@ impl LayoutTask {
                     };
                     assert!(node.has_layout_data(), "Node has display item but no layout data");
 
-                    let layout_data = node.layout_data();
-                    layout_data.boxes.display_list = Some(display_list.clone());
+                    do node.write_layout_data |layout_data| {
+                        layout_data.boxes.display_list = Some(display_list.clone());
 
-                    if layout_data.boxes.range.is_none() {
-                        debug!("Creating initial range for node");
-                        layout_data.boxes.range = Some(Range::new(i,1)); 
-                    } else {
+                        if layout_data.boxes.range.is_none() {
+                            debug!("Creating initial range for node");
+                            layout_data.boxes.range = Some(Range::new(i,1));
+                        } else {
                             debug!("Appending item to range");
                             unsafe {
                                 let old_node: AbstractNode<()> = transmute(node);
@@ -343,6 +339,7 @@ impl LayoutTask {
                             }
 
                             layout_data.boxes.range.unwrap().extend_by(1);
+                        }
                     }
                 }
 
@@ -376,28 +373,30 @@ impl LayoutTask {
                     transmute(node)
                 };
 
-                let response = match (node.layout_data().boxes.display_list.clone(), node.layout_data().boxes.range) {
-                    (Some(display_list), Some(range)) => {
-                        let mut rect: Option<Rect<Au>> = None;
-                        for i in range.eachi() {
-                            rect = match rect {
-                                Some(acc) => Some(acc.union(&display_list.get().list[i].bounds())),
-                                None => Some(display_list.get().list[i].bounds())
+                let response = do node.read_layout_data |layout_data| {
+                    match (layout_data.boxes.display_list.clone(), layout_data.boxes.range) {
+                        (Some(display_list), Some(range)) => {
+                            let mut rect: Option<Rect<Au>> = None;
+                            for i in range.eachi() {
+                                rect = match rect {
+                                    Some(acc) => Some(acc.union(&display_list.get().list[i].bounds())),
+                                    None => Some(display_list.get().list[i].bounds())
+                                }
+                            }
+
+                            match rect {
+                                None => {
+                                    error!("no boxes for node");
+                                    Err(())
+                                }
+                                Some(rect) => Ok(ContentBoxResponse(rect))
                             }
                         }
-                        
-                        match rect {
-                            None => {
-                                error!("no boxes for node");
-                                Err(())
-                            }
-                            Some(rect) => Ok(ContentBoxResponse(rect))
+                        _ => {
+                            error!("no display list present");
+                            Err(())
                         }
                     }
-                    _ => {
-                        error!("no display list present");
-                        Err(())
-                    }
                 };
 
                 reply_chan.send(response)
@@ -408,16 +407,18 @@ impl LayoutTask {
                     transmute(node)
                 };
 
-                let response = match (node.layout_data().boxes.display_list.clone(), node.layout_data().boxes.range) {
-                    (Some(display_list), Some(range)) => {
-                        let mut boxes = ~[];
-                        for i in range.eachi() {
-                            boxes.push(display_list.get().list[i].bounds());
-                        }
+                let response = do node.read_layout_data |layout_data| {
+                    match (layout_data.boxes.display_list.clone(), layout_data.boxes.range) {
+                        (Some(display_list), Some(range)) => {
+                            let mut boxes = ~[];
+                            for i in range.eachi() {
+                                boxes.push(display_list.get().list[i].bounds());
+                            }
 
-                        Ok(ContentBoxesResponse(boxes))
+                            Ok(ContentBoxesResponse(boxes))
+                        }
+                        _ => Err(()),
                     }
-                    _ => Err(()),
                 };
 
                 reply_chan.send(response)

src/components/script/dom/node.rs

@@ -16,12 +16,17 @@ use dom::htmliframeelement::HTMLIFrameElement;
 use dom::text::Text;
 
 use std::cast;
+use std::cell::Cell;
 use std::cast::transmute;
 use std::libc::c_void;
+use extra::arc::Arc;
 use js::jsapi::{JSObject, JSContext};
 use js::rust::Compartment;
 use netsurfcss::util::VoidPtrLike;
+use newcss::complete::CompleteSelectResults;
 use servo_util::tree::{TreeNode, TreeNodeRef};
+use servo_util::range::Range;
+use gfx::display_list::DisplayList;
 
 //
 // The basic Node structure
@@ -56,7 +61,7 @@ pub struct AbstractNodeChildrenIterator<View> {
 ///
 /// `View` describes extra data associated with this node that this task has access to. For
 /// the script task, this is the unit type `()`. For the layout task, this is
-/// `layout::aux::LayoutData`.
+/// `LayoutData`.
 pub struct Node<View> {
     /// The JavaScript wrapper for this node.
     wrapper: WrapperCache,
@@ -85,7 +90,7 @@ pub struct Node<View> {
     owner_doc: Option<AbstractDocument>,
 
     /// Layout information. Only the layout task may touch this data.
-    priv layout_data: Option<@mut ()>
+    priv layout_data: Option<LayoutData>,
 }
 
 /// The different types of nodes.
@@ -172,31 +177,6 @@ impl<'self, View> AbstractNode<View> {
         }
     }
 
-    /// Returns the layout data, unsafely cast to whatever type layout wishes. Only layout is
-    /// allowed to call this. This is wildly unsafe and is therefore marked as such.
-    pub unsafe fn unsafe_layout_data<T>(self) -> @mut T {
-        do self.with_base |base| {
-            transmute(base.layout_data.unwrap())
-        }
-    }
-    /// Returns true if this node has layout data and false otherwise.
-    pub unsafe fn unsafe_has_layout_data(self) -> bool {
-        do self.with_base |base| {
-            base.layout_data.is_some()
-        }
-    }
-    /// Sets the layout data, unsafely casting the type as layout wishes. Only layout is allowed
-    /// to call this. This is wildly unsafe and is therefore marked as such.
-    pub unsafe fn unsafe_set_layout_data<T>(self, data: @mut T) {
-        // Don't decrement the refcount on data, since we're giving it to the
-        // base structure.
-        cast::forget(data);
-
-        do self.with_mut_base |base| {
-            base.layout_data = Some(transmute(data))
-        }
-    }
-
     // Convenience accessors
 
     /// Returns the type ID of this node. Fails if this node is borrowed mutably.
@@ -631,3 +611,65 @@ impl BindingObject for Node<ScriptView> {
     }
 }
 
+// This stuff is notionally private to layout, but we put it here because it needs
+// to be stored in a Node, and we can't have cross-crate cyclic dependencies.
+
+pub struct DisplayBoxes {
+    display_list: Option<Arc<DisplayList<AbstractNode<()>>>>,
+    range: Option<Range>,
+}
+
+/// Data that layout associates with a node.
+pub struct LayoutData {
+    /// The results of CSS styling for this node.
+    style: Option<CompleteSelectResults>,
+
+    /// Description of how to account for recent style changes.
+    restyle_damage: Option<int>,
+
+    /// The boxes assosiated with this flow.
+    /// Used for getBoundingClientRect and friends.
+    boxes: DisplayBoxes,
+}
+
+impl LayoutData {
+    /// Creates new layout data.
+    pub fn new() -> LayoutData {
+        LayoutData {
+            style: None,
+            restyle_damage: None,
+            boxes: DisplayBoxes { display_list: None, range: None },
+        }
+    }
+}
+
+impl AbstractNode<LayoutView> {
+    pub fn set_layout_data(self, data: LayoutData) {
+        let cell = Cell::new(data);
+        do self.with_mut_base |b| {
+            b.layout_data = Some(cell.take());
+        }
+    }
+
+    pub fn has_layout_data(self) -> bool {
+        do self.with_base |b| {
+            b.layout_data.is_some()
+        }
+    }
+
+    // These accessors take a continuation rather than returning a reference, because
+    // an AbstractNode doesn't have a lifetime parameter relating to the underlying
+    // Node.  Also this makes it easier to switch to RWArc if we decide that is
+    // necessary.
+    pub fn read_layout_data<R>(self, blk: &fn(data: &LayoutData) -> R) -> R {
+        do self.with_base |b| {
+            blk(b.layout_data.get_ref())
+        }
+    }
+
+    pub fn write_layout_data<R>(self, blk: &fn(data: &mut LayoutData) -> R) -> R {
+        do self.with_mut_base |b| {
+            blk(b.layout_data.get_mut_ref())
+        }
+    }
+}