Merge pull request #1161 from devongovett/reference-crash

fix(napi): handle the referenced object is finalized before `Reference::drop`

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

@@ -32,13 +32,25 @@ unsafe impl<T: Sync> Sync for Reference<T> {}
 
 impl<T> Drop for Reference<T> {
   fn drop(&mut self) {
-    let status = unsafe {
+    let rc_strong_count = Rc::strong_count(&self.finalize_callbacks);
-      crate::sys::napi_reference_unref(self.env as crate::sys::napi_env, self.napi_ref, &mut 0)
+    let mut ref_count = 0;
+    // If Rc strong count == 1, then the referenced object is dropped on GC
+    // It would happen when the process is exiting
+    // In general, the `drop` of the `Reference` would happen first
+    if rc_strong_count > 1 {
+      let status = unsafe {
+        crate::sys::napi_reference_unref(
+          self.env as crate::sys::napi_env,
+          self.napi_ref,
+          &mut ref_count,
+        )
+      };
+      debug_assert!(
+        status == crate::sys::Status::napi_ok,
+        "Reference unref failed, status code: {}",
+        crate::Status::from(status)
+      );
     };
-    debug_assert!(
-      status == crate::sys::Status::napi_ok,
-      "Reference unref failed"
-    );
   }
 }
 

crates/napi/src/bindgen_runtime/mod.rs

@@ -30,8 +30,25 @@ pub unsafe extern "C" fn raw_finalize_unchecked<T>(
     REFERENCE_MAP.with(|reference_map| reference_map.borrow_mut().remove(&finalize_data))
   {
     let finalize_callbacks_rc = unsafe { Rc::from_raw(finalize_callbacks_ptr) };
-    debug_assert!(Rc::strong_count(&finalize_callbacks_rc) == 1);
+
-    debug_assert!(Rc::weak_count(&finalize_callbacks_rc) == 0);
+    #[cfg(debug_assertions)]
+    {
+      let rc_strong_count = Rc::strong_count(&finalize_callbacks_rc);
+      let rc_weak_count = Rc::weak_count(&finalize_callbacks_rc);
+      // If `Rc` strong count is 2, it means the finalize of referenced `Object` is called before the `fn drop` of the `Reference`
+      // It always happened on exiting process
+      // In general, the `fn drop` would happen first
+      assert!(
+        rc_strong_count == 1 || rc_strong_count == 2,
+        "Rc strong count is: {}, it should be 1 or 2",
+        rc_strong_count
+      );
+      assert!(
+        rc_weak_count == 0,
+        "Rc weak count is: {}, it should be 0",
+        rc_weak_count
+      );
+    }
     let finalize = unsafe { Box::from_raw(finalize_callbacks_rc.get()) };
     finalize();
     let delete_reference_status = unsafe { sys::napi_delete_reference(env, ref_val) };

examples/napi/__test__/typegen.spec.ts.md

@@ -279,6 +279,11 @@ Generated by [AVA](https://avajs.dev).
     export class CssStyleSheet {␊
       constructor(rules: Array<string>)␊
       get rules(): CssRuleList␊
+      anotherCssStyleSheet(): AnotherCssStyleSheet␊
+    }␊
+    export type AnotherCSSStyleSheet = AnotherCssStyleSheet␊
+    export class AnotherCssStyleSheet {␊
+      get rules(): CssRuleList␊
     }␊
     export namespace xxh3 {␊
       export const ALIGNMENT: number␊

examples/napi/__test__/values.spec.ts

@@ -205,6 +205,8 @@ test('should be able to into_reference', (t) => {
   const sheet = new CssStyleSheet(rules)
   t.is(sheet.rules, sheet.rules)
   t.deepEqual(sheet.rules.getRules(), rules)
+  const anotherStyleSheet = sheet.anotherCssStyleSheet()
+  t.is(anotherStyleSheet.rules, sheet.rules)
 })
 
 test('callback', (t) => {

examples/napi/index.d.ts

@@ -269,6 +269,11 @@ export type CSSStyleSheet = CssStyleSheet
 export class CssStyleSheet {
   constructor(rules: Array<string>)
   get rules(): CssRuleList
+  anotherCssStyleSheet(): AnotherCssStyleSheet
+}
+export type AnotherCSSStyleSheet = AnotherCssStyleSheet
+export class AnotherCssStyleSheet {
+  get rules(): CssRuleList
 }
 export namespace xxh3 {
   export const ALIGNMENT: number

examples/napi/src/reference.rs

@@ -76,26 +76,54 @@ impl CSSRuleList {
 
 #[napi]
 pub struct CSSStyleSheet {
+  inner: Rc<RefCell<OwnedStyleSheet>>,
+  rules: Option<Reference<CSSRuleList>>,
+}
+
+#[napi]
+pub struct AnotherCSSStyleSheet {
   inner: Rc<RefCell<OwnedStyleSheet>>,
   rules: Reference<CSSRuleList>,
 }
 
 #[napi]
-impl CSSStyleSheet {
+impl AnotherCSSStyleSheet {
-  #[napi(constructor)]
-  pub fn new(env: Env, rules: Vec<String>) -> Result<Self> {
-    let inner = Rc::new(RefCell::new(OwnedStyleSheet { rules }));
-    let rules = CSSRuleList::into_reference(
-      CSSRuleList {
-        owned: inner.clone(),
-      },
-      env,
-    )?;
-    Ok(CSSStyleSheet { inner, rules })
-  }
-
   #[napi(getter)]
   pub fn rules(&self, env: Env) -> Result<Reference<CSSRuleList>> {
     self.rules.clone(env)
   }
 }
+
+#[napi]
+impl CSSStyleSheet {
+  #[napi(constructor)]
+  pub fn new(rules: Vec<String>) -> Result<Self> {
+    let inner = Rc::new(RefCell::new(OwnedStyleSheet { rules }));
+    Ok(CSSStyleSheet { inner, rules: None })
+  }
+
+  #[napi(getter)]
+  pub fn rules(&mut self, env: Env) -> Result<Reference<CSSRuleList>> {
+    if let Some(rules) = &self.rules {
+      return rules.clone(env);
+    }
+
+    let rules = CSSRuleList::into_reference(
+      CSSRuleList {
+        owned: self.inner.clone(),
+      },
+      env,
+    )?;
+
+    self.rules = Some(rules.clone(env)?);
+    Ok(rules)
+  }
+
+  #[napi]
+  pub fn another_css_style_sheet(&self, env: Env) -> Result<AnotherCSSStyleSheet> {
+    Ok(AnotherCSSStyleSheet {
+      inner: self.inner.clone(),
+      rules: self.rules.as_ref().unwrap().clone(env)?,
+    })
+  }
+}