From 405a874c3662660665d3e8762046552a4e6008fb Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Sun, 31 May 2020 18:45:00 +0200 Subject: [PATCH] Fix remaining `Local::from_raw()` misuse, and correct some lifetimes (#388) --- src/array_buffer_view.rs | 2 +- src/isolate.rs | 14 +++++++++----- src/json.rs | 11 +++++++---- src/module.rs | 9 +++++++-- src/promise.rs | 33 +++++++++++++++++---------------- src/scope_traits.rs | 8 ++++++-- src/script_compiler.rs | 23 ++++++++++++----------- src/script_or_module.rs | 4 ++-- src/shared_array_buffer.rs | 5 +++-- src/try_catch.rs | 11 +++++++++-- src/uint8_array.rs | 11 +++++------ src/value.rs | 16 ++++++++-------- tests/test_api.rs | 6 +++--- 13 files changed, 89 insertions(+), 64 deletions(-) diff --git a/src/array_buffer_view.rs b/src/array_buffer_view.rs index d7508470..3705c568 100644 --- a/src/array_buffer_view.rs +++ b/src/array_buffer_view.rs @@ -24,7 +24,7 @@ impl ArrayBufferView { /// Returns underlying ArrayBuffer. pub fn buffer<'sc>( &self, - scope: &'_ mut impl ToLocal<'sc>, + scope: &mut impl ToLocal<'sc>, ) -> Option> { unsafe { scope.to_local(v8__ArrayBufferView__Buffer(self)) } } diff --git a/src/isolate.rs b/src/isolate.rs index a5e77e03..1d878558 100644 --- a/src/isolate.rs +++ b/src/isolate.rs @@ -320,14 +320,18 @@ impl Isolate { /// exception has been scheduled it is illegal to invoke any JavaScript /// operation; the caller must return immediately and only after the exception /// has been handled does it become legal to invoke JavaScript operations. - pub fn throw_exception<'sc>( + /// + /// This function always returns the `undefined` value. + pub fn throw_exception( &mut self, exception: Local, - ) -> Local<'sc, Value> { - unsafe { - let ptr = v8__Isolate__ThrowException(self, &*exception); - Local::from_raw(ptr).unwrap() + ) -> Local<'_, Value> { + let result = unsafe { + Local::from_raw(v8__Isolate__ThrowException(self, &*exception)) } + .unwrap(); + debug_assert!(result.is_undefined()); + result } /// Runs the default MicrotaskQueue until it gets empty. diff --git a/src/json.rs b/src/json.rs index 85d0b9cd..4e289cde 100644 --- a/src/json.rs +++ b/src/json.rs @@ -3,6 +3,7 @@ use crate::Context; use crate::Local; use crate::String; +use crate::ToLocal; use crate::Value; extern "C" { @@ -19,17 +20,19 @@ extern "C" { /// Tries to parse the string `json_string` and returns it as value if /// successful. pub fn parse<'sc>( - context: Local<'sc, Context>, - json_string: Local<'sc, String>, + scope: &mut impl ToLocal<'sc>, + context: Local<'_, Context>, + json_string: Local<'_, String>, ) -> Option> { - unsafe { Local::from_raw(v8__JSON__Parse(&*context, &*json_string)) } + unsafe { scope.to_local(v8__JSON__Parse(&*context, &*json_string)) } } /// Tries to stringify the JSON-serializable object `json_object` and returns /// it as string if successful. pub fn stringify<'sc>( + scope: &mut impl ToLocal<'sc>, context: Local<'sc, Context>, json_object: Local<'sc, Value>, ) -> Option> { - unsafe { Local::from_raw(v8__JSON__Stringify(&*context, &*json_object)) } + unsafe { scope.to_local(v8__JSON__Stringify(&*context, &*json_object)) } } diff --git a/src/module.rs b/src/module.rs index dd8f0583..52be656f 100644 --- a/src/module.rs +++ b/src/module.rs @@ -32,7 +32,6 @@ use crate::Value; /// Some(resolved_module) /// } /// ``` -/// // System V AMD64 ABI: Local returned in a register. #[cfg(not(target_os = "windows"))] @@ -147,7 +146,9 @@ impl Module { /// For a module in kErrored status, this returns the corresponding exception. pub fn get_exception(&self) -> Local { - unsafe { Local::from_raw(v8__Module__GetException(self)).unwrap() } + // Note: the returned value is not actually stored in a HandleScope, + // therefore we don't need a scope object here. + unsafe { Local::from_raw(v8__Module__GetException(self)) }.unwrap() } /// Returns the number of modules requested by this module. @@ -160,6 +161,8 @@ impl Module { /// Returns the ith module specifier in this module. /// i must be < self.get_module_requests_length() and >= 0. pub fn get_module_request(&self, i: usize) -> Local { + // Note: the returned value is not actually stored in a HandleScope, + // therefore we don't need a scope object here. unsafe { Local::from_raw(v8__Module__GetModuleRequest(self, i.try_into().unwrap())) } @@ -189,6 +192,8 @@ impl Module { /// /// The module's status must be at least kInstantiated. pub fn get_module_namespace(&self) -> Local { + // Note: the returned value is not actually stored in a HandleScope, + // therefore we don't need a scope object here. unsafe { Local::from_raw(v8__Module__GetModuleNamespace(self)).unwrap() } } diff --git a/src/promise.rs b/src/promise.rs index ce6addad..2c609ed7 100644 --- a/src/promise.rs +++ b/src/promise.rs @@ -91,10 +91,11 @@ impl Promise { /// See `Self::then2`. pub fn catch<'sc>( &self, - context: Local<'sc, Context>, - handler: Local<'sc, Function>, + scope: &mut impl ToLocal<'sc>, + context: Local, + handler: Local, ) -> Option> { - unsafe { Local::from_raw(v8__Promise__Catch(&*self, &*context, &*handler)) } + unsafe { scope.to_local(v8__Promise__Catch(&*self, &*context, &*handler)) } } /// Register a resolution handler with a promise. @@ -102,10 +103,11 @@ impl Promise { /// See `Self::then2`. pub fn then<'sc>( &self, - context: Local<'sc, Context>, - handler: Local<'sc, Function>, + scope: &mut impl ToLocal<'sc>, + context: Local, + handler: Local, ) -> Option> { - unsafe { Local::from_raw(v8__Promise__Then(&*self, &*context, &*handler)) } + unsafe { scope.to_local(v8__Promise__Then(&*self, &*context, &*handler)) } } /// Register a resolution/rejection handler with a promise. @@ -114,12 +116,13 @@ impl Promise { /// invoked at the end of turn. pub fn then2<'sc>( &self, - context: Local<'sc, Context>, - on_fulfilled: Local<'sc, Function>, - on_rejected: Local<'sc, Function>, + scope: &mut impl ToLocal<'sc>, + context: Local, + on_fulfilled: Local, + on_rejected: Local, ) -> Option> { unsafe { - Local::from_raw(v8__Promise__Then2( + scope.to_local(v8__Promise__Then2( &*self, &*context, &*on_fulfilled, @@ -182,9 +185,8 @@ pub struct PromiseRejectMessage<'msg>([usize; 3], PhantomData<&'msg ()>); impl<'msg> PromiseRejectMessage<'msg> { pub fn get_promise(&self) -> Local<'msg, Promise> { - unsafe { - Local::from_raw(v8__PromiseRejectMessage__GetPromise(self)).unwrap() - } + unsafe { Local::from_raw(v8__PromiseRejectMessage__GetPromise(self)) } + .unwrap() } pub fn get_event(&self) -> PromiseRejectEvent { @@ -192,8 +194,7 @@ impl<'msg> PromiseRejectMessage<'msg> { } pub fn get_value(&self) -> Local<'msg, Value> { - unsafe { - Local::from_raw(v8__PromiseRejectMessage__GetValue(self)).unwrap() - } + unsafe { Local::from_raw(v8__PromiseRejectMessage__GetValue(self)) } + .unwrap() } } diff --git a/src/scope_traits.rs b/src/scope_traits.rs index fecb8bff..12158a13 100644 --- a/src/scope_traits.rs +++ b/src/scope_traits.rs @@ -156,12 +156,16 @@ pub trait ToLocal<'s>: InIsolate { } fn get_current_context(&mut self) -> Option> { - unsafe { Local::from_raw(v8__Isolate__GetCurrentContext(self.isolate())) } + unsafe { + let ptr = v8__Isolate__GetCurrentContext(self.isolate()); + self.to_local(ptr) + } } fn get_entered_or_microtask_context(&mut self) -> Option> { unsafe { - Local::from_raw(v8__Isolate__GetEnteredOrMicrotaskContext(self.isolate())) + let ptr = v8__Isolate__GetEnteredOrMicrotaskContext(self.isolate()); + self.to_local(ptr) } } } diff --git a/src/script_compiler.rs b/src/script_compiler.rs index e331a258..0f29db9f 100644 --- a/src/script_compiler.rs +++ b/src/script_compiler.rs @@ -1,12 +1,12 @@ // Copyright 2019-2020 the Deno authors. All rights reserved. MIT license. -//! For compiling scripts. -use crate::InIsolate; +use std::mem::MaybeUninit; + use crate::Isolate; use crate::Local; use crate::Module; use crate::ScriptOrigin; use crate::String; -use std::mem::MaybeUninit; +use crate::ToLocal; extern "C" { fn v8__ScriptCompiler__Source__CONSTRUCT( @@ -77,10 +77,10 @@ pub enum NoCacheReason { /// /// Corresponds to the ParseModule abstract operation in the ECMAScript /// specification. -pub fn compile_module<'a>( - scope: &mut impl InIsolate, +pub fn compile_module<'sc>( + scope: &mut impl ToLocal<'sc>, source: Source, -) -> Option> { +) -> Option> { compile_module2( scope, source, @@ -90,18 +90,19 @@ pub fn compile_module<'a>( } /// Same as compile_module with more options. -pub fn compile_module2<'a>( - scope: &mut impl InIsolate, +pub fn compile_module2<'sc>( + scope: &mut impl ToLocal<'sc>, mut source: Source, options: CompileOptions, no_cache_reason: NoCacheReason, -) -> Option> { +) -> Option> { unsafe { - Local::from_raw(v8__ScriptCompiler__CompileModule( + let ptr = v8__ScriptCompiler__CompileModule( scope.isolate(), &mut source, options, no_cache_reason, - )) + ); + scope.to_local(ptr) } } diff --git a/src/script_or_module.rs b/src/script_or_module.rs index 5a266ddb..13f41972 100644 --- a/src/script_or_module.rs +++ b/src/script_or_module.rs @@ -17,7 +17,7 @@ extern "C" { impl ScriptOrModule { /// The name that was passed by the embedder as ResourceName to the /// ScriptOrigin. This can be either a v8::String or v8::Undefined. - pub fn get_resource_name(&self) -> Local<'_, Value> { + pub fn get_resource_name(&self) -> Local { unsafe { let ptr = v8__ScriptOrModule__GetResourceName(self); Local::from_raw(ptr).unwrap() @@ -26,7 +26,7 @@ impl ScriptOrModule { /// The options that were passed by the embedder as HostDefinedOptions to the /// ScriptOrigin. - pub fn get_host_defined_options(&self) -> Local<'_, PrimitiveArray> { + pub fn get_host_defined_options(&self) -> Local { unsafe { let ptr = v8__ScriptOrModule__GetHostDefinedOptions(self); Local::from_raw(ptr).unwrap() diff --git a/src/shared_array_buffer.rs b/src/shared_array_buffer.rs index 5d9651b2..88cb4a35 100644 --- a/src/shared_array_buffer.rs +++ b/src/shared_array_buffer.rs @@ -50,10 +50,11 @@ impl SharedArrayBuffer { byte_length: usize, ) -> Option> { unsafe { - Local::from_raw(v8__SharedArrayBuffer__New__with_byte_length( + let ptr = v8__SharedArrayBuffer__New__with_byte_length( scope.isolate(), byte_length, - )) + ); + scope.to_local(ptr) } } diff --git a/src/try_catch.rs b/src/try_catch.rs index e132a839..257432d0 100644 --- a/src/try_catch.rs +++ b/src/try_catch.rs @@ -161,8 +161,15 @@ impl<'tc> TryCatch<'tc> { /// it is illegal to execute any JavaScript operations after calling /// ReThrow; the caller must return immediately to where the exception /// is caught. - pub fn rethrow<'a>(&'_ mut self) -> Option> { - unsafe { Local::from_raw(v8__TryCatch__ReThrow(&mut self.0)) } + /// + /// This function returns the `undefined` value when successful, or `None` if + /// no exception was caught and therefore there was nothing to rethrow. + pub fn rethrow(&mut self) -> Option> { + let result = unsafe { Local::from_raw(v8__TryCatch__ReThrow(&mut self.0)) }; + if let Some(value) = result { + debug_assert!(value.is_undefined()) + } + result } /// Returns true if verbosity is enabled. diff --git a/src/uint8_array.rs b/src/uint8_array.rs index f97fe7d8..1f52183f 100644 --- a/src/uint8_array.rs +++ b/src/uint8_array.rs @@ -1,7 +1,7 @@ -use std::ops::DerefMut; - +// Copyright 2019-2020 the Deno authors. All rights reserved. MIT license. use crate::ArrayBuffer; use crate::Local; +use crate::ToLocal; use crate::Uint8Array; extern "C" { @@ -14,12 +14,11 @@ extern "C" { impl Uint8Array { pub fn new<'sc>( - mut buf: Local, + scope: &mut impl ToLocal<'sc>, + buf: Local, byte_offset: usize, length: usize, ) -> Option> { - unsafe { - Local::from_raw(v8__Uint8Array__New(buf.deref_mut(), byte_offset, length)) - } + unsafe { scope.to_local(v8__Uint8Array__New(&*buf, byte_offset, length)) } } } diff --git a/src/value.rs b/src/value.rs index e4fb6cd4..9ec5058d 100644 --- a/src/value.rs +++ b/src/value.rs @@ -424,7 +424,7 @@ impl Value { scope: &mut impl ToLocal<'sc>, ) -> Option> { scope.get_current_context().and_then(|context| unsafe { - Local::from_raw(v8__Value__ToBigInt(self, &*context)) + scope.to_local(v8__Value__ToBigInt(self, &*context)) }) } @@ -433,7 +433,7 @@ impl Value { scope: &mut impl ToLocal<'sc>, ) -> Option> { scope.get_current_context().and_then(|context| unsafe { - Local::from_raw(v8__Value__ToNumber(self, &*context)) + scope.to_local(v8__Value__ToNumber(self, &*context)) }) } @@ -442,7 +442,7 @@ impl Value { scope: &mut impl ToLocal<'sc>, ) -> Option> { scope.get_current_context().and_then(|context| unsafe { - Local::from_raw(v8__Value__ToString(self, &*context)) + scope.to_local(v8__Value__ToString(self, &*context)) }) } @@ -451,7 +451,7 @@ impl Value { scope: &mut impl ToLocal<'sc>, ) -> Option> { scope.get_current_context().and_then(|context| unsafe { - Local::from_raw(v8__Value__ToDetailString(self, &*context)) + scope.to_local(v8__Value__ToDetailString(self, &*context)) }) } @@ -460,7 +460,7 @@ impl Value { scope: &mut impl ToLocal<'sc>, ) -> Option> { scope.get_current_context().and_then(|context| unsafe { - Local::from_raw(v8__Value__ToObject(self, &*context)) + scope.to_local(v8__Value__ToObject(self, &*context)) }) } @@ -469,7 +469,7 @@ impl Value { scope: &mut impl ToLocal<'sc>, ) -> Option> { scope.get_current_context().and_then(|context| unsafe { - Local::from_raw(v8__Value__ToInteger(self, &*context)) + scope.to_local(v8__Value__ToInteger(self, &*context)) }) } @@ -478,7 +478,7 @@ impl Value { scope: &mut impl ToLocal<'sc>, ) -> Option> { scope.get_current_context().and_then(|context| unsafe { - Local::from_raw(v8__Value__ToUint32(self, &*context)) + scope.to_local(v8__Value__ToUint32(self, &*context)) }) } @@ -487,7 +487,7 @@ impl Value { scope: &mut impl ToLocal<'sc>, ) -> Option> { scope.get_current_context().and_then(|context| unsafe { - Local::from_raw(v8__Value__ToInt32(self, &*context)) + scope.to_local(v8__Value__ToInt32(self, &*context)) }) } diff --git a/tests/test_api.rs b/tests/test_api.rs index 9f6af598..0334698a 100644 --- a/tests/test_api.rs +++ b/tests/test_api.rs @@ -1070,10 +1070,10 @@ fn json() { let mut cs = v8::ContextScope::new(scope, context); let scope = cs.enter(); let json_string = v8_str(scope, "{\"a\": 1, \"b\": 2}"); - let maybe_value = v8::json::parse(context, json_string); + let maybe_value = v8::json::parse(scope, context, json_string); assert!(maybe_value.is_some()); let value = maybe_value.unwrap(); - let maybe_stringified = v8::json::stringify(context, value); + let maybe_stringified = v8::json::stringify(scope, context, value); assert!(maybe_stringified.is_some()); let stringified = maybe_stringified.unwrap(); let rust_str = stringified.to_rust_string_lossy(scope); @@ -1973,7 +1973,7 @@ fn uint8_array() { let maybe_ab = result.buffer(scope); assert!(maybe_ab.is_some()); let ab = maybe_ab.unwrap(); - let uint8_array = v8::Uint8Array::new(ab, 0, 0); + let uint8_array = v8::Uint8Array::new(scope, ab, 0, 0); assert!(uint8_array.is_some()); } }