From 88a1cbb28be8721c70237b603356e7863892b798 Mon Sep 17 00:00:00 2001 From: Eli Friedman Date: Wed, 14 Oct 2015 16:20:47 -0700 Subject: [PATCH] Stop implementing Copy for JS. A copy of a JS doesn't have the rooting properties of the original, so it makes no sense for it to implement Copy. --- components/script/dom/bindings/js.rs | 60 +++++++++++-------- components/script/dom/browsercontext.rs | 2 +- .../script/dom/canvasrenderingcontext2d.rs | 4 +- components/script/dom/htmlcanvaselement.rs | 2 +- components/script/dom/htmlinputelement.rs | 2 +- components/script/dom/nodelist.rs | 2 +- .../script/dom/webglrenderingcontext.rs | 10 ++-- 7 files changed, 47 insertions(+), 35 deletions(-) diff --git a/components/script/dom/bindings/js.rs b/components/script/dom/bindings/js.rs index 16fa12e7189..c7365a13bf8 100644 --- a/components/script/dom/bindings/js.rs +++ b/components/script/dom/bindings/js.rs @@ -32,9 +32,10 @@ use js::jsapi::{Heap, JSObject, JSTracer}; use js::jsval::JSVal; use layout_interface::TrustedNodeAddress; use script_task::STACK_ROOTS; -use std::cell::{Cell, UnsafeCell}; +use std::cell::UnsafeCell; use std::default::Default; use std::ops::Deref; +use std::ptr; use util::mem::HeapSizeOf; /// A traced reference to a DOM object. Must only be used as a field in other @@ -54,7 +55,7 @@ impl HeapSizeOf for JS { impl JS { /// Returns `LayoutJS` containing the same pointer. - pub unsafe fn to_layout(self) -> LayoutJS { + pub unsafe fn to_layout(&self) -> LayoutJS { LayoutJS { ptr: self.ptr.clone() } @@ -109,8 +110,6 @@ impl LayoutJS { } } -impl Copy for JS {} - impl Copy for LayoutJS {} impl PartialEq for JS { @@ -208,56 +207,62 @@ impl MutHeapJSVal { /// `JS`. #[must_root] #[derive(JSTraceable)] -#[derive(HeapSizeOf)] -pub struct MutHeap { - val: Cell, +pub struct MutHeap { + val: UnsafeCell, } -impl MutHeap { +impl MutHeap { /// Create a new `MutHeap`. pub fn new(initial: T) -> MutHeap { MutHeap { - val: Cell::new(initial), + val: UnsafeCell::new(initial), } } /// Set this `MutHeap` to the given value. pub fn set(&self, val: T) { - self.val.set(val) + unsafe { *self.val.get() = val; } } /// Set the value in this `MutHeap`. pub fn get(&self) -> T { - self.val.get() + unsafe { ptr::read(self.val.get()) } + } +} + +impl HeapSizeOf for MutHeap { + fn heap_size_of_children(&self) -> usize { + // See comment on HeapSizeOf for JS. + 0 } } /// A mutable holder for GC-managed values such as `JSval` and `JS`, with -/// nullability represented by an enclosing Option wrapper. Must be used in -/// place of traditional internal mutability to ensure that the proper GC -/// barriers are enforced. +/// nullability represented by an enclosing Option wrapper. Roughly equivalent +/// to a DOMRefCell>>, but smaller; the cost is that values which +/// are read must be immediately rooted. #[must_root] -#[derive(JSTraceable, HeapSizeOf)] -pub struct MutNullableHeap { - ptr: Cell> +#[derive(JSTraceable)] +pub struct MutNullableHeap { + ptr: UnsafeCell> } -impl MutNullableHeap { +impl MutNullableHeap { /// Create a new `MutNullableHeap`. pub fn new(initial: Option) -> MutNullableHeap { MutNullableHeap { - ptr: Cell::new(initial) + ptr: UnsafeCell::new(initial) } } /// Set this `MutNullableHeap` to the given value. pub fn set(&self, val: Option) { - self.ptr.set(val); + unsafe { *self.ptr.get() = val; } } /// Retrieve a copy of the current optional inner value. pub fn get(&self) -> Option { - self.ptr.get() + unsafe { ptr::read(self.ptr.get()) } } } @@ -280,7 +285,7 @@ impl MutNullableHeap> { /// Retrieve a copy of the inner optional `JS` as `LayoutJS`. /// For use by layout, which can't use safe types like Temporary. pub unsafe fn get_inner_as_layout(&self) -> Option> { - self.ptr.get().map(|js| js.to_layout()) + ptr::read(self.ptr.get()).map(|js| js.to_layout()) } /// Get a rooted value out of this object @@ -290,15 +295,22 @@ impl MutNullableHeap> { } } -impl Default for MutNullableHeap { +impl Default for MutNullableHeap { #[allow(unrooted_must_root)] fn default() -> MutNullableHeap { MutNullableHeap { - ptr: Cell::new(None) + ptr: UnsafeCell::new(None) } } } +impl HeapSizeOf for MutNullableHeap { + fn heap_size_of_children(&self) -> usize { + // See comment on HeapSizeOf for JS. + 0 + } +} + impl LayoutJS { /// Returns an unsafe pointer to the interior of this JS object. This is /// the only method that be safely accessed from layout. (The fact that diff --git a/components/script/dom/browsercontext.rs b/components/script/dom/browsercontext.rs index 3df44a5ba9b..af2bfbdc81e 100644 --- a/components/script/dom/browsercontext.rs +++ b/components/script/dom/browsercontext.rs @@ -54,7 +54,7 @@ impl BrowsingContext { } pub fn frame_element(&self) -> Option> { - self.frame_element.map(Root::from_rooted) + self.frame_element.as_ref().map(JS::root) } pub fn window_proxy(&self) -> *mut JSObject { diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs index 3664574814e..89d07cf815c 100644 --- a/components/script/dom/canvasrenderingcontext2d.rs +++ b/components/script/dom/canvasrenderingcontext2d.rs @@ -763,7 +763,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { serialize(rgba, &mut result).unwrap(); StringOrCanvasGradientOrCanvasPattern::eString(result) }, - CanvasFillOrStrokeStyle::Gradient(gradient) => { + CanvasFillOrStrokeStyle::Gradient(ref gradient) => { StringOrCanvasGradientOrCanvasPattern::eCanvasGradient(gradient.root()) }, } @@ -803,7 +803,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { serialize(rgba, &mut result).unwrap(); StringOrCanvasGradientOrCanvasPattern::eString(result) }, - CanvasFillOrStrokeStyle::Gradient(gradient) => { + CanvasFillOrStrokeStyle::Gradient(ref gradient) => { StringOrCanvasGradientOrCanvasPattern::eCanvasGradient(gradient.root()) }, } diff --git a/components/script/dom/htmlcanvaselement.rs b/components/script/dom/htmlcanvaselement.rs index f698f42fe26..d71c86a3879 100644 --- a/components/script/dom/htmlcanvaselement.rs +++ b/components/script/dom/htmlcanvaselement.rs @@ -32,7 +32,7 @@ const DEFAULT_WIDTH: u32 = 300; const DEFAULT_HEIGHT: u32 = 150; #[must_root] -#[derive(JSTraceable, Clone, Copy, HeapSizeOf)] +#[derive(JSTraceable, Clone, HeapSizeOf)] pub enum CanvasContext { Context2d(JS), WebGL(JS), diff --git a/components/script/dom/htmlinputelement.rs b/components/script/dom/htmlinputelement.rs index cc0aabdc9cb..cdfe8716775 100644 --- a/components/script/dom/htmlinputelement.rs +++ b/components/script/dom/htmlinputelement.rs @@ -758,7 +758,7 @@ impl Activatable for HTMLInputElement { InputType::InputRadio => { // We want to restore state only if the element had been changed in the first place if cache.was_mutable { - let old_checked: Option> = cache.checked_radio.map(|t| t.root()); + let old_checked = cache.checked_radio.as_ref().map(|t| t.root()); let name = self.get_radio_group_name(); match old_checked { Some(ref o) => { diff --git a/components/script/dom/nodelist.rs b/components/script/dom/nodelist.rs index d997cad2558..dfea6e98eb4 100644 --- a/components/script/dom/nodelist.rs +++ b/components/script/dom/nodelist.rs @@ -64,7 +64,7 @@ impl NodeListMethods for NodeList { fn Item(&self, index: u32) -> Option> { match self.list_type { NodeListType::Simple(ref elems) => { - elems.get(index as usize).map(|node| Root::from_rooted(*node)) + elems.get(index as usize).map(|node| node.root()) }, NodeListType::Children(ref list) => list.item(index), } diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs index f5a3569da13..cae331679a9 100644 --- a/components/script/dom/webglrenderingcontext.rs +++ b/components/script/dom/webglrenderingcontext.rs @@ -12,7 +12,7 @@ use dom::bindings::codegen::InheritTypes::{EventCast, EventTargetCast, NodeCast} use dom::bindings::codegen::UnionTypes::ImageDataOrHTMLImageElementOrHTMLCanvasElementOrHTMLVideoElement; use dom::bindings::conversions::ToJSValConvertible; use dom::bindings::global::{GlobalField, GlobalRef}; -use dom::bindings::js::{JS, LayoutJS, Root}; +use dom::bindings::js::{JS, LayoutJS, MutNullableHeap, Root}; use dom::bindings::utils::{Reflector, reflect_dom_object}; use dom::event::{EventBubbles, EventCancelable}; use dom::htmlcanvaselement::HTMLCanvasElement; @@ -78,8 +78,8 @@ pub struct WebGLRenderingContext { canvas: JS, last_error: Cell>, texture_unpacking_settings: Cell, - bound_texture_2d: Cell>>, - bound_texture_cube_map: Cell>>, + bound_texture_2d: MutNullableHeap>, + bound_texture_cube_map: MutNullableHeap>, } impl WebGLRenderingContext { @@ -104,8 +104,8 @@ impl WebGLRenderingContext { canvas: JS::from_ref(canvas), last_error: Cell::new(None), texture_unpacking_settings: Cell::new(CONVERT_COLORSPACE), - bound_texture_2d: Cell::new(None), - bound_texture_cube_map: Cell::new(None), + bound_texture_2d: MutNullableHeap::new(None), + bound_texture_cube_map: MutNullableHeap::new(None), } }) }