From c20b50bbbbcdc8ce3551adbc1e039a727cf89995 Mon Sep 17 00:00:00 2001
From: Keegan McAllister <mcallister.keegan@gmail.com>
Date: Mon, 4 Aug 2014 15:29:07 -0700
Subject: [PATCH] Implement a simple version of JS<T> safety checking

This forbids the use of JS<T> (really, any type with the #[unrooted_js_managed]
attribute) wherever the lint is enabled.  This will miss some unsafe constructs,
like stack-allocating a struct containing JS<T>, but it's a reasonable start.

The lint is default-deny.  In cases where I wasn't sure about the safety of the
existing code, I added #[warn(unrooted_js_managed)] so we can revisit these.
---
 .../compiler_plugins/js_managed_lint.rs       | 36 +++++++++++++++++++
 src/components/compiler_plugins/lib.rs        | 16 ++++++++-
 src/components/script/dom/attrlist.rs         |  1 +
 .../dom/bindings/codegen/CodegenRust.py       |  2 ++
 .../script/dom/bindings/conversions.rs        |  2 ++
 src/components/script/dom/bindings/global.rs  |  1 +
 src/components/script/dom/bindings/js.rs      |  5 ++-
 src/components/script/dom/bindings/trace.rs   |  1 +
 src/components/script/dom/browsercontext.rs   |  1 +
 src/components/script/dom/clientrect.rs       |  1 +
 src/components/script/dom/clientrectlist.rs   |  1 +
 src/components/script/dom/document.rs         |  1 +
 .../script/dom/domimplementation.rs           |  1 +
 src/components/script/dom/domparser.rs        |  1 +
 src/components/script/dom/domtokenlist.rs     |  1 +
 src/components/script/dom/element.rs          |  2 ++
 src/components/script/dom/formdata.rs         |  1 +
 src/components/script/dom/htmlcollection.rs   |  1 +
 src/components/script/dom/htmlimageelement.rs |  1 +
 src/components/script/dom/node.rs             |  4 +++
 src/components/script/dom/nodelist.rs         |  1 +
 src/components/script/dom/performance.rs      |  1 +
 src/components/script/dom/xmlhttprequest.rs   |  1 +
 src/components/script/page.rs                 |  1 +
 24 files changed, 82 insertions(+), 2 deletions(-)
 create mode 100644 src/components/compiler_plugins/js_managed_lint.rs

diff --git a/src/components/compiler_plugins/js_managed_lint.rs b/src/components/compiler_plugins/js_managed_lint.rs
new file mode 100644
index 000000000000..51954d5bb448
--- /dev/null
+++ b/src/components/compiler_plugins/js_managed_lint.rs
@@ -0,0 +1,36 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * 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 syntax::ast;
+
+use rustc::lint::{LintPass, LintArray, Context};
+use rustc::middle::{ty, def};
+
+declare_lint!(UNROOTED_JS_MANAGED, Deny,
+    "warn about unrooted JS-managed pointers")
+
+pub struct UnrootedJSManaged;
+
+impl LintPass for UnrootedJSManaged {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(UNROOTED_JS_MANAGED)
+    }
+
+    fn check_ty(&mut self, cx: &Context, ty: &ast::Ty) {
+        match ty.node {
+            ast::TyPath(_, _, id) => {
+                match cx.tcx.def_map.borrow().get_copy(&id) {
+                    def::DefTy(def_id) => {
+                        if ty::has_attr(cx.tcx, def_id, "unrooted_js_managed") {
+                            cx.span_lint(UNROOTED_JS_MANAGED, ty.span,
+                                "unrooted JS<T> is not allowed here");
+                        }
+                    }
+                    _ => (),
+                }
+            }
+            _ => (),
+        }
+    }
+}
diff --git a/src/components/compiler_plugins/lib.rs b/src/components/compiler_plugins/lib.rs
index f53a07a1f43e..c57b92db4703 100644
--- a/src/components/compiler_plugins/lib.rs
+++ b/src/components/compiler_plugins/lib.rs
@@ -5,9 +5,23 @@
 #![crate_name = "compiler_plugins"]
 #![crate_type = "dylib"]
 
-#![feature(macro_rules)]
+#![feature(macro_rules, plugin_registrar, phase)]
+
+extern crate syntax;
+
+#[phase(plugin, link)]
+extern crate rustc;
 
 #[cfg(test)]
 extern crate sync;
 
+use rustc::plugin::Registry;
+
 mod macros;
