Skip to content

Commit

Permalink
Implement a simple version of JS<T> safety checking
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
kmcallister committed Aug 4, 2014
1 parent 23e148a commit c20b50b
Show file tree
Hide file tree
Showing 24 changed files with 82 additions and 2 deletions.
36 changes: 36 additions & 0 deletions src/components/compiler_plugins/js_managed_lint.rs
Original file line number Diff line number Diff line change
@@ -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");
}
}
_ => (),
}
}
_ => (),
}
}
}
16 changes: 15 additions & 1 deletion src/components/compiler_plugins/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
1 change: 1 addition & 0 deletions src/components/script/dom/attrlist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions src/components/script/dom/bindings/codegen/CodegenRust.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions src/components/script/dom/bindings/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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)
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/bindings/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>),
Expand Down
5 changes: 4 additions & 1 deletion src/components/script/dom/bindings/js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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
}
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/bindings/trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/browsercontext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ impl BrowserContext {
}
}

#[allow(unrooted_js_managed)]
#[deriving(Encodable)]
pub struct SessionHistoryEntry {
document: JS<Document>,
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/clientrect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/clientrectlist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ pub enum IsHTMLDocument {
NonHTMLDocument,
}

#[allow(unrooted_js_managed)]
#[deriving(Encodable)]
pub struct Document {
pub node: Node,
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/domimplementation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/domparser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/domtokenlist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions src/components/script/dom/element.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/formdata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/htmlcollection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>>),
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/htmlimageelement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
4 changes: 4 additions & 0 deletions src/components/script/dom/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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>>,
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/nodelist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>>),
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/performance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use time;

pub type DOMHighResTimeStamp = f64;

#[allow(unrooted_js_managed)]
#[deriving(Encodable)]
pub struct Performance {
reflector_: Reflector,
Expand Down
1 change: 1 addition & 0 deletions src/components/script/dom/xmlhttprequest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ enum SyncOrAsync<'a, 'b> {
}


#[allow(unrooted_js_managed)]
#[deriving(Encodable)]
pub struct XMLHttpRequest {
eventtarget: XMLHttpRequestEventTarget,
Expand Down
1 change: 1 addition & 0 deletions src/components/script/page.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit c20b50b

Please sign in to comment.