auto merge of #3797 : kmcallister/servo/domrefcell, r=jdm

r? @mbrubeck, @jdm

Alternative to #3770 and #3716.

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;
+use servo_util::task_state;
+use servo_util::task_state::{Script, InGC};
 
-/// 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 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,
 }
 
-pub type Ref<'a, T> = cell::Ref<'a, T>;
-pub type RefMut<'a, T> = cell::RefMut<'a, T>;
-
+// Functionality specific to Servo's `DOMRefCell` type
+// ===================================================
 
 impl<T> DOMRefCell<T> {
-    #[inline(always)]
-    pub fn new(value: T) -> DOMRefCell<T> {
-        DOMRefCell {
-            base: RefCell::new(value),
+    /// 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 })
+            }
         }
     }
 
-    #[inline(always)]
-    pub fn unwrap(self) -> T {
-        self.base.unwrap()
-    }
-
-    #[inline(always)]
-    pub fn try_borrow<'a>(&'a self) -> Option<Ref<'a, T>> {
-        self.base.try_borrow()
-    }
-
-    #[inline(always)]
-    pub fn borrow<'a>(&'a self) -> Ref<'a, T> {
-        self.base.borrow()
-    }
-
-    #[inline(always)]
     pub fn try_borrow_mut<'a>(&'a self) -> Option<RefMut<'a, T>> {
-        self.base.try_borrow_mut()
-    }
-
-    #[inline(always)]
-    pub fn borrow_mut<'a>(&'a self) -> RefMut<'a, T> {
-        self.base.borrow_mut()
-    }
-
-    /// 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
+        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).base.borrow().trace(trc)
+        (*self).borrow().trace(trc)
+    }
+}
+
+// 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> {
+    pub fn new(value: T) -> DOMRefCell<T> {
+        DOMRefCell {
+            value: UnsafeCell::new(value),
+            borrow: Cell::new(UNUSED),
+            nocopy: marker::NoCopy,
+            nosync: marker::NoSync,
+        }
+    }
+
+    pub fn unwrap(self) -> T {
+        debug_assert!(self.borrow.get() == UNUSED);
+        unsafe{self.value.unwrap()}
+    }
+
+    pub fn borrow<'a>(&'a self) -> Ref<'a, T> {
+        match self.try_borrow() {
+            Some(ptr) => ptr,
+            None => fail!("DOMRefCell<T> already mutably borrowed")
+        }
+    }
+
+    pub fn borrow_mut<'a>(&'a self) -> RefMut<'a, T> {
+        match self.try_borrow_mut() {
+            Some(ptr) => ptr,
+            None => fail!("DOMRefCell<T> already borrowed")
+        }
+    }
+}
+
+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);
+    }
+}
+
+impl<'b, T> Deref<T> for Ref<'b, T> {
+    #[inline]
+    fn deref<'a>(&'a self) -> &'a T {
+        unsafe { &*self._parent.value.get() }
+    }
+}
+
+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<'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;
 

components/util/task.rs

@@ -8,22 +8,29 @@ use std::comm::Sender;
 use std::task::TaskBuilder;
 use native::task::NativeTaskBuilder;
 
+use task_state;
+
 pub fn spawn_named<S: IntoMaybeOwned<'static>>(name: S, f: proc():Send) {
     let builder = task::TaskBuilder::new().named(name);
     builder.spawn(f);
 }
 
