Debug-only dynamic checks for layout and GC use of DOMRefCell

components/gfx/render_task.rs

@@ -30,6 +30,7 @@ use servo_util::geometry;
 use servo_util::opts;
 use servo_util::smallvec::{SmallVec, SmallVec1};
 use servo_util::task::spawn_named_with_send_on_failure;
+use servo_util::task_state;
 use servo_util::time::{TimeProfilerChan, profile};
 use servo_util::time;
 use std::comm::{Receiver, Sender, channel};
@@ -151,7 +152,7 @@ impl<C> RenderTask<C> where C: RenderListener + Send {
                   time_profiler_chan: TimeProfilerChan,
                   shutdown_chan: Sender<()>) {
         let ConstellationChan(c) = constellation_chan.clone();
-        spawn_named_with_send_on_failure("RenderTask", proc() {
+        spawn_named_with_send_on_failure("RenderTask", task_state::Render, proc() {
             { // Ensures RenderTask and graphics context are destroyed before shutdown msg
                 let native_graphics_context = compositor.get_graphics_metadata().map(
                     |md| NativePaintingGraphicsContext::from_metadata(&md));

components/layout/layout_task.rs

@@ -53,6 +53,7 @@ use servo_util::logical_geometry::LogicalPoint;
 use servo_util::opts;
 use servo_util::smallvec::{SmallVec, SmallVec1, VecLike};
 use servo_util::task::spawn_named_with_send_on_failure;
+use servo_util::task_state;
 use servo_util::time::{TimeProfilerChan, profile};
 use servo_util::time;
 use servo_util::workqueue::WorkQueue;
@@ -181,7 +182,7 @@ impl LayoutTaskFactory for LayoutTask {
                   time_profiler_chan: TimeProfilerChan,
                   shutdown_chan: Sender<()>) {
         let ConstellationChan(con_chan) = constellation_chan.clone();
-        spawn_named_with_send_on_failure("LayoutTask", proc() {
+        spawn_named_with_send_on_failure("LayoutTask", task_state::Layout, proc() {
             { // Ensures layout task is destroyed before we send shutdown message
                 let sender = chan.sender();
                 let layout =
@@ -251,7 +252,8 @@ impl LayoutTask {
         let screen_size = Size2D(Au(0), Au(0));
         let device = Device::new(Screen, opts::get().initial_window_size.as_f32());
         let parallel_traversal = if opts::get().layout_threads != 1 {
-            Some(WorkQueue::new("LayoutWorker", opts::get().layout_threads, ptr::null()))
+            Some(WorkQueue::new("LayoutWorker", task_state::Layout,
+                                opts::get().layout_threads, ptr::null()))
         } else {
             None
         };

components/script/dom/bindings/cell.rs

@@ -5,64 +5,164 @@
 use dom::bindings::trace::JSTraceable;
 use js::jsapi::{JSTracer};
 
-use std::cell;
-use std::cell::RefCell;
-use std::mem;
-
-/// A mutable field in DOM for large sized value.
-/// This has a special method to return the pointer of itself
-/// for used in layout task.
-/// This simply wraps `RefCell<T>` to add the special method.
+use servo_util::task_state;
+use servo_util::task_state::{Script, InGC};
+
+use std::cell::{Cell, UnsafeCell};
+use std::kinds::marker;
+
+/// A mutable field in the DOM.
+///
+/// This extends the API of `core::cell::RefCell` to allow unsafe access in
+/// certain situations, with dynamic checking in debug builds.
 pub struct DOMRefCell<T> {
-    base: RefCell<T>,
+    value: UnsafeCell<T>,
+    borrow: Cell<BorrowFlag>,
+    nocopy: marker::NoCopy,
+    nosync: marker::NoSync,
+}
+
+// Functionality specific to Servo's `DOMRefCell` type
+// ===================================================
+
+impl<T> DOMRefCell<T> {
+    /// Return a reference to the contents.
+    ///
+    /// For use in the layout task only.
+    pub unsafe fn borrow_for_layout<'a>(&'a self) -> &'a T {
+        debug_assert!(task_state::get().is_layout());
+        &*self.value.get()
+    }
+
+    /// Borrow the contents for the purpose of GC tracing.
+    ///
+    /// This succeeds even if the object is mutably borrowed,
+    /// so you have to be careful in trace code!
+    pub unsafe fn borrow_for_gc_trace<'a>(&'a self) -> &'a T {
+        debug_assert!(task_state::get().contains(Script | InGC));
+        &*self.value.get()
+    }
+
+    /// Is the cell mutably borrowed?
+    ///
+    /// For safety checks in debug builds only.
+    pub fn is_mutably_borrowed(&self) -> bool {
+        self.borrow.get() == WRITING
+    }
+
+    pub fn try_borrow<'a>(&'a self) -> Option<Ref<'a, T>> {
+        debug_assert!(task_state::get().is_script());
+        match self.borrow.get() {
+            WRITING => None,
+            borrow => {
+                self.borrow.set(borrow + 1);
+                Some(Ref { _parent: self })
+            }
+        }
+    }
+
+    pub fn try_borrow_mut<'a>(&'a self) -> Option<RefMut<'a, T>> {
+        debug_assert!(task_state::get().is_script());
+        match self.borrow.get() {
+            UNUSED => {
+                self.borrow.set(WRITING);
+                Some(RefMut { _parent: self })
+            },
+            _ => None
+        }
+    }
+}
+
+impl<T: JSTraceable> JSTraceable for DOMRefCell<T> {
+    fn trace(&self, trc: *mut JSTracer) {
+        (*self).borrow().trace(trc)
+    }
 }
 
-pub type Ref<'a, T> = cell::Ref<'a, T>;
-pub type RefMut<'a, T> = cell::RefMut<'a, T>;
+// Functionality duplicated with `core::cell::RefCell`
+// ===================================================
+//
+// This can shrink once rust-lang/rust#18131 is fixed.
 
+// Values [1, MAX-1] represent the number of `Ref` active
+// (will not outgrow its range since `uint` is the size of the address space)
+type BorrowFlag = uint;
+static UNUSED: BorrowFlag = 0;
+static WRITING: BorrowFlag = -1;
 
 impl<T> DOMRefCell<T> {
-    #[inline(always)]
     pub fn new(value: T) -> DOMRefCell<T> {
         DOMRefCell {
-            base: RefCell::new(value),
+            value: UnsafeCell::new(value),
+            borrow: Cell::new(UNUSED),
+            nocopy: marker::NoCopy,
+            nosync: marker::NoSync,
         }
     }
 
-    #[inline(always)]
     pub fn unwrap(self) -> T {
-        self.base.unwrap()
+        debug_assert!(self.borrow.get() == UNUSED);
+        unsafe{self.value.unwrap()}
     }
 
-    #[inline(always)]
-    pub fn try_borrow<'a>(&'a self) -> Option<Ref<'a, T>> {
-        self.base.try_borrow()
+    pub fn borrow<'a>(&'a self) -> Ref<'a, T> {
+        match self.try_borrow() {
+            Some(ptr) => ptr,
+            None => fail!("DOMRefCell<T> already mutably borrowed")
+        }
     }
 
-    #[inline(always)]
-    pub fn borrow<'a>(&'a self) -> Ref<'a, T> {
-        self.base.borrow()
+    pub fn borrow_mut<'a>(&'a self) -> RefMut<'a, T> {
+        match self.try_borrow_mut() {
+            Some(ptr) => ptr,
+            None => fail!("DOMRefCell<T> already borrowed")
+        }
     }
