Skip to content

Commit f9e10bb

Browse files
Fix QuickJS NAPI weak-reference and ExternalCallback self-cycle leaks
Two correctness bugs in the QuickJS NAPI implementation caused objects to be pinned forever, contributing to the assert(list_empty(&rt->gc_obj_list)) failure in JS_FreeRuntime at teardown: 1. napi_create_reference always called JS_DupValue regardless of the initial refcount. Per Node-API semantics a reference created with count=0 is a *weak* reference that must not keep its value alive. The dup silently promoted it to strong, which meant every ObjectWrap<T> instance (which uses napi_wrap -> napi_create_reference with count=0 as a weak self-ref) was pinned forever, its FinalizeCallback never fired, and its C++ destructor never ran. Fixed by only dupping when count > 0, and paired the matching delete/ref/unref paths so: - napi_delete_reference only frees the dup when count > 0 - napi_reference_ref on 0->1 takes a dup (weak -> strong) - napi_reference_unref on 1->0 frees the dup (strong -> weak) 2. ExternalCallback::newTarget was stored as JS_DupValue(func) which created a self-cycle: func -> c_function_data -> opaque ExternalCallback -> newTarget -> func. Because NapiCallback class has gc_mark=nullptr the cycle collector cannot break this. Store the raw JSValue instead; it is only read during callback invocation when QuickJS itself holds func alive. Measured impact on UnitTests on Android arm64 (debug): - Leaked GC objects at teardown: 11147 -> 10751 (-396) - Leaked ObjectWrap instances: 151 -> 52 (-99) - Leaked NapiWrap prototypes: 151 -> 52 (-99) - 136 tests still pass (no regressions) The assert still fires (remaining leaks are dominated by bytecode closures transitively pinned via persistent polyfill singletons), but these two bugs are genuine leaks independent of the rest. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent fe26c61 commit f9e10bb

1 file changed

Lines changed: 45 additions & 11 deletions

File tree

Core/Node-API/Source/js_native_api_quickjs.cc

Lines changed: 45 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,8 @@ static JSValue Callback(JSContext *ctx, JSValueConst this_val, int argc, JSValue
176176
void* opaque = JS_GetOpaque(val, js_callback_class_id);
177177
ExternalCallback* externalCallback = reinterpret_cast<ExternalCallback*>(opaque);
178178
if (externalCallback != nullptr) {
179-
JS_FreeValueRT(rt, externalCallback->newTarget);
179+
// newTarget is NOT a strong reference (we do not dup when assigning),
180+
// so do NOT JS_FreeValueRT it here. See create_function_internal.
180181
delete externalCallback;
181182
}
182183
}
@@ -942,7 +943,14 @@ static napi_status create_function_internal(napi_env env, const char* utf8name,
942943
JS_FreeAtom(env->context, nameAtom);
943944
}
944945

945-
externalCallback->newTarget = JS_DupValue(env->context, func);
946+
// newTarget is set to the function itself so that constructor callbacks
947+
// can read `func.prototype`. We intentionally store it as a RAW JSValue
948+
// (no JS_DupValue) to avoid a self-cycle: `func -> func_data[callbackData]
949+
// -> opaque ExternalCallback -> newTarget -> func` (the NapiCallback class
950+
// has gc_mark=nullptr so the cycle collector cannot break this). This is
951+
// safe because `newTarget` is only read during ExternalCallback::Callback,
952+
// during which QuickJS is itself holding `func` alive.
953+
externalCallback->newTarget = func;
946954

947955
*result = FromJSValue(env, func);
948956
napi_clear_last_error(env);
@@ -1682,8 +1690,15 @@ napi_status napi_create_reference(napi_env env, napi_value value, uint32_t initi
16821690
CHECK_ARG(env, result);
16831691

16841692
JSValue jsValue = ToJSValue(value);
1685-
RefInfo* info = new RefInfo{ JS_DupValue(env->context, jsValue), initial_refcount };
1686-
1693+
// A reference with initial_refcount == 0 is a WEAK reference: it must not
1694+
// keep the JS value alive. Only the count > 0 (strong) case dups. Otherwise
1695+
// the reference self-pins the value (e.g. every ObjectWrap instance would
1696+
// leak forever because napi_wrap creates a weak self-reference).
1697+
JSValue stored = (initial_refcount == 0)
1698+
? jsValue
1699+
: JS_DupValue(env->context, jsValue);
1700+
RefInfo* info = new RefInfo{ stored, initial_refcount };
1701+
16871702
*result = reinterpret_cast<napi_ref>(info);
16881703
napi_clear_last_error(env);
16891704
return napi_ok;
@@ -1694,7 +1709,12 @@ napi_status napi_delete_reference(napi_env env, napi_ref ref) {
16941709
CHECK_ARG(env, ref);
16951710

16961711
RefInfo* info = reinterpret_cast<RefInfo*>(ref);
1697-
JS_FreeValue(env->context, info->value);
1712+
// Only a strong reference (count > 0) owns a dup that must be freed.
1713+
// A weak reference does not own the JS value; freeing it here would be a
1714+
// spurious decrement and could even touch an already-finalized value.
1715+
if (info->count > 0) {
1716+
JS_FreeValue(env->context, info->value);
1717+
}
16981718
delete info;
16991719

17001720
napi_clear_last_error(env);
@@ -1704,31 +1724,45 @@ napi_status napi_delete_reference(napi_env env, napi_ref ref) {
17041724
napi_status napi_reference_ref(napi_env env, napi_ref ref, uint32_t* result) {
17051725
CHECK_ENV(env);
17061726
CHECK_ARG(env, ref);
1707-
1727+
17081728
RefInfo* info = reinterpret_cast<RefInfo*>(ref);
1729+
// 0 -> 1 transition: promote to strong by taking a dup. The caller is
1730+
// responsible for ensuring the underlying value is still alive at this
1731+
// point (weak references do not keep the value alive).
1732+
if (info->count == 0) {
1733+
info->value = JS_DupValue(env->context, info->value);
1734+
}
17091735
info->count++;
1710-
1736+
17111737
if (result != nullptr) {
17121738
*result = info->count;
17131739
}
1714-
1740+
17151741
napi_clear_last_error(env);
17161742
return napi_ok;
17171743
}
17181744

17191745
napi_status napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) {
17201746
CHECK_ENV(env);
17211747
CHECK_ARG(env, ref);
1722-
1748+
17231749
RefInfo* info = reinterpret_cast<RefInfo*>(ref);
17241750
if (info->count > 0) {
17251751
info->count--;
1752+
// 1 -> 0 transition: demote to weak by releasing the dup we owned. The
1753+
// raw JSValue is retained (unowned) so that a subsequent ref can dup it
1754+
// again, but we no longer keep the value alive.
1755+
if (info->count == 0) {
1756+
JSValue v = info->value;
1757+
JS_FreeValue(env->context, v);
1758+
// Leave info->value as the (now unowned) raw tag/pointer; do not touch.
1759+
}
17261760
}
1727-
1761+
17281762
if (result != nullptr) {
17291763
*result = info->count;
17301764
}
1731-
1765+
17321766
napi_clear_last_error(env);
17331767
return napi_ok;
17341768
}

0 commit comments

Comments
 (0)