-/// Arrange to send a particular message to a channel if the task built by
-/// this `TaskBuilder` fails.
+/// Arrange to send a particular message to a channel if the task fails.
 pub fn spawn_named_with_send_on_failure<T: Send>(name: &'static str,
+                                                 state: task_state::TaskState,
                                                  f: proc(): Send,
                                                  msg: T,
                                                  dest: Sender<T>,
                                                  native: bool) {
+    let with_state = proc() {
+        task_state::initialize(state);
+        f()
+    };
+
     let future_result = if native {
-        TaskBuilder::new().named(name).native().try_future(f)
+        TaskBuilder::new().named(name).native().try_future(with_state)
     } else {
-        TaskBuilder::new().named(name).try_future(f)
+        TaskBuilder::new().named(name).try_future(with_state)
     };
 
     let watched_name = name.to_string();

components/util/task_state.rs

@@ -0,0 +1,91 @@
+/* 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/. */
+
+//! Supports dynamic assertions in debug builds about what sort of task is
+//! running and what state it's in.
+//!
+//! In release builds, `get` returns 0.  All of the other functions inline
+//! away to nothing.
+
+pub use self::imp::{initialize, get, enter, exit};
+
+bitflags! {
+    #[deriving(Show)]
+    flags TaskState: u32 {
+        static Script        = 0x01,
+        static Layout        = 0x02,
+        static Render        = 0x04,
+
+        static InWorker      = 0x0100,
+        static InGC          = 0x0200,
+        static InHTMLParser  = 0x0400,
+    }
+}
+
+macro_rules! task_types ( ( $( $fun:ident = $flag:ident ; )* ) => (
+    impl TaskState {
+        $(
+            pub fn $fun(self) -> bool {
+                self.contains($flag)
+            }
+        )*
+    }
+
+    #[cfg(not(ndebug))]
+    static TYPES: &'static [TaskState]
+        = &[ $( $flag ),* ];
+))
+
+task_types! {
+    is_script = Script;
+    is_layout = Layout;
+    is_render = Render;
+}
+
+#[cfg(not(ndebug))]
+mod imp {
+    use super::{TaskState, TYPES};
+
+    local_data_key!(STATE: TaskState)
+
+    pub fn initialize(x: TaskState) {
+        match STATE.replace(Some(x)) {
+            None => (),
+            Some(s) => fail!("Task state already initialized as {}", s),
+        };
+        get(); // check the assertion below
+    }
+
+    pub fn get() -> TaskState {
+        let state = match STATE.get() {
+            None => fail!("Task state not initialized"),
+            Some(s) => *s,
+        };
+
+        // Exactly one of the task type flags should be set.
+        assert_eq!(1, TYPES.iter().filter(|&&ty| state.contains(ty)).count());
+        state
+    }
+
+    pub fn enter(x: TaskState) {
+        let state = get();
+        assert!(!state.intersects(x));
+        STATE.replace(Some(state | x));
+    }
+
+    pub fn exit(x: TaskState) {
+        let state = get();
+        assert!(state.contains(x));
+        STATE.replace(Some(state & !x));
+    }
+}
+
+#[cfg(ndebug)]
+mod imp {
+    use super::TaskState;
+    #[inline(always)] pub fn initialize(_: TaskState) { }
+    #[inline(always)] pub fn get() -> TaskState { TaskState::empty() }
+    #[inline(always)] pub fn enter(_: TaskState) { }
+    #[inline(always)] pub fn exit(_: TaskState) { }
+}

components/util/workqueue.rs

@@ -7,6 +7,8 @@
 //! Data associated with queues is simply a pair of unsigned integers. It is expected that a
 //! higher-level API on top of this could allow safe fork-join parallelism.
 
+use task_state;
+
 use native::task::NativeTaskBuilder;
 use rand::{Rng, XorShiftRng};
 use std::mem;
@@ -196,7 +198,10 @@ pub struct WorkQueue<QueueData, WorkData> {
 impl<QueueData: Send, WorkData: Send> WorkQueue<QueueData, WorkData> {
     /// Creates a new work queue and spawns all the threads associated with
     /// it.
-    pub fn new(task_name: &'static str, thread_count: uint, user_data: QueueData) -> WorkQueue<QueueData, WorkData> {
+    pub fn new(task_name: &'static str,
+               state: task_state::TaskState,
+               thread_count: uint,
+               user_data: QueueData) -> WorkQueue<QueueData, WorkData> {
         // Set up data structures.
         let (supervisor_chan, supervisor_port) = channel();
         let (mut infos, mut threads) = (vec!(), vec!());
@@ -231,6 +236,7 @@ impl<QueueData: Send, WorkData: Send> WorkQueue<QueueData, WorkData> {
         // Spawn threads.
         for thread in threads.into_iter() {
             TaskBuilder::new().named(task_name).native().spawn(proc() {
+                task_state::initialize(state | task_state::InWorker);
                 let mut thread = thread;
                 thread.start()
             })