auto merge of #1490 : pcwalton/servo/distrust-layout-new, r=jdm

Pointers to DOM nodes from layout could go stale if incremental reflow
does not correctly destroy dead nodes. Therefore, we ask the JavaScript
garbage collector to verify that each DOM node is indeed a valid pointer
before calling event handlers on it, and fail otherwise.

Depends on the `get-addressable-gc-thing` branches of `mozjs` and `rust-mozjs`.

r? @jdm

src/components/main/layout/layout_task.rs

@@ -29,7 +29,7 @@ use gfx::opts::Opts;
 use gfx::render_task::{RenderMsg, RenderChan, RenderLayer};
 use gfx::{render_task, color};
 use script::dom::event::ReflowEvent;
-use script::dom::node::{AbstractNode, ElementNodeTypeId, LayoutDataRef};
+use script::dom::node::{ElementNodeTypeId, LayoutDataRef};
 use script::dom::element::{HTMLBodyElementTypeId, HTMLHtmlElementTypeId};
 use script::layout_interface::{AddStylesheetMsg, ContentBoxQuery};
 use script::layout_interface::{ContentBoxesQuery, ContentBoxesResponse, ExitNowMsg, LayoutQuery};
@@ -608,15 +608,9 @@ impl LayoutTask {
                                 bounds.origin.x <= x &&
                                 y < bounds.origin.y + bounds.size.height &&
                                 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 = unsafe {
-                                item.base().extra.to_script_node()
-                            };
-                            let resp = Some(HitTestResponse(node));
-                            return resp;
+                            return Some(HitTestResponse(item.base()
+                                                            .extra
+                                                            .to_untrusted_node_address()))
                         }
                     }
 

src/components/main/layout/util.rs

@@ -7,8 +7,9 @@ use layout::construct::{ConstructionResult, NoConstructionResult};
 use layout::wrapper::LayoutNode;
 
 use extra::arc::Arc;
+use script::dom::bindings::utils::Reflectable;
 use script::dom::node::AbstractNode;
-use script::layout_interface::LayoutChan;
+use script::layout_interface::{LayoutChan, UntrustedNodeAddress};
 use servo_util::range::Range;
 use std::cast;
 use std::cell::{Ref, RefMut};