+mod js_managed_lint;
+
+// NB: This needs to be public or we get a linker error.
+#[plugin_registrar]
+pub fn plugin_registrar(reg: &mut Registry) {
+    reg.register_lint_pass(box js_managed_lint::UnrootedJSManaged);
+}
diff --git a/src/components/script/dom/attrlist.rs b/src/components/script/dom/attrlist.rs
index e4812b756c25..7548887db78b 100644
--- a/src/components/script/dom/attrlist.rs
+++ b/src/components/script/dom/attrlist.rs
@@ -11,6 +11,7 @@ use dom::bindings::utils::{Reflectable, Reflector, reflect_dom_object};
 use dom::element::Element;
 use dom::window::Window;
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub struct AttrList {
     reflector_: Reflector,
diff --git a/src/components/script/dom/bindings/codegen/CodegenRust.py b/src/components/script/dom/bindings/codegen/CodegenRust.py
index 42175de35e4c..fc594ebbc3f6 100644
--- a/src/components/script/dom/bindings/codegen/CodegenRust.py
+++ b/src/components/script/dom/bindings/codegen/CodegenRust.py
@@ -1339,9 +1339,11 @@ def __init__(self, child, descriptors, imports):
             'unused_mut',
             'dead_assignment',
             'dead_code',
+            'unrooted_js_managed',
         ]
 
         statements = ['#![allow(%s)]' % ','.join(ignored_warnings)]
+        statements.append('#![warn(unrooted_js_managed)]')
         statements.extend('use %s;' % i for i in sorted(imports))
 
         CGWrapper.__init__(self, child,
diff --git a/src/components/script/dom/bindings/conversions.rs b/src/components/script/dom/bindings/conversions.rs
index 8ce5b55e9d3a..165384763197 100644
--- a/src/components/script/dom/bindings/conversions.rs
+++ b/src/components/script/dom/bindings/conversions.rs
@@ -317,6 +317,7 @@ impl ToJSValConvertible for Reflector {
     }
 }
 
