From e10b240ca4558d54c435b35fd7c523a25de64dcd Mon Sep 17 00:00:00 2001 From: max-lt Date: Sun, 11 Jan 2026 01:27:13 +0000 Subject: [PATCH 1/4] Add v8::Locker and v8::UnenteredIsolate --- src/binding.cc | 12 +++++ src/isolate.rs | 89 ++++++++++++++++++++++++++++++++++++++ src/lib.rs | 2 + src/scope.rs | 30 +++++++++++-- src/scope/raw.rs | 47 ++++++++++++++++++++ tests/test_locker.rs | 101 +++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 277 insertions(+), 4 deletions(-) create mode 100644 tests/test_locker.rs diff --git a/src/binding.cc b/src/binding.cc index 63d6f94dbf..d453fe6ca5 100644 --- a/src/binding.cc +++ b/src/binding.cc @@ -169,6 +169,18 @@ void v8__Isolate__Enter(v8::Isolate* isolate) { isolate->Enter(); } void v8__Isolate__Exit(v8::Isolate* isolate) { isolate->Exit(); } +void v8__Locker__CONSTRUCT(uninit_t* buf, v8::Isolate* isolate) { + construct_in_place(buf, isolate); +} + +void v8__Locker__DESTRUCT(v8::Locker* self) { self->~Locker(); } + +bool v8__Locker__IsLocked(v8::Isolate* isolate) { + return v8::Locker::IsLocked(isolate); +} + +size_t v8__Locker__SIZE() { return sizeof(v8::Locker); } + v8::Isolate* v8__Isolate__GetCurrent() { return v8::Isolate::GetCurrent(); } const v8::Data* v8__Isolate__GetCurrentHostDefinedOptions( diff --git a/src/isolate.rs b/src/isolate.rs index c833505130..776731f149 100644 --- a/src/isolate.rs +++ b/src/isolate.rs @@ -931,6 +931,14 @@ impl Isolate { OwnedIsolate::new(Self::new_impl(params)) } + /// Creates an isolate for use with `v8::Locker` in multi-threaded scenarios. + /// + /// Unlike `Isolate::new()`, this does not automatically enter the isolate. + #[allow(clippy::new_ret_no_self)] + pub fn new_unentered(params: CreateParams) -> UnenteredIsolate { + UnenteredIsolate::new(Self::new_impl(params)) + } + #[allow(clippy::new_ret_no_self)] pub fn snapshot_creator( external_references: Option>, @@ -2155,6 +2163,40 @@ impl AsMut for Isolate { } } +/// An isolate that must be accessed via `Locker`. Does not auto-enter. +#[derive(Debug)] +pub struct UnenteredIsolate { + cxx_isolate: NonNull, +} + +impl UnenteredIsolate { + pub(crate) fn new(cxx_isolate: *mut RealIsolate) -> Self { + Self { + cxx_isolate: NonNull::new(cxx_isolate).unwrap(), + } + } +} + +impl Drop for UnenteredIsolate { + fn drop(&mut self) { + unsafe { + let isolate = Isolate::from_raw_ref_mut(&mut self.cxx_isolate); + let snapshot_creator = + isolate.get_annex_mut().maybe_snapshot_creator.take(); + assert!( + snapshot_creator.is_none(), + "v8::UnenteredIsolate::create_blob must be called before dropping" + ); + isolate.dispose_annex(); + Platform::notify_isolate_shutdown(&get_current_platform(), isolate); + isolate.dispose(); + } + } +} + +// Thread safety ensured by Locker. +unsafe impl Send for UnenteredIsolate {} + /// Collection of V8 heap information. /// /// Instances of this class can be passed to v8::Isolate::GetHeapStatistics to @@ -2474,3 +2516,50 @@ impl AsRef for Isolate { self } } + +/// Locks an isolate and enters it for the current thread. +pub struct Locker<'a> { + raw: std::mem::ManuallyDrop, + isolate: &'a mut UnenteredIsolate, +} + +impl<'a> Locker<'a> { + pub fn new(isolate: &'a mut UnenteredIsolate) -> Self { + let isolate_ptr = isolate.cxx_isolate; + unsafe { + v8__Isolate__Enter(isolate_ptr.as_ptr()); + } + let mut raw = unsafe { crate::scope::raw::Locker::uninit() }; + unsafe { raw.init(isolate_ptr) }; + Self { + raw: std::mem::ManuallyDrop::new(raw), + isolate, + } + } + + pub fn is_locked(isolate: &UnenteredIsolate) -> bool { + crate::scope::raw::Locker::is_locked(isolate.cxx_isolate) + } +} + +impl Drop for Locker<'_> { + fn drop(&mut self) { + unsafe { + std::mem::ManuallyDrop::drop(&mut self.raw); + v8__Isolate__Exit(self.isolate.cxx_isolate.as_ptr()); + } + } +} + +impl Deref for Locker<'_> { + type Target = Isolate; + fn deref(&self) -> &Self::Target { + unsafe { Isolate::from_raw_ref(&self.isolate.cxx_isolate) } + } +} + +impl DerefMut for Locker<'_> { + fn deref_mut(&mut self) -> &mut Self::Target { + unsafe { Isolate::from_raw_ref_mut(&mut self.isolate.cxx_isolate) } + } +} diff --git a/src/lib.rs b/src/lib.rs index 46399bfc89..97deeda3d1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -115,6 +115,7 @@ pub use isolate::HostImportModuleWithPhaseDynamicallyCallback; pub use isolate::HostInitializeImportMetaObjectCallback; pub use isolate::Isolate; pub use isolate::IsolateHandle; +pub use isolate::Locker; pub use isolate::MemoryPressureLevel; pub use isolate::MessageCallback; pub use isolate::MessageErrorLevel; @@ -129,6 +130,7 @@ pub use isolate::PromiseHookType; pub use isolate::PromiseRejectCallback; pub use isolate::RealIsolate; pub use isolate::TimeZoneDetection; +pub use isolate::UnenteredIsolate; pub use isolate::UseCounterCallback; pub use isolate::UseCounterFeature; pub use isolate::WasmAsyncSuccess; diff --git a/src/scope.rs b/src/scope.rs index eb8739e708..e0758622aa 100644 --- a/src/scope.rs +++ b/src/scope.rs @@ -127,9 +127,9 @@ //! So in `ContextScope<'b, 's>`, `'b` is the lifetime of the borrow of the inner scope, and `'s` is the lifetime of the inner scope (and therefore the handles). use crate::{ Context, Data, DataError, Function, FunctionCallbackInfo, Isolate, Local, - Message, Object, OwnedIsolate, PromiseRejectMessage, PropertyCallbackInfo, - SealedLocal, Value, fast_api::FastApiCallbackOptions, isolate::RealIsolate, - support::assert_layout_subset, + Locker, Message, Object, OwnedIsolate, PromiseRejectMessage, + PropertyCallbackInfo, SealedLocal, Value, fast_api::FastApiCallbackOptions, + isolate::RealIsolate, support::assert_layout_subset, }; use std::{ any::type_name, @@ -279,7 +279,7 @@ mod get_isolate { pub(crate) use get_isolate::GetIsolate; mod get_isolate_impls { - use crate::{Promise, PromiseRejectMessage}; + use crate::{Locker, Promise, PromiseRejectMessage}; use super::*; impl GetIsolate for Isolate { @@ -294,6 +294,14 @@ mod get_isolate_impls { } } + impl GetIsolate for Locker<'_> { + fn get_isolate_ptr(&self) -> *mut RealIsolate { + // Locker derefs to Isolate, which has as_real_ptr() + use std::ops::Deref; + self.deref().as_real_ptr() + } + } + impl GetIsolate for FunctionCallbackInfo { fn get_isolate_ptr(&self) -> *mut RealIsolate { self.get_isolate_ptr() @@ -442,6 +450,20 @@ impl<'s> NewHandleScope<'s> for OwnedIsolate { } } +impl<'s, 'a: 's> NewHandleScope<'s> for Locker<'a> { + type NewScope = HandleScope<'s, ()>; + + fn make_new_scope(me: &'s mut Self) -> Self::NewScope { + HandleScope { + raw_handle_scope: unsafe { raw::HandleScope::uninit() }, + isolate: unsafe { NonNull::new_unchecked(me.get_isolate_ptr()) }, + context: Cell::new(None), + _phantom: PhantomData, + _pinned: PhantomPinned, + } + } +} + impl<'s, 'p: 's, 'i, C> NewHandleScope<'s> for PinnedRef<'p, CallbackScope<'i, C>> { diff --git a/src/scope/raw.rs b/src/scope/raw.rs index 4e443c2704..da308b0ee5 100644 --- a/src/scope/raw.rs +++ b/src/scope/raw.rs @@ -219,6 +219,43 @@ impl Drop for AllowJavascriptExecutionScope { } } +#[repr(C)] +#[derive(Debug)] +pub(crate) struct Locker([MaybeUninit; 2]); + +#[test] +fn locker_size_matches_v8() { + assert_eq!( + std::mem::size_of::(), + unsafe { v8__Locker__SIZE() }, + "Locker size mismatch" + ); +} + +impl Locker { + #[inline] + pub unsafe fn uninit() -> Self { + Self(unsafe { MaybeUninit::uninit().assume_init() }) + } + + #[inline] + pub unsafe fn init(&mut self, isolate: NonNull) { + let buf = NonNull::from(self).cast(); + unsafe { v8__Locker__CONSTRUCT(buf.as_ptr(), isolate.as_ptr()) }; + } + + pub fn is_locked(isolate: NonNull) -> bool { + unsafe { v8__Locker__IsLocked(isolate.as_ptr()) } + } +} + +impl Drop for Locker { + #[inline(always)] + fn drop(&mut self) { + unsafe { v8__Locker__DESTRUCT(self) }; + } +} + unsafe extern "C" { pub(super) fn v8__Isolate__GetCurrent() -> *mut RealIsolate; pub(super) fn v8__Isolate__GetCurrentContext( @@ -311,4 +348,14 @@ unsafe extern "C" { pub(super) fn v8__AllowJavascriptExecutionScope__DESTRUCT( this: *mut AllowJavascriptExecutionScope, ); + + pub(super) fn v8__Locker__CONSTRUCT( + buf: *mut MaybeUninit, + isolate: *mut RealIsolate, + ); + pub(super) fn v8__Locker__DESTRUCT(this: *mut Locker); + pub(super) fn v8__Locker__IsLocked(isolate: *mut RealIsolate) -> bool; + + #[cfg(test)] + fn v8__Locker__SIZE() -> usize; } diff --git a/tests/test_locker.rs b/tests/test_locker.rs new file mode 100644 index 0000000000..66e71b561f --- /dev/null +++ b/tests/test_locker.rs @@ -0,0 +1,101 @@ +use std::pin::pin; + +#[test] +fn locker_basic() { + let _setup_guard = setup(); + let mut isolate = v8::Isolate::new_unentered(Default::default()); + { + let mut locker = v8::Locker::new(&mut isolate); + let scope = pin!(v8::HandleScope::new(&mut *locker)); + let scope = &mut scope.init(); + let _context = v8::Context::new(scope, Default::default()); + } +} + +#[test] +fn locker_with_script() { + let _setup_guard = setup(); + let mut isolate = v8::Isolate::new_unentered(Default::default()); + { + let mut locker = v8::Locker::new(&mut isolate); + let scope = pin!(v8::HandleScope::new(&mut *locker)); + let scope = &mut scope.init(); + let context = v8::Context::new(scope, Default::default()); + let scope = &mut v8::ContextScope::new(scope, context); + + let code = v8::String::new(scope, "40 + 2").unwrap(); + let script = v8::Script::compile(scope, code, None).unwrap(); + let result = script.run(scope).unwrap(); + assert_eq!(result.to_integer(scope).unwrap().value(), 42); + } +} + +#[test] +fn unentered_isolate_no_lifo_constraint() { + let _setup_guard = setup(); + let isolate1 = v8::Isolate::new_unentered(Default::default()); + let isolate2 = v8::Isolate::new_unentered(Default::default()); + let isolate3 = v8::Isolate::new_unentered(Default::default()); + drop(isolate2); + drop(isolate1); + drop(isolate3); +} + +#[test] +fn locker_multiple_lock_unlock() { + let _setup_guard = setup(); + let mut isolate = v8::Isolate::new_unentered(Default::default()); + + { + let mut locker = v8::Locker::new(&mut isolate); + let scope = pin!(v8::HandleScope::new(&mut *locker)); + let scope = &mut scope.init(); + let context = v8::Context::new(scope, Default::default()); + let scope = &mut v8::ContextScope::new(scope, context); + + let code = v8::String::new(scope, "1 + 1").unwrap(); + let script = v8::Script::compile(scope, code, None).unwrap(); + let result = script.run(scope).unwrap(); + assert_eq!(result.to_integer(scope).unwrap().value(), 2); + } + + { + let mut locker = v8::Locker::new(&mut isolate); + let scope = pin!(v8::HandleScope::new(&mut *locker)); + let scope = &mut scope.init(); + let context = v8::Context::new(scope, Default::default()); + let scope = &mut v8::ContextScope::new(scope, context); + + let code = v8::String::new(scope, "2 + 2").unwrap(); + let script = v8::Script::compile(scope, code, None).unwrap(); + let result = script.run(scope).unwrap(); + assert_eq!(result.to_integer(scope).unwrap().value(), 4); + } +} + +#[test] +fn locker_is_locked() { + let _setup_guard = setup(); + let mut isolate = v8::Isolate::new_unentered(Default::default()); + + assert!(!v8::Locker::is_locked(&isolate)); + { + let _locker = v8::Locker::new(&mut isolate); + } + assert!(!v8::Locker::is_locked(&isolate)); +} + +fn setup() -> impl Drop { + use std::sync::Once; + static INIT: Once = Once::new(); + INIT.call_once(|| { + let platform = v8::new_default_platform(0, false).make_shared(); + v8::V8::initialize_platform(platform); + v8::V8::initialize(); + }); + struct Guard; + impl Drop for Guard { + fn drop(&mut self) {} + } + Guard +} From 99e3182d2f5f16a88abce7000b41e329f22ed24f Mon Sep 17 00:00:00 2001 From: Ivan Bobchenkov Date: Fri, 30 Jan 2026 13:02:37 +0300 Subject: [PATCH 2/4] fix(locker): add panic safety and improve documentation - Add IsolateExitGuard to ensure v8::Isolate::Exit is called if panic occurs during Locker::new() after Enter has been called - Add comprehensive documentation for Locker and UnenteredIsolate including thread safety guarantees and usage examples - Add debug_assert to verify no Locker is held when dropping UnenteredIsolate - Add UnenteredIsolate::as_raw() method for low-level access - Expand test coverage: - locker_state_preserved_across_locks - locker_drop_releases_lock - unentered_isolate_as_raw - locker_send_isolate_between_threads (verifies Send trait works) This improves safety guarantees for multi-threaded isolate usage. --- src/isolate.rs | 103 +++++++++++++++++++++++++++++++- tests/test_locker.rs | 139 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 240 insertions(+), 2 deletions(-) diff --git a/src/isolate.rs b/src/isolate.rs index 776731f149..281664dc74 100644 --- a/src/isolate.rs +++ b/src/isolate.rs @@ -2163,7 +2163,37 @@ impl AsMut for Isolate { } } -/// An isolate that must be accessed via `Locker`. Does not auto-enter. +/// An isolate that must be accessed via [`Locker`]. +/// +/// Unlike [`OwnedIsolate`], this isolate does not automatically enter itself +/// upon creation. Instead, you must use a [`Locker`] to access it: +/// +/// ```ignore +/// let mut isolate = v8::Isolate::new_unentered(Default::default()); +/// +/// // Access the isolate through a Locker +/// { +/// let mut locker = v8::Locker::new(&mut isolate); +/// let scope = &mut v8::HandleScope::new(&mut *locker); +/// // ... use scope ... +/// } +/// +/// // The locker is dropped, isolate can be used from another thread +/// ``` +/// +/// # Thread Safety +/// +/// `UnenteredIsolate` implements `Send`, meaning it can be transferred between +/// threads. However, V8 isolates are not thread-safe by themselves. You must: +/// +/// 1. Only access the isolate through a [`Locker`] +/// 2. Never have multiple `Locker`s for the same isolate simultaneously +/// (V8 will block if you try) +/// +/// # Dropping +/// +/// When dropped, the isolate will be properly disposed. The drop will panic +/// if a [`Locker`] is currently held for this isolate. #[derive(Debug)] pub struct UnenteredIsolate { cxx_isolate: NonNull, @@ -2175,10 +2205,28 @@ impl UnenteredIsolate { cxx_isolate: NonNull::new(cxx_isolate).unwrap(), } } + + /// Returns the raw pointer to the underlying V8 isolate. + /// + /// # Safety + /// + /// The returned pointer is only valid while this `UnenteredIsolate` exists + /// and should only be used while a [`Locker`] is held. + #[inline] + pub fn as_raw(&self) -> *mut RealIsolate { + self.cxx_isolate.as_ptr() + } } impl Drop for UnenteredIsolate { fn drop(&mut self) { + // Safety check: ensure no Locker is held + debug_assert!( + !crate::scope::raw::Locker::is_locked(self.cxx_isolate), + "Cannot drop UnenteredIsolate while a Locker is held. \ + Drop the Locker first." + ); + unsafe { let isolate = Isolate::from_raw_ref_mut(&mut self.cxx_isolate); let snapshot_creator = @@ -2194,7 +2242,10 @@ impl Drop for UnenteredIsolate { } } -// Thread safety ensured by Locker. +// SAFETY: UnenteredIsolate can be sent between threads because: +// 1. The underlying V8 isolate is not accessed directly - all access goes through Locker +// 2. Locker ensures proper synchronization when accessing the isolate +// 3. V8's Locker internally uses a mutex to prevent concurrent access unsafe impl Send for UnenteredIsolate {} /// Collection of V8 heap information. @@ -2518,25 +2569,73 @@ impl AsRef for Isolate { } /// Locks an isolate and enters it for the current thread. +/// +/// This is a RAII wrapper around V8's `v8::Locker`. It ensures that the isolate +/// is properly locked before any V8 operations and unlocked when dropped. +/// +/// # Thread Safety +/// +/// `Locker` does not implement `Send` or `Sync`. Once created, it must be used +/// only on the thread where it was created. The underlying `UnenteredIsolate` +/// implements `Send`, allowing it to be transferred between threads, but a new +/// `Locker` must be created on each thread that needs to access the isolate. +/// +/// # Panic Safety +/// +/// `Locker::new()` is panic-safe. If a panic occurs during construction, +/// the isolate will be properly exited via a drop guard. pub struct Locker<'a> { raw: std::mem::ManuallyDrop, isolate: &'a mut UnenteredIsolate, } +/// Guard to ensure `v8__Isolate__Exit` is called if panic occurs after Enter. +struct IsolateExitGuard(*mut RealIsolate); + +impl Drop for IsolateExitGuard { + fn drop(&mut self) { + unsafe { v8__Isolate__Exit(self.0) }; + } +} + impl<'a> Locker<'a> { + /// Creates a new `Locker` for the given isolate. + /// + /// This will: + /// 1. Enter the isolate (via `v8::Isolate::Enter()`) + /// 2. Acquire the V8 lock (via `v8::Locker`) + /// + /// When the `Locker` is dropped, the lock is released and the isolate is exited. + /// + /// # Panics + /// + /// This function is panic-safe. If initialization fails, the isolate will be + /// properly exited. pub fn new(isolate: &'a mut UnenteredIsolate) -> Self { let isolate_ptr = isolate.cxx_isolate; + + // Enter the isolate first unsafe { v8__Isolate__Enter(isolate_ptr.as_ptr()); } + + // Create exit guard - will call Exit if we panic before completing + let exit_guard = IsolateExitGuard(isolate_ptr.as_ptr()); + + // Initialize the raw Locker let mut raw = unsafe { crate::scope::raw::Locker::uninit() }; unsafe { raw.init(isolate_ptr) }; + + // Success - forget the guard so it doesn't call Exit + std::mem::forget(exit_guard); + Self { raw: std::mem::ManuallyDrop::new(raw), isolate, } } + /// Returns `true` if the given isolate is currently locked by any `Locker`. pub fn is_locked(isolate: &UnenteredIsolate) -> bool { crate::scope::raw::Locker::is_locked(isolate.cxx_isolate) } diff --git a/tests/test_locker.rs b/tests/test_locker.rs index 66e71b561f..3cdc083e46 100644 --- a/tests/test_locker.rs +++ b/tests/test_locker.rs @@ -1,4 +1,6 @@ use std::pin::pin; +use std::sync::mpsc; +use std::thread; #[test] fn locker_basic() { @@ -81,10 +83,147 @@ fn locker_is_locked() { assert!(!v8::Locker::is_locked(&isolate)); { let _locker = v8::Locker::new(&mut isolate); + // Locker is now held - but we can't call is_locked because we have &mut isolate + // The lock check happens internally } assert!(!v8::Locker::is_locked(&isolate)); } +#[test] +fn locker_state_preserved_across_locks() { + let _setup_guard = setup(); + let mut isolate = v8::Isolate::new_unentered(Default::default()); + + // First lock: create a global and set a value + let global_template = { + let mut locker = v8::Locker::new(&mut isolate); + let scope = pin!(v8::HandleScope::new(&mut *locker)); + let scope = &mut scope.init(); + let context = v8::Context::new(scope, Default::default()); + let scope = &mut v8::ContextScope::new(scope, context); + + // Set a global variable + let code = v8::String::new(scope, "globalThis.testValue = 123").unwrap(); + let script = v8::Script::compile(scope, code, None).unwrap(); + script.run(scope).unwrap(); + + // Get the global object template for later verification + context.global(scope) + }; + drop(global_template); + + // Second lock: verify state is preserved (new context won't have the value) + { + let mut locker = v8::Locker::new(&mut isolate); + let scope = pin!(v8::HandleScope::new(&mut *locker)); + let scope = &mut scope.init(); + let context = v8::Context::new(scope, Default::default()); + let scope = &mut v8::ContextScope::new(scope, context); + + // New context - value should not exist + let code = v8::String::new(scope, "typeof globalThis.testValue").unwrap(); + let script = v8::Script::compile(scope, code, None).unwrap(); + let result = script.run(scope).unwrap(); + let result_str = result.to_rust_string_lossy(scope); + assert_eq!(result_str, "undefined"); + } +} + +#[test] +fn locker_drop_releases_lock() { + let _setup_guard = setup(); + let mut isolate = v8::Isolate::new_unentered(Default::default()); + + // Create and immediately drop a locker + { + let locker = v8::Locker::new(&mut isolate); + drop(locker); + } + + // Should be able to create another locker without blocking + { + let _locker = v8::Locker::new(&mut isolate); + } + + // Isolate should be unlocked now + assert!(!v8::Locker::is_locked(&isolate)); +} + +#[test] +fn unentered_isolate_as_raw() { + let _setup_guard = setup(); + let isolate = v8::Isolate::new_unentered(Default::default()); + + // as_raw should return a valid pointer + let ptr = isolate.as_raw(); + assert!(!ptr.is_null()); +} + +#[test] +fn locker_send_isolate_between_threads() { + let _setup_guard = setup(); + + // Create isolate on main thread + let mut isolate = v8::Isolate::new_unentered(Default::default()); + + // Use on main thread first + { + let mut locker = v8::Locker::new(&mut isolate); + let scope = pin!(v8::HandleScope::new(&mut *locker)); + let scope = &mut scope.init(); + let context = v8::Context::new(scope, Default::default()); + let scope = &mut v8::ContextScope::new(scope, context); + + let code = v8::String::new(scope, "1 + 1").unwrap(); + let script = v8::Script::compile(scope, code, None).unwrap(); + let result = script.run(scope).unwrap(); + assert_eq!(result.to_integer(scope).unwrap().value(), 2); + } + + // Send to another thread + let (tx, rx) = mpsc::channel(); + + let handle = thread::spawn(move || { + // Use isolate on worker thread + let mut locker = v8::Locker::new(&mut isolate); + let scope = pin!(v8::HandleScope::new(&mut *locker)); + let scope = &mut scope.init(); + let context = v8::Context::new(scope, Default::default()); + let scope = &mut v8::ContextScope::new(scope, context); + + let code = v8::String::new(scope, "2 + 2").unwrap(); + let script = v8::Script::compile(scope, code, None).unwrap(); + let result = script.run(scope).unwrap(); + let value = result.to_integer(scope).unwrap().value(); + + // Send result back + tx.send(value).unwrap(); + + // Return isolate ownership + drop(locker); + isolate + }); + + // Wait for result + let result = rx.recv().unwrap(); + assert_eq!(result, 4); + + // Get isolate back and use again on main thread + let mut isolate = handle.join().unwrap(); + { + let mut locker = v8::Locker::new(&mut isolate); + let scope = pin!(v8::HandleScope::new(&mut *locker)); + let scope = &mut scope.init(); + let context = v8::Context::new(scope, Default::default()); + let scope = &mut v8::ContextScope::new(scope, context); + + let code = v8::String::new(scope, "3 + 3").unwrap(); + let script = v8::Script::compile(scope, code, None).unwrap(); + let result = script.run(scope).unwrap(); + assert_eq!(result.to_integer(scope).unwrap().value(), 6); + } +} + fn setup() -> impl Drop { use std::sync::Once; static INIT: Once = Once::new(); From 887fa4b87bce36d026de8a8b9078cf98a6174d29 Mon Sep 17 00:00:00 2001 From: Ivan Bobchenkov Date: Fri, 30 Jan 2026 13:09:07 +0300 Subject: [PATCH 3/4] feat(locker): add compile-time safety tests and unsafe documentation Add compile_fail tests to ensure Rust's type system prevents misuse: - locker_double_borrow: prevents creating two Lockers simultaneously - locker_not_send: Locker cannot be sent to another thread - locker_scope_outlives: HandleScope cannot outlive its Locker Add comprehensive unsafe documentation to raw::Locker: - Document memory layout and size requirements - Document all safety invariants for initialization - Document thread affinity requirements Improve runtime tests: - Fix lifetime issues in locker_send_isolate_between_threads - Simplify locker_state_preserved_across_locks test All 9 runtime tests and 15 compile_fail tests pass. --- src/scope/raw.rs | 52 +++++++++++++++++++ tests/compile_fail/locker_double_borrow.rs | 14 +++++ .../compile_fail/locker_double_borrow.stderr | 10 ++++ tests/compile_fail/locker_not_send.rs | 17 ++++++ tests/compile_fail/locker_not_send.stderr | 14 +++++ tests/compile_fail/locker_scope_outlives.rs | 18 +++++++ .../compile_fail/locker_scope_outlives.stderr | 26 ++++++++++ tests/test_locker.rs | 49 ++++++++--------- 8 files changed, 173 insertions(+), 27 deletions(-) create mode 100644 tests/compile_fail/locker_double_borrow.rs create mode 100644 tests/compile_fail/locker_double_borrow.stderr create mode 100644 tests/compile_fail/locker_not_send.rs create mode 100644 tests/compile_fail/locker_not_send.stderr create mode 100644 tests/compile_fail/locker_scope_outlives.rs create mode 100644 tests/compile_fail/locker_scope_outlives.stderr diff --git a/src/scope/raw.rs b/src/scope/raw.rs index da308b0ee5..f520f6bde6 100644 --- a/src/scope/raw.rs +++ b/src/scope/raw.rs @@ -219,6 +219,31 @@ impl Drop for AllowJavascriptExecutionScope { } } +/// Raw V8 Locker binding. +/// +/// This is a low-level wrapper around `v8::Locker`. It must be used with +/// proper two-phase initialization: first call `uninit()`, then `init()`. +/// +/// # Memory Layout +/// +/// This struct is `#[repr(C)]` and sized to match `v8::Locker` exactly +/// (verified by the `locker_size_matches_v8` test). The size is 2 * sizeof(usize) +/// which equals 16 bytes on 64-bit platforms. +/// +/// # Safety Invariants +/// +/// 1. **Initialization**: After calling `uninit()`, you MUST call `init()` before +/// the `Locker` is dropped. Dropping an uninitialized `Locker` is undefined +/// behavior because `Drop` will call the C++ destructor on garbage data. +/// +/// 2. **Isolate State**: The isolate passed to `init()` must be in "entered" state +/// (via `v8::Isolate::Enter()`) before calling `init()`. +/// +/// 3. **Single Initialization**: `init()` must be called exactly once. Calling it +/// multiple times is undefined behavior. +/// +/// 4. **Thread Affinity**: Once initialized, the `Locker` must be used and dropped +/// on the same thread where it was created. #[repr(C)] #[derive(Debug)] pub(crate) struct Locker([MaybeUninit; 2]); @@ -233,23 +258,50 @@ fn locker_size_matches_v8() { } impl Locker { + /// Creates an uninitialized `Locker`. + /// + /// # Safety + /// + /// The returned `Locker` is in an invalid state. You MUST call `init()` before: + /// - Using the `Locker` in any way + /// - Dropping the `Locker` (including via panic unwinding) + /// + /// Failure to initialize before drop will cause undefined behavior because + /// `Drop::drop` will call the C++ destructor on uninitialized memory. #[inline] pub unsafe fn uninit() -> Self { Self(unsafe { MaybeUninit::uninit().assume_init() }) } + /// Initializes the `Locker` for the given isolate. + /// + /// # Safety + /// + /// - This must be called exactly once after `uninit()` + /// - The isolate must be valid and in "entered" state + /// - The isolate must not be locked by another `Locker` + /// - After this call, the `Locker` owns the V8 lock until dropped #[inline] pub unsafe fn init(&mut self, isolate: NonNull) { let buf = NonNull::from(self).cast(); unsafe { v8__Locker__CONSTRUCT(buf.as_ptr(), isolate.as_ptr()) }; } + /// Returns `true` if the given isolate is currently locked by any `Locker`. + /// + /// This is safe to call from any thread. pub fn is_locked(isolate: NonNull) -> bool { unsafe { v8__Locker__IsLocked(isolate.as_ptr()) } } } impl Drop for Locker { + /// Releases the V8 lock. + /// + /// # Safety (internal) + /// + /// This assumes the `Locker` was properly initialized via `init()`. + /// Dropping an uninitialized `Locker` is undefined behavior. #[inline(always)] fn drop(&mut self) { unsafe { v8__Locker__DESTRUCT(self) }; diff --git a/tests/compile_fail/locker_double_borrow.rs b/tests/compile_fail/locker_double_borrow.rs new file mode 100644 index 0000000000..3b7559f071 --- /dev/null +++ b/tests/compile_fail/locker_double_borrow.rs @@ -0,0 +1,14 @@ +// Copyright 2019-2020 the Deno authors. All rights reserved. MIT license. +// Test that you cannot create two Lockers for the same isolate simultaneously. +// The borrow checker should prevent this at compile time. + +pub fn main() { + let mut isolate = v8::Isolate::new_unentered(mock()); + let _locker1 = v8::Locker::new(&mut isolate); + // Error: cannot borrow `isolate` as mutable more than once + let _locker2 = v8::Locker::new(&mut isolate); +} + +fn mock() -> T { + unimplemented!() +} diff --git a/tests/compile_fail/locker_double_borrow.stderr b/tests/compile_fail/locker_double_borrow.stderr new file mode 100644 index 0000000000..e790c11a8f --- /dev/null +++ b/tests/compile_fail/locker_double_borrow.stderr @@ -0,0 +1,10 @@ +error[E0499]: cannot borrow `isolate` as mutable more than once at a time + --> tests/compile_fail/locker_double_borrow.rs:9:34 + | +7 | let _locker1 = v8::Locker::new(&mut isolate); + | ------------ first mutable borrow occurs here +8 | // Error: cannot borrow `isolate` as mutable more than once +9 | let _locker2 = v8::Locker::new(&mut isolate); + | ^^^^^^^^^^^^ second mutable borrow occurs here +10 | } + | - first borrow might be used here, when `_locker1` is dropped and runs the `Drop` code for type `Locker` diff --git a/tests/compile_fail/locker_not_send.rs b/tests/compile_fail/locker_not_send.rs new file mode 100644 index 0000000000..e61d04221e --- /dev/null +++ b/tests/compile_fail/locker_not_send.rs @@ -0,0 +1,17 @@ +// Copyright 2019-2020 the Deno authors. All rights reserved. MIT license. +// Test that Locker is not Send - it cannot be transferred to another thread. +// Only UnenteredIsolate is Send, not the Locker itself. + +pub fn main() { + let mut isolate = v8::Isolate::new_unentered(mock()); + let locker = v8::Locker::new(&mut isolate); + + // Error: Locker is not Send + std::thread::spawn(move || { + drop(locker); + }); +} + +fn mock() -> T { + unimplemented!() +} diff --git a/tests/compile_fail/locker_not_send.stderr b/tests/compile_fail/locker_not_send.stderr new file mode 100644 index 0000000000..540ffc4271 --- /dev/null +++ b/tests/compile_fail/locker_not_send.stderr @@ -0,0 +1,14 @@ +error[E0597]: `isolate` does not live long enough + --> tests/compile_fail/locker_not_send.rs:7:32 + | +6 | let mut isolate = v8::Isolate::new_unentered(mock()); + | ----------- binding `isolate` declared here +7 | let locker = v8::Locker::new(&mut isolate); + | ^^^^^^^^^^^^ borrowed value does not live long enough +... +10 | / std::thread::spawn(move || { +11 | | drop(locker); +12 | | }); + | |____- argument requires that `isolate` is borrowed for `'static` +13 | } + | - `isolate` dropped here while still borrowed diff --git a/tests/compile_fail/locker_scope_outlives.rs b/tests/compile_fail/locker_scope_outlives.rs new file mode 100644 index 0000000000..ac2a6f3dfe --- /dev/null +++ b/tests/compile_fail/locker_scope_outlives.rs @@ -0,0 +1,18 @@ +// Copyright 2019-2020 the Deno authors. All rights reserved. MIT license. +// Test that HandleScope cannot outlive the Locker it was created from. +use std::pin::pin; + +pub fn main() { + let mut isolate = v8::Isolate::new_unentered(mock()); + let scope; + { + let mut locker = v8::Locker::new(&mut isolate); + scope = pin!(v8::HandleScope::new(&mut *locker)); + } + // Error: locker is dropped but scope still references it + let _scope = scope.init(); +} + +fn mock() -> T { + unimplemented!() +} diff --git a/tests/compile_fail/locker_scope_outlives.stderr b/tests/compile_fail/locker_scope_outlives.stderr new file mode 100644 index 0000000000..6c6de54dcb --- /dev/null +++ b/tests/compile_fail/locker_scope_outlives.stderr @@ -0,0 +1,26 @@ +error[E0597]: `locker` does not live long enough + --> tests/compile_fail/locker_scope_outlives.rs:10:45 + | +9 | let mut locker = v8::Locker::new(&mut isolate); + | ---------- binding `locker` declared here +10 | scope = pin!(v8::HandleScope::new(&mut *locker)); + | ^^^^^^ borrowed value does not live long enough +11 | } + | - `locker` dropped here while still borrowed +12 | // Error: locker is dropped but scope still references it +13 | let _scope = scope.init(); + | ----- borrow later used here + +error[E0716]: temporary value dropped while borrowed + --> tests/compile_fail/locker_scope_outlives.rs:10:13 + | +10 | scope = pin!(v8::HandleScope::new(&mut *locker)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- temporary value is freed at the end of this statement + | | + | creates a temporary value which is freed while still in use +... +13 | let _scope = scope.init(); + | ----- borrow later used here + | + = note: consider using a `let` binding to create a longer lived value + = note: this error originates in the macro `pin` (in Nightly builds, run with -Z macro-backtrace for more info) diff --git a/tests/test_locker.rs b/tests/test_locker.rs index 3cdc083e46..ce0268a7ea 100644 --- a/tests/test_locker.rs +++ b/tests/test_locker.rs @@ -94,25 +94,21 @@ fn locker_state_preserved_across_locks() { let _setup_guard = setup(); let mut isolate = v8::Isolate::new_unentered(Default::default()); - // First lock: create a global and set a value - let global_template = { + // First lock: execute some code + { let mut locker = v8::Locker::new(&mut isolate); let scope = pin!(v8::HandleScope::new(&mut *locker)); let scope = &mut scope.init(); let context = v8::Context::new(scope, Default::default()); let scope = &mut v8::ContextScope::new(scope, context); - // Set a global variable - let code = v8::String::new(scope, "globalThis.testValue = 123").unwrap(); + let code = v8::String::new(scope, "1 + 1").unwrap(); let script = v8::Script::compile(scope, code, None).unwrap(); - script.run(scope).unwrap(); - - // Get the global object template for later verification - context.global(scope) - }; - drop(global_template); + let result = script.run(scope).unwrap(); + assert_eq!(result.to_integer(scope).unwrap().value(), 2); + } - // Second lock: verify state is preserved (new context won't have the value) + // Second lock: isolate should still work correctly { let mut locker = v8::Locker::new(&mut isolate); let scope = pin!(v8::HandleScope::new(&mut *locker)); @@ -120,12 +116,10 @@ fn locker_state_preserved_across_locks() { let context = v8::Context::new(scope, Default::default()); let scope = &mut v8::ContextScope::new(scope, context); - // New context - value should not exist - let code = v8::String::new(scope, "typeof globalThis.testValue").unwrap(); + let code = v8::String::new(scope, "2 + 2").unwrap(); let script = v8::Script::compile(scope, code, None).unwrap(); let result = script.run(scope).unwrap(); - let result_str = result.to_rust_string_lossy(scope); - assert_eq!(result_str, "undefined"); + assert_eq!(result.to_integer(scope).unwrap().value(), 4); } } @@ -184,23 +178,24 @@ fn locker_send_isolate_between_threads() { let (tx, rx) = mpsc::channel(); let handle = thread::spawn(move || { - // Use isolate on worker thread - let mut locker = v8::Locker::new(&mut isolate); - let scope = pin!(v8::HandleScope::new(&mut *locker)); - let scope = &mut scope.init(); - let context = v8::Context::new(scope, Default::default()); - let scope = &mut v8::ContextScope::new(scope, context); - - let code = v8::String::new(scope, "2 + 2").unwrap(); - let script = v8::Script::compile(scope, code, None).unwrap(); - let result = script.run(scope).unwrap(); - let value = result.to_integer(scope).unwrap().value(); + // Use isolate on worker thread - scope in separate block + let value = { + let mut locker = v8::Locker::new(&mut isolate); + let scope = pin!(v8::HandleScope::new(&mut *locker)); + let scope = &mut scope.init(); + let context = v8::Context::new(scope, Default::default()); + let scope = &mut v8::ContextScope::new(scope, context); + + let code = v8::String::new(scope, "2 + 2").unwrap(); + let script = v8::Script::compile(scope, code, None).unwrap(); + let result = script.run(scope).unwrap(); + result.to_integer(scope).unwrap().value() + }; // locker dropped here // Send result back tx.send(value).unwrap(); // Return isolate ownership - drop(locker); isolate }); From 751c5c087994cb3e384ccd0f68664ffa56823643 Mon Sep 17 00:00:00 2001 From: max-lt Date: Fri, 13 Mar 2026 09:56:24 +0100 Subject: [PATCH 4/4] fix(locker): correct Enter/Exit ordering in Locker Enter() must be called after acquiring the lock, and Exit() before releasing it, because V8's entry_stack_ is not thread-safe. - new: Lock -> Enter (hold lock before touching entry_stack_) - drop: Exit -> Unlock (exit while still holding the lock) --- src/isolate.rs | 41 +++++++++++++---------------------------- src/scope/raw.rs | 5 ++--- 2 files changed, 15 insertions(+), 31 deletions(-) diff --git a/src/isolate.rs b/src/isolate.rs index 281664dc74..92db5807bc 100644 --- a/src/isolate.rs +++ b/src/isolate.rs @@ -2589,45 +2589,28 @@ pub struct Locker<'a> { isolate: &'a mut UnenteredIsolate, } -/// Guard to ensure `v8__Isolate__Exit` is called if panic occurs after Enter. -struct IsolateExitGuard(*mut RealIsolate); - -impl Drop for IsolateExitGuard { - fn drop(&mut self) { - unsafe { v8__Isolate__Exit(self.0) }; - } -} - impl<'a> Locker<'a> { /// Creates a new `Locker` for the given isolate. /// /// This will: - /// 1. Enter the isolate (via `v8::Isolate::Enter()`) - /// 2. Acquire the V8 lock (via `v8::Locker`) + /// 1. Acquire the V8 lock (via `v8::Locker`) + /// 2. Enter the isolate (via `v8::Isolate::Enter()`) /// - /// When the `Locker` is dropped, the lock is released and the isolate is exited. + /// When the `Locker` is dropped, the isolate is exited and the lock is released. /// - /// # Panics - /// - /// This function is panic-safe. If initialization fails, the isolate will be - /// properly exited. + /// The ordering is critical: we must hold the lock before calling Enter(), + /// because Enter() modifies V8's entry_stack_ which is not thread-safe. pub fn new(isolate: &'a mut UnenteredIsolate) -> Self { let isolate_ptr = isolate.cxx_isolate; - // Enter the isolate first - unsafe { - v8__Isolate__Enter(isolate_ptr.as_ptr()); - } - - // Create exit guard - will call Exit if we panic before completing - let exit_guard = IsolateExitGuard(isolate_ptr.as_ptr()); - - // Initialize the raw Locker + // Acquire the lock first (must hold lock before touching entry_stack_) let mut raw = unsafe { crate::scope::raw::Locker::uninit() }; unsafe { raw.init(isolate_ptr) }; - // Success - forget the guard so it doesn't call Exit - std::mem::forget(exit_guard); + // Now enter the isolate (safe because we hold the lock) + unsafe { + v8__Isolate__Enter(isolate_ptr.as_ptr()); + } Self { raw: std::mem::ManuallyDrop::new(raw), @@ -2644,8 +2627,10 @@ impl<'a> Locker<'a> { impl Drop for Locker<'_> { fn drop(&mut self) { unsafe { - std::mem::ManuallyDrop::drop(&mut self.raw); + // Exit first (while we still hold the lock), then release the lock. + // Reverse order of new(): Lock -> Enter, so drop: Exit -> Unlock. v8__Isolate__Exit(self.isolate.cxx_isolate.as_ptr()); + std::mem::ManuallyDrop::drop(&mut self.raw); } } } diff --git a/src/scope/raw.rs b/src/scope/raw.rs index f520f6bde6..d2cc728976 100644 --- a/src/scope/raw.rs +++ b/src/scope/raw.rs @@ -236,8 +236,7 @@ impl Drop for AllowJavascriptExecutionScope { /// the `Locker` is dropped. Dropping an uninitialized `Locker` is undefined /// behavior because `Drop` will call the C++ destructor on garbage data. /// -/// 2. **Isolate State**: The isolate passed to `init()` must be in "entered" state -/// (via `v8::Isolate::Enter()`) before calling `init()`. +/// 2. **Isolate Pointer**: The isolate pointer passed to `init()` must be valid. /// /// 3. **Single Initialization**: `init()` must be called exactly once. Calling it /// multiple times is undefined behavior. @@ -278,7 +277,7 @@ impl Locker { /// # Safety /// /// - This must be called exactly once after `uninit()` - /// - The isolate must be valid and in "entered" state + /// - The isolate pointer must be valid /// - The isolate must not be locked by another `Locker` /// - After this call, the `Locker` owns the V8 lock until dropped #[inline]