Don't use embedder slot in HostImportModuleDynamicallyCallback ABI adapter (#1089)

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<v8::Context> context, v8::Local<v8::Data> host_defined_options,
-    v8::Local<v8::Value> resource_name, v8::Local<v8::String> specifier,
-    v8::Local<v8::FixedArray> import_assertions);
-
-v8::MaybeLocal<v8::Promise> HostImportModuleDynamicallyCallback(
-    v8::Local<v8::Context> context, v8::Local<v8::Data> host_defined_options,
-    v8::Local<v8::Value> resource_name, v8::Local<v8::String> specifier,
-    v8::Local<v8::FixedArray> import_assertions) {
-  auto* isolate = context->GetIsolate();
-  void* d = isolate->GetData(SLOT_INTERNAL(isolate, kSlotDynamicImport));
-  auto* callback = reinterpret_cast<v8__HostImportModuleDynamicallyCallback>(d);
-  assert(callback != nullptr);
-  auto* promise_ptr = callback(context, host_defined_options, resource_name,
-                               specifier, import_assertions);
-  if (promise_ptr == nullptr) {
-    return v8::MaybeLocal<v8::Promise>();
-  } else {
-    return v8::MaybeLocal<v8::Promise>(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) {
+    v8::Isolate* isolate, v8::HostImportModuleDynamicallyCallback callback) {
-  isolate->SetData(SLOT_INTERNAL(isolate, kSlotDynamicImport),
+  isolate->SetHostImportModuleDynamicallyCallback(callback);
-                   reinterpret_cast<void*>(callback));
-  isolate->SetHostImportModuleDynamicallyCallback(
-      HostImportModuleDynamicallyCallback);
 }
 
 void v8__Isolate__SetHostCreateShadowRealmContextCallback(

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<Context>, Local<Module>, Local<Object>);
 
-/// HostImportModuleDynamicallyCallback is called when we require the
+/// HostImportModuleDynamicallyCallback is called when we require the embedder
-/// embedder to load a module. This is used as part of the dynamic
+/// to load a module. This is used as part of the dynamic import syntax.
-/// import syntax.
 ///
-/// The referrer contains metadata about the script/module that calls
+/// The referrer contains metadata about the script/module that calls import.
-/// import.
 ///
 /// The specifier is the name of the module that should be imported.
 ///
-/// The embedder must compile, instantiate, evaluate the Module, and
+/// The import_assertions are import assertions for this request in the form:
-/// obtain it's namespace object.
+/// [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
+/// The embedder must compile, instantiate, evaluate the Module, and obtain its
-/// JavaScript. The embedder must resolve this promise with the module
+/// namespace object.
-/// namespace object. In case of an exception, the embedder must reject
+///
-/// this promise with the exception. If the promise creation itself
+/// The Promise returned from this function is forwarded to userland JavaScript.
-/// fails (e.g. due to stack overflow), the embedder must propagate
+/// The embedder must resolve this promise with the module namespace object. In
-/// that exception by returning an empty MaybeLocal.
+/// case of an exception, the embedder must reject this promise with the
-pub type HostImportModuleDynamicallyCallback = extern "C" fn(
+/// exception. If the promise creation itself fails (e.g. due to stack
-  Local<Context>,
+/// overflow), the embedder must propagate that exception by returning an empty
-  Local<Data>,
+/// MaybeLocal.
-  Local<Value>,
+///
-  Local<String>,
+/// # Example
-  Local<FixedArray>,
+///
-) -> *mut Promise;
+/// ```
+/// 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<v8::Local<'s, v8::Promise>> {
+///   todo!()
+/// }
+/// ```
+pub trait HostImportModuleDynamicallyCallback:
+  UnitType
+  + for<'s> FnOnce(
+    &mut HandleScope<'s>,
+    Local<'s, Data>,
+    Local<'s, Value>,
+    Local<'s, String>,
+    Local<'s, FixedArray>,
+  ) -> Option<Local<'s, Promise>>
+{
+  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<F> 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<Local<'s, Promise>>,
+{
+  #[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<Local<'s, Promise>> {
+      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::<F>(
+        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::<F>(
+            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::<F>
+  }
+}
 
 /// `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(),
+      )
     }
   }
 

tests/test_api.rs

@@ -3475,15 +3475,13 @@ fn import_assertions() {
     Some(module)
   }
 
-  extern "C" fn dynamic_import_cb(
+  fn dynamic_import_cb<'s>(
-    context: v8::Local<v8::Context>,
+    scope: &mut v8::HandleScope<'s>,
-    _host_defined_options: v8::Local<v8::Data>,
+    _host_defined_options: v8::Local<'s, v8::Data>,
-    _resource_name: v8::Local<v8::Value>,
+    _resource_name: v8::Local<'s, v8::Value>,
-    _specifier: v8::Local<v8::String>,
+    _specifier: v8::Local<'s, v8::String>,
-    import_assertions: v8::Local<v8::FixedArray>,
+    import_assertions: v8::Local<'s, v8::FixedArray>,
-  ) -> *mut v8::Promise {
+  ) -> Option<v8::Local<'s, v8::Promise>> {
-    let scope = &mut unsafe { v8::CallbackScope::new(context) };
-    let scope = &mut v8::HandleScope::new(scope);
     // "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::<v8::Value>::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(
+  fn dynamic_import_cb<'s>(
-    context: v8::Local<v8::Context>,
+    scope: &mut v8::HandleScope<'s>,
-    _host_defined_options: v8::Local<v8::Data>,
+    _host_defined_options: v8::Local<'s, v8::Data>,
-    _resource_name: v8::Local<v8::Value>,
+    _resource_name: v8::Local<'s, v8::Value>,
-    specifier: v8::Local<v8::String>,
+    specifier: v8::Local<'s, v8::String>,
-    _import_assertions: v8::Local<v8::FixedArray>,
+    _import_assertions: v8::Local<'s, v8::FixedArray>,
-  ) -> *mut v8::Promise {
+  ) -> Option<v8::Local<'s, v8::Promise>> {
-    let scope = &mut unsafe { v8::CallbackScope::new(context) };
-    let scope = &mut v8::HandleScope::new(scope);
     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);