+}
 
-    #[inline(always)]
-    pub fn try_borrow_mut<'a>(&'a self) -> Option<RefMut<'a, T>> {
-        self.base.try_borrow_mut()
+pub struct Ref<'b, T:'b> {
+    _parent: &'b DOMRefCell<T>
+}
+
+#[unsafe_destructor]
+impl<'b, T> Drop for Ref<'b, T> {
+    fn drop(&mut self) {
+        let borrow = self._parent.borrow.get();
+        debug_assert!(borrow != WRITING && borrow != UNUSED);
+        self._parent.borrow.set(borrow - 1);
     }
+}
 
-    #[inline(always)]
-    pub fn borrow_mut<'a>(&'a self) -> RefMut<'a, T> {
-        self.base.borrow_mut()
+impl<'b, T> Deref<T> for Ref<'b, T> {
+    #[inline]
+    fn deref<'a>(&'a self) -> &'a T {
+        unsafe { &*self._parent.value.get() }
     }
+}
 
-    /// This returns the pointer which refers T in `RefCell<T>` directly.
-    pub unsafe fn borrow_for_layout<'a>(&'a self) -> &'a T {
-        let val = mem::transmute::<&RefCell<T>, &T>(&self.base);
-        val
+pub struct RefMut<'b, T:'b> {
+    _parent: &'b DOMRefCell<T>
+}
+
+#[unsafe_destructor]
+impl<'b, T> Drop for RefMut<'b, T> {
+    fn drop(&mut self) {
+        let borrow = self._parent.borrow.get();
+        debug_assert!(borrow == WRITING);
+        self._parent.borrow.set(UNUSED);
     }
 }
 
