Quickjs#132
Open
CedricGuillemet wants to merge 30 commits into
Open
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>
Brings in: * Guard base-n target against parent-project collision (BabylonJS#184) * Add File / FileReader polyfill (BabylonJS#169) * Stop mutating instance [[Prototype]] chain in napi_wrap on Chakra (BabylonJS#179) * Fix .prototype pollution on JSC (BabylonJS#177) * Add TextEncoder polyfill (BabylonJS#171) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
quickjs-ng's quickjs.h has 'v.u.short_big_int = d' where d is int64_t and short_big_int is int32_t. Apple Clang's -Wshorten-64-to-32 is default-on; combined with BabylonNative's -Werror it fails the build on macOS/iOS. The pattern was already handled with #pragma warning(disable:4244) for MSVC -- mirror it with #pragma clang diagnostic ignored -Wshorten-64-to-32 in the 4 files that include <quickjs.h>. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# Conflicts: # .github/workflows/ci.yml # CMakeLists.txt
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a QuickJS JavaScript engine backend for JsRuntimeHost, wiring it through the Node-API layer and runtime bootstrapping, plus a few polyfill adjustments to better match QuickJS GC/host behavior.
Changes:
- Added a QuickJS Node-API implementation (env attach/detach +
napi_*API surface). - Integrated QuickJS runtime creation and microtask draining into
AppRuntime. - Updated WebSocket/Performance polyfills and expanded CI/build configuration to cover QuickJS.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Polyfills/WebSocket/Source/WebSocket.h | Adds a strong self-reference member to keep the wrapper alive across async callbacks. |
| Polyfills/WebSocket/Source/WebSocket.cpp | Keeps the JS wrapper alive for the connection lifetime; releases it on close. |
| Polyfills/Performance/Source/Performance.cpp | Avoids redefining performance when already present (e.g., engine-provided). |
| Core/Node-API/Source/js_native_api_quickjs.h | Introduces QuickJS napi_env__ definition and shared utilities. |
| Core/Node-API/Source/js_native_api_quickjs.cc | Adds a QuickJS-backed napi_* implementation. |
| Core/Node-API/Source/env_quickjs.cc | Implements QuickJS Napi::Attach/Detach and teardown cleanup behavior. |
| Core/Node-API/Include/Engine/QuickJS/napi/env.h | Declares the QuickJS engine Napi::Env entry points. |
| Core/Node-API/CMakeLists.txt | Wires QuickJS sources into the Node-API build. |
| Core/AppRuntime/Source/AppRuntime.cpp | Adds a post-dispatcher-tick callback hook. |
| Core/AppRuntime/Source/AppRuntime_QuickJS.cpp | Implements QuickJS runtime/context creation and microtask/job pumping. |
| Core/AppRuntime/Include/Babylon/AppRuntime.h | Exposes the post-tick callback setter. |
| Core/AppRuntime/CMakeLists.txt | Links QuickJS runtime dependency for the QuickJS build. |
| CMakeLists.txt | Fetches/builds quickjs-ng when QuickJS is the selected engine. |
| .github/workflows/ci.yml | Adds QuickJS CI jobs across Win32/Ubuntu/Android. |
| .github/workflows/build-linux.yml | Adds a js-engine input and forwards it to CMake. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ryantrem
reviewed
Jul 2, 2026
| , m_url(info[0].As<Napi::String>()) | ||
| , m_cancellationSource{std::make_shared<arcana::cancellation_source>()} | ||
| { | ||
| // Keep the JS wrapper alive for the lifetime of the connection. The |
Member
There was a problem hiding this comment.
Is this how it behaves in the browser? Why is this only just now needed, and not in the past?
Collaborator
Author
There was a problem hiding this comment.
QuickJS is deterministic with ref counting: it's freed immediately when it reaches 0. Other engines with GC gave it more time to finish its async work.
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
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.