From 1fe3793e242302f6b57a1466fad3688428a2a6df Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Wed, 17 Sep 2025 17:03:37 -0700 Subject: [PATCH 1/7] Cleanup on GcRefCell and fix an UB in Gc - Empty enums are uninhabited types and referencing them is UB. - Removed the ref in `GcRefMut<>` and `GcRef<>` in favor of `NonNull<>` for good hygiene. - Simplified the type of `GcRefMut` by having a ref to the flags instead of keeping a ref to the `GcRefCell` itself. - Unconstrained the types for `Deref` and `DerefMut` for `Ref`/`RefMut`. --- core/engine/src/builtins/array_buffer/mod.rs | 7 +- core/engine/src/interop/into_js_arguments.rs | 3 +- .../src/object/builtins/jsarraybuffer.rs | 4 +- core/engine/src/object/jsobject.rs | 10 +- core/gc/src/cell.rs | 193 +++++++++--------- core/gc/src/pointers/gc.rs | 10 +- 6 files changed, 113 insertions(+), 114 deletions(-) diff --git a/core/engine/src/builtins/array_buffer/mod.rs b/core/engine/src/builtins/array_buffer/mod.rs index 8cedbbd51de..7bbd43f8522 100644 --- a/core/engine/src/builtins/array_buffer/mod.rs +++ b/core/engine/src/builtins/array_buffer/mod.rs @@ -27,7 +27,7 @@ use crate::{ context::intrinsics::{Intrinsics, StandardConstructor, StandardConstructors}, error::JsNativeError, js_string, - object::{JsObject, Object, internal_methods::get_prototype_from_constructor}, + object::{JsObject, internal_methods::get_prototype_from_constructor}, property::Attribute, realm::Realm, string::StaticJsStrings, @@ -160,10 +160,7 @@ impl BufferObject { #[track_caller] pub(crate) fn as_buffer_mut( &self, - ) -> BufferRefMut< - GcRefMut<'_, Object, ArrayBuffer>, - GcRefMut<'_, Object, SharedArrayBuffer>, - > { + ) -> BufferRefMut, GcRefMut<'_, SharedArrayBuffer>> { match self { Self::Buffer(buf) => { BufferRefMut::Buffer(GcRefMut::map(buf.borrow_mut(), |o| o.data_mut())) diff --git a/core/engine/src/interop/into_js_arguments.rs b/core/engine/src/interop/into_js_arguments.rs index 843d126925e..e39e9c78398 100644 --- a/core/engine/src/interop/into_js_arguments.rs +++ b/core/engine/src/interop/into_js_arguments.rs @@ -1,4 +1,3 @@ -use boa_engine::object::Object; use boa_engine::value::TryFromJs; use boa_engine::{Context, JsNativeError, JsObject, JsResult, JsValue, NativeObject}; use boa_gc::{GcRef, GcRefMut}; @@ -265,7 +264,7 @@ impl JsClass { /// /// Panics if the object is currently mutably borrowed. #[must_use] - pub fn borrow_mut(&self) -> GcRefMut<'_, Object, T> { + pub fn borrow_mut(&self) -> GcRefMut<'_, T> { GcRefMut::map(self.inner.borrow_mut(), |obj| obj.data_mut()) } } diff --git a/core/engine/src/object/builtins/jsarraybuffer.rs b/core/engine/src/object/builtins/jsarraybuffer.rs index 0a845535cb1..c343ced0bad 100644 --- a/core/engine/src/object/builtins/jsarraybuffer.rs +++ b/core/engine/src/object/builtins/jsarraybuffer.rs @@ -4,7 +4,7 @@ use crate::{ builtins::array_buffer::ArrayBuffer, context::intrinsics::StandardConstructors, error::JsNativeError, - object::{JsObject, Object, internal_methods::get_prototype_from_constructor}, + object::{JsObject, internal_methods::get_prototype_from_constructor}, value::TryFromJs, }; use boa_gc::{Finalize, GcRef, GcRefMut, Trace}; @@ -283,7 +283,7 @@ impl JsArrayBuffer { /// ``` #[inline] #[must_use] - pub fn data_mut(&self) -> Option, [u8]>> { + pub fn data_mut(&self) -> Option> { GcRefMut::try_map(self.inner.borrow_mut(), |o| o.data_mut().bytes_mut()) } } diff --git a/core/engine/src/object/jsobject.rs b/core/engine/src/object/jsobject.rs index 44ca1072b8c..684551e28a4 100644 --- a/core/engine/src/object/jsobject.rs +++ b/core/engine/src/object/jsobject.rs @@ -45,7 +45,7 @@ use std::ptr::NonNull; pub type Ref<'a, T> = boa_gc::GcRef<'a, T>; /// A wrapper type for a mutably borrowed type T. -pub type RefMut<'a, T, U> = boa_gc::GcRefMut<'a, T, U>; +pub type RefMut<'a, T> = boa_gc::GcRefMut<'a, T>; pub(crate) type ErasedVTableObject = VTableObject; @@ -54,7 +54,7 @@ pub type ErasedObject = Object; /// A erased object data type that must never be used directly. #[derive(Debug, Trace, Finalize)] -pub enum ErasedObjectData {} +pub struct ErasedObjectData {} impl JsData for ErasedObjectData {} @@ -265,7 +265,7 @@ impl JsObject { /// Panics if the object is currently borrowed. #[must_use] #[track_caller] - pub fn downcast_mut(&self) -> Option> { + pub fn downcast_mut(&self) -> Option> { if self.is::() { let obj = self.borrow_mut(); @@ -676,7 +676,7 @@ impl JsObject { #[inline] #[must_use] #[track_caller] - pub fn borrow_mut(&self) -> RefMut<'_, Object, Object> { + pub fn borrow_mut(&self) -> RefMut<'_, Object> { self.try_borrow_mut().expect("Object already borrowed") } @@ -698,7 +698,7 @@ impl JsObject { /// /// This is the non-panicking variant of [`borrow_mut`](#method.borrow_mut). #[inline] - pub fn try_borrow_mut(&self) -> StdResult, Object>, BorrowMutError> { + pub fn try_borrow_mut(&self) -> StdResult>, BorrowMutError> { self.inner .object .try_borrow_mut() diff --git a/core/gc/src/cell.rs b/core/gc/src/cell.rs index 1986413f7e2..779c4bf5e4f 100644 --- a/core/gc/src/cell.rs +++ b/core/gc/src/cell.rs @@ -4,18 +4,18 @@ use crate::{ Tracer, trace::{Finalize, Trace}, }; +use std::marker::PhantomData; +use std::ptr::NonNull; use std::{ cell::{Cell, UnsafeCell}, cmp::Ordering, fmt::{self, Debug, Display}, hash::Hash, - mem::ManuallyDrop, ops::{Deref, DerefMut}, - ptr, }; /// `BorrowFlag` represent the internal state of a `GcCell` and -/// keeps track of the amount of current borrows. +/// keeps track of the number of current borrows. #[derive(Copy, Clone)] struct BorrowFlag(usize); @@ -34,7 +34,7 @@ enum BorrowState { const WRITING: usize = !0; const UNUSED: usize = 0; -/// The base borrowflag init is rooted, and has no outstanding borrows. +/// The base borrow flag init is rooted, and has no outstanding borrows. const BORROWFLAG_INIT: BorrowFlag = BorrowFlag(UNUSED); impl BorrowFlag { @@ -58,13 +58,13 @@ impl BorrowFlag { /// - This method will panic if the current `BorrowState` is writing. /// - This method will panic after incrementing if the borrow count overflows. fn add_reading(self) -> Self { - assert!(self.borrowed() != BorrowState::Writing); + assert_ne!(self.borrowed(), BorrowState::Writing); let flags = Self(self.0 + 1); // This will fail if the borrow count overflows, which shouldn't happen, // but let's be safe { - assert!(flags.borrowed() == BorrowState::Reading); + assert_eq!(flags.borrowed(), BorrowState::Reading); } flags } @@ -74,7 +74,7 @@ impl BorrowFlag { /// # Panic /// - This method will panic if the current `BorrowState` is not reading. fn sub_reading(self) -> Self { - assert!(self.borrowed() == BorrowState::Reading); + assert_eq!(self.borrowed(), BorrowState::Reading); Self(self.0 - 1) } } @@ -92,7 +92,7 @@ impl Debug for BorrowFlag { /// /// This object is a `RefCell` that can be used inside of a `Gc`. pub struct GcRefCell { - flags: Cell, + borrow: Cell, cell: UnsafeCell, } @@ -100,7 +100,7 @@ impl GcRefCell { /// Creates a new `GcCell` containing `value`. pub const fn new(value: T) -> Self { Self { - flags: Cell::new(BORROWFLAG_INIT), + borrow: Cell::new(BORROWFLAG_INIT), cell: UnsafeCell::new(value), } } @@ -155,16 +155,16 @@ impl GcRefCell { /// /// Returns an `Err` if the value is currently mutably borrowed. pub fn try_borrow(&self) -> Result, BorrowError> { - if self.flags.get().borrowed() == BorrowState::Writing { + if self.borrow.get().borrowed() == BorrowState::Writing { return Err(BorrowError); } - self.flags.set(self.flags.get().add_reading()); + self.borrow.set(self.borrow.get().add_reading()); // SAFETY: calling value on a rooted value may cause Undefined Behavior unsafe { Ok(GcRef { - flags: &self.flags, - value: &*self.cell.get(), + borrow: &self.borrow, + value: NonNull::new_unchecked(self.cell.get()), }) } } @@ -180,17 +180,18 @@ impl GcRefCell { /// /// Returns an `Err` if the value is currently borrowed. pub fn try_borrow_mut(&self) -> Result, BorrowMutError> { - if self.flags.get().borrowed() != BorrowState::Unused { + if self.borrow.get().borrowed() != BorrowState::Unused { return Err(BorrowMutError); } - self.flags.set(self.flags.get().set_writing()); + self.borrow.set(self.borrow.get().set_writing()); // SAFETY: This is safe as the value is rooted if it was not previously rooted, // so it cannot be dropped. unsafe { Ok(GcRefMut { - gc_cell: self, - value: &mut *self.cell.get(), + borrow: &self.borrow, + value: NonNull::new_unchecked(self.cell.get()), + marker: PhantomData, }) } } @@ -218,13 +219,13 @@ impl Display for BorrowMutError { impl Finalize for GcRefCell {} -// SAFETY: GcCell maintains it's own BorrowState and rootedness. GcCell's implementation -// focuses on only continuing Trace based methods while the cell state is not written. +// SAFETY: GcCell maintains its own BorrowState and rootedness. GcCell's implementation +// focuses on only continuing Trace-based methods while the cell state is not written. // Implementing a Trace while the cell is being written to or incorrectly implementing Trace // on GcCell's value may cause Undefined Behavior unsafe impl Trace for GcRefCell { unsafe fn trace(&self, tracer: &mut Tracer) { - match self.flags.get().borrowed() { + match self.borrow.get().borrowed() { BorrowState::Writing => (), // SAFETY: Please see GcCell's Trace impl Safety note. _ => unsafe { (*self.cell.get()).trace(tracer) }, @@ -232,7 +233,7 @@ unsafe impl Trace for GcRefCell { } unsafe fn trace_non_roots(&self) { - match self.flags.get().borrowed() { + match self.borrow.get().borrowed() { BorrowState::Writing => (), // SAFETY: Please see GcCell's Trace impl Safety note. _ => unsafe { (*self.cell.get()).trace_non_roots() }, @@ -241,7 +242,7 @@ unsafe impl Trace for GcRefCell { fn run_finalizer(&self) { Finalize::finalize(self); - match self.flags.get().borrowed() { + match self.borrow.get().borrowed() { BorrowState::Writing => (), // SAFETY: Please see GcCell's Trace impl Safety note. _ => unsafe { (*self.cell.get()).run_finalizer() }, @@ -251,24 +252,22 @@ unsafe impl Trace for GcRefCell { /// A wrapper type for an immutably borrowed value from a `GcCell`. pub struct GcRef<'a, T: ?Sized + 'static> { - flags: &'a Cell, - value: &'a T, + value: NonNull, + borrow: &'a Cell, } impl<'a, T: ?Sized> GcRef<'a, T> { /// Casts to a `GcRef` of another type. /// /// # Safety - /// * The caller must ensure that `T` can be safeley cast to `U`. + /// * The caller must ensure that `T` can be safely cast to `U`. #[must_use] pub unsafe fn cast(self) -> GcRef<'a, U> { - let value = ptr::from_ref(self.value); - // SAFETY: This is safe as `GcCellRef` is already borrowed, so the value is rooted. - // The caller must ensure that `T` can be safeley cast to `U`. - let value = unsafe { &*(value.cast::()) }; + // The caller must ensure that `T` can be safely cast to `U`. + let value = self.value.cast::(); let cell = GcRef { - flags: self.flags, + borrow: self.borrow, value, }; @@ -290,9 +289,9 @@ impl<'a, T: ?Sized> GcRef<'a, T> { #[allow(clippy::should_implement_trait)] #[must_use] pub fn clone(orig: &GcRef<'a, T>) -> GcRef<'a, T> { - orig.flags.set(orig.flags.get().add_reading()); + orig.borrow.set(orig.borrow.get().add_reading()); GcRef { - flags: orig.flags, + borrow: orig.borrow, value: orig.value, } } @@ -311,8 +310,8 @@ impl<'a, T: ?Sized> GcRef<'a, T> { F: FnOnce(&T) -> Option<&U>, { let ret = GcRef { - flags: orig.flags, - value: f(orig.value)?, + borrow: orig.borrow, + value: NonNull::from(f(&*orig)?), }; // We have to tell the compiler not to call the destructor of GcCellRef, @@ -335,8 +334,8 @@ impl<'a, T: ?Sized> GcRef<'a, T> { F: FnOnce(&T) -> &U, { let ret = GcRef { - flags: orig.flags, - value: f(orig.value), + borrow: orig.borrow, + value: NonNull::from(f(&*orig)), }; // We have to tell the compiler not to call the destructor of GcCellRef, @@ -358,18 +357,18 @@ impl<'a, T: ?Sized> GcRef<'a, T> { V: ?Sized, F: FnOnce(&T) -> (&U, &V), { - let (a, b) = f(orig.value); + let (a, b) = f(&*orig); - orig.flags.set(orig.flags.get().add_reading()); + orig.borrow.set(orig.borrow.get().add_reading()); let ret = ( GcRef { - flags: orig.flags, - value: a, + borrow: orig.borrow, + value: NonNull::from(a), }, GcRef { - flags: orig.flags, - value: b, + borrow: orig.borrow, + value: NonNull::from(b), }, ); @@ -385,14 +384,14 @@ impl Deref for GcRef<'_, T> { type Target = T; fn deref(&self) -> &T { - self.value + unsafe { self.value.as_ref() } } } impl Drop for GcRef<'_, T> { fn drop(&mut self) { - debug_assert!(self.flags.get().borrowed() == BorrowState::Reading); - self.flags.set(self.flags.get().sub_reading()); + debug_assert!(self.borrow.get().borrowed() == BorrowState::Reading); + self.borrow.set(self.borrow.get().sub_reading()); } } @@ -409,30 +408,35 @@ impl Display for GcRef<'_, T> { } /// A wrapper type for a mutably borrowed value from a `GcCell`. -pub struct GcRefMut<'a, T: ?Sized + 'static, U: ?Sized = T> { - pub(crate) gc_cell: &'a GcRefCell, - pub(crate) value: &'a mut U, +pub struct GcRefMut<'a, T: ?Sized> { + // NB: we use a pointer instead of `&'a mut U` to avoid `noalias` violations, because + // a `GcRefMut` argument doesn't hold exclusivity for its whole scope, only until it + // drops. + value: NonNull, + borrow: &'a Cell, + // `NonNull` is covariant over `T`, so we need to reintroduce invariance. + marker: PhantomData<&'a mut T>, } -impl<'a, T: ?Sized, U: ?Sized> GcRefMut<'a, T, U> { +impl<'a, T: ?Sized> GcRefMut<'a, T> { /// Casts to a `GcRefMut` of another type. /// /// # Safety - /// * The caller must ensure that `U` can be safeley cast to `V`. + /// * The caller must ensure that `U` can be safely cast to `V`. #[must_use] - pub unsafe fn cast(self) -> GcRefMut<'a, T, V> { - let gc_cell = self.gc_cell; - - let this = ManuallyDrop::new(self); - let value = unsafe { ptr::read(&raw const this.value) }; - - let value = ptr::from_mut::(value); + pub unsafe fn cast(self) -> GcRefMut<'a, V> { + let borrow = self.borrow; + let value = self.value.cast::(); - // SAFETY: This is safe as `GcCellRefMut` is already borrowed, so the value is rooted. - // The caller must ensure that `U` can be safeley cast to `V`. - let value = unsafe { &mut *(value.cast::()) }; + // We have to tell the compiler not to call the destructor of GcCellRef, + // because it will update the borrow flags. + std::mem::forget(self); - GcRefMut { gc_cell, value } + GcRefMut { + borrow, + value, + marker: PhantomData, + } } /// Tries to make a new `GcCellRefMut` for a component of the borrowed data, returning `None` @@ -443,18 +447,16 @@ impl<'a, T: ?Sized, U: ?Sized> GcRefMut<'a, T, U> { /// This is an associated function that needs to be used as /// `GcCellRefMut::map(...)`. A method would interfere with methods of the same /// name on the contents of a `GcCell` used through `Deref`. - pub fn try_map(orig: Self, f: F) -> Option> + pub fn try_map(mut orig: GcRefMut<'a, T>, f: F) -> Option> where V: ?Sized, - F: FnOnce(&mut U) -> Option<&mut V>, + F: FnOnce(&mut T) -> Option<&mut V>, { - #[allow(trivial_casts)] - // SAFETY: This is safe as `GcCellRefMut` is already borrowed, so the value is rooted. - let value = unsafe { &mut *ptr::from_mut::(orig.value) }; - + let value = NonNull::from(f(&mut *orig)?); let ret = GcRefMut { - gc_cell: orig.gc_cell, - value: f(value)?, + borrow: orig.borrow, + value, + marker: PhantomData, }; // We have to tell the compiler not to call the destructor of GcCellRef, @@ -472,56 +474,55 @@ impl<'a, T: ?Sized, U: ?Sized> GcRefMut<'a, T, U> { /// This is an associated function that needs to be used as /// `GcCellRefMut::map(...)`. A method would interfere with methods of the same /// name on the contents of a `GcCell` used through `Deref`. - pub fn map(orig: Self, f: F) -> GcRefMut<'a, T, V> + pub fn map(mut orig: Self, f: F) -> GcRefMut<'a, V> where V: ?Sized, - F: FnOnce(&mut U) -> &mut V, + F: FnOnce(&mut T) -> &mut V, { - #[allow(trivial_casts)] - // SAFETY: This is safe as `GcCellRefMut` is already borrowed, so the value is rooted. - let value = unsafe { &mut *ptr::from_mut::(orig.value) }; - - let ret = GcRefMut { - gc_cell: orig.gc_cell, - value: f(value), - }; - + let borrow = orig.borrow; + let value = NonNull::from(f(&mut *orig)); // We have to tell the compiler not to call the destructor of GcCellRefMut, // because it will update the borrow flags. std::mem::forget(orig); - ret + GcRefMut { + borrow, + value, + marker: PhantomData, + } } } -impl Deref for GcRefMut<'_, T, U> { - type Target = U; +impl Deref for GcRefMut<'_, T> { + type Target = T; - fn deref(&self) -> &U { - self.value + fn deref(&self) -> &T { + // SAFETY: the value is accessible as long as we hold our borrow. + unsafe { self.value.as_ref() } } } -impl DerefMut for GcRefMut<'_, T, U> { - fn deref_mut(&mut self) -> &mut U { - self.value +impl DerefMut for GcRefMut<'_, T> { + fn deref_mut(&mut self) -> &mut T { + // SAFETY: the value is accessible as long as we hold our borrow. + unsafe { self.value.as_mut() } } } -impl Drop for GcRefMut<'_, T, U> { +impl Drop for GcRefMut<'_, T> { fn drop(&mut self) { - debug_assert!(self.gc_cell.flags.get().borrowed() == BorrowState::Writing); - self.gc_cell.flags.set(BorrowFlag(UNUSED)); + debug_assert_eq!(self.borrow.get().borrowed(), BorrowState::Writing); + self.borrow.set(BorrowFlag(UNUSED)); } } -impl Debug for GcRefMut<'_, T, U> { +impl Debug for GcRefMut<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { Debug::fmt(&**self, f) } } -impl Display for GcRefMut<'_, T, U> { +impl Display for GcRefMut<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { Display::fmt(&**self, f) } @@ -588,15 +589,15 @@ impl Ord for GcRefCell { impl Debug for GcRefCell { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self.flags.get().borrowed() { + match self.borrow.get().borrowed() { BorrowState::Unused | BorrowState::Reading => f .debug_struct("GcCell") - .field("flags", &self.flags.get()) + .field("flags", &self.borrow.get()) .field("value", &self.borrow()) .finish_non_exhaustive(), BorrowState::Writing => f .debug_struct("GcCell") - .field("flags", &self.flags.get()) + .field("flags", &self.borrow.get()) .field("value", &"") .finish_non_exhaustive(), } diff --git a/core/gc/src/pointers/gc.rs b/core/gc/src/pointers/gc.rs index 9ce9796c1f4..988a62f7dfc 100644 --- a/core/gc/src/pointers/gc.rs +++ b/core/gc/src/pointers/gc.rs @@ -276,10 +276,10 @@ impl Gc { #[inline] #[must_use] pub unsafe fn cast_unchecked(this: Self) -> Gc { - let inner_ptr = this.inner_ptr.cast(); + let inner_ptr = this.inner_ptr.cast::(); core::mem::forget(this); // Prevents double free. Gc { - inner_ptr, + inner_ptr: inner_ptr.cast(), marker: PhantomData, } } @@ -346,11 +346,13 @@ unsafe impl Trace for Gc { impl Clone for Gc { fn clone(&self) -> Self { let ptr = self.inner_ptr(); - self.inner().inc_ref_count(); // SAFETY: though `ptr` doesn't come from a `into_raw` call, it essentially does the same, // but it skips the call to `std::mem::forget` since we have a reference instead of an owned // value. - unsafe { Self::from_raw(ptr) } + unsafe { + ptr.as_ref().inc_ref_count(); + Self::from_raw(ptr) + } } } From 3841afe9d5153db80ddf4b855564751b8de6017c Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Wed, 17 Sep 2025 17:52:15 -0700 Subject: [PATCH 2/7] Fix a second UB because of downcasting confusing Miri --- core/engine/src/object/datatypes.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/core/engine/src/object/datatypes.rs b/core/engine/src/object/datatypes.rs index 084793e12b0..ade3f37144b 100644 --- a/core/engine/src/object/datatypes.rs +++ b/core/engine/src/object/datatypes.rs @@ -228,7 +228,16 @@ pub(crate) struct ObjectData { // Because we want to trigger the compile-time const assertion below. // // It is fine if we have as_ref/as_mut to it or any access. + #[cfg(not(miri))] data: T, + + // Miri triggers a UB on `downcast_ref` and `downcast_mut` because it cannot + // figure out that casting T to U does not result in UB as we check that + // objects are U before casting them. Instead of trying to get Miri to + // behave properly, we use a `Box` which always has the same size as + // `Box`. + #[cfg(miri)] + data: Box, } impl Default for ObjectData { @@ -250,7 +259,12 @@ impl ObjectData { // force assertion to triger when we instantiate `ObjectData::new`. let () = Self::OBJECT_DATA_ALIGNMENT_REQUIREMENT; - Self { data: value } + Self { + #[cfg(not(miri))] + data: value, + #[cfg(miri)] + data: Box::new(value), + } } } From d2f32425acb2087ff0309355f434f1a1f9fea90a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Juli=C3=A1n=20Espina?= Date: Thu, 18 Sep 2025 10:35:08 -0700 Subject: [PATCH 3/7] Move Drop borrow logic to its own type so RefMut and Ref dont have to implement Drop --- core/engine/src/object/datatypes.rs | 7 ++ core/engine/src/object/jsobject.rs | 10 +- core/gc/src/cell.rs | 157 +++++++++++++--------------- 3 files changed, 85 insertions(+), 89 deletions(-) diff --git a/core/engine/src/object/datatypes.rs b/core/engine/src/object/datatypes.rs index ade3f37144b..3467e78b7d2 100644 --- a/core/engine/src/object/datatypes.rs +++ b/core/engine/src/object/datatypes.rs @@ -228,6 +228,7 @@ pub(crate) struct ObjectData { // Because we want to trigger the compile-time const assertion below. // // It is fine if we have as_ref/as_mut to it or any access. + // TODO: see below. #[cfg(not(miri))] data: T, @@ -236,6 +237,12 @@ pub(crate) struct ObjectData { // objects are U before casting them. Instead of trying to get Miri to // behave properly, we use a `Box` which always has the same size as // `Box`. + // + // Note: The `tree-borrows` checker is fine with the above in miri, it is + // only the stack borrow checker that complaints. + // + // TODO: his when we have a task runner OR when tree-borrows is + // the default in Miri. #[cfg(miri)] data: Box, } diff --git a/core/engine/src/object/jsobject.rs b/core/engine/src/object/jsobject.rs index 684551e28a4..979b04f1012 100644 --- a/core/engine/src/object/jsobject.rs +++ b/core/engine/src/object/jsobject.rs @@ -22,7 +22,7 @@ use crate::{ property::{PropertyDescriptor, PropertyKey}, value::PreferredType, }; -use boa_gc::{self, Finalize, Gc, GcRefCell, Trace}; +use boa_gc::{self, Finalize, Gc, GcRef, GcRefCell, GcRefMut, Trace}; use core::ptr::fn_addr_eq; use std::collections::HashSet; use std::{ @@ -42,10 +42,10 @@ use boa_gc::GcBox; use std::ptr::NonNull; /// A wrapper type for an immutably borrowed type T. -pub type Ref<'a, T> = boa_gc::GcRef<'a, T>; +pub type Ref<'a, T> = GcRef<'a, T>; /// A wrapper type for a mutably borrowed type T. -pub type RefMut<'a, T> = boa_gc::GcRefMut<'a, T>; +pub type RefMut<'a, T> = GcRefMut<'a, T>; pub(crate) type ErasedVTableObject = VTableObject; @@ -250,7 +250,7 @@ impl JsObject { let obj = self.borrow(); // SAFETY: We have verified that the object is of type `T`, so we can safely cast it. - let obj = unsafe { obj.cast::>() }; + let obj = unsafe { GcRef::cast::>(obj) }; return Some(Ref::map(obj, |r| r.data())); } @@ -270,7 +270,7 @@ impl JsObject { let obj = self.borrow_mut(); // SAFETY: We have verified that the object is of type `T`, so we can safely cast it. - let obj = unsafe { obj.cast::>() }; + let obj = unsafe { GcRefMut::cast::>(obj) }; return Some(RefMut::map(obj, |c| c.data_mut())); } diff --git a/core/gc/src/cell.rs b/core/gc/src/cell.rs index 779c4bf5e4f..491096bcb4f 100644 --- a/core/gc/src/cell.rs +++ b/core/gc/src/cell.rs @@ -96,7 +96,7 @@ pub struct GcRefCell { cell: UnsafeCell, } -impl GcRefCell { +impl GcRefCell { /// Creates a new `GcCell` containing `value`. pub const fn new(value: T) -> Self { Self { @@ -111,7 +111,7 @@ impl GcRefCell { } } -impl GcRefCell { +impl GcRefCell { /// Immutably borrows the wrapped value. /// /// The borrow lasts until the returned `GcCellRef` exits scope. @@ -163,7 +163,9 @@ impl GcRefCell { // SAFETY: calling value on a rooted value may cause Undefined Behavior unsafe { Ok(GcRef { - borrow: &self.borrow, + borrow: BorrowGcRef { + borrow: &self.borrow, + }, value: NonNull::new_unchecked(self.cell.get()), }) } @@ -189,7 +191,9 @@ impl GcRefCell { // so it cannot be dropped. unsafe { Ok(GcRefMut { - borrow: &self.borrow, + borrow: BorrowGcRefMut { + borrow: &self.borrow, + }, value: NonNull::new_unchecked(self.cell.get()), marker: PhantomData, }) @@ -250,10 +254,31 @@ unsafe impl Trace for GcRefCell { } } +struct BorrowGcRef<'a> { + borrow: &'a Cell, +} + +impl Drop for BorrowGcRef<'_> { + fn drop(&mut self) { + debug_assert!(self.borrow.get().borrowed() == BorrowState::Reading); + self.borrow.set(self.borrow.get().sub_reading()); + } +} + +impl Clone for BorrowGcRef<'_> { + #[inline] + fn clone(&self) -> Self { + self.borrow.set(self.borrow.get().add_reading()); + BorrowGcRef { + borrow: self.borrow, + } + } +} + /// A wrapper type for an immutably borrowed value from a `GcCell`. pub struct GcRef<'a, T: ?Sized + 'static> { value: NonNull, - borrow: &'a Cell, + borrow: BorrowGcRef<'a>, } impl<'a, T: ?Sized> GcRef<'a, T> { @@ -262,20 +287,13 @@ impl<'a, T: ?Sized> GcRef<'a, T> { /// # Safety /// * The caller must ensure that `T` can be safely cast to `U`. #[must_use] - pub unsafe fn cast(self) -> GcRef<'a, U> { - // SAFETY: This is safe as `GcCellRef` is already borrowed, so the value is rooted. - // The caller must ensure that `T` can be safely cast to `U`. - let value = self.value.cast::(); - let cell = GcRef { - borrow: self.borrow, - value, - }; - - // We have to tell the compiler not to call the destructor of GcCellRef, - // because it will update the borrow flags. - core::mem::forget(self); + pub unsafe fn cast(orig: Self) -> GcRef<'a, U> { + let value = orig.value.cast::(); - cell + GcRef { + borrow: orig.borrow, + value, + } } /// Copies a `GcCellRef`. @@ -289,9 +307,8 @@ impl<'a, T: ?Sized> GcRef<'a, T> { #[allow(clippy::should_implement_trait)] #[must_use] pub fn clone(orig: &GcRef<'a, T>) -> GcRef<'a, T> { - orig.borrow.set(orig.borrow.get().add_reading()); GcRef { - borrow: orig.borrow, + borrow: orig.borrow.clone(), value: orig.value, } } @@ -309,15 +326,13 @@ impl<'a, T: ?Sized> GcRef<'a, T> { U: ?Sized, F: FnOnce(&T) -> Option<&U>, { + let value = NonNull::from(f(&*orig)?); + let ret = GcRef { borrow: orig.borrow, - value: NonNull::from(f(&*orig)?), + value, }; - // We have to tell the compiler not to call the destructor of GcCellRef, - // because it will update the borrow flags. - std::mem::forget(orig); - Some(ret) } @@ -333,16 +348,12 @@ impl<'a, T: ?Sized> GcRef<'a, T> { U: ?Sized, F: FnOnce(&T) -> &U, { - let ret = GcRef { - borrow: orig.borrow, - value: NonNull::from(f(&*orig)), - }; - - // We have to tell the compiler not to call the destructor of GcCellRef, - // because it will update the borrow flags. - std::mem::forget(orig); + let value = NonNull::from(f(&*orig)); - ret + GcRef { + borrow: orig.borrow, + value, + } } /// Splits a `GcCellRef` into multiple `GcCellRef`s for different components of the borrowed data. @@ -358,25 +369,18 @@ impl<'a, T: ?Sized> GcRef<'a, T> { F: FnOnce(&T) -> (&U, &V), { let (a, b) = f(&*orig); + let borrow = orig.borrow.clone(); - orig.borrow.set(orig.borrow.get().add_reading()); - - let ret = ( + ( GcRef { - borrow: orig.borrow, + borrow, value: NonNull::from(a), }, GcRef { - borrow: orig.borrow, value: NonNull::from(b), + borrow: orig.borrow, }, - ); - - // We have to tell the compiler not to call the destructor of GcCellRef, - // because it will update the borrow flags. - std::mem::forget(orig); - - ret + ) } } @@ -388,13 +392,6 @@ impl Deref for GcRef<'_, T> { } } -impl Drop for GcRef<'_, T> { - fn drop(&mut self) { - debug_assert!(self.borrow.get().borrowed() == BorrowState::Reading); - self.borrow.set(self.borrow.get().sub_reading()); - } -} - impl Debug for GcRef<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { Debug::fmt(&**self, f) @@ -407,13 +404,24 @@ impl Display for GcRef<'_, T> { } } +struct BorrowGcRefMut<'a> { + borrow: &'a Cell, +} + +impl Drop for BorrowGcRefMut<'_> { + fn drop(&mut self) { + debug_assert_eq!(self.borrow.get().borrowed(), BorrowState::Writing); + self.borrow.set(BorrowFlag(UNUSED)); + } +} + /// A wrapper type for a mutably borrowed value from a `GcCell`. pub struct GcRefMut<'a, T: ?Sized> { // NB: we use a pointer instead of `&'a mut U` to avoid `noalias` violations, because // a `GcRefMut` argument doesn't hold exclusivity for its whole scope, only until it // drops. value: NonNull, - borrow: &'a Cell, + borrow: BorrowGcRefMut<'a>, // `NonNull` is covariant over `T`, so we need to reintroduce invariance. marker: PhantomData<&'a mut T>, } @@ -424,16 +432,11 @@ impl<'a, T: ?Sized> GcRefMut<'a, T> { /// # Safety /// * The caller must ensure that `U` can be safely cast to `V`. #[must_use] - pub unsafe fn cast(self) -> GcRefMut<'a, V> { - let borrow = self.borrow; - let value = self.value.cast::(); - - // We have to tell the compiler not to call the destructor of GcCellRef, - // because it will update the borrow flags. - std::mem::forget(self); + pub unsafe fn cast(orig: Self) -> GcRefMut<'a, V> { + let value = orig.value.cast::(); GcRefMut { - borrow, + borrow: orig.borrow, value, marker: PhantomData, } @@ -453,16 +456,13 @@ impl<'a, T: ?Sized> GcRefMut<'a, T> { F: FnOnce(&mut T) -> Option<&mut V>, { let value = NonNull::from(f(&mut *orig)?); + let ret = GcRefMut { borrow: orig.borrow, value, marker: PhantomData, }; - // We have to tell the compiler not to call the destructor of GcCellRef, - // because it will update the borrow flags. - std::mem::forget(orig); - Some(ret) } @@ -479,14 +479,10 @@ impl<'a, T: ?Sized> GcRefMut<'a, T> { V: ?Sized, F: FnOnce(&mut T) -> &mut V, { - let borrow = orig.borrow; let value = NonNull::from(f(&mut *orig)); - // We have to tell the compiler not to call the destructor of GcCellRefMut, - // because it will update the borrow flags. - std::mem::forget(orig); GcRefMut { - borrow, + borrow: orig.borrow, value, marker: PhantomData, } @@ -509,13 +505,6 @@ impl DerefMut for GcRefMut<'_, T> { } } -impl Drop for GcRefMut<'_, T> { - fn drop(&mut self) { - debug_assert_eq!(self.borrow.get().borrowed(), BorrowState::Writing); - self.borrow.set(BorrowFlag(UNUSED)); - } -} - impl Debug for GcRefMut<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { Debug::fmt(&**self, f) @@ -537,24 +526,24 @@ impl Clone for GcRefCell { } } -impl Default for GcRefCell { +impl Default for GcRefCell { fn default() -> Self { Self::new(Default::default()) } } #[allow(clippy::inline_always)] -impl PartialEq for GcRefCell { +impl PartialEq for GcRefCell { #[inline(always)] fn eq(&self, other: &Self) -> bool { *self.borrow() == *other.borrow() } } -impl Eq for GcRefCell {} +impl Eq for GcRefCell {} #[allow(clippy::inline_always)] -impl PartialOrd for GcRefCell { +impl PartialOrd for GcRefCell { #[inline(always)] fn partial_cmp(&self, other: &Self) -> Option { (*self.borrow()).partial_cmp(&*other.borrow()) @@ -581,13 +570,13 @@ impl PartialOrd for GcRefCell { } } -impl Ord for GcRefCell { +impl Ord for GcRefCell { fn cmp(&self, other: &Self) -> Ordering { (*self.borrow()).cmp(&*other.borrow()) } } -impl Debug for GcRefCell { +impl Debug for GcRefCell { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self.borrow.get().borrowed() { BorrowState::Unused | BorrowState::Reading => f From 11b238637989408834a29f622f994791263a0f7a Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Thu, 18 Sep 2025 10:43:22 -0700 Subject: [PATCH 4/7] Remove assert_eq and assert_ne --- core/gc/src/cell.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/core/gc/src/cell.rs b/core/gc/src/cell.rs index 491096bcb4f..674cf86ddb8 100644 --- a/core/gc/src/cell.rs +++ b/core/gc/src/cell.rs @@ -57,14 +57,15 @@ impl BorrowFlag { /// # Panic /// - This method will panic if the current `BorrowState` is writing. /// - This method will panic after incrementing if the borrow count overflows. + #[inline] fn add_reading(self) -> Self { - assert_ne!(self.borrowed(), BorrowState::Writing); + assert!(self.borrowed() != BorrowState::Writing); let flags = Self(self.0 + 1); // This will fail if the borrow count overflows, which shouldn't happen, // but let's be safe { - assert_eq!(flags.borrowed(), BorrowState::Reading); + assert!(flags.borrowed() == BorrowState::Reading); } flags } @@ -74,7 +75,7 @@ impl BorrowFlag { /// # Panic /// - This method will panic if the current `BorrowState` is not reading. fn sub_reading(self) -> Self { - assert_eq!(self.borrowed(), BorrowState::Reading); + assert!(self.borrowed() == BorrowState::Reading); Self(self.0 - 1) } } @@ -410,7 +411,7 @@ struct BorrowGcRefMut<'a> { impl Drop for BorrowGcRefMut<'_> { fn drop(&mut self) { - debug_assert_eq!(self.borrow.get().borrowed(), BorrowState::Writing); + debug_assert!(self.borrow.get().borrowed() == BorrowState::Writing); self.borrow.set(BorrowFlag(UNUSED)); } } From 8baccbe5d5509ff95db8e2d594c59d40baca00a0 Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Thu, 18 Sep 2025 10:51:32 -0700 Subject: [PATCH 5/7] Remove unnecessary parentheses --- core/engine/src/context/icu.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/engine/src/context/icu.rs b/core/engine/src/context/icu.rs index 611df6ee23c..76f57f45b40 100644 --- a/core/engine/src/context/icu.rs +++ b/core/engine/src/context/icu.rs @@ -84,7 +84,7 @@ impl IntlProvider { /// /// Returns an error if any of the tools required cannot be constructed. pub(crate) fn try_new_buffer( - provider: (impl DynamicDryDataProvider + 'static), + provider: impl DynamicDryDataProvider + 'static, ) -> IntlProvider { Self { locale_canonicalizer: OnceCell::new(), From 30ef1beb65a6d5eba0461a24fcc6ac01596daf45 Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Thu, 18 Sep 2025 11:02:15 -0700 Subject: [PATCH 6/7] Apply lints --- core/engine/src/builtins/date/mod.rs | 2 +- core/engine/src/value/mod.rs | 2 +- core/parser/src/lexer/number.rs | 2 +- core/runtime/src/text/encodings.rs | 12 ++++++------ 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/core/engine/src/builtins/date/mod.rs b/core/engine/src/builtins/date/mod.rs index 6e29887a1e6..799f941282b 100644 --- a/core/engine/src/builtins/date/mod.rs +++ b/core/engine/src/builtins/date/mod.rs @@ -1599,7 +1599,7 @@ impl Date { let tv = this.to_primitive(context, PreferredType::Number)?; // 3. If Type(tv) is Number and tv is not finite, return null. - if tv.as_number().map(f64::is_finite) == Some(false) { + if tv.as_number().is_some_and(|x| !f64::is_finite(x)) { return Ok(JsValue::null()); } diff --git a/core/engine/src/value/mod.rs b/core/engine/src/value/mod.rs index 3bc71760696..0d4db912885 100644 --- a/core/engine/src/value/mod.rs +++ b/core/engine/src/value/mod.rs @@ -776,7 +776,7 @@ impl JsValue { } // 8. If f is odd, return 𝔽(f + 1). - if f as u8 % 2 != 0 { + if !(f as u8).is_multiple_of(2) { return Ok(f as u8 + 1); } diff --git a/core/parser/src/lexer/number.rs b/core/parser/src/lexer/number.rs index 94cd449d558..88d61af07a4 100644 --- a/core/parser/src/lexer/number.rs +++ b/core/parser/src/lexer/number.rs @@ -142,7 +142,7 @@ where return Err(Error::syntax("separator is not allowed", pos)); } Some(c) => { - if char::from_u32(c).map(|ch| ch.is_digit(kind.base())) == Some(true) { + if char::from_u32(c).is_some_and(|ch| ch.is_digit(kind.base())) { prev_is_underscore = false; #[allow(clippy::cast_possible_truncation)] buf.push(c as u8); diff --git a/core/runtime/src/text/encodings.rs b/core/runtime/src/text/encodings.rs index 28c95ff3a9f..a8f2e7e99ca 100644 --- a/core/runtime/src/text/encodings.rs +++ b/core/runtime/src/text/encodings.rs @@ -31,11 +31,11 @@ pub(crate) mod utf16le { pub(crate) fn decode(mut input: &[u8]) -> JsString { // After this point, input is of even length. - let dangling = if input.len() % 2 != 0 { + let dangling = if input.len().is_multiple_of(2) { + false + } else { input = &input[0..input.len() - 1]; true - } else { - false }; let input: &[u16] = bytemuck::cast_slice(input); @@ -62,12 +62,12 @@ pub(crate) mod utf16be { pub(crate) fn decode(mut input: Vec) -> JsString { let mut input = input.as_mut_slice(); // After this point, input is of even length. - let dangling = if input.len() % 2 != 0 { + let dangling = if input.len().is_multiple_of(2) { + false + } else { let new_len = input.len() - 1; input = &mut input[0..new_len]; true - } else { - false }; let input: &mut [u16] = bytemuck::cast_slice_mut(input); From 89e7fc7f0101c7fbe14cf5ab1381947d95691987 Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Thu, 18 Sep 2025 11:19:36 -0700 Subject: [PATCH 7/7] Use .cargo/config.toml to set miri to use tree-borrows --- .cargo/config.toml | 3 +++ core/engine/src/object/datatypes.rs | 23 +---------------------- 2 files changed, 4 insertions(+), 22 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index cac6e671b05..2520a916838 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -4,3 +4,6 @@ rustflags = ['--cfg', 'getrandom_backend="wasm_js"'] # TODO: track https://github.com/rust-lang/rust/issues/141626 for a resolution [target.x86_64-pc-windows-msvc] rustflags = ['-Csymbol-mangling-version=v0'] + +[env] +MIRIFLAGS = "-Zmiri-tree-borrows" diff --git a/core/engine/src/object/datatypes.rs b/core/engine/src/object/datatypes.rs index 3467e78b7d2..084793e12b0 100644 --- a/core/engine/src/object/datatypes.rs +++ b/core/engine/src/object/datatypes.rs @@ -228,23 +228,7 @@ pub(crate) struct ObjectData { // Because we want to trigger the compile-time const assertion below. // // It is fine if we have as_ref/as_mut to it or any access. - // TODO: see below. - #[cfg(not(miri))] data: T, - - // Miri triggers a UB on `downcast_ref` and `downcast_mut` because it cannot - // figure out that casting T to U does not result in UB as we check that - // objects are U before casting them. Instead of trying to get Miri to - // behave properly, we use a `Box` which always has the same size as - // `Box`. - // - // Note: The `tree-borrows` checker is fine with the above in miri, it is - // only the stack borrow checker that complaints. - // - // TODO: his when we have a task runner OR when tree-borrows is - // the default in Miri. - #[cfg(miri)] - data: Box, } impl Default for ObjectData { @@ -266,12 +250,7 @@ impl ObjectData { // force assertion to triger when we instantiate `ObjectData::new`. let () = Self::OBJECT_DATA_ALIGNMENT_REQUIREMENT; - Self { - #[cfg(not(miri))] - data: value, - #[cfg(miri)] - data: Box::new(value), - } + Self { data: value } } }