From 91c62c46165cd7767daec63062222e1e876bb0fb Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Mon, 2 May 2022 20:27:18 -0400 Subject: [PATCH] fix(napi): handle the referenced object is finalized before `Reference::drop` --- .../bindgen_runtime/js_values/value_ref.rs | 24 ++++++-- crates/napi/src/bindgen_runtime/mod.rs | 21 ++++++- examples/napi/__test__/typegen.spec.ts.md | 5 ++ examples/napi/__test__/typegen.spec.ts.snap | Bin 2989 -> 3018 bytes examples/napi/__test__/values.spec.ts | 2 + examples/napi/index.d.ts | 5 ++ examples/napi/src/reference.rs | 54 +++++++++++++----- 7 files changed, 90 insertions(+), 21 deletions(-) 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 6443d810..a6bb39b8 100644 --- a/crates/napi/src/bindgen_runtime/js_values/value_ref.rs +++ b/crates/napi/src/bindgen_runtime/js_values/value_ref.rs @@ -32,13 +32,25 @@ unsafe impl Sync for Reference {} impl Drop for Reference { fn drop(&mut self) { - let status = unsafe { - crate::sys::napi_reference_unref(self.env as crate::sys::napi_env, self.napi_ref, &mut 0) + let rc_strong_count = Rc::strong_count(&self.finalize_callbacks); + 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" - ); } } diff --git a/crates/napi/src/bindgen_runtime/mod.rs b/crates/napi/src/bindgen_runtime/mod.rs index 13b69bf7..2e10e27a 100644 --- a/crates/napi/src/bindgen_runtime/mod.rs +++ b/crates/napi/src/bindgen_runtime/mod.rs @@ -30,8 +30,25 @@ pub unsafe extern "C" fn raw_finalize_unchecked( 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) }; diff --git a/examples/napi/__test__/typegen.spec.ts.md b/examples/napi/__test__/typegen.spec.ts.md index 83adb06e..9700e0f1 100644 --- a/examples/napi/__test__/typegen.spec.ts.md +++ b/examples/napi/__test__/typegen.spec.ts.md @@ -279,6 +279,11 @@ Generated by [AVA](https://avajs.dev). export class CssStyleSheet {␊ constructor(rules: Array)␊ get rules(): CssRuleList␊ + anotherCssStyleSheet(): AnotherCssStyleSheet␊ + }␊ + export type AnotherCSSStyleSheet = AnotherCssStyleSheet␊ + export class AnotherCssStyleSheet {␊ + get rules(): CssRuleList␊ }␊ export namespace xxh3 {␊ export const ALIGNMENT: number␊ diff --git a/examples/napi/__test__/typegen.spec.ts.snap b/examples/napi/__test__/typegen.spec.ts.snap index fdcffa602b5c1174ff084c4527241b75d3b3576b..7743c80efa3ef74f44de036a0973db5c0ae8ad82 100644 GIT binary patch delta 2980 zcmV;V3tRN97s?lZK~_N^Q*L2!b7*gLAa*kf0{}@67Pk}R2bS~HNwj<8Uyq%V!Afhi zJf}!sAyL{Ee>oqE2mk;800003ts2{I8@V-YQ55jo{sEh!*ioP)z63!kBo1WR&N_)N zu;gwQUSmVeP!i+OP#JP;t6l@$Kj?E`irqi!AM7Xe9CA2+!?{WFHh}{VIp@xE<@o2( zFcki!{OK2_xC+_LU*$w3VL-+#Vlh>W_#%#(uZWt@7zx;jjCjbnjOYv?mHg@FAfSqV z=l%MJ2Y-5i|9Z-P28o45|1sBRFJRW`mxn2&o$?X~;F{6Qq!a4c+>yF+iZ6y)^ zoG>Ei)1e6U>+^S=ba~Ms1K7G-y8}@kk->yZ0zVXVkEB8gtN?nwySLvxdq4Q0cibOz zNR&*6EVdA%B=Qv(5!g?CE8;+Utq$qNF`d7ZD(2CDcw4i0!;~aaOj(L99VRY`qh#t( zFtFPdK$%Q6AiS3)*jA%}O1za|LZmLVB`XkUzkN9DuAE1{H=}9-2GdFa&Pgn$T(Xxt z$!Jt36Gf!MBvgEGEj${Jtso9=lRiT}1{diDfk!*(qD_YOv1K1Bcs@w1*bW~bM9NzY z?BMx-vjvc58xHI2`LnLR6l42RUe>=3lTdS*WIYMWdM9smLS zRY4as{IwS~QfL!cZM?Qi@+0^+#3qj*c76myhvAAm`ijP?iAxPx!}y+xXeJA^S;|MR zHot(n$B+)nQrwFy1}~&OPF|5)as`2lv|o{b%{Ccq=e;LSo&aIU7&MxnUuYWeR1KF@z*9}b#_nL- zvk(W-EKxvDQy73qIZT^4>ajS^wKInd_JR}v>DSeuNTMPSlp8h>M>=X)5-mO~pvHV3_@Nm=9!r$nVef~^Lo@Gul2WHhR<&ozgNqV>M2a%f~wD9~0#syj5k)a?QxyJ8I4X1$%hc?Yg7 zp)^6)Vcz$sg7+Kt{eVg}Pcak!64O?GC=R>5=?vhIU)m6TClYv%UEvVY)+GUdRY7Vt zgLDy)5ri`wQN*q@#S&@MeNwMZFOf=M$FhejBxbn3g^PWW#PIwK zpZoj<9-rgm9v@Fo$rrdd;+oFiG%~}yys=*AcEENZ4z@Nu)+q|Jpj(1eaIJHf*OX&T zTVcCXPx5~-C-zERYg6k}B$9T29B5Y-DX04?QM0XOO&gNQ!_tQ)QJQt1CdyT88KE}% z!r$?IrhKivz(qLba^0^&?Tc4#T`i{RJw50Q&C_y>0SqyLVVEYJGss;p^v(+Wbk}Dw zmvy5VmPukBwvAc-OrIK&uts>nvI6Bo5BT&R82M@5-ybZEteNnKw)!uBD^C{n8!9J< zbY@L?%3#V2UfO1FWeJX;i9oyR9%#8y&urU-I9U(10-`O(os|_#`xkFknRZr~e=LzS z{G}844f95Zq32nRp`RA$rsOG$DEJJ_ShtYHMON4DGCt=Q6||e13Cuzvr&$k`#a=@k zKoWwZo}nhKrlYPKX)TgKnQHIrB2zj$)-AsWwt4N;>`L$Kyu)=vOQl_{)ma3RyH``n zqh_Qv{Pak>%%4VlC?e|n+>4e2Lu0ETh~Kffld=UTf4r0IhM_q?o0xQ`fF7GoLa?S| zmK||S1W^>Uz=`1=0iVKby!1f{8r6d~V z8%Qk7f4vEf#>{lO=wZh&U&lOPnNJqC?a95)-d7dLJo{Vd&mry3SghtK$F1p5)5tI> ztc?=me-O*d(?XT10ZA3}n&VO5ft`ej+*n^15SA)Jqc5Y z&+=<27~Mc7r#?4TTcF@m42M;!xdA>?qmAdQgTJRRdu^-&A6vaw6h}fF>u%y8LVq`% z8->$hG*k=|dud0~vhWqKYluvdsO}JUdtm2SlW_+mf6i~Y>TlJWa-^!z8sw9v?yLV_ zdi&tp;h_lF7bf?cMTCG!2e+1O9;=yihRMXvcge`V+;1ZpfmdzUKp)cYnDFTgKv5>*kVm+`oa0Ts=j^sF*4!&8{W-B4 z@hJ_n>otAX29=STZCwE6Y+ap<^pGS9%7C=HVyx9^xi&T2z5#dFm!*X|U9UDw2M{2A^Tlrw|PG6LvW77_jfpFK+Lie&2cc zFjK)yaL4Fb`brmN0`Y3zU?*uV4SUY{V!q*V;M}Kb=fWG>noi91jN-YJ@R8>I2xvFx zL4{XNW13ObAMtlWddW72cn8DqPSvb;f6Q99p#7=1#H)C0m2`k=y!7)tFLa-&oGuCb z;UjZdHuo;syki-|y45LhB_x>z`p!$YVp%59xv*AzppwfDi&}vY=$4d>imd^=F@w;Q z&WG%XG5juLd$*!_TSaAdLB3STtZzyQA~_wjP9)sbB^z|1UGs6k@u9kNHi4d=e`52o zw+nz3FFNAUCpt)C6|$zepr=b3nd2>cgy2MJ_&-CW|Ld1R;708Fl+Groq(j&^-XWEj zJrSCK$Qan3uSr1@-bNi6)1Ugo@*Cdz8bZrqD* zuF>KT^-b*cr}_ICr5x8bJ@2O_w8qz+NIt4m#ZHBUDWG+b3W{46&@>?ie{fy%=S&sy zeKikRf5KSGAYWHlIG|+{#K^70%LZd?C2_?k&>HB@us~PKTeDvh+k%v&uf;4S^eTik zSiPJXV>~7~!+V>Xo5_|&D%>%4-ygg=I@~)NI0c~QHh$}e=g*q1;!SP`ppDHJIg(E1 zHIx8oF`*AAr_tM`VRk-PIwh(J)L@$eZ!oiRU*@OlJ-!e47@7~y72&SIcAKv4T!l9A a?rq71d%aCREd9!HxAs3Zu#WsYDF6Ue&%mhw delta 2933 zcmV-*3ySp07p)h6K~_N^Q*L2!b7*gLAa*kf0|19?JG!#R=*W4%TlF+eK$^DjX_75(0O z`-caAdVv3a_s0i+{{6S!(?>+fkVmQ$a7l+DBafc0!6wt2kDX22rKu8+Ezf`qXXL1R za`5=HuL0P9O)L^c{E$jX_HGnQA{rhKKZ9H^2ixR!jgW-VKt$n;eDh65Z}}D zr$d%lh*28(ii-&Br@j+OAiY+Hbd!Y6UP_hlXuPd|S-fFN5-FxELzfN{m&8#zbto9v z?Fyhwry3C6%Mxs>Q9vc$NiZQ&7uu2)2(;fm9ClaEBHxRtnt;Ky5`c4(h$)xsk;UMJ)W^vya!altaFO;ave_noBXFuTVG>s74<$kw)v-1m01D>kkk_mXKY1r5uY5s-de4dzKyikK6 zA|kP?@eX+ya(Q1o$VD56JW3TSVd5`~2N{7SBkD7~D|Ho;==vELAuI}BXN$WEH^$Y3 zB__!nC-eH{C!gog29eD{?`cvNImancX^>#6fhjx;g$NmqD(nl*p`vKLud3XdG=8>} zA3OowMN0dOU$Gvx2f5`8@lHYPozwt-`yw&sSxWCPRlk`e<_U;V3f>K;o=(XX4O8$5 znqWs2Dexr$O~%%yG%i9$;P7O<(rakk_dE);RgvlrO)ho2K*+8bL$+COXK&tvYfC6i z&~=#iJ*wdShJ8PvQq3~V)W5{ERUC@LZf_a`9P-N=qVGit@3AW!LfX0{pejgz&0hw~n1a>TYxI#Y15wn6qx1jSWK28#b`&+o!7ij{|&+xg= zZ{YDcKJM}H1eJV&izBY-e5;We=H-p`I=2J1197mm>9NkdFbldRNCnqAcX>@Y*0dG2 zJM|?02XkVt)U`IXKJ!Gfjsxv~$|B`-UnOd`wXA7Fa(P(x&?HK;?$cDciY+75Mql_l zzR#7fwHLSu$6T)aRj7T*%B`!#G`*(>ouPSJjxm5CCNK=Mq;m$j>xJG~fuHXBEatLq zG{-VY%)_=Z&!5>-BNEmKFIZNfT<8Ix-UFjJ&HMXx(S5wNPW~4Rz^hmqRpGJHrBI^6xiu%=^{A8t*l&pNpZEi@0{Re99+228b4rw=LiJGAtx28kQBEzJxHcE{DK`gIG3stHHBvZ_5jz@h5 zb`qv?V|{(KN-%f*7&#WgE<1)X8vOkUUMo1Abpdo@0N&yI$|+|v0JQG*+5#dNJNb;P z84R*_4pzW_Vi;Rla^Qh0|d)R16b)Sx3^c@D;FYh)j{H z?htl+VCO`WbO$7VE^fK%Z`GP|q^i&w6qBaztN&km`{3K*p$OPlCij~~gn-Egx0Y=l ztC=&#WMb#LbmVlI4H!7;;=RRE(hx0>LW@Qe{ww8WxnIF=fnX)xUr(uw;&Y=6D zpbqm|s|74!qheS^r!H37vGuYS(y1liT$qO1G$&L_^tN<=JSo|DrH?zOEJ%G;dqy>7 zZH+u4pLFwtJ&Q;_WjFG=vVAhUpP(LmNjYAkLp(w6%fdhsCCCt}R5}c~-$pV5uiCDG zKBV6<;nNsEQ6}P$N4US9<4wKi?6xk}+$$;lIk6k@DGl@MHGS6xm64ilT@>>ViI?!Q zqe1d=YHBrq)=`I#L31{|0<%3=mRlu4lt3j1gc+GK$curYB*AaEuag^+aHyfLYDobK zyB9stwVd)T?Pd*E{0!=_Hp?k~vwIxkRhloRQx+++kIby$lmGtb-~ZJ6DBkr% z&?%o|>*{2rha@ql3`n~x##)`0Yg5DREx5bBEY%->N&N{;z%2Nk6t5ifC2030g_eU! zgQX@^k?aFD_zaUig_6X5J;6bu438w>l-Rgrsqx z@4R#?mS+;33v0y(DuwK@s1*o-Zb`|g*cz}KGYC!TY{-rn!|yV-cbhkFYhIaMkS`N5 z@0&7$NKOZ>6A5>9$p&3$*L)mse5mf6O`vCgr`UY#?K!}T7aj5FGaaOf3R%-!(9|Wse6=f0&W7cR96uESnlGO-%G3y_`@rsS7HDZ^u#B`-|`RSz!x* ziW3vX_}ctk*G=RuN%Ps>l34g1z07$rO_bG=-MAOsLZihW>YLc>PxJRPN;$4=dfv}U zXpOHsk$hCCik%7xQ$Xt=6%@C!0OiktHQrRgIAyV@zy&;UGkO9wSLyIUczmv-gZsJ4 zjj`72_fON1ec%$~peLnWm26qO&@>@`25?>T=S&syeKiYNf5KSCpjcN}IG|+{B*?AA z%LZd&C2_^ivg>X7ap~8CyS4uTRd-KSlPCZHmKKpe diff --git a/examples/napi/__test__/values.spec.ts b/examples/napi/__test__/values.spec.ts index fcab599d..6dad5eec 100644 --- a/examples/napi/__test__/values.spec.ts +++ b/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) => { diff --git a/examples/napi/index.d.ts b/examples/napi/index.d.ts index f4539e09..96b1ae39 100644 --- a/examples/napi/index.d.ts +++ b/examples/napi/index.d.ts @@ -269,6 +269,11 @@ export type CSSStyleSheet = CssStyleSheet export class CssStyleSheet { constructor(rules: Array) get rules(): CssRuleList + anotherCssStyleSheet(): AnotherCssStyleSheet +} +export type AnotherCSSStyleSheet = AnotherCssStyleSheet +export class AnotherCssStyleSheet { + get rules(): CssRuleList } export namespace xxh3 { export const ALIGNMENT: number diff --git a/examples/napi/src/reference.rs b/examples/napi/src/reference.rs index ace4f229..9c9f1ee1 100644 --- a/examples/napi/src/reference.rs +++ b/examples/napi/src/reference.rs @@ -76,26 +76,54 @@ impl CSSRuleList { #[napi] pub struct CSSStyleSheet { + inner: Rc>, + rules: Option>, +} + +#[napi] +pub struct AnotherCSSStyleSheet { inner: Rc>, rules: Reference, } #[napi] -impl CSSStyleSheet { - #[napi(constructor)] - pub fn new(env: Env, rules: Vec) -> Result { - let inner = Rc::new(RefCell::new(OwnedStyleSheet { rules })); - let rules = CSSRuleList::into_reference( - CSSRuleList { - owned: inner.clone(), - }, - env, - )?; - Ok(CSSStyleSheet { inner, rules }) - } - +impl AnotherCSSStyleSheet { #[napi(getter)] pub fn rules(&self, env: Env) -> Result> { self.rules.clone(env) } } + +#[napi] +impl CSSStyleSheet { + #[napi(constructor)] + pub fn new(rules: Vec) -> Result { + let inner = Rc::new(RefCell::new(OwnedStyleSheet { rules })); + Ok(CSSStyleSheet { inner, rules: None }) + } + + #[napi(getter)] + pub fn rules(&mut self, env: Env) -> Result> { + 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 { + Ok(AnotherCSSStyleSheet { + inner: self.inner.clone(), + rules: self.rules.as_ref().unwrap().clone(env)?, + }) + } +}