From 196b5f60e4bf5a80b398c7e1daeb1f9f7b6a35d3 Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Sat, 21 Dec 2019 06:08:00 +0100 Subject: [PATCH] Fix more mutability and lifetime issues (#103) --- src/binding.cc | 36 +++++++++---------- src/exception.rs | 4 +-- src/function.rs | 27 +++++++++------ src/handle_scope.rs | 3 +- src/locker.rs | 23 ++++++++++--- src/property.rs | 5 +-- tests/test_api.rs | 84 ++++++++++++++++++++++----------------------- 7 files changed, 102 insertions(+), 80 deletions(-) diff --git a/src/binding.cc b/src/binding.cc index d04b1292..70a2a5d7 100644 --- a/src/binding.cc +++ b/src/binding.cc @@ -296,32 +296,32 @@ v8::Function* v8__FunctionTemplate__GetFunction( return maybe_local_to_ptr(self->GetFunction(context)); } int v8__FunctionCallbackInfo__Length( - v8::FunctionCallbackInfo* self) { - return self->Length(); + const v8::FunctionCallbackInfo& self) { + return self.Length(); } v8::Isolate* v8__FunctionCallbackInfo__GetIsolate( - v8::FunctionCallbackInfo* self) { - return self->GetIsolate(); + const v8::FunctionCallbackInfo& self) { + return self.GetIsolate(); } void v8__FunctionCallbackInfo__GetReturnValue( - v8::FunctionCallbackInfo* self, + const v8::FunctionCallbackInfo& self, v8::ReturnValue* out) { - *out = self->GetReturnValue(); + *out = self.GetReturnValue(); } -void v8__ReturnValue__Set(v8::ReturnValue* self, +void v8__ReturnValue__Set(v8::ReturnValue& self, v8::Local value) { - self->Set(value); + self.Set(value); } -v8::Value* v8__ReturnValue__Get(v8::ReturnValue* self) { - return local_to_ptr(self->Get()); +v8::Value* v8__ReturnValue__Get(const v8::ReturnValue& self) { + return local_to_ptr(self.Get()); } -v8::Isolate* v8__ReturnValue__GetIsolate(v8::ReturnValue* self) { - return self->GetIsolate(); +v8::Isolate* v8__ReturnValue__GetIsolate(v8::ReturnValue& self) { + return self.GetIsolate(); } int v8__StackTrace__GetFrameCount(v8::StackTrace* self) { @@ -478,19 +478,19 @@ v8::Value* v8__PromiseRejectMessage__GetValue( } v8::Isolate* v8__PropertyCallbackInfo__GetIsolate( - const v8::PropertyCallbackInfo* self) { - return self->GetIsolate(); + const v8::PropertyCallbackInfo& self) { + return self.GetIsolate(); } v8::Object* v8__PropertyCallbackInfo__This( - const v8::PropertyCallbackInfo* self) { - return local_to_ptr(self->This()); + const v8::PropertyCallbackInfo& self) { + return local_to_ptr(self.This()); } void v8__PropertyCallbackInfo__GetReturnValue( - const v8::PropertyCallbackInfo* self, + const v8::PropertyCallbackInfo& self, v8::ReturnValue* out) { - *out = self->GetReturnValue(); + *out = self.GetReturnValue(); } v8::Platform* v8__platform__NewDefaultPlatform() { diff --git a/src/exception.rs b/src/exception.rs index 7a2ab83d..b55e8a56 100644 --- a/src/exception.rs +++ b/src/exception.rs @@ -51,8 +51,8 @@ impl Message { } #[allow(clippy::mut_from_ref)] - pub fn get_isolate(&self) -> &mut Isolate { - unsafe { v8__Message__GetIsolate(self) } + pub unsafe fn get_isolate(&self) -> &mut Isolate { + v8__Message__GetIsolate(self) } } diff --git a/src/function.rs b/src/function.rs index 70727482..b033822b 100644 --- a/src/function.rs +++ b/src/function.rs @@ -1,10 +1,12 @@ +use std::marker::PhantomData; +use std::mem::MaybeUninit; + use crate::support::{int, Opaque}; use crate::Context; use crate::HandleScope; use crate::Isolate; use crate::Local; use crate::Value; -use std::mem::MaybeUninit; extern "C" { fn v8__Function__New( @@ -37,19 +39,22 @@ extern "C" { out: *mut ReturnValue, ); - fn v8__ReturnValue__Set(rv: *mut ReturnValue, value: *mut Value) -> (); - fn v8__ReturnValue__Get(rv: *mut ReturnValue) -> *mut Value; + fn v8__ReturnValue__Set(rv: &mut ReturnValue, value: *mut Value); + fn v8__ReturnValue__Get(rv: &ReturnValue) -> *mut Value; fn v8__ReturnValue__GetIsolate(rv: &ReturnValue) -> *mut Isolate; } +// Npte: the 'cb lifetime is required because the ReturnValue object must not +// outlive the FunctionCallbackInfo/PropertyCallbackInfo object from which it +// is derived. #[repr(C)] -pub struct ReturnValue([usize; 1]); +pub struct ReturnValue<'cb>(*mut Opaque, PhantomData<&'cb ()>); /// In V8 ReturnValue<> has a type parameter, but /// it turns out that in most of the APIs it's ReturnValue /// and for our purposes we currently don't need /// other types. So for now it's a simplified version. -impl ReturnValue { +impl<'cb> ReturnValue<'cb> { // NOTE: simplest setter, possibly we'll need to add // more setters specialized per type pub fn set(&mut self, mut value: Local) { @@ -68,7 +73,7 @@ impl ReturnValue { &mut self, _scope: &mut HandleScope<'sc>, ) -> Local<'sc, Value> { - unsafe { Local::from_raw(v8__ReturnValue__Get(&mut *self)).unwrap() } + unsafe { Local::from_raw(v8__ReturnValue__Get(self)).unwrap() } } } @@ -81,23 +86,23 @@ pub struct FunctionCallbackInfo(Opaque); impl FunctionCallbackInfo { /// The ReturnValue for the call. - #[allow(clippy::mut_from_ref)] pub fn get_return_value(&self) -> ReturnValue { let mut rv = MaybeUninit::::uninit(); unsafe { - v8__FunctionCallbackInfo__GetReturnValue(&*self, rv.as_mut_ptr()); + v8__FunctionCallbackInfo__GetReturnValue(self, rv.as_mut_ptr()); rv.assume_init() } } /// The current Isolate. - pub fn get_isolate(&self) -> &Isolate { - unsafe { v8__FunctionCallbackInfo__GetIsolate(self) } + #[allow(clippy::mut_from_ref)] + pub unsafe fn get_isolate(&self) -> &mut Isolate { + v8__FunctionCallbackInfo__GetIsolate(self) } /// The number of available arguments. pub fn length(&self) -> int { - unsafe { v8__FunctionCallbackInfo__Length(&*self) } + unsafe { v8__FunctionCallbackInfo__Length(self) } } } diff --git a/src/handle_scope.rs b/src/handle_scope.rs index 70d903a4..947f3689 100644 --- a/src/handle_scope.rs +++ b/src/handle_scope.rs @@ -19,9 +19,10 @@ pub struct HandleScope<'sc>([usize; 3], PhantomData<&'sc mut ()>); impl<'sc> HandleScope<'sc> { pub fn enter( - isolate: &Isolate, + isolate: &mut impl AsMut, mut f: impl FnMut(&mut HandleScope<'_>) -> (), ) { + let isolate = isolate.as_mut(); let mut scope: MaybeUninit = MaybeUninit::uninit(); unsafe { v8__HandleScope__CONSTRUCT(&mut scope, isolate) }; let scope = unsafe { &mut *(scope.as_mut_ptr()) }; diff --git a/src/locker.rs b/src/locker.rs index ebcb42db..b278f6b5 100644 --- a/src/locker.rs +++ b/src/locker.rs @@ -1,6 +1,5 @@ // Copyright 2018-2019 the Deno authors. All rights reserved. MIT license. use crate::isolate::Isolate; -use std::marker::PhantomData; use std::mem::MaybeUninit; extern "C" { @@ -13,9 +12,13 @@ extern "C" { /// construction and destruction, the current thread is allowed to use the locked /// isolate. V8 guarantees that an isolate can be locked by at most one thread at /// any time. In other words, the scope of a v8::Locker is a critical section. -pub struct Locker<'sc>([usize; 2], PhantomData<&'sc mut ()>); +pub struct Locker { + has_lock_: bool, + top_level: bool, + isolate: *mut Isolate, +} -impl<'a> Locker<'a> { +impl Locker { /// Initialize Locker for a given Isolate. pub fn new(isolate: &Isolate) -> Self { let mut buf = MaybeUninit::::uninit(); @@ -26,8 +29,20 @@ impl<'a> Locker<'a> { } } -impl<'a> Drop for Locker<'a> { +impl Drop for Locker { fn drop(&mut self) { unsafe { v8__Locker__DESTRUCT(self) } } } + +impl AsRef for Locker { + fn as_ref(&self) -> &Isolate { + unsafe { &*self.isolate } + } +} + +impl AsMut for Locker { + fn as_mut(&mut self) -> &mut Isolate { + unsafe { &mut *self.isolate } + } +} diff --git a/src/property.rs b/src/property.rs index 6ea8990b..1cd98509 100644 --- a/src/property.rs +++ b/src/property.rs @@ -29,8 +29,9 @@ impl PropertyCallbackInfo { } } - pub fn get_isolate(&self) -> &Isolate { - unsafe { v8__PropertyCallbackInfo__GetIsolate(self) } + #[allow(clippy::mut_from_ref)] + pub unsafe fn get_isolate(&self) -> &mut Isolate { + v8__PropertyCallbackInfo__GetIsolate(self) } pub fn this(&self) -> Local { diff --git a/tests/test_api.rs b/tests/test_api.rs index eb6d4066..be0a7b82 100644 --- a/tests/test_api.rs +++ b/tests/test_api.rs @@ -46,9 +46,9 @@ fn handle_scope_nested() { v8::array_buffer::Allocator::new_default_allocator(), ); let isolate = v8::Isolate::new(params); - let locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |_scope1| { - v8::HandleScope::enter(&isolate, |_scope2| {}); + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |scope1| { + v8::HandleScope::enter(scope1, |_scope2| {}); }); drop(locker); drop(g); @@ -63,11 +63,11 @@ fn handle_scope_numbers() { v8::array_buffer::Allocator::new_default_allocator(), ); let isolate = v8::Isolate::new(params); - let locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |scope| { - let l1 = v8::Integer::new(scope, -123); - let l2 = v8::Integer::new_from_unsigned(scope, 456); - v8::HandleScope::enter(&isolate, |scope2| { + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |scope1| { + let l1 = v8::Integer::new(scope1, -123); + let l2 = v8::Integer::new_from_unsigned(scope1, 456); + v8::HandleScope::enter(scope1, |scope2| { let l3 = v8::Number::new(scope2, 78.9); assert_eq!(l1.value(), -123); assert_eq!(l2.value(), 456); @@ -88,8 +88,8 @@ fn test_string() { v8::array_buffer::Allocator::new_default_allocator(), ); let isolate = v8::Isolate::new(params); - let locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |scope| { + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |scope| { let reference = "Hello 🦕 world!"; let local = v8::String::new(scope, reference).unwrap(); assert_eq!(15, local.length()); @@ -125,8 +125,8 @@ fn try_catch() { v8::array_buffer::Allocator::new_default_allocator(), ); let isolate = v8::Isolate::new(params); - let _locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |scope| { + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |scope| { let mut context = v8::Context::new(scope); context.enter(); { @@ -193,16 +193,16 @@ fn isolate_add_message_listener() { _exception: Local, ) { CALL_COUNT.fetch_add(1, Ordering::SeqCst); - let isolate = message.get_isolate(); - v8::HandleScope::enter(&isolate, |scope| { + let isolate = unsafe { message.get_isolate() }; + v8::HandleScope::enter(isolate, |scope| { let message_str = message.get(scope); assert_eq!(message_str.to_rust_string_lossy(scope), "Uncaught foo"); }); } isolate.add_message_listener(check_message_0); - let locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |s| { + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |s| { let mut context = v8::Context::new(s); context.enter(); let source = v8::String::new(s, "throw 'foo'").unwrap(); @@ -223,9 +223,9 @@ fn script_compile_and_run() { v8::array_buffer::Allocator::new_default_allocator(), ); let isolate = v8::Isolate::new(params); - let locker = v8::Locker::new(&isolate); + let mut locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |s| { + v8::HandleScope::enter(&mut locker, |s| { let mut context = v8::Context::new(s); context.enter(); let source = v8::String::new(s, "'Hello ' + 13 + 'th planet'").unwrap(); @@ -249,9 +249,9 @@ fn script_origin() { v8::array_buffer::Allocator::new_default_allocator(), ); let isolate = v8::Isolate::new(params); - let locker = v8::Locker::new(&isolate); + let mut locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |s| { + v8::HandleScope::enter(&mut locker, |s| { let mut context = v8::Context::new(s); context.enter(); @@ -341,8 +341,8 @@ fn test_primitives() { v8::array_buffer::Allocator::new_default_allocator(), ); let isolate = v8::Isolate::new(params); - let locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |scope| { + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |scope| { let null = v8::new_null(scope); assert!(!null.is_undefined()); assert!(null.is_null()); @@ -374,9 +374,9 @@ fn exception() { v8::array_buffer::Allocator::new_default_allocator(), ); let mut isolate = v8::Isolate::new(params); - let locker = v8::Locker::new(&isolate); + let mut locker = v8::Locker::new(&isolate); isolate.enter(); - v8::HandleScope::enter(&isolate, |scope| { + v8::HandleScope::enter(&mut locker, |scope| { let mut context = v8::Context::new(scope); context.enter(); let reference = "This is a test error"; @@ -408,8 +408,8 @@ fn json() { v8::array_buffer::Allocator::new_default_allocator(), ); let isolate = v8::Isolate::new(params); - let locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |s| { + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |s| { let mut context = v8::Context::new(s); context.enter(); let json_string = v8_str(s, "{\"a\": 1, \"b\": 2}"); @@ -440,8 +440,8 @@ fn object() { v8::array_buffer::Allocator::new_default_allocator(), ); let isolate = v8::Isolate::new(params); - let locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |scope| { + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |scope| { let mut context = v8::Context::new(scope); context.enter(); let null: v8::Local = new_null(scope).into(); @@ -468,8 +468,8 @@ fn promise_resolved() { v8::array_buffer::Allocator::new_default_allocator(), ); let isolate = v8::Isolate::new(params); - let locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |scope| { + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |scope| { let mut context = v8::Context::new(scope); context.enter(); let maybe_resolver = v8::PromiseResolver::new(scope, context); @@ -506,8 +506,8 @@ fn promise_rejected() { v8::array_buffer::Allocator::new_default_allocator(), ); let isolate = v8::Isolate::new(params); - let locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |scope| { + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |scope| { let mut context = v8::Context::new(scope); context.enter(); let maybe_resolver = v8::PromiseResolver::new(scope, context); @@ -539,8 +539,8 @@ fn promise_rejected() { extern "C" fn fn_callback(info: &FunctionCallbackInfo) { assert_eq!(info.length(), 0); - let isolate = info.get_isolate(); - v8::HandleScope::enter(&isolate, |scope| { + let isolate = unsafe { info.get_isolate() }; + v8::HandleScope::enter(isolate, |scope| { let s = v8::String::new(scope, "Hello callback!").unwrap(); let value: Local = s.into(); let rv = &mut info.get_return_value(); @@ -558,8 +558,8 @@ fn function() { v8::array_buffer::Allocator::new_default_allocator(), ); let isolate = v8::Isolate::new(params); - let locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |scope| { + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |scope| { let mut context = v8::Context::new(scope); context.enter(); let global = context.global(); @@ -593,8 +593,8 @@ extern "C" fn promise_reject_callback(msg: v8::PromiseRejectMessage) { let promise_obj: v8::Local = cast(promise); let isolate = promise_obj.get_isolate(); let value = msg.get_value(); - let locker = v8::Locker::new(isolate); - v8::HandleScope::enter(&isolate, |scope| { + let mut locker = v8::Locker::new(isolate); + v8::HandleScope::enter(&mut locker, |scope| { let value_str: v8::Local = cast(value); let rust_str = value_str.to_rust_string_lossy(scope); assert_eq!(rust_str, "promise rejected".to_string()); @@ -612,8 +612,8 @@ fn set_promise_reject_callback() { let mut isolate = v8::Isolate::new(params); isolate.set_promise_reject_callback(promise_reject_callback); isolate.enter(); - let locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |scope| { + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |scope| { let mut context = v8::Context::new(scope); context.enter(); let mut resolver = v8::PromiseResolver::new(scope, context).unwrap(); @@ -661,8 +661,8 @@ fn script_compiler_source() { let mut isolate = v8::Isolate::new(params); isolate.set_promise_reject_callback(promise_reject_callback); isolate.enter(); - let locker = v8::Locker::new(&isolate); - v8::HandleScope::enter(&isolate, |scope| { + let mut locker = v8::Locker::new(&isolate); + v8::HandleScope::enter(&mut locker, |scope| { let mut context = v8::Context::new(scope); context.enter();