auto merge of #914 : jdm/servo/unsafepage, r=jdm,metajack

Haven't figured out what to do about page_from_context yet, but this is a big improvement.

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

@@ -6,7 +6,6 @@ use dom::bindings::codegen::PrototypeList;
 use dom::bindings::codegen::PrototypeList::MAX_PROTO_CHAIN_LENGTH;
 use dom::bindings::node;
 use dom::node::{AbstractNode, ScriptView};
-use script_task::page_from_context;
 
 use std::libc::c_uint;
 use std::cast;
@@ -31,7 +30,6 @@ use js::jsapi::{JSContext, JSObject, JSBool, jsid, JSClass, JSNative, JSTracer};
 use js::jsapi::{JSFunctionSpec, JSPropertySpec, JSVal, JSPropertyDescriptor};
 use js::jsapi::{JSPropertyOp, JSStrictPropertyOp, JS_NewGlobalObject, JS_InitStandardClasses};
 use js::jsfriendapi::bindgen::JS_NewObjectWithUniqueType;
-use js::rust::Compartment;
 use js::{JSPROP_ENUMERATE, JSVAL_NULL};
 use js::{JSPROP_PERMANENT, JSID_VOID, JSPROP_NATIVE_ACCESSORS, JSPROP_GETTER};
 use js::{JSPROP_SETTER, JSVAL_VOID, JSVAL_TRUE, JSVAL_FALSE};
@@ -237,32 +235,6 @@ pub unsafe fn domstring_to_jsval(cx: *JSContext, string: &DOMString) -> JSVal {
     }
 }
 
-pub fn get_compartment(cx: *JSContext) -> @mut Compartment {
-    unsafe {
-        let page = page_from_context(cx);
-        let compartment = (*page).js_info.get_ref().js_compartment;
-        assert!(cx == compartment.cx.ptr);
-        compartment
-    }
-}
-
-extern fn has_instance(_cx: *JSContext, obj: **JSObject, v: *JSVal, bp: *mut JSBool) -> JSBool {
-    //XXXjdm this is totally broken for non-object values
-    unsafe {
-        let mut o = RUST_JSVAL_TO_OBJECT(*v);
-        let obj = *obj;
-        *bp = 0;
-        while o.is_not_null() {
-            if o == obj {
-                *bp = 1;
-                break;
-            }
-            o = JS_GetPrototype(o);
-        }
-        return 1;
-    }
-}
-
 // We use slot 0 for holding the raw object.  This is safe for both
 // globals and non-globals.
 pub static DOM_OBJECT_SLOT: uint = 0;

src/components/script/dom/document.rs

@@ -107,7 +107,7 @@ impl Document {
             parent: HTMLElement::new(HTMLHtmlElementTypeId, ~"html")
         };
 
-        let cx = unsafe {(*owner.page).js_info.get_ref().js_compartment.cx.ptr};
+        let cx = owner.page.js_info.get_ref().js_compartment.cx.ptr;
         let root = unsafe { Node::as_abstract_node(cx, root) };
         AbstractDocument::as_abstract(cx, @mut Document::new(root, None, XML))
     }
