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

6 files changed, 212 insertions, 230 deletions

diff --git a/cli/napi/js_native_api.rs b/cli/napi/js_native_api.rs
index cbce113dc..6b31e1fd2 100644
--- a/cli/napi/js_native_api.rs
+++ b/cli/napi/js_native_api.rs
@@ -179,14 +179,15 @@ fn napi_get_last_error_info(
 }
 
 #[napi_sym]
-fn napi_create_function(
-  env: &mut Env,
+fn napi_create_function<'s>(
+  env: &'s mut Env,
   name: *const c_char,
   length: usize,
-  cb: napi_callback,
+  cb: Option<napi_callback>,
   cb_info: napi_callback_info,
-  result: *mut napi_value,
+  result: *mut napi_value<'s>,
 ) -> napi_status {
+  let env_ptr = env as *mut Env;
   check_arg!(env, result);
   check_arg!(env, cb);
 
@@ -200,7 +201,9 @@ fn napi_create_function(
   };
 
   unsafe {
-    *result = create_function(env, name, cb, cb_info).into();
+    *result =
+      create_function(&mut env.scope(), env_ptr, name, cb.unwrap(), cb_info)
+        .into();
   }
 
   napi_ok
@@ -212,12 +215,13 @@ fn napi_define_class<'s>(
   env: &'s mut Env,
   utf8name: *const c_char,
   length: usize,
-  constructor: napi_callback,
+  constructor: Option<napi_callback>,
   callback_data: *mut c_void,
   property_count: usize,
   properties: *const napi_property_descriptor,
   result: *mut napi_value<'s>,
 ) -> napi_status {
+  let env_ptr = env as *mut Env;
   check_arg!(env, result);
   check_arg!(env, constructor);
 
@@ -230,9 +234,13 @@ fn napi_define_class<'s>(
     Err(status) => return status,
   };
 
-  let tpl = unsafe {
-    create_function_template(env, Some(name), constructor, callback_data)
-  };
+  let tpl = create_function_template(
+    &mut env.scope(),
+    env_ptr,
+    Some(name),
+    constructor.unwrap(),
+    callback_data,
+  );
 
   let napi_properties: &[napi_property_descriptor] = if property_count > 0 {
     unsafe { std::slice::from_raw_parts(properties, property_count) }
@@ -248,28 +256,18 @@ fn napi_define_class<'s>(
       continue;
     }
 
