From 946581456dc08262056e69145d53e4068aaad6e6 Mon Sep 17 00:00:00 2001 From: Iban Eguia Moraza Date: Sun, 10 May 2020 11:59:25 +0200 Subject: [PATCH 1/4] Trying to fix unsoundness in `InternalState` --- boa/src/builtins/console/mod.rs | 3 ++- boa/src/builtins/function/mod.rs | 6 +++++- boa/src/builtins/object/internal_state.rs | 17 +++++++++-------- boa/src/builtins/regexp/mod.rs | 12 +++++++++--- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/boa/src/builtins/console/mod.rs b/boa/src/builtins/console/mod.rs index bd5cf26cea0..4b8a14f5eda 100644 --- a/boa/src/builtins/console/mod.rs +++ b/boa/src/builtins/console/mod.rs @@ -23,11 +23,12 @@ use crate::{ }, exec::Interpreter, }; +use gc::{Finalize, Trace}; use rustc_hash::FxHashMap; use std::time::SystemTime; /// This is the internal console object state. -#[derive(Debug, Default)] +#[derive(Debug, Default, Trace, Finalize)] pub struct ConsoleState { count_map: FxHashMap, timer_map: FxHashMap, diff --git a/boa/src/builtins/function/mod.rs b/boa/src/builtins/function/mod.rs index e0c7a346bc4..d7fbad405f4 100644 --- a/boa/src/builtins/function/mod.rs +++ b/boa/src/builtins/function/mod.rs @@ -39,12 +39,16 @@ pub enum ConstructorKind { /// Defines how this references are interpreted within the formal parameters and code body of the function. /// /// Arrow functions don't define a `this` and thus are lexical, `function`s do define a this and thus are NonLexical -#[derive(Trace, Finalize, Debug, Clone)] +#[derive(Finalize, Copy, Debug, Clone)] pub enum ThisMode { Lexical, NonLexical, } +unsafe impl Trace for ThisMode { + unsafe_empty_trace!(); +} + /// FunctionBody is specific to this interpreter, it will either be Rust code or JavaScript code (AST Node) #[derive(Clone, Finalize)] pub enum FunctionBody { diff --git a/boa/src/builtins/object/internal_state.rs b/boa/src/builtins/object/internal_state.rs index 06955258cc5..8c417684412 100644 --- a/boa/src/builtins/object/internal_state.rs +++ b/boa/src/builtins/object/internal_state.rs @@ -1,5 +1,6 @@ //! Implementations for storing normal rust structs inside any object as internal state. +use gc::{unsafe_empty_trace, Finalize, Trace}; use std::{ any::Any, fmt::{self, Debug}, @@ -7,13 +8,11 @@ use std::{ rc::Rc, }; -use gc::{unsafe_empty_trace, Finalize, Trace}; - /// Wrapper around `Rc` to implement `Trace` and `Finalize`. #[derive(Clone)] pub struct InternalStateCell { /// The internal state. - state: Rc, + state: Rc, } impl Finalize for InternalStateCell {} @@ -23,7 +22,7 @@ unsafe impl Trace for InternalStateCell { } impl Deref for InternalStateCell { - type Target = dyn Any; + type Target = dyn InternalState; fn deref(&self) -> &Self::Target { Deref::deref(&self.state) } @@ -45,20 +44,22 @@ impl Debug for InternalStateCell { impl InternalStateCell { /// Create new `InternalStateCell` from a value. - pub fn new(value: T) -> Self { + pub fn new(value: T) -> Self { Self { state: Rc::new(value), } } + /// Get a reference to the stored value and cast it to `T`. pub fn downcast_ref(&self) -> Option<&T> { self.deref().downcast_ref::() } + /// Get a mutable reference to the stored value and cast it to `T`. - pub fn downcast_mut(&mut self) -> Option<&mut T> { - self.deref_mut().downcast_mut::() + pub fn downcast_mut(&mut self) -> Option<&mut T> { + self.state.downcast_mut::() } } /// This trait must be implemented by all structs used for internal state. -pub trait InternalState: Debug {} +pub trait InternalState: Debug + Trace + Any {} diff --git a/boa/src/builtins/regexp/mod.rs b/boa/src/builtins/regexp/mod.rs index b2cbecc2610..168453bc69f 100644 --- a/boa/src/builtins/regexp/mod.rs +++ b/boa/src/builtins/regexp/mod.rs @@ -9,9 +9,9 @@ //! [spec]: https://tc39.es/ecma262/#sec-regexp-constructor //! [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp -use std::ops::Deref; - +use gc::{unsafe_empty_trace, Finalize, Gc, Trace}; use regex::Regex; +use std::ops::Deref; use crate::{ builtins::{ @@ -26,7 +26,7 @@ use crate::{ mod tests; /// The internal representation on a `RegExp` object. -#[derive(Debug)] +#[derive(Debug, Finalize)] struct RegExp { /// Regex matcher. matcher: Regex, @@ -56,6 +56,12 @@ struct RegExp { unicode: bool, } +// FIXME: Maybe `Regex` could at some point implement `Trace`, and we need to take that into +// account. +unsafe impl Trace for RegExp { + unsafe_empty_trace!(); +} + impl InternalState for RegExp {} /// Create a new `RegExp` From 92e0b7c425f9b4b1a1e8788855f74c627db606c0 Mon Sep 17 00:00:00 2001 From: Iban Eguia Moraza Date: Sun, 10 May 2020 12:02:30 +0200 Subject: [PATCH 2/4] Removed non-needed import --- boa/src/builtins/regexp/mod.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/boa/src/builtins/regexp/mod.rs b/boa/src/builtins/regexp/mod.rs index 168453bc69f..6601841e8e5 100644 --- a/boa/src/builtins/regexp/mod.rs +++ b/boa/src/builtins/regexp/mod.rs @@ -9,10 +9,6 @@ //! [spec]: https://tc39.es/ecma262/#sec-regexp-constructor //! [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp -use gc::{unsafe_empty_trace, Finalize, Gc, Trace}; -use regex::Regex; -use std::ops::Deref; - use crate::{ builtins::{ object::{InternalState, Object, ObjectInternalMethods, ObjectKind, PROTOTYPE}, @@ -21,6 +17,9 @@ use crate::{ }, exec::Interpreter, }; +use gc::{unsafe_empty_trace, Finalize, Trace}; +use regex::Regex; +use std::ops::Deref; #[cfg(test)] mod tests; From e4731e756065f434c3c8c08f53d1b065914a75c7 Mon Sep 17 00:00:00 2001 From: Iban Eguia Moraza Date: Sun, 10 May 2020 12:37:08 +0200 Subject: [PATCH 3/4] Trying to fix borrows --- boa/src/builtins/object/internal_state.rs | 25 ++++++++--------------- boa/src/builtins/value/mod.rs | 21 ++++++++++--------- 2 files changed, 19 insertions(+), 27 deletions(-) diff --git a/boa/src/builtins/object/internal_state.rs b/boa/src/builtins/object/internal_state.rs index 8c417684412..a5f2f85be80 100644 --- a/boa/src/builtins/object/internal_state.rs +++ b/boa/src/builtins/object/internal_state.rs @@ -4,7 +4,7 @@ use gc::{unsafe_empty_trace, Finalize, Trace}; use std::{ any::Any, fmt::{self, Debug}, - ops::{Deref, DerefMut}, + ops::Deref, rc::Rc, }; @@ -21,19 +21,6 @@ unsafe impl Trace for InternalStateCell { unsafe_empty_trace!(); } -impl Deref for InternalStateCell { - type Target = dyn InternalState; - fn deref(&self) -> &Self::Target { - Deref::deref(&self.state) - } -} - -impl DerefMut for InternalStateCell { - fn deref_mut(&mut self) -> &mut Self::Target { - Rc::get_mut(&mut self.state).expect("failed to get mutable") - } -} - /// The derived version would print 'InternalStateCell { state: ... }', this custom implementation /// only prints the actual internal state. impl Debug for InternalStateCell { @@ -51,13 +38,17 @@ impl InternalStateCell { } /// Get a reference to the stored value and cast it to `T`. - pub fn downcast_ref(&self) -> Option<&T> { - self.deref().downcast_ref::() + pub fn downcast_ref(&self) -> Option<&T> { + let state = Deref::deref(&mut self.state); + + (state as &dyn Any).downcast_ref::() } /// Get a mutable reference to the stored value and cast it to `T`. pub fn downcast_mut(&mut self) -> Option<&mut T> { - self.state.downcast_mut::() + let state = Rc::get_mut(&mut self.state).expect("failed to get mutable"); + + (state as &mut dyn Any).downcast_mut::() } } diff --git a/boa/src/builtins/value/mod.rs b/boa/src/builtins/value/mod.rs index 583d3146495..4b2563079d3 100644 --- a/boa/src/builtins/value/mod.rs +++ b/boa/src/builtins/value/mod.rs @@ -487,18 +487,19 @@ impl ValueData { /// /// This will panic if this value doesn't have an internal state or if the internal state doesn't /// have the concrete type `S`. - pub fn with_internal_state_ref R>( - &self, - f: F, - ) -> R { + pub fn with_internal_state_ref R>(&self, f: F) -> R { + // fn get_type(_: &T) { + // dbg!(type_name::()); + // } + + dbg!(std::any::type_name::()); + if let Self::Object(ref obj) = *self { let o = obj.borrow(); - let state = o - .state - .as_ref() - .expect("no state") - .downcast_ref() - .expect("wrong state type"); + dbg!(&o.state); + let state = o.state.as_ref().expect("no state").downcast_ref(); + dbg!(&state); + let state = state.expect("wrong state type"); f(state) } else { panic!("not an object"); From 99a0bf47c5d2a1801783e7aec0b5e4a6e257d9ed Mon Sep 17 00:00:00 2001 From: Iban Eguia Moraza Date: Sun, 10 May 2020 12:38:00 +0200 Subject: [PATCH 4/4] Removed some debugging code --- boa/src/builtins/value/mod.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/boa/src/builtins/value/mod.rs b/boa/src/builtins/value/mod.rs index 4b2563079d3..5ba7472f9ce 100644 --- a/boa/src/builtins/value/mod.rs +++ b/boa/src/builtins/value/mod.rs @@ -488,18 +488,14 @@ impl ValueData { /// This will panic if this value doesn't have an internal state or if the internal state doesn't /// have the concrete type `S`. pub fn with_internal_state_ref R>(&self, f: F) -> R { - // fn get_type(_: &T) { - // dbg!(type_name::()); - // } - - dbg!(std::any::type_name::()); - if let Self::Object(ref obj) = *self { let o = obj.borrow(); - dbg!(&o.state); - let state = o.state.as_ref().expect("no state").downcast_ref(); - dbg!(&state); - let state = state.expect("wrong state type"); + let state = o + .state + .as_ref() + .expect("no state") + .downcast_ref() + .expect("wrong state type"); f(state) } else { panic!("not an object");