@@ -211,12 +211,12 @@ impl Document {
 
     fn get_cx(&self) -> *JSContext {
         let win = self.window.get_ref();
-        unsafe {(*win.page).js_info.get_ref().js_compartment.cx.ptr}
+        win.page.js_info.get_ref().js_compartment.cx.ptr
     }
 
     fn get_scope_and_cx(&self) -> (*JSObject, *JSContext) {
         let win = self.window.get_ref();
-        let cx = unsafe {(*win.page).js_info.get_ref().js_compartment.cx.ptr};
+        let cx = win.page.js_info.get_ref().js_compartment.cx.ptr;
         let cache = win.get_wrappercache();
         let scope = cache.get_wrapper();
         (scope, cx)

src/components/script/dom/domparser.rs

@@ -26,7 +26,7 @@ impl DOMParser {
         };
 
         // TODO(tkuehn): This just handles the top-level page. Need to handle subframes.
-        let cx = unsafe {(*owner.page).js_info.get_ref().js_compartment.cx.ptr};
+        let cx = owner.page.js_info.get_ref().js_compartment.cx.ptr;
         let cache = owner.get_wrappercache();
         let scope = cache.get_wrapper();
         parser.wrap_object_shared(cx, scope);

src/components/script/dom/element.rs

@@ -178,7 +178,7 @@ impl<'self> Element {
     fn get_scope_and_cx(&self) -> (*JSObject, *JSContext) {
         let doc = self.parent.owner_doc.unwrap();
         let win = doc.with_base(|doc| doc.window.unwrap());
-        let cx = unsafe {(*win.page).js_info.get_ref().js_compartment.cx.ptr};
+        let cx = win.page.js_info.get_ref().js_compartment.cx.ptr;
         let cache = win.get_wrappercache();
         let scope = cache.get_wrapper();
         (scope, cx)
@@ -271,9 +271,9 @@ impl Element {
                         assert!(node.is_element());
                         let page = win.page;
                         let (port, chan) = comm::stream();
-                        match unsafe {(*page).query_layout(ContentBoxesQuery(node, chan), port)} {
+                        match page.query_layout(ContentBoxesQuery(node, chan), port) {
                             ContentBoxesResponse(rects) => {
-                                let cx = unsafe {(*page).js_info.get_ref().js_compartment.cx.ptr};
+                                let cx = page.js_info.get_ref().js_compartment.cx.ptr;
                                 let cache = win.get_wrappercache();
                                 let scope = cache.get_wrapper();
                                 let rects = do rects.map |r| {
@@ -313,9 +313,9 @@ impl Element {
                         let node = abstract_self;
                         assert!(node.is_element());
                         let (port, chan) = comm::stream();
-                        match unsafe{(*page).query_layout(ContentBoxQuery(node, chan), port)} {
+                        match page.query_layout(ContentBoxQuery(node, chan), port) {
                             ContentBoxResponse(rect) => {
-                                let cx = unsafe {(*page).js_info.get_ref().js_compartment.cx.ptr};
+                                let cx = page.js_info.get_ref().js_compartment.cx.ptr;
                                 let cache = win.get_wrappercache();
                                 let scope = cache.get_wrapper();
                                 ClientRect::new(

src/components/script/dom/htmldatalistelement.rs

@@ -16,7 +16,7 @@ impl HTMLDataListElement {
     fn get_scope_and_cx(&self) -> (*JSObject, *JSContext) {
         let doc = self.parent.parent.parent.owner_doc.unwrap();
         let win = doc.with_base(|doc| doc.window.unwrap());
-        let cx = unsafe {(*win.page).js_info.get_ref().js_compartment.cx.ptr};
+        let cx = win.page.js_info.get_ref().js_compartment.cx.ptr;
         let cache = win.get_wrappercache();
         let scope = cache.get_wrapper();
         (scope, cx)

src/components/script/dom/htmldocument.rs

@@ -29,13 +29,13 @@ impl HTMLDocument {
             parent: Document::new(root, window, HTML)
         };
 
-        let compartment = unsafe { (*window.get_ref().page).js_info.get_ref().js_compartment };
+        let compartment = window.get_ref().page.js_info.get_ref().js_compartment;
         AbstractDocument::as_abstract(compartment.cx.ptr, doc)
     }
 
     fn get_scope_and_cx(&self) -> (*JSObject, *JSContext) {
         let win = self.parent.window.get_ref();
-        let cx = unsafe {(*win.page).js_info.get_ref().js_compartment.cx.ptr};
+        let cx = win.page.js_info.get_ref().js_compartment.cx.ptr;
         let cache = win.get_wrappercache();
         let scope = cache.get_wrapper();
         (scope, cx)

src/components/script/dom/htmlfieldsetelement.rs

@@ -40,7 +40,7 @@ impl HTMLFieldSetElement {
     fn get_scope_and_cx(&self) -> (*JSObject, *JSContext) {
         let doc = self.parent.parent.parent.owner_doc.unwrap();
         let win = doc.with_base(|doc| doc.window.unwrap());
-        let cx = unsafe {(*win.page).js_info.get_ref().js_compartment.cx.ptr};
+        let cx = win.page.js_info.get_ref().js_compartment.cx.ptr;
         let cache = win.get_wrappercache();
         let scope = cache.get_wrapper();
         (scope, cx)

src/components/script/dom/htmlformelement.rs

@@ -18,7 +18,7 @@ impl HTMLFormElement {
     fn get_scope_and_cx(&self) -> (*JSObject, *JSContext) {
         let doc = self.parent.parent.parent.owner_doc.unwrap();
         let win = doc.with_base(|doc| doc.window.unwrap());
-        let cx = unsafe {(*win.page).js_info.get_ref().js_compartment.cx.ptr};
+        let cx = win.page.js_info.get_ref().js_compartment.cx.ptr;
         let cache = win.get_wrappercache();
         let scope = cache.get_wrapper();
         (scope, cx)

src/components/script/dom/htmlimageelement.rs

@@ -56,13 +56,11 @@ impl HTMLImageElement {
             Some(doc) => {
                 match doc.with_base(|doc| doc.window) {
                     Some(win) => {
-                        unsafe {
+                        let page = win.page;
-                            let page = win.page;
+                        let (port, chan) = stream();
-                            let (port, chan) = stream();
+                        match page.query_layout(ContentBoxQuery(abstract_self, chan), port) {
-                            match (*page).query_layout(ContentBoxQuery(abstract_self, chan), port) {
+                            ContentBoxResponse(rect) => {
-                                ContentBoxResponse(rect) => {
+                                to_px(rect.size.width) as u32
-                                    to_px(rect.size.width) as u32
-                                }
                             }
                         }
                     }
@@ -91,13 +89,11 @@ impl HTMLImageElement {
             Some(doc) => {
                 match doc.with_base(|doc| doc.window) {
                     Some(win) => {
-                        unsafe {
+                        let page = win.page;
-                            let page = win.page;
+                        let (port, chan) = stream();
-                            let (port, chan) = stream();
+                        match page.query_layout(ContentBoxQuery(abstract_self, chan), port) {
-                            match (*page).query_layout(ContentBoxQuery(abstract_self, chan), port) {
+                            ContentBoxResponse(rect) => {
-                                ContentBoxResponse(rect) => {
+                                to_px(rect.size.height) as u32
-                                    to_px(rect.size.height) as u32
-                                }
                             }
                         }
                     }

src/components/script/dom/htmlmapelement.rs

@@ -19,10 +19,10 @@ impl HTMLMapElement {
     pub fn SetName(&mut self, _name: &DOMString, _rv: &mut ErrorResult) {
     }
 
-        fn get_scope_and_cx(&self) -> (*JSObject, *JSContext) {
+    fn get_scope_and_cx(&self) -> (*JSObject, *JSContext) {
         let doc = self.parent.parent.parent.owner_doc.unwrap();
         let win = doc.with_base(|doc| doc.window.unwrap());
-        let cx = unsafe {(*win.page).js_info.get_ref().js_compartment.cx.ptr};
+        let cx = win.page.js_info.get_ref().js_compartment.cx.ptr;
         let cache = win.get_wrappercache();
         let scope = cache.get_wrapper();
         (scope, cx)

src/components/script/dom/text.rs

@@ -21,7 +21,7 @@ impl Text {
     }
 
     pub fn Constructor(owner: @mut Window, text: &DOMString, _rv: &mut ErrorResult) -> AbstractNode<ScriptView> {
-        let cx = unsafe {(*owner.page).js_info.get_ref().js_compartment.cx.ptr};
+        let cx = owner.page.js_info.get_ref().js_compartment.cx.ptr;
         unsafe { Node::as_abstract_node(cx, @Text::new(text.to_str())) }
     }
 

src/components/script/dom/window.rs

@@ -36,9 +36,8 @@ pub enum TimerControlMsg {
     TimerMessage_TriggerExit //XXXjdm this is just a quick hack to talk to the script task
 }
 
-//FIXME If we're going to store the page, find a way to do so safely.
 pub struct Window {
-    page: *mut Page,
+    page: @mut Page,
     script_chan: ScriptChan,
     compositor: @ScriptListener,
     wrapper: WrapperCache,
@@ -72,9 +71,7 @@ impl Window {
     }
 
     pub fn Document(&self) -> AbstractDocument {
-        unsafe {
+        self.page.frame.unwrap().document
-            (*self.page).frame.unwrap().document
-        }
     }
 
     pub fn Name(&self) -> DOMString {
@@ -168,16 +165,14 @@ impl Window {
     }
 
     pub fn content_changed(&self) {
-        unsafe {
+        // FIXME This should probably be ReflowForQuery, not Display. All queries currently
-            // FIXME This should probably be ReflowForQuery, not Display. All queries currently
+        // currently rely on the display list, which means we can't destroy it by
-            // currently rely on the display list, which means we can't destroy it by
+        // doing a query reflow.
-            // doing a query reflow.
+        self.page.reflow_all(ReflowForDisplay, self.script_chan.clone(), self.compositor);
-            (*self.page).reflow_all(ReflowForDisplay, self.script_chan.clone(), self.compositor);
-        }
     }
 
     #[fixed_stack_segment]
-    pub fn new(cx: *JSContext, page: *mut Page, script_chan: ScriptChan, compositor: @ScriptListener)
+    pub fn new(cx: *JSContext, page: @mut Page, script_chan: ScriptChan, compositor: @ScriptListener)
                -> @mut Window {
         let script_chan_clone = script_chan.clone();
         let win = @mut Window {
@@ -187,11 +182,12 @@ impl Window {
             wrapper: WrapperCache::new(),
             timer_chan: {
                 let (timer_port, timer_chan) = comm::stream::<TimerControlMsg>();
+                let id = page.id.clone();
                 do spawn {
                     loop {
                         match timer_port.recv() {
                             TimerMessage_Close => break,
-                            TimerMessage_Fire(td) => unsafe {script_chan_clone.chan.send(FireTimerMsg((*page).id.clone(), td))},
+                            TimerMessage_Fire(td) => script_chan_clone.chan.send(FireTimerMsg(id, td)),
                             TimerMessage_TriggerExit => script_chan_clone.chan.send(ExitMsg),
                         }
                     }
@@ -223,7 +219,7 @@ impl Traceable for Window {
     fn trace(&self, tracer: *mut JSTracer) {
         debug!("tracing window");
         unsafe {
-            match (*self.page).frame {
+            match self.page.frame {
                 Some(frame) => {
                     do frame.document.with_base |doc| {
                         (*tracer).debugPrinter = ptr::null();

src/components/script/script_task.rs

@@ -482,17 +482,18 @@ impl ScriptTask {
 
         let page_tree = self.page_tree.find(id).expect("ScriptTask: received fire timer msg for a
             pipeline ID not associated with this script task. This is a bug.");
-        let js_info = page_tree.page.js_info.get_ref();
+        let compartment = page_tree.page.js_info.get_ref().js_compartment;
+        let cx = page_tree.page.js_info.get_ref().js_context;
 
         match read_whole_file(&Path(url.path)) {
             Err(msg) => println(fmt!("Error opening %s: %s", url.to_str(), msg)),
 
             Ok(bytes) => {
-                js_info.js_compartment.define_functions(debug_fns);
+                compartment.define_functions(debug_fns);
-                js_info.js_context.evaluate_script(js_info.js_compartment.global_obj,
+                cx.evaluate_script(compartment.global_obj,
-                                                   bytes,
+                                   bytes,
-                                                   url.path.clone(),
+                                   url.path.clone(),
-                                                   1);
+                                   1);
             }
         }
     }
@@ -582,16 +583,7 @@ impl ScriptTask {
 
         let cx = self.js_runtime.cx();
         // Create the window and document objects.
-        let window = {
+        let window = Window::new(cx.ptr, page, self.chan.clone(), self.compositor);
-            // Need an extra block here due to Rust #6248
-            //
-            // FIXME(Servo #655): Unrelated to the Rust #6248 warning, this is fundamentally
-            // unsafe because the box could go away or get moved while we're holding this raw
-            // pointer.  We think it's safe here because the main task will hold onto the box,
-            // and because the current refcounting implementation of @ doesn't move.
-            let page = &mut *page;
-            Window::new(cx.ptr, page, self.chan.clone(), self.compositor)
-        };
         page.initialize_js_info(cx, window.get_wrappercache().get_wrapper());
 
         RegisterBindings::Register(page.js_info.get_ref().js_compartment);
@@ -669,15 +661,16 @@ impl ScriptTask {
         page.url = Some((url, false));
 
         // Define debug functions.
-        let js_info = page.js_info.get_ref();
+        let compartment = page.js_info.get_ref().js_compartment;
-        js_info.js_compartment.define_functions(debug_fns);
+        let cx = page.js_info.get_ref().js_context;
+        compartment.define_functions(debug_fns);
 
         // Evaluate every script in the document.
         for file in js_scripts.iter() {
-            let _ = js_info.js_context.evaluate_script(js_info.js_compartment.global_obj,
+            let _ = cx.evaluate_script(compartment.global_obj,
-                                                       file.data.clone(),
+                                       file.data.clone(),
-                                                       file.url.to_str(),
+                                       file.url.to_str(),
-                                                       1);
+                                       1);
         }
     }