auto merge of #1772 : pcwalton/servo/borrow-flags-race, r=jdm

r? @jdm

src/components/main/layout/wrapper.rs

@@ -4,19 +4,26 @@
 
 //! A safe wrapper for DOM nodes that prevents layout from mutating the DOM, from letting DOM nodes
 //! escape, and from generally doing anything that it isn't supposed to. This is accomplished via
-//! a simple whitelist of allowed operations.
+//! a simple whitelist of allowed operations, along with some lifetime magic to prevent nodes from
+//! escaping.
 //!
 //! As a security wrapper is only as good as its whitelist, be careful when adding operations to
 //! this list. The cardinal rules are:
 //!
-//! (1) Layout is not allowed to mutate the DOM.
+//! 1. Layout is not allowed to mutate the DOM.
 //!
-//! (2) Layout is not allowed to see anything with `JS` in the name, because it could hang
-//!     onto these objects and cause use-after-free.
+//! 2. Layout is not allowed to see anything with `JS` in the name, because it could hang
+//!    onto these objects and cause use-after-free.
+//!
+//! When implementing wrapper functions, be careful that you do not touch the borrow flags, or you
+//! will race and cause spurious task failure. (Note that I do not believe these races are
+//! exploitable, but they'll result in brokenness nonetheless.) In general, you must not use the
+//! `Cast` functions; use explicit checks and `transmute_copy` instead. You must also not use
+//! `.get()`; instead, use `.unsafe_get()`.
 
 use extra::url::Url;
-use script::dom::bindings::codegen::InheritTypes::{HTMLImageElementCast, HTMLIFrameElementCast};
-use script::dom::bindings::codegen::InheritTypes::{TextCast, ElementCast};
+use script::dom::bindings::codegen::InheritTypes::{ElementDerived, HTMLIFrameElementDerived};
+use script::dom::bindings::codegen::InheritTypes::{HTMLImageElementDerived, TextDerived};
 use script::dom::bindings::js::JS;
 use script::dom::element::{Element, HTMLAreaElementTypeId, HTMLAnchorElementTypeId};
 use script::dom::element::{HTMLLinkElementTypeId};
@@ -29,20 +36,19 @@ use servo_util::namespace;
 use servo_util::namespace::Namespace;
 use std::cast;
 use std::cell::{Ref, RefMut};
-use style::{PropertyDeclarationBlock, TElement, TNode,
-            AttrSelector, SpecificNamespace, AnyNamespace};
+use style::{PropertyDeclarationBlock, TElement, TNode, AttrSelector, SpecificNamespace};
+use style::{AnyNamespace};
 
 use layout::util::LayoutDataWrapper;
 
 /// Allows some convenience methods on generic layout nodes.
 pub trait TLayoutNode {
     /// Creates a new layout node with the same lifetime as this layout node.
-    unsafe fn new_with_this_lifetime(&self, node: JS<Node>) -> Self;
+    unsafe fn new_with_this_lifetime(&self, node: &JS<Node>) -> Self;
 
     /// Returns the type ID of this node. Fails if this node is borrowed mutably.
     fn type_id(&self) -> NodeTypeId;
 
-
     /// Returns the interior of this node as a `JS`. This is highly unsafe for layout to
     /// call and as such is marked `unsafe`.
     unsafe fn get_jsmanaged<'a>(&'a self) -> &'a JS<Node>;
@@ -50,7 +56,7 @@ pub trait TLayoutNode {
     /// Returns the interior of this node as a `Node`. This is highly unsafe for layout to call
     /// and as such is marked `unsafe`.
     unsafe fn get<'a>(&'a self) -> &'a Node {
