From 49234484d6539a4d8df8374a9548c2004b8e68b7 Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Thu, 23 Oct 2014 18:10:00 -0700 Subject: [PATCH] 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 4aa993fec459..f15e829c87b5 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 6b81ab512c5f..88f0c9c82752 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 933202e81f61..8eca823236ae 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 398e9b8d4dbe..a5658ac6c5e8 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 7c714118b09f..76732cae4bea 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, } }