-    let name = match unsafe { v8_name_from_property_descriptor(env, p) } {
+    let name = match unsafe { v8_name_from_property_descriptor(env_ptr, p) } {
       Ok(name) => name,
       Err(status) => return status,
     };
 
-    let method = p.method;
-    let getter = p.getter;
-    let setter = p.setter;
-
-    if getter.is_some() || setter.is_some() {
-      let getter: Option<v8::Local<v8::FunctionTemplate>> = if getter.is_some()
-      {
-        Some(unsafe { create_function_template(env, None, p.getter, p.data) })
-      } else {
-        None
-      };
-      let setter: Option<v8::Local<v8::FunctionTemplate>> = if setter.is_some()
-      {
-        Some(unsafe { create_function_template(env, None, p.setter, p.data) })
-      } else {
-        None
-      };
+    if p.getter.is_some() || p.setter.is_some() {
+      let getter = p.getter.map(|g| {
+        create_function_template(&mut env.scope(), env_ptr, None, g, p.data)
+      });
+      let setter = p.setter.map(|s| {
+        create_function_template(&mut env.scope(), env_ptr, None, s, p.data)
+      });
 
       let mut accessor_property = v8::PropertyAttribute::NONE;
       if getter.is_some()
@@ -290,9 +288,14 @@ fn napi_define_class<'s>(
 
       let proto = tpl.prototype_template(&mut env.scope());
       proto.set_accessor_property(name, getter, setter, accessor_property);
-    } else if method.is_some() {
-      let function =
-        unsafe { create_function_template(env, None, p.method, p.data) };
+    } else if let Some(method) = p.method {
+      let function = create_function_template(
+        &mut env.scope(),
+        env_ptr,
+        None,
+        method,
+        p.data,
+      );
       let proto = tpl.prototype_template(&mut env.scope());
       proto.set(name, function.into());
     } else {
@@ -301,7 +304,6 @@ fn napi_define_class<'s>(
     }
   }
 
-  let env_ptr = env as *mut Env;
   let value: v8::Local<v8::Value> =
     tpl.get_function(&mut env.scope()).unwrap().into();
 
@@ -781,17 +783,19 @@ fn napi_define_properties(
     let configurable = property.attributes & napi_configurable != 0;
 
     if property.getter.is_some() || property.setter.is_some() {
-      let local_getter: v8::Local<v8::Value> = if property.getter.is_some() {
-        unsafe {
-          create_function(env_ptr, None, property.getter, property.data).into()
-        }
+      let local_getter: v8::Local<v8::Value> = if let Some(getter) =
+        property.getter
+      {
+        create_function(&mut env.scope(), env_ptr, None, getter, property.data)
+          .into()
       } else {
         v8::undefined(scope).into()
       };
-      let local_setter: v8::Local<v8::Value> = if property.setter.is_some() {
-        unsafe {
-          create_function(env_ptr, None, property.setter, property.data).into()
-        }
+      let local_setter: v8::Local<v8::Value> = if let Some(setter) =
+        property.setter
+      {
+        create_function(&mut env.scope(), env_ptr, None, setter, property.data)
+          .into()
       } else {
         v8::undefined(scope).into()
       };
@@ -807,11 +811,15 @@ fn napi_define_properties(
       {
         return napi_invalid_arg;
       }
-    } else if property.method.is_some() {
+    } else if let Some(method) = property.method {
       let method: v8::Local<v8::Value> = {
-        let function = unsafe {
-          create_function(env_ptr, None, property.method, property.data)
-        };
+        let function = create_function(
+          &mut env.scope(),
+          env_ptr,
+          None,
+          method,
+          property.data,
+        );
         function.into()
       };
 
@@ -1692,18 +1700,16 @@ fn napi_call_function(
 }
 
 #[napi_sym]
-fn napi_get_global(env: *mut Env, result: *mut napi_value) -> napi_status {
-  let env = check_env!(env);
+fn napi_get_global(env_ptr: *mut Env, result: *mut napi_value) -> napi_status {
+  let env = check_env!(env_ptr);
   check_arg!(env, result);
 
+  let global = v8::Local::new(&mut env.scope(), &env.global);
   unsafe {
-    *result = std::mem::transmute::<NonNull<v8::Value>, v8::Local<v8::Value>>(
-      env.global,
-    )
-    .into();
+    *result = global.into();
   }
 
-  return napi_clear_last_error(env);
+  return napi_clear_last_error(env_ptr);
 }
 
 #[napi_sym]
@@ -2869,7 +2875,7 @@ fn napi_create_arraybuffer<'s>(
 
   if !data.is_null() {
     unsafe {
-      *data = get_array_buffer_ptr(buffer) as _;
+      *data = get_array_buffer_ptr(buffer);
     }
   }
 
@@ -2915,7 +2921,7 @@ fn napi_create_external_arraybuffer<'s>(
 fn napi_get_arraybuffer_info(
   env: *mut Env,
   value: napi_value,
-  data: *mut *mut u8,
+  data: *mut *mut c_void,
   length: *mut usize,
 ) -> napi_status {
   let env = check_env!(env);
@@ -3121,7 +3127,7 @@ fn napi_get_typedarray_info(
 fn napi_create_dataview<'s>(
   env: &'s mut Env,
   byte_length: usize,
-  arraybuffer: napi_value,
+  arraybuffer: napi_value<'s>,
   byte_offset: usize,
   result: *mut napi_value<'s>,
 ) -> napi_status {
@@ -3144,26 +3150,8 @@ fn napi_create_dataview<'s>(
     }
   }
 
-  // let dataview = v8::DataView::new(&mut env, buffer, byte_offset, byte_length);
-  let dataview = {
-    let context = &mut env.scope().get_current_context();
-    let global = context.global(&mut env.scope());
-    let data_view_name = v8::String::new(&mut env.scope(), "DataView").unwrap();
-    let data_view =
-      global.get(&mut env.scope(), data_view_name.into()).unwrap();
-    let Ok(data_view) = v8::Local::<v8::Function>::try_from(data_view) else {
-      return napi_function_expected;
-    };
-    let byte_offset = v8::Number::new(&mut env.scope(), byte_offset as f64);
-    let byte_length = v8::Number::new(&mut env.scope(), byte_length as f64);
-    let Some(dv) = data_view.new_instance(
-      &mut env.scope(),
-      &[buffer.into(), byte_offset.into(), byte_length.into()],
-    ) else {
-      return napi_generic_failure;
-    };
-    dv
-  };
+  let dataview =
+    v8::DataView::new(&mut env.scope(), buffer, byte_offset, byte_length);
 
   unsafe {
     *result = dataview.into();
diff --git a/cli/napi/node_api.rs b/cli/napi/node_api.rs
index 28a11d614..268715fa8 100644
--- a/cli/napi/node_api.rs
+++ b/cli/napi/node_api.rs
@@ -9,10 +9,11 @@ use super::util::napi_set_last_error;
 use super::util::SendPtr;
 use crate::check_arg;
 use crate::check_env;
+use deno_core::parking_lot::Condvar;
+use deno_core::parking_lot::Mutex;
 use deno_core::V8CrossThreadTaskSpawner;
 use deno_runtime::deno_napi::*;
 use napi_sym::napi_sym;
-use std::ptr::NonNull;
 use std::sync::atomic::AtomicBool;
 use std::sync::atomic::AtomicU8;
 use std::sync::atomic::AtomicUsize;
@@ -40,14 +41,7 @@ fn napi_add_env_cleanup_hook(
 
   let fun = fun.unwrap();
 
-  let mut env_cleanup_hooks = env.cleanup_hooks.borrow_mut();
-  if env_cleanup_hooks
-    .iter()
-    .any(|pair| pair.0 == fun && pair.1 == arg)
-  {
-    panic!("Cleanup hook with this data already registered");
-  }
-  env_cleanup_hooks.push((fun, arg));
+  env.add_cleanup_hook(fun, arg);
 
   napi_ok
 }
@@ -63,27 +57,21 @@ fn napi_remove_env_cleanup_hook(
 
   let fun = fun.unwrap();
 
-  let mut env_cleanup_hooks = env.cleanup_hooks.borrow_mut();
-  // Hooks are supposed to be removed in LIFO order
-  let maybe_index = env_cleanup_hooks
-    .iter()
-    .rposition(|&pair| pair.0 == fun && pair.1 == arg);
-
-  if let Some(index) = maybe_index {
-    env_cleanup_hooks.remove(index);
-  } else {
-    panic!("Cleanup hook with this data not found");
-  }
+  env.remove_cleanup_hook(fun, arg);
 
   napi_ok
 }
 
-type AsyncCleanupHandle = (*mut Env, napi_async_cleanup_hook, *mut c_void);
+struct AsyncCleanupHandle {
+  env: *mut Env,
+  hook: napi_async_cleanup_hook,
+  data: *mut c_void,
+}
 
 unsafe extern "C" fn async_cleanup_handler(arg: *mut c_void) {
   unsafe {
-    let (env, hook, arg) = *Box::<AsyncCleanupHandle>::from_raw(arg as _);
-    hook(env as _, arg);
+    let handle = Box::<AsyncCleanupHandle>::from_raw(arg as _);
+    (handle.hook)(arg, handle.data);
   }
 }
 
@@ -99,12 +87,13 @@ fn napi_add_async_cleanup_hook(
 
   let hook = hook.unwrap();
 
-  let handle =
-    Box::into_raw(Box::<AsyncCleanupHandle>::new((env, hook, arg))) as _;
+  let handle = Box::into_raw(Box::new(AsyncCleanupHandle {
+    env,
+    hook,
+    data: arg,
+  })) as *mut c_void;
 
-  unsafe {
-    napi_add_env_cleanup_hook(env, Some(async_cleanup_handler), handle);
-  }
+  env.add_cleanup_hook(async_cleanup_handler, handle);
 
   if !remove_handle.is_null() {
     unsafe {
@@ -123,17 +112,12 @@ fn napi_remove_async_cleanup_hook(
     return napi_invalid_arg;
   }
 
-  let handle = unsafe { &*(remove_handle as *mut AsyncCleanupHandle) };
+  let handle =
+    unsafe { Box::<AsyncCleanupHandle>::from_raw(remove_handle as _) };
 
-  let env = unsafe { &mut *handle.0 };
+  let env = unsafe { &mut *handle.env };
 
-  unsafe {
-    napi_remove_env_cleanup_hook(
-      env,
-      Some(async_cleanup_handler),
-      remove_handle,
-    );
-  }
+  env.remove_cleanup_hook(async_cleanup_handler, remove_handle);
 
   napi_ok
 }
@@ -142,11 +126,7 @@ fn napi_remove_async_cleanup_hook(
 fn napi_fatal_exception(env: &mut Env, err: napi_value) -> napi_status {
   check_arg!(env, err);
 
-  let report_error = unsafe {
-    std::mem::transmute::<NonNull<v8::Function>, v8::Local<v8::Function>>(
-      env.report_error,
-    )
-  };
+  let report_error = v8::Local::new(&mut env.scope(), &env.report_error);
 
   let this = v8::undefined(&mut env.scope());
   if report_error
@@ -262,7 +242,7 @@ fn napi_make_callback<'s>(
   recv: napi_value,
   func: napi_value,
   argc: usize,
-  argv: *const napi_value,
+  argv: *const napi_value<'s>,
   result: *mut napi_value<'s>,
 ) -> napi_status {
   check_arg!(env, recv);
@@ -312,11 +292,8 @@ fn napi_create_buffer<'s>(
 
   let ab = v8::ArrayBuffer::new(&mut env.scope(), length);
 
-  let buffer_constructor = unsafe {
-    std::mem::transmute::<NonNull<v8::Function>, v8::Local<v8::Function>>(
-      env.buffer_constructor,
-    )
-  };
+  let buffer_constructor =
+    v8::Local::new(&mut env.scope(), &env.buffer_constructor);
   let Some(buffer) =
     buffer_constructor.new_instance(&mut env.scope(), &[ab.into()])
   else {
@@ -325,7 +302,7 @@ fn napi_create_buffer<'s>(
 
   if !data.is_null() {
     unsafe {
-      *data = get_array_buffer_ptr(ab) as _;
+      *data = get_array_buffer_ptr(ab);
     }
   }
 
@@ -359,11 +336,8 @@ fn napi_create_external_buffer<'s>(
   let ab =
     v8::ArrayBuffer::with_backing_store(&mut env.scope(), &store.make_shared());
 
-  let buffer_constructor = unsafe {
-    std::mem::transmute::<NonNull<v8::Function>, v8::Local<v8::Function>>(
-      env.buffer_constructor,
-    )
-  };
+  let buffer_constructor =
+    v8::Local::new(&mut env.scope(), &env.buffer_constructor);
   let Some(buffer) =
     buffer_constructor.new_instance(&mut env.scope(), &[ab.into()])
   else {
@@ -389,18 +363,15 @@ fn napi_create_buffer_copy<'s>(
 
   let ab = v8::ArrayBuffer::new(&mut env.scope(), length);
 
-  let buffer_constructor = unsafe {
-    std::mem::transmute::<NonNull<v8::Function>, v8::Local<v8::Function>>(
-      env.buffer_constructor,
-    )
-  };
+  let buffer_constructor =
+    v8::Local::new(&mut env.scope(), &env.buffer_constructor);
   let Some(buffer) =
     buffer_constructor.new_instance(&mut env.scope(), &[ab.into()])
   else {
     return napi_generic_failure;
   };
 
-  let ptr = get_array_buffer_ptr(ab) as *mut c_void;
+  let ptr = get_array_buffer_ptr(ab);
   unsafe {
     std::ptr::copy(data, ptr, length);
   }
@@ -428,11 +399,8 @@ fn napi_is_buffer(
   check_arg!(env, value);
   check_arg!(env, result);
 
-  let buffer_constructor = unsafe {
-    std::mem::transmute::<NonNull<v8::Function>, v8::Local<v8::Function>>(
-      env.buffer_constructor,
-    )
-  };
+  let buffer_constructor =
+    v8::Local::new(&mut env.scope(), &env.buffer_constructor);
 
   let Some(is_buffer) = value
     .unwrap()
@@ -464,11 +432,8 @@ fn napi_get_buffer_info(
     return napi_set_last_error(env, napi_invalid_arg);
   };
 
-  let buffer_constructor = unsafe {
-    std::mem::transmute::<NonNull<v8::Function>, v8::Local<v8::Function>>(
-      env.buffer_constructor,
-    )
-  };
+  let buffer_constructor =
+    v8::Local::new(&mut env.scope(), &env.buffer_constructor);
 
   if !ta
     .instance_of(&mut env.scope(), buffer_constructor.into())
@@ -703,7 +668,9 @@ extern "C" fn default_call_js_cb(
 struct TsFn {
   env: *mut Env,
   func: Option<v8::Global<v8::Function>>,
-  _max_queue_size: usize,
+  max_queue_size: usize,
+  queue_size: Mutex<usize>,
+  queue_cond: Condvar,
   thread_count: AtomicUsize,
   thread_finalize_data: *mut c_void,
   thread_finalize_cb: Option<napi_finalize>,
@@ -771,6 +738,7 @@ impl TsFn {
         .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst)
         .is_ok()
     {
+      tsfn.queue_cond.notify_all();
       let tsfnptr = SendPtr(tsfn);
       // drop must be queued in order to preserve ordering consistent
       // with Node.js and so that the finalizer runs on the main thread.
@@ -811,26 +779,34 @@ impl TsFn {
   pub fn call(
     &self,
     data: *mut c_void,
-    _mode: napi_threadsafe_function_call_mode,
+    mode: napi_threadsafe_function_call_mode,
   ) -> napi_status {
-    // TODO:
-    // if self.max_queue_size > 0 && queued >= self.max_queue_size {
-    //   if mode == napi_tsfn_blocking {
-    //     wait somehow
-    //   } else {
-    //     return napi_queue_full;
-    //   }
-    // }
-
     if self.is_closing.load(Ordering::SeqCst) {
       return napi_closing;
     }
 
+    if self.max_queue_size > 0 {
+      let mut queue_size = self.queue_size.lock();
+      while *queue_size >= self.max_queue_size {
+        if mode == napi_tsfn_blocking {
+          self.queue_cond.wait(&mut queue_size);
+
+          if self.is_closing.load(Ordering::SeqCst) {
+            return napi_closing;
+          }
+        } else {
+          return napi_queue_full;
+        }
+      }
+      *queue_size += 1;
+    }
+
     let is_closed = self.is_closed.clone();
     let tsfn = SendPtr(self);
     let data = SendPtr(data);
     let context = SendPtr(self.context);
     let call_js_cb = self.call_js_cb;
+
     self.sender.spawn(move |scope: &mut v8::HandleScope| {
       let data = data.take();
 
@@ -849,6 +825,15 @@ impl TsFn {
 
         let tsfn = unsafe { &*tsfn };
 
+        if tsfn.max_queue_size > 0 {
+          let mut queue_size = tsfn.queue_size.lock();
+          let size = *queue_size;
+          *queue_size -= 1;
+          if size == tsfn.max_queue_size {
+            tsfn.queue_cond.notify_one();
+          }
+        }
+
         let func = tsfn.func.as_ref().map(|f| v8::Local::new(scope, f));
 
         unsafe {
@@ -918,7 +903,9 @@ fn napi_create_threadsafe_function(
   let mut tsfn = Box::new(TsFn {
     env,
     func,
-    _max_queue_size: max_queue_size,
+    max_queue_size,
+    queue_size: Mutex::new(0),
+    queue_cond: Condvar::new(),
     thread_count: AtomicUsize::new(initial_thread_count),
     thread_finalize_data,
     thread_finalize_cb,
diff --git a/cli/napi/util.rs b/cli/napi/util.rs
index cd3ef14a2..63d8effbf 100644
--- a/cli/napi/util.rs
+++ b/cli/napi/util.rs
@@ -15,10 +15,11 @@ impl<T> SendPtr<T> {
 unsafe impl<T> Send for SendPtr<T> {}
 unsafe impl<T> Sync for SendPtr<T> {}
 
-pub fn get_array_buffer_ptr(ab: v8::Local<v8::ArrayBuffer>) -> *mut u8 {
-  // SAFETY: Thanks to the null pointer optimization, NonNull<T> and Option<NonNull<T>> are guaranteed
-  // to have the same size and alignment.
-  unsafe { std::mem::transmute(ab.data()) }
+pub fn get_array_buffer_ptr(ab: v8::Local<v8::ArrayBuffer>) -> *mut c_void {
+  match ab.data() {
+    Some(p) => p.as_ptr(),
+    None => std::ptr::null_mut(),
+  }
 }
 
 struct BufferFinalizer {
@@ -216,10 +217,6 @@ impl<'s> Nullable for napi_value<'s> {
   }
 }
 
-// TODO: replace Nullable with some sort of "CheckedUnwrap" trait
-// *mut T -> &mut MaybeUninit<T>
-// Option<T> -> T
-// napi_value -> Local<Value>
 #[macro_export]
 macro_rules! check_arg {
   ($env: expr, $ptr: expr) => {
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)
   }
 }