-        self.get_jsmanaged().get()
+        cast::transmute::<*mut Node,&'a Node>(self.get_jsmanaged().unsafe_get())
     }
 
     fn node_is_element(&self) -> bool {
@@ -72,8 +78,11 @@ pub trait TLayoutNode {
     /// FIXME(pcwalton): Don't copy URLs.
     fn image_url(&self) -> Option<Url> {
         unsafe {
-            let image_element: JS<HTMLImageElement> = HTMLImageElementCast::to(self.get_jsmanaged());
-            image_element.get().extra.image.as_ref().map(|url| (*url).clone())
+            if !self.get().is_htmlimageelement() {
+                fail!("not an image!")
+            }
+            let image_element: JS<HTMLImageElement> = self.get_jsmanaged().transmute_copy();
+            (*image_element.unsafe_get()).extra.image.as_ref().map(|url| (*url).clone())
         }
     }
 
@@ -81,8 +90,11 @@ pub trait TLayoutNode {
     /// not an iframe element, fails.
     fn iframe_pipeline_and_subpage_ids(&self) -> (PipelineId, SubpageId) {
         unsafe {
-            let iframe_element: JS<HTMLIFrameElement> = HTMLIFrameElementCast::to(self.get_jsmanaged());
-            let size = iframe_element.get().size.unwrap();
+            if !self.get().is_htmliframeelement() {
+                fail!("not an iframe element!")
+            }
+            let iframe_element: JS<HTMLIFrameElement> = self.get_jsmanaged().transmute_copy();
+            let size = (*iframe_element.unsafe_get()).size.unwrap();
             (size.pipeline_id, size.subpage_id)
         }
     }
@@ -92,23 +104,24 @@ pub trait TLayoutNode {
     /// FIXME(pcwalton): Don't copy text. Atomically reference count instead.
     fn text(&self) -> ~str {
         unsafe {
-            let text: JS<Text> = TextCast::to(self.get_jsmanaged());
-            text.get().characterdata.data.to_str()
+            if !self.get().is_text() {
+                fail!("not text!")
+            }
+            let text: JS<Text> = self.get_jsmanaged().transmute_copy();
+            (*text.unsafe_get()).characterdata.data.to_str()
         }
     }
 
     /// Returns the first child of this node.
     fn first_child(&self) -> Option<Self> {
         unsafe {
-            self.get_jsmanaged().first_child().map(|node| self.new_with_this_lifetime(node))
+            self.get().first_child_ref().map(|node| self.new_with_this_lifetime(node))
         }
     }
 
     /// Dumps this node tree, for debugging.
     fn dump(&self) {
-        unsafe {
-            self.get_jsmanaged().dump()
-        }
+        // TODO(pcwalton): Reimplement this in a way that's safe for layout to call.
     }
 }
 
@@ -124,9 +137,9 @@ pub struct LayoutNode<'a> {
 }
 
 impl<'ln> TLayoutNode for LayoutNode<'ln> {
-    unsafe fn new_with_this_lifetime(&self, node: JS<Node>) -> LayoutNode<'ln> {
+    unsafe fn new_with_this_lifetime(&self, node: &JS<Node>) -> LayoutNode<'ln> {
         LayoutNode {
-            node: node,
+            node: node.transmute_copy(),
             chain: self.chain,
         }
     }
@@ -168,30 +181,31 @@ impl<'ln> LayoutNode<'ln> {
 impl<'ln> TNode<LayoutElement<'ln>> for LayoutNode<'ln> {
     fn parent_node(&self) -> Option<LayoutNode<'ln>> {
         unsafe {
-            self.node.parent_node().map(|node| self.new_with_this_lifetime(node))
+            self.get().parent_node_ref().map(|node| self.new_with_this_lifetime(node))
         }
     }
 
     fn prev_sibling(&self) -> Option<LayoutNode<'ln>> {
         unsafe {
-            self.node.prev_sibling().map(|node| self.new_with_this_lifetime(node))
+            self.get().prev_sibling_ref().map(|node| self.new_with_this_lifetime(node))
         }
     }
 
     fn next_sibling(&self) -> Option<LayoutNode<'ln>> {
         unsafe {
-            self.node.next_sibling().map(|node| self.new_with_this_lifetime(node))
+            self.get().next_sibling_ref().map(|node| self.new_with_this_lifetime(node))
         }
     }
 
     /// If this is an element, accesses the element data. Fails if this is not an element node.
     #[inline]
     fn with_element<R>(&self, f: |&LayoutElement<'ln>| -> R) -> R {
-        let elem: JS<Element> = ElementCast::to(&self.node);
-        let element = elem.get();
-        // FIXME(pcwalton): Workaround until Rust gets multiple lifetime parameters on
-        // implementations.
         unsafe {
+            if !self.node.is_element() {
+                fail!("not an element!")
+            }
+            let elem: JS<Element> = self.node.transmute_copy();
+            let element = elem.get();
             f(&LayoutElement {
                 element: cast::transmute_region(element),
             })
@@ -340,9 +354,9 @@ pub struct ThreadSafeLayoutNode<'ln> {
 
 impl<'ln> TLayoutNode for ThreadSafeLayoutNode<'ln> {
     /// Creates a new layout node with the same lifetime as this layout node.
-    unsafe fn new_with_this_lifetime(&self, node: JS<Node>) -> ThreadSafeLayoutNode<'ln> {
+    unsafe fn new_with_this_lifetime(&self, node: &JS<Node>) -> ThreadSafeLayoutNode<'ln> {
         ThreadSafeLayoutNode {
-            node: node,
+            node: node.transmute_copy(),
             chain: self.chain,
         }
     }
@@ -374,7 +388,7 @@ impl<'ln> ThreadSafeLayoutNode<'ln> {
 
     /// Returns the next sibling of this node. Unsafe and private because this can lead to races.
     unsafe fn next_sibling(&self) -> Option<ThreadSafeLayoutNode<'ln>> {
-        self.node.next_sibling().map(|node| self.new_with_this_lifetime(node))
+        self.node.get().next_sibling_ref().map(|node| self.new_with_this_lifetime(node))
     }
 
     /// Returns an iterator over this node's children.
@@ -388,12 +402,15 @@ impl<'ln> ThreadSafeLayoutNode<'ln> {
     #[inline]
     pub fn with_element<R>(&self, f: |&ThreadSafeLayoutElement| -> R) -> R {
         unsafe {
-            let elem: JS<Element> = ElementCast::to(&self.node);
-            let element = elem.get();
+            if !self.node.is_element() {
+                fail!("not an element!")
+            }
+            let elem: JS<Element> = self.node.transmute_copy();
+            let element = elem.unsafe_get();
             // FIXME(pcwalton): Workaround until Rust gets multiple lifetime parameters on
             // implementations.
             f(&ThreadSafeLayoutElement {
-                element: cast::transmute_region(element),
+                element: cast::transmute::<*mut Element,&mut Element>(element),
             })
         }
     }

src/components/script/dom/bindings/js.rs

@@ -89,6 +89,13 @@ impl<T> JS<T> {
             &mut (**borrowed.get())
         }
     }
+
+    /// Returns an unsafe pointer to the interior of this JS object without touching the borrow
+    /// flags. This is the only method that be safely accessed from layout. (The fact that this
+    /// is unsafe is what necessitates the layout wrappers.)
+    pub unsafe fn unsafe_get(&self) -> *mut T {
+        cast::transmute_copy(&self.ptr)
+    }
 }
 
 impl<From, To> JS<From> {
@@ -96,4 +103,8 @@ impl<From, To> JS<From> {
     pub unsafe fn transmute(self) -> JS<To> {
         cast::transmute(self)
     }
+
+    pub unsafe fn transmute_copy(&self) -> JS<To> {
+        cast::transmute_copy(self)
+    }
 }

src/components/script/dom/node.rs

@@ -332,7 +332,7 @@ impl NodeHelpers for JS<Node> {
     fn parent_node(&self) -> Option<JS<Node>> {
         self.get().parent_node.clone()
     }
-
+    
     fn first_child(&self) -> Option<JS<Node>> {
         self.get().first_child.clone()
     }
@@ -1590,6 +1590,31 @@ impl Node {
     pub fn set_hover_state(&mut self, state: bool) {
         self.flags.set_is_in_hover_state(state);
     }
+
+    #[inline]
+    pub fn parent_node_ref<'a>(&'a self) -> Option<&'a JS<Node>> {
+        self.parent_node.as_ref()
+    }
+
+    #[inline]
+    pub fn first_child_ref<'a>(&'a self) -> Option<&'a JS<Node>> {
+        self.first_child.as_ref()
+    }
+
+    #[inline]
+    pub fn last_child_ref<'a>(&'a self) -> Option<&'a JS<Node>> {
+        self.last_child.as_ref()
+    }
+
+    #[inline]
+    pub fn prev_sibling_ref<'a>(&'a self) -> Option<&'a JS<Node>> {
+        self.prev_sibling.as_ref()
+    }
+
+    #[inline]
+    pub fn next_sibling_ref<'a>(&'a self) -> Option<&'a JS<Node>> {
+        self.next_sibling.as_ref()
+    }
 }
 
 impl Reflectable for Node {