+#[warn(unrooted_js_managed)]
 impl<T: Reflectable+IDLInterface> FromJSValConvertible<()> for JS<T> {
     fn from_jsval(_cx: *mut JSContext, value: JSVal, _option: ()) -> Result<JS<T>, ()> {
         if !value.is_object() {
@@ -340,6 +341,7 @@ impl<'a, T: Reflectable> ToJSValConvertible for JSRef<'a, T> {
     }
 }
 
+#[warn(unrooted_js_managed)]
 impl<'a, T: Reflectable> ToJSValConvertible for JS<T> {
     fn to_jsval(&self, cx: *mut JSContext) -> JSVal {
         self.reflector().to_jsval(cx)
diff --git a/src/components/script/dom/bindings/global.rs b/src/components/script/dom/bindings/global.rs
index ed7807f9d729..8989976f9d1b 100644
--- a/src/components/script/dom/bindings/global.rs
+++ b/src/components/script/dom/bindings/global.rs
@@ -33,6 +33,7 @@ pub enum GlobalRoot<'a, 'b> {
 
 /// A traced reference to a global object, for use in fields of traced Rust
 /// structures.
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub enum GlobalField {
     WindowField(JS<Window>),
diff --git a/src/components/script/dom/bindings/js.rs b/src/components/script/dom/bindings/js.rs
index 6a57b45449fc..6e4f865da653 100644
--- a/src/components/script/dom/bindings/js.rs
+++ b/src/components/script/dom/bindings/js.rs
@@ -45,6 +45,8 @@
 //! - `OptionalSettable`: allows assigning `Option` values of `JSRef`/`Temporary` to fields of `Option<JS<T>>`
 //! - `RootedReference`: makes obtaining an `Option<JSRef<T>>` from an `Option<Root<T>>` easy
 
+#![allow(unrooted_js_managed)]
+
 use dom::bindings::utils::{Reflector, Reflectable};
 use dom::node::Node;
 use dom::xmlhttprequest::{XMLHttpRequest, TrustedXHRAddress};
@@ -104,7 +106,8 @@ impl<T: Reflectable> Temporary<T> {
     }
 }
 
-/// A rooted, JS-owned value. Must only be used as a field in other JS-owned types.
+/// A JS-owned value. Must only be used as a field in other JS-owned types.
+#[unrooted_js_managed]
 pub struct JS<T> {
     ptr: *const T
 }
diff --git a/src/components/script/dom/bindings/trace.rs b/src/components/script/dom/bindings/trace.rs
index f26af0877962..8ab672a83887 100644
--- a/src/components/script/dom/bindings/trace.rs
+++ b/src/components/script/dom/bindings/trace.rs
@@ -48,6 +48,7 @@ fn get_jstracer<'a, S: Encoder<E>, E>(s: &'a mut S) -> &'a mut JSTracer {
     }
 }
 
+#[allow(unrooted_js_managed)]
 impl<T: Reflectable+Encodable<S, E>, S: Encoder<E>, E> Encodable<S, E> for JS<T> {
     fn encode(&self, s: &mut S) -> Result<(), E> {
         trace_reflector(get_jstracer(s), "", self.reflector());
diff --git a/src/components/script/dom/browsercontext.rs b/src/components/script/dom/browsercontext.rs
index 1a16d30290d2..9fc3b113d2d0 100644
--- a/src/components/script/dom/browsercontext.rs
+++ b/src/components/script/dom/browsercontext.rs
@@ -66,6 +66,7 @@ impl BrowserContext {
     }
 }
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub struct SessionHistoryEntry {
     document: JS<Document>,
diff --git a/src/components/script/dom/clientrect.rs b/src/components/script/dom/clientrect.rs
index b80d6a8853b5..a22f95d809ca 100644
--- a/src/components/script/dom/clientrect.rs
+++ b/src/components/script/dom/clientrect.rs
@@ -10,6 +10,7 @@ use dom::bindings::utils::{Reflectable, Reflector, reflect_dom_object};
 use dom::window::Window;
 use servo_util::geometry::Au;
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub struct ClientRect {
     reflector_: Reflector,
diff --git a/src/components/script/dom/clientrectlist.rs b/src/components/script/dom/clientrectlist.rs
index 86e7d3bd3096..9b2325e5b472 100644
--- a/src/components/script/dom/clientrectlist.rs
+++ b/src/components/script/dom/clientrectlist.rs
@@ -10,6 +10,7 @@ use dom::bindings::utils::{Reflectable, Reflector, reflect_dom_object};
 use dom::clientrect::ClientRect;
 use dom::window::Window;
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub struct ClientRectList {
     reflector_: Reflector,
diff --git a/src/components/script/dom/document.rs b/src/components/script/dom/document.rs
index 2f52d3048880..3029ae4c00b8 100644
--- a/src/components/script/dom/document.rs
+++ b/src/components/script/dom/document.rs
@@ -61,6 +61,7 @@ pub enum IsHTMLDocument {
     NonHTMLDocument,
 }
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub struct Document {
     pub node: Node,
diff --git a/src/components/script/dom/domimplementation.rs b/src/components/script/dom/domimplementation.rs
index 0dbb842ebe4d..14d0880afb8e 100644
--- a/src/components/script/dom/domimplementation.rs
+++ b/src/components/script/dom/domimplementation.rs
@@ -22,6 +22,7 @@ use dom::node::Node;
 use dom::text::Text;
 use servo_util::str::DOMString;
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub struct DOMImplementation {
     document: JS<Document>,
diff --git a/src/components/script/dom/domparser.rs b/src/components/script/dom/domparser.rs
index 65273cab7560..9f47c0a3d9eb 100644
--- a/src/components/script/dom/domparser.rs
+++ b/src/components/script/dom/domparser.rs
@@ -13,6 +13,7 @@ use dom::document::{Document, HTMLDocument, NonHTMLDocument};
 use dom::window::Window;
 use servo_util::str::DOMString;
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub struct DOMParser {
     window: JS<Window>, //XXXjdm Document instead?
diff --git a/src/components/script/dom/domtokenlist.rs b/src/components/script/dom/domtokenlist.rs
index cf44d5f7abc3..4237390e19a1 100644
--- a/src/components/script/dom/domtokenlist.rs
+++ b/src/components/script/dom/domtokenlist.rs
@@ -14,6 +14,7 @@ use dom::node::window_from_node;
 use servo_util::namespace::Null;
 use servo_util::str::DOMString;
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub struct DOMTokenList {
     reflector_: Reflector,
diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs
index 04d9603bdb69..fe915f9d3541 100644
--- a/src/components/script/dom/element.rs
+++ b/src/components/script/dom/element.rs
@@ -42,6 +42,7 @@ use std::ascii::StrAsciiExt;
 use std::cell::{Cell, RefCell};
 use std::mem;
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub struct Element {
     pub node: Node,
@@ -205,6 +206,7 @@ pub trait LayoutElementHelpers {
     unsafe fn html_element_in_html_document_for_layout(&self) -> bool;
 }
 
+#[warn(unrooted_js_managed)]
 impl LayoutElementHelpers for JS<Element> {
     unsafe fn html_element_in_html_document_for_layout(&self) -> bool {
         if (*self.unsafe_get()).namespace != namespace::HTML {
diff --git a/src/components/script/dom/formdata.rs b/src/components/script/dom/formdata.rs
index c50f50fb71fa..2ddf29de83c8 100644
--- a/src/components/script/dom/formdata.rs
+++ b/src/components/script/dom/formdata.rs
@@ -18,6 +18,7 @@ use servo_util::str::DOMString;
 use std::cell::RefCell;
 use std::collections::hashmap::HashMap;
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable, Clone)]
 pub enum FormDatum {
     StringData(DOMString),
diff --git a/src/components/script/dom/htmlcollection.rs b/src/components/script/dom/htmlcollection.rs
index 42712cf50b47..c14e27bd46ae 100644
--- a/src/components/script/dom/htmlcollection.rs
+++ b/src/components/script/dom/htmlcollection.rs
@@ -27,6 +27,7 @@ impl<S: Encoder<E>, E> Encodable<S, E> for Box<CollectionFilter> {
     }
 }
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub enum CollectionTypeId {
     Static(Vec<JS<Element>>),
diff --git a/src/components/script/dom/htmlimageelement.rs b/src/components/script/dom/htmlimageelement.rs
index 21c109195a3f..7037ca27c68a 100644
--- a/src/components/script/dom/htmlimageelement.rs
+++ b/src/components/script/dom/htmlimageelement.rs
@@ -85,6 +85,7 @@ pub trait LayoutHTMLImageElementHelpers {
     unsafe fn image(&self) -> Option<Url>;
 }
 
+#[warn(unrooted_js_managed)]
 impl LayoutHTMLImageElementHelpers for JS<HTMLImageElement> {
     unsafe fn image(&self) -> Option<Url> {
         (*self.unsafe_get()).image.borrow().clone()
diff --git a/src/components/script/dom/node.rs b/src/components/script/dom/node.rs
index a73c4d7e8798..a916ce33f64b 100644
--- a/src/components/script/dom/node.rs
+++ b/src/components/script/dom/node.rs
@@ -679,11 +679,13 @@ pub trait LayoutNodeHelpers {
     unsafe fn prev_sibling_ref(&self) -> Option<JS<Node>>;
     unsafe fn next_sibling_ref(&self) -> Option<JS<Node>>;
 
+    #[warn(unrooted_js_managed)]
     unsafe fn owner_doc_for_layout(&self) -> JS<Document>;
 
     unsafe fn is_element_for_layout(&self) -> bool;
 }
 
+#[warn(unrooted_js_managed)]
 impl LayoutNodeHelpers for JS<Node> {
     #[inline]
     unsafe fn type_id_for_layout(&self) -> NodeTypeId {
@@ -720,6 +722,7 @@ impl LayoutNodeHelpers for JS<Node> {
         (*self.unsafe_get()).next_sibling.get()
     }
 
+    #[warn(unrooted_js_managed)]
     #[inline]
     unsafe fn owner_doc_for_layout(&self) -> JS<Document> {
         (*self.unsafe_get()).owner_doc.get().unwrap()
@@ -804,6 +807,7 @@ impl<'a> Iterator<JSRef<'a, Node>> for TreeIterator<'a> {
     }
 }
 
+#[warn(unrooted_js_managed)]
 pub struct NodeIterator {
     pub start_node: JS<Node>,
     pub current_node: Option<JS<Node>>,
diff --git a/src/components/script/dom/nodelist.rs b/src/components/script/dom/nodelist.rs
index 61a9884e9a90..b5ba0296a994 100644
--- a/src/components/script/dom/nodelist.rs
+++ b/src/components/script/dom/nodelist.rs
@@ -10,6 +10,7 @@ use dom::bindings::utils::{Reflectable, Reflector, reflect_dom_object};
 use dom::node::{Node, NodeHelpers};
 use dom::window::Window;
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub enum NodeListType {
     Simple(Vec<JS<Node>>),
diff --git a/src/components/script/dom/performance.rs b/src/components/script/dom/performance.rs
index 82b931b0c2e6..198c2d329715 100644
--- a/src/components/script/dom/performance.rs
+++ b/src/components/script/dom/performance.rs
@@ -13,6 +13,7 @@ use time;
 
 pub type DOMHighResTimeStamp = f64;
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub struct Performance {
     reflector_: Reflector,
diff --git a/src/components/script/dom/xmlhttprequest.rs b/src/components/script/dom/xmlhttprequest.rs
index b2bfd64eb4e7..c44efba8411e 100644
--- a/src/components/script/dom/xmlhttprequest.rs
+++ b/src/components/script/dom/xmlhttprequest.rs
@@ -97,6 +97,7 @@ enum SyncOrAsync<'a, 'b> {
 }
 
 
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub struct XMLHttpRequest {
     eventtarget: XMLHttpRequestEventTarget,
diff --git a/src/components/script/page.rs b/src/components/script/page.rs
index 8543c887da7b..736b192a476e 100644
--- a/src/components/script/page.rs
+++ b/src/components/script/page.rs
@@ -418,6 +418,7 @@ impl Page {
 }
 
 /// Information for one frame in the browsing context.
+#[allow(unrooted_js_managed)]
 #[deriving(Encodable)]
 pub struct Frame {
     /// The document for this frame.