-impl<T: JSTraceable> JSTraceable for DOMRefCell<T> {
-    fn trace(&self, trc: *mut JSTracer) {
-        (*self).base.borrow().trace(trc)
+impl<'b, T> Deref<T> for RefMut<'b, T> {
+    #[inline]
+    fn deref<'a>(&'a self) -> &'a T {
+        unsafe { &*self._parent.value.get() }
+    }
+}
+
+impl<'b, T> DerefMut<T> for RefMut<'b, T> {
+    #[inline]
+    fn deref_mut<'a>(&'a mut self) -> &'a mut T {
+        unsafe { &mut *self._parent.value.get() }
     }
 }

components/script/dom/dedicatedworkerglobalscope.rs

@@ -24,6 +24,8 @@ use script_task::WorkerPostMessage;
 use script_task::StackRootTLS;
 
 use servo_net::resource_task::{ResourceTask, load_whole_resource};
+use servo_util::task_state;
+use servo_util::task_state::{Script, InWorker};
 
 use js::glue::JS_STRUCTURED_CLONE_VERSION;
 use js::jsapi::{JSContext, JS_ReadStructuredClone, JS_WriteStructuredClone, JS_ClearPendingException};
@@ -90,6 +92,9 @@ impl DedicatedWorkerGlobalScope {
             .native()
             .named(format!("Web Worker at {}", worker_url.serialize()))
             .spawn(proc() {
+
+            task_state::initialize(Script | InWorker);
+
             let roots = RootCollection::new();
             let _stack_roots_tls = StackRootTLS::new(&roots);
 

components/script/dom/htmlimageelement.rs

@@ -90,7 +90,7 @@ pub trait LayoutHTMLImageElementHelpers {
 
 impl LayoutHTMLImageElementHelpers for JS<HTMLImageElement> {
     unsafe fn image(&self) -> Option<Url> {
-        (*self.unsafe_get()).image.borrow().clone()
+        (*self.unsafe_get()).image.borrow_for_layout().clone()
     }
 }
 

components/script/dom/servohtmlparser.rs

@@ -15,6 +15,8 @@ use dom::node::TrustedNodeAddress;
 use dom::document::{Document, DocumentHelpers};
 use parse::html::JSMessage;
 
+use servo_util::task_state;
+
 use std::default::Default;
 use url::Url;
 use js::jsapi::JSTracer;
@@ -91,15 +93,23 @@ impl tree_builder::Tracer<TrustedNodeAddress> for Tracer {
 
 impl JSTraceable for ServoHTMLParser {
     fn trace(&self, trc: *mut JSTracer) {
+        self.reflector_.trace(trc);
+
         let tracer = Tracer {
             trc: trc,
         };
         let tracer = &tracer as &tree_builder::Tracer<TrustedNodeAddress>;
 
-        self.reflector_.trace(trc);
-        let tokenizer = self.tokenizer.borrow();
-        let tree_builder = tokenizer.sink();
-        tree_builder.trace_handles(tracer);
-        tree_builder.sink().trace(trc);
+        unsafe {
+            // Assertion: If the parser is mutably borrowed, we're in the
+            // parsing code paths.
+            debug_assert!(task_state::get().contains(task_state::InHTMLParser)
+                || !self.tokenizer.is_mutably_borrowed());
+
+            let tokenizer = self.tokenizer.borrow_for_gc_trace();
+            let tree_builder = tokenizer.sink();
+            tree_builder.trace_handles(tracer);
+            tree_builder.sink().trace(trc);
+        }
     }
 }

components/script/parse/html.rs

@@ -25,6 +25,8 @@ use encoding::types::{Encoding, DecodeReplace};
 use servo_net::resource_task::{Load, LoadData, Payload, Done, ResourceTask, load_whole_resource};
 use servo_msg::constellation_msg::LoadData as MsgLoadData;
 use servo_util::task::spawn_named;
+use servo_util::task_state;
+use servo_util::task_state::InHTMLParser;
 use servo_util::str::DOMString;
 use std::ascii::StrAsciiExt;
 use std::comm::{channel, Sender, Receiver};
@@ -480,6 +482,8 @@ pub fn parse_html(page: &Page,
     let parser = ServoHTMLParser::new(js_chan.clone(), base_url.clone(), document).root();
     let parser: JSRef<ServoHTMLParser> = *parser;
 
+    task_state::enter(InHTMLParser);
+
     match input {
         InputString(s) => {
             parser.tokenizer().borrow_mut().feed(s);
@@ -512,6 +516,8 @@ pub fn parse_html(page: &Page,
 
     parser.tokenizer().borrow_mut().end();
 
+    task_state::exit(InHTMLParser);
+
     debug!("finished parsing");
     js_chan.send(JSTaskExit);
 

components/script/script_task.rs

@@ -54,11 +54,13 @@ use servo_net::resource_task::ResourceTask;
 use servo_util::geometry::to_frac_px;
 use servo_util::smallvec::{SmallVec1, SmallVec};
 use servo_util::task::spawn_named_with_send_on_failure;
+use servo_util::task_state;
 
 use geom::point::Point2D;
 use js::jsapi::{JS_SetWrapObjectCallbacks, JS_SetGCZeal, JS_DEFAULT_ZEAL_FREQ, JS_GC};
 use js::jsapi::{JSContext, JSRuntime, JSTracer};
 use js::jsapi::{JS_SetGCParameter, JSGC_MAX_BYTES};
+use js::jsapi::{JS_SetGCCallback, JSGCStatus, JSGC_BEGIN, JSGC_END};
 use js::rust::{Cx, RtUtils};
 use js::rust::with_compartment;
 use js;
@@ -262,7 +264,7 @@ impl ScriptTaskFactory for ScriptTask {
         let ConstellationChan(const_chan) = constellation_chan.clone();
         let (script_chan, script_port) = channel();
         let layout_chan = LayoutChan(layout_chan.sender());
-        spawn_named_with_send_on_failure("ScriptTask", proc() {
+        spawn_named_with_send_on_failure("ScriptTask", task_state::Script, proc() {
             let script_task = ScriptTask::new(id,
                                               compositor as Box<ScriptListener>,
                                               layout_chan,
@@ -284,6 +286,16 @@ impl ScriptTaskFactory for ScriptTask {
     }
 }
 
+unsafe extern "C" fn debug_gc_callback(rt: *mut JSRuntime, status: JSGCStatus) {
+    js::rust::gc_callback(rt, status);
+
+    match status {
+        JSGC_BEGIN => task_state::enter(task_state::InGC),
+        JSGC_END   => task_state::exit(task_state::InGC),
+        _ => (),
+    }
+}
+
 impl ScriptTask {
     /// Creates a new script task.
     pub fn new(id: PipelineId,
@@ -375,6 +387,13 @@ impl ScriptTask {
             JS_SetGCZeal((*js_context).ptr, 0, JS_DEFAULT_ZEAL_FREQ);
         }
 
+        // Needed for debug assertions about whether GC is running.
+        if !cfg!(ndebug) {
+            unsafe {
+                JS_SetGCCallback(js_runtime.ptr, Some(debug_gc_callback));
+            }
+        }
+
         (js_runtime, js_context)
     }
 

components/util/lib.rs

@@ -52,6 +52,7 @@ pub mod task;
 pub mod tid;
 pub mod time;
 pub mod taskpool;
+pub mod task_state;
 pub mod vec;
 pub mod workqueue;