@@ -211,21 +212,28 @@ impl OpaqueNode {
     /// Converts a DOM node (layout view) to an `OpaqueNode`.
     pub fn from_layout_node(node: &LayoutNode) -> OpaqueNode {
         unsafe {
-            OpaqueNode(cast::transmute_copy(node))
+            let abstract_node = node.get_abstract();
+            let ptr: uintptr_t = cast::transmute(abstract_node.reflector().get_jsobject());
+            OpaqueNode(ptr)
         }
     }
 
     /// Converts a DOM node (script view) to an `OpaqueNode`.
     pub fn from_script_node(node: &AbstractNode) -> OpaqueNode {
         unsafe {
-            OpaqueNode(cast::transmute_copy(node))
+            let ptr: uintptr_t = cast::transmute(node.reflector().get_jsobject());
+            OpaqueNode(ptr)
         }
     }
 
-    /// Unsafely converts an `OpaqueNode` to a DOM node (script view). Use this only if you
-    /// absolutely know what you're doing.
-    pub unsafe fn to_script_node(&self) -> AbstractNode {
-        cast::transmute(**self)
+    /// Converts this node to an `UntrustedNodeAddress`. An `UntrustedNodeAddress` is just the type
+    /// of node that script expects to receive in a hit test.
+    pub fn to_untrusted_node_address(&self) -> UntrustedNodeAddress {
+        unsafe {
+            let OpaqueNode(addr) = *self;
+            let addr: UntrustedNodeAddress = cast::transmute(addr);
+            addr
+        }
     }
 
     /// Returns the address of this node, for debugging purposes.

src/components/main/layout/wrapper.rs

@@ -61,6 +61,12 @@ impl<'ln> LayoutNode<'ln> {
         cast::transmute(self.node.node())
     }
 
+    /// Returns the interior of this node as an `AbstractNode`. This is highly unsafe for layout to
+    /// call and as such is marked `unsafe`.
+    pub unsafe fn get_abstract(&self) -> AbstractNode {
+        self.node
+    }
+
     /// Returns the first child of this node.
     pub fn first_child(&self) -> Option<LayoutNode<'ln>> {
         unsafe {

src/components/script/dom/node.rs

@@ -7,6 +7,7 @@
 use dom::bindings::utils::{Reflectable, Reflector, reflect_dom_object};
 use dom::bindings::utils::{DOMString, null_str_as_empty};
 use dom::bindings::utils::{ErrorResult, Fallible, NotFound, HierarchyRequest};
+use dom::bindings::utils;
 use dom::characterdata::CharacterData;
 use dom::document::{AbstractDocument, DocumentTypeId};
 use dom::documenttype::DocumentType;
@@ -17,15 +18,15 @@ use dom::htmliframeelement::HTMLIFrameElement;
 use dom::htmlimageelement::HTMLImageElement;
 use dom::nodelist::{NodeList};
 use dom::text::Text;
+use layout_interface::{LayoutChan, ReapLayoutDataMsg, UntrustedNodeAddress};
 
-use js::jsapi::{JSObject, JSContext};
-
-use layout_interface::{LayoutChan, ReapLayoutDataMsg};
-
-use std::cast;
+use js::jsapi::{JSContext, JSObject, JSRuntime};
+use js::jsfriendapi;
 use std::cast::transmute;
+use std::cast;
 use std::cell::{RefCell, Ref, RefMut};
 use std::iter::Filter;
+use std::libc::uintptr_t;
 use std::util;
 use std::unstable::raw::Box;
 
@@ -241,6 +242,22 @@ impl AbstractNode {
             _ => false
         }
     }
+
+    /// If the given untrusted node address represents a valid DOM node in the given runtime,
+    /// returns it.
+    pub fn from_untrusted_node_address(runtime: *JSRuntime, candidate: UntrustedNodeAddress)
+                                       -> AbstractNode {
+        unsafe {
+            let candidate: uintptr_t = cast::transmute(candidate);
+            let object: *JSObject = jsfriendapi::bindgen::JS_GetAddressableObject(runtime,
+                                                                                  candidate);
+            if object.is_null() {
+                fail!("Attempted to create an `AbstractNode` from an invalid pointer!")
+            }
+            let boxed_node: *mut Box<Node> = utils::unwrap(object);
+            AbstractNode::from_box(boxed_node)
+        }
+    }
 }
 
 impl<'a> AbstractNode {

src/components/script/layout_interface.rs

@@ -14,8 +14,9 @@ use geom::rect::Rect;
 use geom::size::Size2D;
 use script_task::{ScriptChan};
 use servo_util::geometry::Au;
-use std::comm::{Chan, SharedChan};
 use std::cmp;
+use std::comm::{Chan, SharedChan};
+use std::libc::c_void;
 use style::Stylesheet;
 
 /// Asynchronous messages that script can send to layout.
@@ -58,9 +59,13 @@ pub enum LayoutQuery {
     HitTestQuery(AbstractNode, Point2D<f32>, Chan<Result<HitTestResponse, ()>>),
 }
 
+/// The address of a node. Layout sends these back. They must be validated via
+/// `from_untrusted_node_address` before they can be used, because we do not trust layout.
+pub type UntrustedNodeAddress = *c_void;
+
 pub struct ContentBoxResponse(Rect<Au>);
 pub struct ContentBoxesResponse(~[Rect<Au>]);
-pub struct HitTestResponse(AbstractNode);
+pub struct HitTestResponse(UntrustedNodeAddress);
 
 /// Determines which part of the 
 #[deriving(Eq, Ord)]

src/components/script/script_task.rs

@@ -850,31 +850,31 @@ impl ScriptTask {
                 }
                 let (port, chan) = Chan::new();
                 match page.query_layout(HitTestQuery(root.unwrap(), point, chan), port) {
-                    Ok(node) => match node {
-                        HitTestResponse(node) => {
-                            debug!("clicked on {:s}", node.debug_str());
-                            let mut node = node;
-                            // traverse node generations until a node that is an element is found
-                            while !node.is_element() {
-                                match node.parent_node() {
-                                    Some(parent) => {
-                                        node = parent;
-                                    }
-                                    None => break
-                                }
-                            }
-                            if node.is_element() {
-                                node.with_imm_element(|element| {
-                                    if "a" == element.tag_name {
-                                        self.load_url_from_element(page, element)
-                                    }
-                                })
+                    Ok(HitTestResponse(node_address)) => {
+                        debug!("node address is {:?}", node_address);
+                        let mut node = AbstractNode::from_untrusted_node_address(self.js_runtime
+                                                                                     .ptr,
+                                                                                 node_address);
+                        debug!("clicked on {:s}", node.debug_str());
+
+                        // Traverse node generations until a node that is an element is
+                        // found.
+                        while !node.is_element() {
+                            match node.parent_node() {
+                                Some(parent) => node = parent,
+                                None => break,
                             }
                         }
+
+                        if node.is_element() {
+                            node.with_imm_element(|element| {
+                                if "a" == element.tag_name {
+                                    self.load_url_from_element(page, element)
+                                }
+                            })
+                        }
                     },
-                    Err(()) => {
-                        debug!("layout query error");
-                    }
+                    Err(()) => debug!("layout query error"),
                 }
             }
             MouseDownEvent(..) => {}

src/support/spidermonkey/mozjs

@@ -1 +1 @@
-Subproject commit d57edb998865f9616d9164d561f26d54ade49642
+Subproject commit 3e633e7e498e64793cb2c75394ed78d507c383ae

src/support/spidermonkey/rust-mozjs

@@ -1 +1 @@
-Subproject commit da846919d0b99d84bffe89c5410b13ae64a969e4
+Subproject commit c2f282d8762aec1a55007ff75ef85dfb34e61e7e