From 96e180a22c4aa9c4d6c2ed5f3237653ac845fd92 Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Thu, 23 Oct 2014 13:43:14 -0700 Subject: [PATCH 1/6] Customize RefCell instead of wrapping it This gets rid of a dubious transmute: let val = mem::transmute::<&RefCell, &T>(&self.base); The code duplication will be reduced once rust-lang/rust#18131 is fixed. --- components/script/dom/bindings/cell.rs | 172 ++++++++++++++++++------- 1 file changed, 125 insertions(+), 47 deletions(-) diff --git a/components/script/dom/bindings/cell.rs b/components/script/dom/bindings/cell.rs index a4a15b7d5e7..a8b8902f729 100644 --- a/components/script/dom/bindings/cell.rs +++ b/components/script/dom/bindings/cell.rs @@ -5,64 +5,142 @@ use dom::bindings::trace::JSTraceable; use js::jsapi::{JSTracer}; -use std::cell; -use std::cell::RefCell; -use std::mem; +use std::cell::{Cell, UnsafeCell}; +use std::kinds::marker; -/// 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` to add the special method. +/// A mutable field in the DOM. +/// +/// This extends the API of `core::cell::RefCell` to allow unsafe access in +/// certain situations. pub struct DOMRefCell { - base: RefCell, + value: UnsafeCell, + borrow: Cell, + 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 DOMRefCell { - #[inline(always)] - pub fn new(value: T) -> DOMRefCell { - DOMRefCell { - base: RefCell::new(value), - } - } - - #[inline(always)] - pub fn unwrap(self) -> T { - self.base.unwrap() - } - - #[inline(always)] - pub fn try_borrow<'a>(&'a self) -> Option> { - 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> { - 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` directly. + /// Return a reference to the contents. + /// + /// For use in the layout task only. pub unsafe fn borrow_for_layout<'a>(&'a self) -> &'a T { - let val = mem::transmute::<&RefCell, &T>(&self.base); - val + &*self.value.get() } } impl JSTraceable for DOMRefCell { 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 DOMRefCell { + pub fn new(value: T) -> DOMRefCell { + 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 try_borrow<'a>(&'a self) -> Option> { + match self.borrow.get() { + WRITING => None, + borrow => { + self.borrow.set(borrow + 1); + Some(Ref { _parent: self }) + } + } + } + + pub fn borrow<'a>(&'a self) -> Ref<'a, T> { + match self.try_borrow() { + Some(ptr) => ptr, + None => fail!("DOMRefCell already mutably borrowed") + } + } + + pub fn try_borrow_mut<'a>(&'a self) -> Option> { + match self.borrow.get() { + UNUSED => { + self.borrow.set(WRITING); + Some(RefMut { _parent: self }) + }, + _ => None + } + } + + pub fn borrow_mut<'a>(&'a self) -> RefMut<'a, T> { + match self.try_borrow_mut() { + Some(ptr) => ptr, + None => fail!("DOMRefCell already borrowed") + } + } +} + +pub struct Ref<'b, T:'b> { + _parent: &'b DOMRefCell +} + +#[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 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 +} + +#[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 for RefMut<'b, T> { + #[inline] + fn deref<'a>(&'a self) -> &'a T { + unsafe { &*self._parent.value.get() } + } +} + +impl<'b, T> DerefMut for RefMut<'b, T> { + #[inline] + fn deref_mut<'a>(&'a mut self) -> &'a mut T { + unsafe { &mut *self._parent.value.get() } } } From 0162214b1fce3787bb08118790fc4d59d8528306 Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Thu, 23 Oct 2014 16:02:58 -0700 Subject: [PATCH 2/6] Fix a layout method to use borrow_for_layout() --- components/script/dom/htmlimageelement.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/script/dom/htmlimageelement.rs b/components/script/dom/htmlimageelement.rs index bc0e58db9be..635839805c3 100644 --- a/components/script/dom/htmlimageelement.rs +++ b/components/script/dom/htmlimageelement.rs @@ -90,7 +90,7 @@ pub trait LayoutHTMLImageElementHelpers { impl LayoutHTMLImageElementHelpers for JS { unsafe fn image(&self) -> Option { - (*self.unsafe_get()).image.borrow().clone() + (*self.unsafe_get()).image.borrow_for_layout().clone() } } From 6ec0939a2248e0e092242076ed5b2cd2486c736c Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Thu, 23 Oct 2014 14:44:17 -0700 Subject: [PATCH 3/6] Dynamically check DOMRefCell access from layout in debug builds --- components/gfx/render_task.rs | 3 +- components/layout/layout_task.rs | 6 +- components/script/dom/bindings/cell.rs | 47 +++++----- .../script/dom/dedicatedworkerglobalscope.rs | 5 + components/script/script_task.rs | 3 +- components/util/lib.rs | 1 + components/util/task.rs | 15 ++- components/util/task_state.rs | 92 +++++++++++++++++++ components/util/workqueue.rs | 8 +- 9 files changed, 150 insertions(+), 30 deletions(-) create mode 100644 components/util/task_state.rs diff --git a/components/gfx/render_task.rs b/components/gfx/render_task.rs index 976a574f200..0662eef3d99 100644 --- a/components/gfx/render_task.rs +++ b/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 RenderTask 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)); diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index 114f9b85e84..db47f9e5801 100644 --- a/components/layout/layout_task.rs +++ b/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 }; diff --git a/components/script/dom/bindings/cell.rs b/components/script/dom/bindings/cell.rs index a8b8902f729..4aa993fec45 100644 --- a/components/script/dom/bindings/cell.rs +++ b/components/script/dom/bindings/cell.rs @@ -5,13 +5,15 @@ use dom::bindings::trace::JSTraceable; use js::jsapi::{JSTracer}; +use servo_util::task_state; + 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. +/// certain situations, with dynamic checking in debug builds. pub struct DOMRefCell { value: UnsafeCell, borrow: Cell, @@ -27,8 +29,31 @@ impl DOMRefCell { /// /// 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() } + + pub fn try_borrow<'a>(&'a self) -> Option> { + 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> { + debug_assert!(task_state::get().is_script()); + match self.borrow.get() { + UNUSED => { + self.borrow.set(WRITING); + Some(RefMut { _parent: self }) + }, + _ => None + } + } } impl JSTraceable for DOMRefCell { @@ -63,16 +88,6 @@ impl DOMRefCell { unsafe{self.value.unwrap()} } - pub fn try_borrow<'a>(&'a self) -> Option> { - match self.borrow.get() { - WRITING => None, - borrow => { - self.borrow.set(borrow + 1); - Some(Ref { _parent: self }) - } - } - } - pub fn borrow<'a>(&'a self) -> Ref<'a, T> { match self.try_borrow() { Some(ptr) => ptr, @@ -80,16 +95,6 @@ impl DOMRefCell { } } - pub fn try_borrow_mut<'a>(&'a self) -> Option> { - match self.borrow.get() { - UNUSED => { - self.borrow.set(WRITING); - Some(RefMut { _parent: self }) - }, - _ => None - } - } - pub fn borrow_mut<'a>(&'a self) -> RefMut<'a, T> { match self.try_borrow_mut() { Some(ptr) => ptr, diff --git a/components/script/dom/dedicatedworkerglobalscope.rs b/components/script/dom/dedicatedworkerglobalscope.rs index 84fc77d0249..76c30db5b4c 100644 --- a/components/script/dom/dedicatedworkerglobalscope.rs +++ b/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); diff --git a/components/script/script_task.rs b/components/script/script_task.rs index f07fff94983..398e9b8d4db 100644 --- a/components/script/script_task.rs +++ b/components/script/script_task.rs @@ -54,6 +54,7 @@ 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}; @@ -262,7 +263,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, layout_chan, diff --git a/components/util/lib.rs b/components/util/lib.rs index a53f3a1c3d9..0c5c5f65e55 100644 --- a/components/util/lib.rs +++ b/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; diff --git a/components/util/task.rs b/components/util/task.rs index b3e03771610..f286efe5bc3 100644 --- a/components/util/task.rs +++ b/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>(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(name: &'static str, + state: task_state::TaskState, f: proc(): Send, msg: T, dest: Sender, 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(); diff --git a/components/util/task_state.rs b/components/util/task_state.rs new file mode 100644 index 00000000000..7c714118b09 --- /dev/null +++ b/components/util/task_state.rs @@ -0,0 +1,92 @@ +/* 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` is not available; calls must be inside +//! `debug_assert!` or similar. All of the other functions inline away to +//! nothing. + +pub use self::imp::{initialize, enter, exit}; + +#[cfg(not(ndebug))] +pub use self::imp::get; + +bitflags! { + #[deriving(Show)] + flags TaskState: u32 { + static Script = 0x01, + static Layout = 0x02, + static Render = 0x04, + + static InWorker = 0x0100, + } +} + +// Exactly one of these should be set. +static task_types: &'static [TaskState] + = &[Script, Layout, Render]; + +macro_rules! predicates ( ( $( $f:ident = $c:ident ; )* ) => ( + impl TaskState { + $( + pub fn $f(self) -> bool { + self.contains($c) + } + )* + } +)) + +predicates! { + is_script = Script; + is_layout = Layout; + is_render = Render; +} + +#[cfg(not(ndebug))] +mod imp { + use super::{TaskState, task_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, task_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 enter(_: TaskState) { } + #[inline(always)] pub fn exit(_: TaskState) { } +} diff --git a/components/util/workqueue.rs b/components/util/workqueue.rs index a9f9419d81c..3a0b3e32f91 100644 --- a/components/util/workqueue.rs +++ b/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 { impl WorkQueue { /// 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 { + pub fn new(task_name: &'static str, + state: task_state::TaskState, + thread_count: uint, + user_data: QueueData) -> WorkQueue { // Set up data structures. let (supervisor_chan, supervisor_port) = channel(); let (mut infos, mut threads) = (vec!(), vec!()); @@ -231,6 +236,7 @@ impl WorkQueue { // 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() }) From 49234484d6539a4d8df8374a9548c2004b8e68b7 Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Thu, 23 Oct 2014 18:10:00 -0700 Subject: [PATCH 4/6] Ignore the HTML parser's borrow flag in GC tracing Adds some other dynamic checks in debug builds. --- components/script/dom/bindings/cell.rs | 18 ++++++++++++++++++ components/script/dom/servohtmlparser.rs | 20 +++++++++++++++----- components/script/parse/html.rs | 6 ++++++ components/script/script_task.rs | 18 ++++++++++++++++++ components/util/task_state.rs | 2 ++ 5 files changed, 59 insertions(+), 5 deletions(-) diff --git a/components/script/dom/bindings/cell.rs b/components/script/dom/bindings/cell.rs index 4aa993fec45..f15e829c87b 100644 --- a/components/script/dom/bindings/cell.rs +++ b/components/script/dom/bindings/cell.rs @@ -6,6 +6,7 @@ use dom::bindings::trace::JSTraceable; use js::jsapi::{JSTracer}; use servo_util::task_state; +use servo_util::task_state::{Script, InGC}; use std::cell::{Cell, UnsafeCell}; use std::kinds::marker; @@ -33,6 +34,23 @@ impl DOMRefCell { &*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. + #[cfg(not(ndebug))] + pub fn is_mutably_borrowed(&self) -> bool { + self.borrow.get() == WRITING + } + pub fn try_borrow<'a>(&'a self) -> Option> { debug_assert!(task_state::get().is_script()); match self.borrow.get() { diff --git a/components/script/dom/servohtmlparser.rs b/components/script/dom/servohtmlparser.rs index 6b81ab512c5..88f0c9c8275 100644 --- a/components/script/dom/servohtmlparser.rs +++ b/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 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; - 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); + } } } diff --git a/components/script/parse/html.rs b/components/script/parse/html.rs index 933202e81f6..8eca823236a 100644 --- a/components/script/parse/html.rs +++ b/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 = *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); diff --git a/components/script/script_task.rs b/components/script/script_task.rs index 398e9b8d4db..a5658ac6c5e 100644 --- a/components/script/script_task.rs +++ b/components/script/script_task.rs @@ -60,6 +60,7 @@ 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; @@ -285,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, @@ -376,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) } diff --git a/components/util/task_state.rs b/components/util/task_state.rs index 7c714118b09..76732cae4be 100644 --- a/components/util/task_state.rs +++ b/components/util/task_state.rs @@ -22,6 +22,8 @@ bitflags! { static Render = 0x04, static InWorker = 0x0100, + static InGC = 0x0200, + static InHTMLParser = 0x0400, } } From 4dee8ecdf09af1d3e2650b22e3bac02cc3107900 Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Fri, 24 Oct 2014 16:31:51 -0700 Subject: [PATCH 5/6] task_state: Generate the list of task types Also fix warnings. --- components/util/task_state.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/components/util/task_state.rs b/components/util/task_state.rs index 76732cae4be..f760b373285 100644 --- a/components/util/task_state.rs +++ b/components/util/task_state.rs @@ -27,21 +27,21 @@ bitflags! { } } -// Exactly one of these should be set. -static task_types: &'static [TaskState] - = &[Script, Layout, Render]; - -macro_rules! predicates ( ( $( $f:ident = $c:ident ; )* ) => ( +macro_rules! task_types ( ( $( $fun:ident = $flag:ident ; )* ) => ( impl TaskState { $( - pub fn $f(self) -> bool { - self.contains($c) + pub fn $fun(self) -> bool { + self.contains($flag) } )* } + + #[cfg(not(ndebug))] + static TYPES: &'static [TaskState] + = &[ $( $flag ),* ]; )) -predicates! { +task_types! { is_script = Script; is_layout = Layout; is_render = Render; @@ -49,7 +49,7 @@ predicates! { #[cfg(not(ndebug))] mod imp { - use super::{TaskState, task_types}; + use super::{TaskState, TYPES}; local_data_key!(STATE: TaskState) @@ -68,7 +68,7 @@ mod imp { }; // Exactly one of the task type flags should be set. - assert_eq!(1, task_types.iter().filter(|&&ty| state.contains(ty)).count()); + assert_eq!(1, TYPES.iter().filter(|&&ty| state.contains(ty)).count()); state } From f508a82582843cb0fdacdcd88be2f73d59529203 Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Fri, 24 Oct 2014 16:45:09 -0700 Subject: [PATCH 6/6] Provide safety check helpers in release builds debug_assert! uses if cfg!(not(ndebug)) { ... } so the body in a release build is dead code, but it still needs to compile. --- components/script/dom/bindings/cell.rs | 1 - components/util/task_state.rs | 11 ++++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/components/script/dom/bindings/cell.rs b/components/script/dom/bindings/cell.rs index f15e829c87b..18385b66f39 100644 --- a/components/script/dom/bindings/cell.rs +++ b/components/script/dom/bindings/cell.rs @@ -46,7 +46,6 @@ impl DOMRefCell { /// Is the cell mutably borrowed? /// /// For safety checks in debug builds only. - #[cfg(not(ndebug))] pub fn is_mutably_borrowed(&self) -> bool { self.borrow.get() == WRITING } diff --git a/components/util/task_state.rs b/components/util/task_state.rs index f760b373285..8ea42949aec 100644 --- a/components/util/task_state.rs +++ b/components/util/task_state.rs @@ -5,14 +5,10 @@ //! Supports dynamic assertions in debug builds about what sort of task is //! running and what state it's in. //! -//! In release builds, `get` is not available; calls must be inside -//! `debug_assert!` or similar. All of the other functions inline away to -//! nothing. +//! In release builds, `get` returns 0. All of the other functions inline +//! away to nothing. -pub use self::imp::{initialize, enter, exit}; - -#[cfg(not(ndebug))] -pub use self::imp::get; +pub use self::imp::{initialize, get, enter, exit}; bitflags! { #[deriving(Show)] @@ -89,6 +85,7 @@ mod imp { 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) { } }