fix(napi): should also delete the reference while dropping the Buffer

crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs

@@ -67,6 +67,11 @@ macro_rules! impl_typed_array {
             crate::check_status_or_throw!(
               env,
               unsafe { sys::napi_reference_unref(env, ref_, &mut 0) },
+              "Failed to unref Buffer reference in drop"
+            );
+            crate::check_status_or_throw!(
+              env,
+              unsafe { sys::napi_delete_reference(env, ref_) },
               "Failed to delete Buffer reference in drop"
             );
             return;

crates/napi/src/bindgen_runtime/js_values/buffer.rs

@@ -25,49 +25,44 @@ pub struct Buffer {
   pub(crate) len: usize,
   pub(crate) capacity: usize,
   raw: Option<(sys::napi_ref, sys::napi_env)>,
-  // use it as ref count
-  pub(crate) drop_in_vm: Arc<()>,
+  pub(crate) ref_count: Arc<()>,
 }
 
 impl Drop for Buffer {
   fn drop(&mut self) {
+    if Arc::strong_count(&self.ref_count) == 1 {
       if let Some((ref_, env)) = self.raw {
+        let mut ref_count = 0;
         check_status_or_throw!(
           env,
-        unsafe { sys::napi_reference_unref(env, ref_, &mut 0) },
+          unsafe { sys::napi_reference_unref(env, ref_, &mut ref_count) },
           "Failed to unref Buffer reference in drop"
         );
-      return;
-    }
-
-    if Arc::strong_count(&self.drop_in_vm) == 1 {
+        check_status_or_throw!(
+          env,
+          unsafe { sys::napi_delete_reference(env, ref_) },
+          "Failed to delete Buffer reference in drop"
+        );
+      } else {
         unsafe { Vec::from_raw_parts(self.inner.as_ptr(), self.len, self.capacity) };
       }
     }
   }
+}
 
 // SAFETY: This is undefined behavior, as the JS side may always modify the underlying buffer,
 // without synchronization. Also see the docs for the `AsMut` impl.
 unsafe impl Send for Buffer {}
 
-impl Buffer {
-  pub fn clone(&mut self, env: &Env) -> Result<Self> {
-    let raw = if let Some((ref_, _)) = self.raw {
-      check_status!(
-        unsafe { sys::napi_reference_ref(env.0, ref_, &mut 0) },
-        "Failed to ref Buffer reference in Buffer::clone"
-      )?;
-      Some((ref_, env.0))
-    } else {
-      None
-    };
-    Ok(Self {
+impl Clone for Buffer {
+  fn clone(&self) -> Self {
+    Self {
       inner: self.inner,
       len: self.len,
       capacity: self.capacity,
-      raw,
-      drop_in_vm: self.drop_in_vm.clone(),
-    })
+      raw: self.raw,
+      ref_count: self.ref_count.clone(),
+    }
   }
 }
 
@@ -95,7 +90,7 @@ impl From<Vec<u8>> for Buffer {
       len,
       capacity,
       raw: None,
-      drop_in_vm: Arc::new(()),
+      ref_count: Arc::new(()),
     }
   }
 }
@@ -184,7 +179,7 @@ impl FromNapiValue for Buffer {
       len,
       capacity: len,
       raw: Some((ref_, env)),
-      drop_in_vm: Arc::new(()),
+      ref_count: Arc::new(()),
     })
   }
 }
@@ -227,9 +222,16 @@ impl ToNapiValue for Buffer {
   }
 }
 
+impl ToNapiValue for &Buffer {
+  unsafe fn to_napi_value(env: sys::napi_env, val: Self) -> Result<sys::napi_value> {
+    let buf = val.clone();
+    unsafe { ToNapiValue::to_napi_value(env, buf) }
+  }
+}
+
 impl ToNapiValue for &mut Buffer {
   unsafe fn to_napi_value(env: sys::napi_env, val: Self) -> Result<sys::napi_value> {
-    let buf = val.clone(&Env::from(env))?;
+    let buf = val.clone();
     unsafe { ToNapiValue::to_napi_value(env, buf) }
   }
 }

crates/napi/src/bindgen_runtime/mod.rs

@@ -73,7 +73,7 @@ pub unsafe extern "C" fn raw_finalize_unchecked<T: ObjectFinalize>(
 #[doc(hidden)]
 pub unsafe extern "C" fn drop_buffer(
   _env: sys::napi_env,
-  finalize_data: *mut c_void,
+  #[allow(unused)] finalize_data: *mut c_void,
   finalize_hint: *mut c_void,
 ) {
   #[cfg(debug_assertions)]

examples/napi/src/typed_array.rs

@@ -68,8 +68,6 @@ impl Task for AsyncBuffer {
 }
 
 #[napi]
-fn async_reduce_buffer(mut buf: Buffer, env: Env) -> Result<AsyncTask<AsyncBuffer>> {
-  Ok(AsyncTask::new(AsyncBuffer {
-    buf: buf.clone(&env)?,
-  }))
+fn async_reduce_buffer(buf: Buffer) -> Result<AsyncTask<AsyncBuffer>> {
+  Ok(AsyncTask::new(AsyncBuffer { buf: buf.clone() }))
 }

memory-testing/buffer.mjs

@@ -18,6 +18,8 @@ while (true) {
   api.arrayBufferConvert(
     Uint8Array.from(Array.from({ length: 1024 * 10240 }).fill(1)),
   )
+  api.bufferPassThrough(Buffer.from('hello world'.repeat(1024 * 1024)))
+  api.arrayBufferPassThrough(Uint8Array.from('hello world'.repeat(1024 * 1024)))
   if (i % 10 === 0) {
     await setTimeout(100)
     displayMemoryUsageFromNode(initialMemoryUsage)

memory-testing/src/lib.rs

@@ -161,3 +161,13 @@ pub fn array_buffer_convert(array_buffer: Uint8Array) -> Uint8Array {
 pub fn array_buffer_len() -> u32 {
   Uint8Array::new(vec![1; 1024 * 10240]).len() as u32
 }
+
+#[napi]
+pub fn buffer_pass_through(buffer: Buffer) -> Buffer {
+  buffer
+}
+
+#[napi]
+pub fn array_buffer_pass_through(array_buffer: Uint8Array) -> Uint8Array {
+  array_buffer
+}