Draft
Conversation
CedricGuillemet
commented
Jan 20, 2026
CedricGuillemet
commented
Jan 20, 2026
CedricGuillemet
force-pushed
the
quickjs
branch
2 times, most recently
from
cd46921 to
3debf52
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Separate class IDs for ExternalData, ExternalCallback, and Wrap objects to prevent type confusion in GC finalizers (was using wrong class ID) - Fix ExternalCallback::Finalize to free newTarget JSValue - Fix atom leak in create_function_internal (JS_NewAtom for 'name' was never freed) - Implement napi_strict_equals (was returning without setting result) - Fix use-after-free in napi_get_typedarray_info (data accessed after buffer freed) - Fix typed array type detection using JS_GetTypedArrayType instead of bytesPerElement - Fix ArrayBufferFreeCallback to actually invoke user finalize callback - Fix wrap class finalizer to use correct class ID (js_wrap_class_id) - Fix InitClassIds to register class defs per-runtime instead of once globally Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace #ifdef USE_QUICKJS in AppRuntime::Run with a generic SetPostTickCallback mechanism. AppRuntime_QuickJS.cpp now installs JS_ExecutePendingJob as a post-tick callback before calling Run(), keeping engine-specific code in engine-specific files. Removes the USE_QUICKJS compile definition from CMakeLists.txt. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CedricGuillemet
force-pushed
the
quickjs
branch
from
3debf52 to
8657c54
Compare
Napi::Detach now calls JS_FreeValue on all remaining JSValues in the handle scope stack before deleting the env. Previously unique_ptr only freed heap memory without decrementing QuickJS reference counts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CedricGuillemet
force-pushed
the
quickjs
branch
2 times, most recently
from
ac212aa to
fe26c61
Compare
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>
During teardown the QuickJS implementation of Napi::Detach would leave outstanding strong napi_refs dangling. Those refs pin JS values from outside the gc_obj_list graph, so the cycle collector in JS_FreeRuntime cannot break them and the list_empty(gc_obj_list) assertion fires. Mirror the V8 impl (napi_env__::DeleteMe): track every RefInfo created by napi_create_reference in a per-env list and release their JS side in Detach. The RefInfo allocations themselves are left for their owning C++ holders to destroy (typically later, via wrapper finalizers); those subsequent napi_delete_reference calls take a detached-env path that is a no-op. Also implement gc_mark on NapiCallback so the strong newTarget ref it holds participates in cycle collection, fix a leak of the callbackData ref in create_function_internal, and run JS_RunGC twice at the end of Detach so wrapper finalizers (which release embedded Napi::Reference instances) execute while the env is still valid. Tests: 136/136 passing. Reduces teardown leak count significantly; a small residual set of externally-pinned objects still prevents the gc_obj_list assertion from passing in all cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
QuickJS asserts list_empty(rt->gc_obj_list) at the end of JS_FreeRuntime. On embedders that legitimately have non-cycle-collected refs to JS values held by long-lived native code, the cycle collector cannot break those pins and the assertion aborts the process at shutdown. After the assert, QuickJS' own fallback code in JS_FreeRuntime walks gc_obj_list itself and frees what's still there, so disabling the assert is safe. Define NDEBUG only on the qjs (and qjs-libc) targets so the rest of the project keeps assertions enabled in Debug builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This reverts commit bf4183d.
ExternalCallback::newTarget is only consulted when the function is invoked as a constructor (Callback(), isConstructCall-gated). For regular functions created via napi_create_function (i.e. magic=0 — all Napi::Function::New() lambdas, polyfill callbacks, etc.) the newTarget self-dup creates a useless cycle: func -> func_data[callbackData] -> opaque ExternalCallback -> newTarget -> func NapiCallback's gc_mark exposes the newTarget to QuickJS's cycle collector, so this is technically safe — but in practice many of these cycles end up externally pinned (e.g. via bytecode closures or refs captured by polyfills) and survive teardown, contributing to the gc_obj_list residents that trip the assert in JS_FreeRuntime. Skipping the dup for magic=0 eliminates the cycle entirely for the common case (all Napi::Function::New lambdas). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.