fix: clean up some node-api details (#24178)

- fix a few napi_* types
- clean up env hooks
- implement blocking queue in tsfn
- reduce transmutes
- use new `DataView::new` api from rusty_v8

3 files changed, 77 insertions, 67 deletions

diff --git a/ext/napi/function.rs b/ext/napi/function.rs
index 2d2933b95..bdfa7d7e1 100644
--- a/ext/napi/function.rs
+++ b/ext/napi/function.rs
@@ -4,7 +4,7 @@ use crate::*;
 #[repr(C)]
 #[derive(Debug)]
 pub struct CallbackInfo {
-  pub env: napi_env,
+  pub env: *mut Env,
   pub cb: napi_callback,
   pub cb_info: napi_callback_info,
   pub args: *const c_void,
@@ -13,7 +13,7 @@ pub struct CallbackInfo {
 impl CallbackInfo {
   #[inline]
   pub fn new_raw(
-    env: napi_env,
+    env: *mut Env,
     cb: napi_callback,
     cb_info: napi_callback_info,
   ) -> *mut Self {
@@ -41,38 +41,27 @@ extern "C" fn call_fn(info: *const v8::FunctionCallbackInfo) {
   let info = unsafe { &mut *info_ptr };
   info.args = &args as *const _ as *const c_void;
 
-  if let Some(f) = info.cb {
-    // SAFETY: calling user provided function pointer.
-    let value = unsafe { f(info.env, info_ptr as *mut _) };
-    if let Some(exc) = unsafe { &mut *(info.env as *mut Env) }
-      .last_exception
-      .take()
-    {
-      let scope = unsafe { &mut v8::CallbackScope::new(callback_info) };
-      let exc = v8::Local::new(scope, exc);
-      scope.throw_exception(exc);
-    }
-    if let Some(value) = *value {
-      rv.set(value);
-    }
+  // SAFETY: calling user provided function pointer.
+  let value = unsafe { (info.cb)(info.env as napi_env, info_ptr as *mut _) };
+  if let Some(exc) = unsafe { &mut *info.env }.last_exception.take() {
+    let scope = unsafe { &mut v8::CallbackScope::new(callback_info) };
+    let exc = v8::Local::new(scope, exc);
+    scope.throw_exception(exc);
+  }
+  if let Some(value) = *value {
+    rv.set(value);
   }
 }
 
-/// # Safety
-/// env_ptr must be valid
-pub unsafe fn create_function<'a>(
-  env_ptr: *mut Env,
+pub fn create_function<'s>(
+  scope: &mut v8::HandleScope<'s>,
+  env: *mut Env,
   name: Option<v8::Local<v8::String>>,
   cb: napi_callback,
   cb_info: napi_callback_info,
-) -> v8::Local<'a, v8::Function> {
-  let env = unsafe { &mut *env_ptr };
-  let scope = &mut env.scope();
-
-  let external = v8::External::new(
-    scope,
-    CallbackInfo::new_raw(env_ptr as _, cb, cb_info) as *mut _,
-  );
+) -> v8::Local<'s, v8::Function> {
+  let external =
+    v8::External::new(scope, CallbackInfo::new_raw(env, cb, cb_info) as *mut _);
   let function = v8::Function::builder_raw(call_fn)
     .data(external.into())
     .build(scope)
@@ -85,21 +74,15 @@ pub unsafe fn create_function<'a>(
   function
 }
 
-/// # Safety
-/// env_ptr must be valid
-pub unsafe fn create_function_template<'a>(
-  env_ptr: *mut Env,
+pub fn create_function_template<'s>(
+  scope: &mut v8::HandleScope<'s>,
+  env: *mut Env,
   name: Option<v8::Local<v8::String>>,
   cb: napi_callback,
   cb_info: napi_callback_info,
-) -> v8::Local<'a, v8::FunctionTemplate> {
-  let env = unsafe { &mut *env_ptr };
-  let scope = &mut env.scope();
-
-  let external = v8::External::new(
-    scope,
-    CallbackInfo::new_raw(env_ptr as _, cb, cb_info) as *mut _,
-  );
+) -> v8::Local<'s, v8::FunctionTemplate> {
+  let external =
+    v8::External::new(scope, CallbackInfo::new_raw(env, cb, cb_info) as *mut _);
   let function = v8::FunctionTemplate::builder_raw(call_fn)
     .data(external.into())
     .build(scope);
diff --git a/ext/napi/lib.rs b/ext/napi/lib.rs
index 39b303f86..402785ce8 100644
--- a/ext/napi/lib.rs
+++ b/ext/napi/lib.rs
@@ -187,12 +187,10 @@ pub struct napi_type_tag {
   pub upper: u64,
 }
 
-pub type napi_callback = Option<
-  unsafe extern "C" fn(
-    env: napi_env,
-    info: napi_callback_info,
-  ) -> napi_value<'static>,
->;
+pub type napi_callback = unsafe extern "C" fn(
+  env: napi_env,
+  info: napi_callback_info,
+) -> napi_value<'static>;
 
 pub type napi_finalize = unsafe extern "C" fn(
   env: napi_env,
@@ -213,8 +211,10 @@ pub type napi_threadsafe_function_call_js = unsafe extern "C" fn(
   data: *mut c_void,
 );
 
-pub type napi_async_cleanup_hook =
-  unsafe extern "C" fn(env: napi_env, data: *mut c_void);
+pub type napi_async_cleanup_hook = unsafe extern "C" fn(
+  handle: napi_async_cleanup_hook_handle,
+  data: *mut c_void,
+);
 
 pub type napi_cleanup_hook = unsafe extern "C" fn(data: *mut c_void);
 
@@ -235,9 +235,9 @@ pub const napi_default_jsproperty: napi_property_attributes =
 pub struct napi_property_descriptor<'a> {
   pub utf8name: *const c_char,
   pub name: napi_value<'a>,
-  pub method: napi_callback,
-  pub getter: napi_callback,
-  pub setter: napi_callback,
+  pub method: Option<napi_callback>,
+  pub getter: Option<napi_callback>,
+  pub setter: Option<napi_callback>,
   pub value: napi_value<'a>,
   pub attributes: napi_property_attributes,
   pub data: *mut c_void,
@@ -348,13 +348,13 @@ pub struct Env {
   pub open_handle_scopes: usize,
   pub shared: *mut EnvShared,
   pub async_work_sender: V8CrossThreadTaskSpawner,
-  pub cleanup_hooks: Rc<RefCell<Vec<(napi_cleanup_hook, *mut c_void)>>>,
-  pub external_ops_tracker: ExternalOpsTracker,
+  cleanup_hooks: Rc<RefCell<Vec<(napi_cleanup_hook, *mut c_void)>>>,
+  external_ops_tracker: ExternalOpsTracker,
   pub last_error: napi_extended_error_info,
   pub last_exception: Option<v8::Global<v8::Value>>,
-  pub global: NonNull<v8::Value>,
-  pub buffer_constructor: NonNull<v8::Function>,
-  pub report_error: NonNull<v8::Function>,
+  pub global: v8::Global<v8::Object>,
+  pub buffer_constructor: v8::Global<v8::Function>,
+  pub report_error: v8::Global<v8::Function>,
 }
 
 unsafe impl Send for Env {}
@@ -365,7 +365,7 @@ impl Env {
   pub fn new(
     isolate_ptr: *mut v8::OwnedIsolate,
     context: v8::Global<v8::Context>,
-    global: v8::Global<v8::Value>,
+    global: v8::Global<v8::Object>,
     buffer_constructor: v8::Global<v8::Function>,
     report_error: v8::Global<v8::Function>,
     sender: V8CrossThreadTaskSpawner,
@@ -375,9 +375,9 @@ impl Env {
     Self {
       isolate_ptr,
       context: context.into_raw(),
-      global: global.into_raw(),
-      buffer_constructor: buffer_constructor.into_raw(),
-      report_error: report_error.into_raw(),
+      global,
+      buffer_constructor,
+      report_error,
       shared: std::ptr::null_mut(),
       open_handle_scopes: 0,
       async_work_sender: sender,
@@ -435,6 +435,35 @@ impl Env {
   pub fn threadsafe_function_unref(&mut self) {
     self.external_ops_tracker.unref_op();
   }
+
+  pub fn add_cleanup_hook(
+    &mut self,
+    hook: napi_cleanup_hook,
+    data: *mut c_void,
+  ) {
+    let mut hooks = self.cleanup_hooks.borrow_mut();
+    if hooks.iter().any(|pair| pair.0 == hook && pair.1 == data) {
+      panic!("Cannot register cleanup hook with same data twice");
+    }
+    hooks.push((hook, data));
+  }
+
+  pub fn remove_cleanup_hook(
+    &mut self,
+    hook: napi_cleanup_hook,
+    data: *mut c_void,
+  ) {
+    let mut hooks = self.cleanup_hooks.borrow_mut();
+    match hooks
+      .iter()
+      .rposition(|&pair| pair.0 == hook && pair.1 == data)
+    {
+      Some(index) => {
+        hooks.remove(index);
+      }
+      None => panic!("Cannot remove cleanup hook which was not registered"),
+    }
+  }
 }
 
 deno_core::extension!(deno_napi,
@@ -468,7 +497,7 @@ fn op_napi_open<NP, 'scope>(
   scope: &mut v8::HandleScope<'scope>,
   op_state: Rc<RefCell<OpState>>,
   #[string] path: String,
-  global: v8::Local<'scope, v8::Value>,
+  global: v8::Local<'scope, v8::Object>,
   buffer_constructor: v8::Local<'scope, v8::Function>,
   report_error: v8::Local<'scope, v8::Function>,
 ) -> std::result::Result<v8::Local<'scope, v8::Value>, AnyError>
@@ -499,9 +528,6 @@ where
   let type_tag = v8::Private::new(scope, Some(type_tag_name));
   let type_tag = v8::Global::new(scope, type_tag);
 
-  // The `module.exports` object.
-  let exports = v8::Object::new(scope);
-
   let env_shared = EnvShared::new(napi_wrap, type_tag, path.clone());
 
   let ctx = scope.get_current_context();
@@ -542,6 +568,9 @@ where
     slot.take()
   });
 
+  // The `module.exports` object.
+  let exports = v8::Object::new(scope);
+
   let maybe_exports = if let Some(module_to_register) = maybe_module {
     // SAFETY: napi_register_module guarantees that `module_to_register` is valid.
     let nm = unsafe { &*module_to_register };
diff --git a/ext/napi/value.rs b/ext/napi/value.rs
index 6fb96758c..71beac07e 100644
--- a/ext/napi/value.rs
+++ b/ext/napi/value.rs
@@ -31,9 +31,7 @@ where
   v8::Local<'s, T>: Into<v8::Local<'s, v8::Value>>,
 {
   fn from(v: v8::Local<'s, T>) -> Self {
-    // SAFETY: It is safe to cast v8::Local<T> that implements Into<v8::Local<v8::Value>>.
-    //         `fn into(self)` transmutes anyways.
-    Self(unsafe { transmute(v) }, std::marker::PhantomData)
+    Self(Some(NonNull::from(&*v.into())), std::marker::PhantomData)
   }
 }