From c49309fdc29efaa166ad1fd4af2ae8f8cc7e7653 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Sat, 17 Feb 2024 22:14:27 +0800 Subject: [PATCH] fix(napi): memory leak while using Reference (#1954) - Close https://github.com/napi-rs/napi-rs/issues/1952 --- crates/backend/src/codegen/fn.rs | 23 +++++++++---- crates/backend/src/codegen/struct.rs | 12 ++++--- .../napi/src/bindgen_runtime/callback_info.rs | 34 +++++++++++++++---- .../bindgen_runtime/js_values/value_ref.rs | 19 ++++------- 4 files changed, 57 insertions(+), 31 deletions(-) diff --git a/crates/backend/src/codegen/fn.rs b/crates/backend/src/codegen/fn.rs index 095d34bf..3fd41b48 100644 --- a/crates/backend/src/codegen/fn.rs +++ b/crates/backend/src/codegen/fn.rs @@ -267,11 +267,16 @@ impl NapiFn { { if let Some(p) = path.path.segments.first() { if p.ident == *self.parent.as_ref().unwrap() { - args.push( - quote! { napi::bindgen_prelude::Reference::from_value_ptr(this_ptr.cast(), env)? }, - ); + args.push(quote! { + napi::bindgen_prelude::Reference::from_value_ptr(this_ptr.cast(), env)? + }); skipped_arg_count += 1; continue; + } else { + bail_span!( + p, + "The `T` of Reference type must be the same as the class type" + ) } } } @@ -551,16 +556,20 @@ impl NapiFn { let ty_string = ty.into_token_stream().to_string(); let is_return_self = ty_string == "& Self" || ty_string == "&mut Self"; if self.kind == FnKind::Constructor { + let parent = self + .parent + .as_ref() + .expect("Parent must exist for constructor"); if self.is_ret_result { if self.parent_is_generator { - quote! { cb.construct_generator(#js_name, #ret?) } + quote! { cb.construct_generator::(#js_name, #ret?) } } else { - quote! { cb.construct(#js_name, #ret?) } + quote! { cb.construct::(#js_name, #ret?) } } } else if self.parent_is_generator { - quote! { cb.construct_generator(#js_name, #ret) } + quote! { cb.construct_generator::(#js_name, #ret) } } else { - quote! { cb.construct(#js_name, #ret) } + quote! { cb.construct::(#js_name, #ret) } } } else if self.kind == FnKind::Factory { if self.is_ret_result { diff --git a/crates/backend/src/codegen/struct.rs b/crates/backend/src/codegen/struct.rs index 70ab6bbc..93348776 100644 --- a/crates/backend/src/codegen/struct.rs +++ b/crates/backend/src/codegen/struct.rs @@ -227,10 +227,12 @@ impl NapiStruct { quote! { #name {#(#fields),*} } }; + let is_empty_struct_hint = fields_len == 0; + let constructor = if self.implement_iterator { - quote! { unsafe { cb.construct_generator(#js_name_str, #construct) } } + quote! { unsafe { cb.construct_generator::<#is_empty_struct_hint, #name>(#js_name_str, #construct) } } } else { - quote! { unsafe { cb.construct(#js_name_str, #construct) } } + quote! { unsafe { cb.construct::<#is_empty_struct_hint, #name>(#js_name_str, #construct) } } }; quote! { @@ -280,7 +282,7 @@ impl NapiStruct { ) -> napi::Result { if let Some(ctor_ref) = napi::__private::get_class_constructor(#js_name_str) { let wrapped_value = Box::into_raw(Box::new(val)); - let instance_value = #name::new_instance(env, wrapped_value as *mut std::ffi::c_void, ctor_ref)?; + let instance_value = #name::new_instance(env, wrapped_value.cast(), ctor_ref)?; #iterator_implementation Ok(instance_value) } else { @@ -298,12 +300,12 @@ impl NapiStruct { if let Some(ctor_ref) = napi::bindgen_prelude::get_class_constructor(#js_name_str) { unsafe { let wrapped_value = Box::into_raw(Box::new(val)); - let instance_value = #name::new_instance(env.raw(), wrapped_value as *mut std::ffi::c_void, ctor_ref)?; + let instance_value = #name::new_instance(env.raw(), wrapped_value.cast(), ctor_ref)?; { let env = env.raw(); #iterator_implementation } - napi::bindgen_prelude::Reference::<#name>::from_value_ptr(wrapped_value as *mut std::ffi::c_void, env.raw()) + napi::bindgen_prelude::Reference::<#name>::from_value_ptr(wrapped_value.cast(), env.raw()) } } else { Err(napi::bindgen_prelude::Error::new( diff --git a/crates/napi/src/bindgen_runtime/callback_info.rs b/crates/napi/src/bindgen_runtime/callback_info.rs index 54e03968..43c8593c 100644 --- a/crates/napi/src/bindgen_runtime/callback_info.rs +++ b/crates/napi/src/bindgen_runtime/callback_info.rs @@ -12,6 +12,10 @@ thread_local! { pub static ___CALL_FROM_FACTORY: AtomicBool = AtomicBool::new(false); } +#[repr(transparent)] +struct EmptyStructPlaceholder(u8); + +#[doc(hidden)] pub struct CallbackInfo { env: sys::napi_env, pub this: sys::napi_value, @@ -84,14 +88,19 @@ impl CallbackInfo { self.this } - fn _construct( + fn _construct( &self, js_name: &str, obj: T, ) -> Result<(sys::napi_value, *mut T)> { let obj = Box::new(obj); let this = self.this(); - let value_ref = Box::into_raw(obj); + let mut value_ref = Box::into_raw(obj); + // for empty struct like `#[napi] struct A;`, the `value_ref` will be `0x1` + // and it will be overwritten by the others instance of the same class + if IsEmptyStructHint || value_ref as usize == 0x1 { + value_ref = Box::into_raw(Box::new(EmptyStructPlaceholder(0))).cast(); + } let mut object_ref = ptr::null_mut(); let initial_finalize: Box = Box::new(|| {}); let finalize_callbacks_ptr = Rc::into_raw(Rc::new(Cell::new(Box::into_raw(initial_finalize)))); @@ -118,20 +127,25 @@ impl CallbackInfo { Ok((this, value_ref)) } - pub fn construct( + pub fn construct( &self, js_name: &str, obj: T, ) -> Result { - self._construct(js_name, obj).map(|(v, _)| v) + self + ._construct::(js_name, obj) + .map(|(v, _)| v) } - pub fn construct_generator( + pub fn construct_generator< + const IsEmptyStructHint: bool, + T: Generator + ObjectFinalize + 'static, + >( &self, js_name: &str, obj: T, ) -> Result { - let (instance, generator_ptr) = self._construct(js_name, obj)?; + let (instance, generator_ptr) = self._construct::(js_name, obj)?; crate::__private::create_iterator(self.env, instance, generator_ptr); Ok(instance) } @@ -187,7 +201,13 @@ impl CallbackInfo { let initial_finalize: Box = Box::new(|| {}); let finalize_callbacks_ptr = Rc::into_raw(Rc::new(Cell::new(Box::into_raw(initial_finalize)))); let mut object_ref = ptr::null_mut(); - let value_ref = Box::into_raw(obj); + let mut value_ref = Box::into_raw(obj); + + // for empty struct like `#[napi] struct A;`, the `value_ref` will be `0x1` + // and it will be overwritten by the others instance of the same class + if value_ref as usize == 0x1 { + value_ref = Box::into_raw(Box::new(EmptyStructPlaceholder(0))).cast(); + } check_status!( unsafe { sys::napi_wrap( diff --git a/crates/napi/src/bindgen_runtime/js_values/value_ref.rs b/crates/napi/src/bindgen_runtime/js_values/value_ref.rs index ed6f18f6..18479379 100644 --- a/crates/napi/src/bindgen_runtime/js_values/value_ref.rs +++ b/crates/napi/src/bindgen_runtime/js_values/value_ref.rs @@ -7,9 +7,9 @@ use std::rc::{Rc, Weak}; use crate::{bindgen_runtime::ToNapiValue, check_status, Env, Error, Result, Status}; type RefInformation = ( - *mut c_void, - crate::sys::napi_ref, - *const Cell<*mut dyn FnOnce()>, + /* wrapped_value */ *mut c_void, + /* napi_ref */ crate::sys::napi_ref, + /* finalize_callback */ *const Cell<*mut dyn FnOnce()>, ); thread_local! { @@ -27,9 +27,6 @@ pub struct Reference { finalize_callbacks: Rc>, } -unsafe impl Send for Reference {} -unsafe impl Sync for Reference {} - impl Drop for Reference { fn drop(&mut self) { let rc_strong_count = Rc::strong_count(&self.finalize_callbacks); @@ -71,8 +68,9 @@ impl Reference { if let Some((wrapped_value, napi_ref, finalize_callbacks_ptr)) = REFERENCE_MAP.with(|map| map.borrow().get(&t).cloned()) { + let mut ref_count = 0; check_status!( - unsafe { crate::sys::napi_reference_ref(env, napi_ref, &mut 0) }, + unsafe { crate::sys::napi_reference_ref(env, napi_ref, &mut ref_count) }, "Failed to ref napi reference" )?; let finalize_callbacks_raw = unsafe { Rc::from_raw(finalize_callbacks_ptr) }; @@ -80,9 +78,9 @@ impl Reference { // Leak the raw finalize callbacks Rc::into_raw(finalize_callbacks_raw); Ok(Self { - raw: wrapped_value as *mut T, + raw: wrapped_value.cast(), napi_ref, - env: env as *mut c_void, + env: env.cast(), finalize_callbacks, }) } else { @@ -243,9 +241,6 @@ pub struct SharedReference { owner: Reference, } -unsafe impl Send for SharedReference {} -unsafe impl Sync for SharedReference {} - impl SharedReference { pub fn clone(&self, env: Env) -> Result { Ok(SharedReference {