From fc26f634e40c16bc54142460ea448c5ed5066ee0 Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Sat, 8 Oct 2022 02:30:01 +0200 Subject: [PATCH] Don't use embedder slot in HostImportModuleDynamicallyCallback ABI adapter (#1089) --- src/binding.cc | 42 +---------- src/isolate.rs | 177 +++++++++++++++++++++++++++++++++++++++------- tests/test_api.rs | 36 +++++----- 3 files changed, 172 insertions(+), 83 deletions(-) diff --git a/src/binding.cc b/src/binding.cc index f0d34e78..528ea830 100644 --- a/src/binding.cc +++ b/src/binding.cc @@ -98,39 +98,6 @@ static_assert(offsetof(v8::ScriptCompiler::CachedData, buffer_policy) == 12, "CachedData.buffer_policy offset mismatch"); #endif -enum InternalSlots { - kSlotDynamicImport = 0, - kNumInternalSlots, -}; -#define SLOT_NUM_EXTERNAL(isolate) \ - (isolate->GetNumberOfDataSlots() - kNumInternalSlots) -#define SLOT_INTERNAL(isolate, slot) \ - (isolate->GetNumberOfDataSlots() - 1 - slot) - -// This is an extern C calling convention compatible version of -// v8::HostImportModuleDynamicallyCallback -typedef v8::Promise* (*v8__HostImportModuleDynamicallyCallback)( - v8::Local context, v8::Local host_defined_options, - v8::Local resource_name, v8::Local specifier, - v8::Local import_assertions); - -v8::MaybeLocal HostImportModuleDynamicallyCallback( - v8::Local context, v8::Local host_defined_options, - v8::Local resource_name, v8::Local specifier, - v8::Local import_assertions) { - auto* isolate = context->GetIsolate(); - void* d = isolate->GetData(SLOT_INTERNAL(isolate, kSlotDynamicImport)); - auto* callback = reinterpret_cast(d); - assert(callback != nullptr); - auto* promise_ptr = callback(context, host_defined_options, resource_name, - specifier, import_assertions); - if (promise_ptr == nullptr) { - return v8::MaybeLocal(); - } else { - return v8::MaybeLocal(ptr_to_local(promise_ptr)); - } -} - extern "C" { void v8__V8__SetFlagsFromCommandLine(int* argc, char** argv, const char* usage) { @@ -201,7 +168,7 @@ void* v8__Isolate__GetData(v8::Isolate* isolate, uint32_t slot) { } uint32_t v8__Isolate__GetNumberOfDataSlots(v8::Isolate* isolate) { - return SLOT_NUM_EXTERNAL(isolate); + return isolate->GetNumberOfDataSlots(); } const v8::Data* v8__Isolate__GetDataFromSnapshotOnce(v8::Isolate* isolate, @@ -269,11 +236,8 @@ void v8__Isolate__SetHostInitializeImportMetaObjectCallback( } void v8__Isolate__SetHostImportModuleDynamicallyCallback( - v8::Isolate* isolate, v8__HostImportModuleDynamicallyCallback callback) { - isolate->SetData(SLOT_INTERNAL(isolate, kSlotDynamicImport), - reinterpret_cast(callback)); - isolate->SetHostImportModuleDynamicallyCallback( - HostImportModuleDynamicallyCallback); + v8::Isolate* isolate, v8::HostImportModuleDynamicallyCallback callback) { + isolate->SetHostImportModuleDynamicallyCallback(callback); } void v8__Isolate__SetHostCreateShadowRealmContextCallback( diff --git a/src/isolate.rs b/src/isolate.rs index 0126c6c8..59aa07d5 100644 --- a/src/isolate.rs +++ b/src/isolate.rs @@ -1,4 +1,3 @@ -use crate::PromiseResolver; // Copyright 2019-2021 the Deno authors. All rights reserved. MIT license. use crate::function::FunctionCallbackInfo; use crate::handle::FinalizerCallback; @@ -26,6 +25,7 @@ use crate::Message; use crate::Module; use crate::Object; use crate::Promise; +use crate::PromiseResolver; use crate::String; use crate::Value; @@ -119,31 +119,157 @@ pub type WasmAsyncResolvePromiseCallback = extern "C" fn( pub type HostInitializeImportMetaObjectCallback = extern "C" fn(Local, Local, Local); -/// HostImportModuleDynamicallyCallback is called when we require the -/// embedder to load a module. This is used as part of the dynamic -/// import syntax. +/// HostImportModuleDynamicallyCallback is called when we require the embedder +/// to load a module. This is used as part of the dynamic import syntax. /// -/// The referrer contains metadata about the script/module that calls -/// import. +/// The referrer contains metadata about the script/module that calls import. /// /// The specifier is the name of the module that should be imported. /// -/// The embedder must compile, instantiate, evaluate the Module, and -/// obtain it's namespace object. +/// The import_assertions are import assertions for this request in the form: +/// [key1, value1, key2, value2, ...] where the keys and values are of type +/// v8::String. Note, unlike the FixedArray passed to ResolveModuleCallback and +/// returned from ModuleRequest::GetImportAssertions(), this array does not +/// contain the source Locations of the assertions. /// -/// The Promise returned from this function is forwarded to userland -/// JavaScript. The embedder must resolve this promise with the module -/// namespace object. In case of an exception, the embedder must reject -/// this promise with the exception. If the promise creation itself -/// fails (e.g. due to stack overflow), the embedder must propagate -/// that exception by returning an empty MaybeLocal. -pub type HostImportModuleDynamicallyCallback = extern "C" fn( - Local, - Local, - Local, - Local, - Local, -) -> *mut Promise; +/// The embedder must compile, instantiate, evaluate the Module, and obtain its +/// namespace object. +/// +/// The Promise returned from this function is forwarded to userland JavaScript. +/// The embedder must resolve this promise with the module namespace object. In +/// case of an exception, the embedder must reject this promise with the +/// exception. If the promise creation itself fails (e.g. due to stack +/// overflow), the embedder must propagate that exception by returning an empty +/// MaybeLocal. +/// +/// # Example +/// +/// ``` +/// fn host_import_module_dynamically_callback_example<'s>( +/// scope: &mut v8::HandleScope<'s>, +/// host_defined_options: v8::Local<'s, v8::Data>, +/// resource_name: v8::Local<'s, v8::Value>, +/// specifier: v8::Local<'s, v8::String>, +/// import_assertions: v8::Local<'s, v8::FixedArray>, +/// ) -> Option> { +/// todo!() +/// } +/// ``` +pub trait HostImportModuleDynamicallyCallback: + UnitType + + for<'s> FnOnce( + &mut HandleScope<'s>, + Local<'s, Data>, + Local<'s, Value>, + Local<'s, String>, + Local<'s, FixedArray>, + ) -> Option> +{ + fn to_c_fn(self) -> RawHostImportModuleDynamicallyCallback; +} + +#[cfg(target_family = "unix")] +pub(crate) type RawHostImportModuleDynamicallyCallback = + for<'s> extern "C" fn( + Local<'s, Context>, + Local<'s, Data>, + Local<'s, Value>, + Local<'s, String>, + Local<'s, FixedArray>, + ) -> *mut Promise; + +#[cfg(all(target_family = "windows", target_arch = "x86_64"))] +pub type RawHostImportModuleDynamicallyCallback = + for<'s> extern "C" fn( + *mut *mut Promise, + Local<'s, Context>, + Local<'s, Data>, + Local<'s, Value>, + Local<'s, String>, + Local<'s, FixedArray>, + ) -> *mut *mut Promise; + +impl HostImportModuleDynamicallyCallback for F +where + F: UnitType + + for<'s> FnOnce( + &mut HandleScope<'s>, + Local<'s, Data>, + Local<'s, Value>, + Local<'s, String>, + Local<'s, FixedArray>, + ) -> Option>, +{ + #[inline(always)] + fn to_c_fn(self) -> RawHostImportModuleDynamicallyCallback { + #[inline(always)] + fn scope_adapter<'s, F: HostImportModuleDynamicallyCallback>( + context: Local<'s, Context>, + host_defined_options: Local<'s, Data>, + resource_name: Local<'s, Value>, + specifier: Local<'s, String>, + import_assertions: Local<'s, FixedArray>, + ) -> Option> { + let scope = &mut unsafe { CallbackScope::new(context) }; + (F::get())( + scope, + host_defined_options, + resource_name, + specifier, + import_assertions, + ) + } + + #[cfg(target_family = "unix")] + #[inline(always)] + extern "C" fn abi_adapter<'s, F: HostImportModuleDynamicallyCallback>( + context: Local<'s, Context>, + host_defined_options: Local<'s, Data>, + resource_name: Local<'s, Value>, + specifier: Local<'s, String>, + import_assertions: Local<'s, FixedArray>, + ) -> *mut Promise { + scope_adapter::( + context, + host_defined_options, + resource_name, + specifier, + import_assertions, + ) + .map(|return_value| return_value.as_non_null().as_ptr()) + .unwrap_or_else(null_mut) + } + + #[cfg(all(target_family = "windows", target_arch = "x86_64"))] + #[inline(always)] + extern "C" fn abi_adapter<'s, F: HostImportModuleDynamicallyCallback>( + return_value: *mut *mut Promise, + context: Local<'s, Context>, + host_defined_options: Local<'s, Data>, + resource_name: Local<'s, Value>, + specifier: Local<'s, String>, + import_assertions: Local<'s, FixedArray>, + ) -> *mut *mut Promise { + unsafe { + std::ptr::write( + return_value, + scope_adapter::( + context, + host_defined_options, + resource_name, + specifier, + import_assertions, + ) + .map(|return_value| return_value.as_non_null().as_ptr()) + .unwrap_or_else(null_mut), + ); + return_value + } + } + + abi_adapter:: + } +} /// `HostCreateShadowRealmContextCallback` is called each time a `ShadowRealm` /// is being constructed. You can use [`HandleScope::get_current_context`] to @@ -265,7 +391,7 @@ extern "C" { ); fn v8__Isolate__SetHostImportModuleDynamicallyCallback( isolate: *mut Isolate, - callback: HostImportModuleDynamicallyCallback, + callback: RawHostImportModuleDynamicallyCallback, ); #[cfg(not(target_os = "windows"))] fn v8__Isolate__SetHostCreateShadowRealmContextCallback( @@ -698,10 +824,13 @@ impl Isolate { #[inline(always)] pub fn set_host_import_module_dynamically_callback( &mut self, - callback: HostImportModuleDynamicallyCallback, + callback: impl HostImportModuleDynamicallyCallback, ) { unsafe { - v8__Isolate__SetHostImportModuleDynamicallyCallback(self, callback) + v8__Isolate__SetHostImportModuleDynamicallyCallback( + self, + callback.to_c_fn(), + ) } } diff --git a/tests/test_api.rs b/tests/test_api.rs index ab66f39a..9310ca94 100644 --- a/tests/test_api.rs +++ b/tests/test_api.rs @@ -3475,15 +3475,13 @@ fn import_assertions() { Some(module) } - extern "C" fn dynamic_import_cb( - context: v8::Local, - _host_defined_options: v8::Local, - _resource_name: v8::Local, - _specifier: v8::Local, - import_assertions: v8::Local, - ) -> *mut v8::Promise { - let scope = &mut unsafe { v8::CallbackScope::new(context) }; - let scope = &mut v8::HandleScope::new(scope); + fn dynamic_import_cb<'s>( + scope: &mut v8::HandleScope<'s>, + _host_defined_options: v8::Local<'s, v8::Data>, + _resource_name: v8::Local<'s, v8::Value>, + _specifier: v8::Local<'s, v8::String>, + import_assertions: v8::Local<'s, v8::FixedArray>, + ) -> Option> { // "type" keyword, value assert_eq!(import_assertions.length(), 2); let assert1 = import_assertions.get(scope, 0).unwrap(); @@ -3492,7 +3490,7 @@ fn import_assertions() { let assert2 = import_assertions.get(scope, 1).unwrap(); let assert2_val = v8::Local::::try_from(assert2).unwrap(); assert_eq!(assert2_val.to_rust_string_lossy(scope), "json"); - std::ptr::null_mut() + None } isolate.set_host_import_module_dynamically_callback(dynamic_import_cb); @@ -4059,22 +4057,20 @@ fn dynamic_import() { static CALL_COUNT: AtomicUsize = AtomicUsize::new(0); - extern "C" fn dynamic_import_cb( - context: v8::Local, - _host_defined_options: v8::Local, - _resource_name: v8::Local, - specifier: v8::Local, - _import_assertions: v8::Local, - ) -> *mut v8::Promise { - let scope = &mut unsafe { v8::CallbackScope::new(context) }; - let scope = &mut v8::HandleScope::new(scope); + fn dynamic_import_cb<'s>( + scope: &mut v8::HandleScope<'s>, + _host_defined_options: v8::Local<'s, v8::Data>, + _resource_name: v8::Local<'s, v8::Value>, + specifier: v8::Local<'s, v8::String>, + _import_assertions: v8::Local<'s, v8::FixedArray>, + ) -> Option> { assert!( specifier.strict_equals(v8::String::new(scope, "bar.js").unwrap().into()) ); let e = v8::String::new(scope, "boom").unwrap(); scope.throw_exception(e.into()); CALL_COUNT.fetch_add(1, Ordering::SeqCst); - std::ptr::null_mut() + None } isolate.set_host_import_module_dynamically_callback(dynamic_import_cb);