script: Stop trusting pointers to DOM nodes that layout provides.

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.

8 files changed, 76 insertions, 46 deletions

diff --git a/src/components/main/layout/layout_task.rs b/src/components/main/layout/layout_task.rs
index 1810ff38772..7b2dbfe1014 100644
--- a/src/components/main/layout/layout_task.rs
+++ b/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()))
                         }
                     }
 
diff --git a/src/components/main/layout/util.rs b/src/components/main/layout/util.rs
index 65cf0b6b676..8ef031bfb3f 100644
--- a/src/components/main/layout/util.rs
+++ b/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.
diff --git a/src/components/main/layout/wrapper.rs b/src/components/main/layout/wrapper.rs
index f41fdb67d26..d8225ab5078 100644
--- a/src/components/main/layout/wrapper.rs
+++ b/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 {
diff --git a/src/components/script/dom/node.rs b/src/components/script/dom/node.rs
index a98f89b4817..dc5de2aac24 100644
--- a/src/components/script/dom/node.rs
+++ b/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 {
diff --git a/src/components/script/layout_interface.rs b/src/components/script/layout_interface.rs
index 613e61a3429..b0b9ec479f4 100644
--- a/src/components/script/layout_interface.rs
+++ b/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)]
diff --git a/src/components/script/script_task.rs b/src/components/script/script_task.rs
index faec6d8031b..9ca9af2f8a6 100644
--- a/src/components/script/script_task.rs
+++ b/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(..) => {}
diff --git a/src/support/spidermonkey/mozjs b/src/support/spidermonkey/mozjs
-Subproject d57edb998865f9616d9164d561f26d54ade4964
+Subproject 3e633e7e498e64793cb2c75394ed78d507c383a
diff --git a/src/support/spidermonkey/rust-mozjs b/src/support/spidermonkey/rust-mozjs
-Subproject da846919d0b99d84bffe89c5410b13ae64a969e
+Subproject c2f282d8762aec1a55007ff75ef85dfb34e61e7