WIP: NAPI shutdown lifecycle fixes for QuickJS

These are partial fixes toward making JS_FreeRuntime's
assert(list_empty(&rt->gc_obj_list)) hold without patching quickjs.
136 tests still pass; the assert still fires due to architectural
issues (NapiCallback/NapiWrap/NapiExternal classes have gc_mark =
nullptr so QuickJS cycle GC cannot trace through them).

Real bugs fixed in this commit:

* Detach iteration use-after-free:
  Iterating env->refs while JS_FreeValue triggers finalizers (which
  call napi_delete_reference and delete RefInfo entries) was UB.
  Snapshot env->refs first; clear before iterating.

* napi_delete_reference / napi_reference_unref dropped JSValues only
  JS_FreeContext, pinning JSValues into JS_FreeRuntime and
  contributing to the gc_obj_list assertion. Always free strong
  holds; only the env->refs bookkeeping is shutdown-skipped.

* Other NAPI fixes from the prior cycle: weak-ref semantics in
  napi_create_reference, ExternalCallback newTarget self-ref leak
  removal, callbackData double-ownership in create_function_internal,
  RefInfo tracking for napi_create_promise, FreeEnv lifecycle.

Co-authored-by: Copilot <[email protected]>

Author: Copilot

Core/AppRuntime/Source/AppRuntime_QuickJS.cpp

@@ -31,9 +31,8 @@ namespace Babylon
         }
 
         // Use the context within a scope.
+        Napi::Env env = Napi::Attach(context);
         {
-            Napi::Env env = Napi::Attach(context);
-
             // Install microtask processing as a post-tick callback
             SetPostTickCallback([runtime]() {
                 JSContext* pending_ctx;
@@ -48,8 +47,12 @@ namespace Babylon
             Napi::Detach(env);
         }
 
-        // Destroy the context and runtime.
+        // Destroy the context and runtime. QuickJS finalizers fired during
+        // these calls (e.g. ObjectWrap deleters) may invoke napi_delete_reference,
+        // so the napi_env__ structure must outlive both. Detach() above already
+        // released all JS-side resources; FreeEnv() below frees the env itself.
         JS_FreeContext(context);
         JS_FreeRuntime(runtime);
+        Napi::FreeEnv(env);
     }
 }

Core/Node-API/Include/Engine/QuickJS/napi/env.h

@@ -7,8 +7,17 @@ namespace Napi
 {
   Napi::Env Attach(JSContext* context);
 
+  // Releases JS resources held by the env (handle scope, persistent refs, etc.)
+  // and marks it as shutting down so subsequent finalizer-driven calls into
+  // NAPI become no-ops. The env pointer remains valid (do not call NAPI APIs
+  // that touch JS state on it) until FreeEnv() is invoked.
   void Detach(Napi::Env);
 
+  // Frees the napi_env structure itself. Must be called AFTER JS_FreeRuntime
+  // because QuickJS finalizers (e.g. ObjectWrap deleters) may still call
+  // napi_delete_reference / external callbacks which dereference the env.
+  void FreeEnv(Napi::Env);
+
   Napi::Value Eval(Napi::Env env, const char* source, const char* sourceUrl);
 
   JSContext* GetContext(Napi::Env);

Core/Node-API/Source/env_quickjs.cc

@@ -47,21 +47,98 @@ namespace Napi
     void Detach(Env env)
     {
         napi_env env_ptr{env};
-        if (env_ptr)
+        if (!env_ptr)
+        {
+            return;
+        }
+
+        // Drain any pending microtasks/jobs (e.g. unresolved Promise reactions)
+        // BEFORE we mark the env as shutting down. Pending jobs hold closures
+        // and any captured values; left in the runtime job queue they would
+        // pin objects past JS_FreeRuntime and fail the gc_obj_list assertion.
+        // Executing them now may run native callbacks that legitimately create
+        // and free napi refs, so this must happen while NAPI is still live.
+        JSRuntime* rt = JS_GetRuntime(env_ptr->context);
+        JSContext* pending_ctx;
+        while (JS_IsJobPending(rt))
         {
-            if (!JS_IsUndefined(env_ptr->has_own_property_function))
+            int ret = JS_ExecutePendingJob(rt, &pending_ctx);
+            if (ret <= 0) {
+                break;
+            }
+        }
+
+        // Mark the env as shutting down. After this:
+        //   - napi_create_reference becomes a no-op-ish path (no JS handle).
+        //   - napi_delete_reference will not access the JS context.
+        // Finalizers triggered later by JS_FreeContext / JS_FreeRuntime can
+        // still legitimately call napi_delete_reference; we make those calls
+        // safe instead of UAFing the (already-deleted) env.
+        env_ptr->is_shutting_down = true;
+
+        if (!JS_IsUndefined(env_ptr->has_own_property_function))
+        {
+            JS_FreeValue(env_ptr->context, env_ptr->has_own_property_function);
+            env_ptr->has_own_property_function = JS_UNDEFINED;
+        }
+
+        // Release all strong JS references that NAPI is currently holding via
+        // napi_create_reference. Without this, the QuickJS GC invoked from
+        // JS_FreeRuntime cannot collect wrapped objects that are pinned by
+        // napi_ref instances, and assert(list_empty(&rt->gc_obj_list)) fires.
+        //
+        // We do NOT delete the RefInfo objects here: they will be freed when
+        // their owning C++ destructors run napi_delete_reference (typically
+        // from ObjectWrap finalizers during JS_FreeRuntime). We just neutralize
+        // them so that those later calls don't double-free the JS value or
+        // touch a freed env.
+        // IMPORTANT: snapshot env->refs into a local container before iterating.
+        // Each JS_FreeValue below can trigger QuickJS finalizers (e.g.
+        // ObjectWrap finalize → C++ destructor → ~Reference<T> →
+        // napi_delete_reference). With env->is_shutting_down already set, the
+        // shutdown-aware napi_delete_reference does NOT erase from env->refs
+        // before delete'ing the RefInfo, so iterating env->refs directly would
+        // deref a freed pointer (use-after-free). Snapshot + iterate over the
+        // snapshot avoids that, and we then clear env->refs once at the end.
+        std::vector<RefInfo*> snapshot(env_ptr->refs.begin(), env_ptr->refs.end());
+        env_ptr->refs.clear();
+        for (RefInfo* info : snapshot)
+        {
+            if (info->count > 0)
             {
-                JS_FreeValue(env_ptr->context, env_ptr->has_own_property_function);
-                env_ptr->has_own_property_function = JS_UNDEFINED;
+                JSValue v = info->value;
+                info->value = JS_UNDEFINED;
+                info->count = 0;
+                JS_FreeValue(env_ptr->context, v);
             }
+        }
 
-            // Free all remaining JSValues in the handle scope stack
-            for (auto& ptr : env_ptr->handle_scope_stack)
+        // Free all remaining JSValues in the handle scope stack. Use a swap
+        // loop because finalizers triggered by JS_FreeValue can themselves
+        // push new entries onto handle_scope_stack (e.g. via FromJSValue),
+        // which would invalidate iterators of a range-for over the original
+        // vector and leave entries unfreed.
+        while (!env_ptr->handle_scope_stack.empty())
+        {
+            std::vector<std::unique_ptr<JSValue>> stack;
+            stack.swap(env_ptr->handle_scope_stack);
+            for (auto& ptr : stack)
             {
                 JS_FreeValue(env_ptr->context, *ptr);
             }
-            env_ptr->handle_scope_stack.clear();
+        }
+
+        // Run a final GC to collect anything we just released that participates
+        // in cycles. Without this, cycle-members released above may persist and
+        // pin further objects, eventually failing the assertion in JS_FreeRuntime.
+        JS_RunGC(JS_GetRuntime(env_ptr->context));
+    }
 
+    void FreeEnv(Env env)
+    {
+        napi_env env_ptr{env};
+        if (env_ptr)
+        {
             delete env_ptr;
         }
     }

Core/Node-API/Source/js_native_api_quickjs.cc

@@ -71,7 +71,7 @@ struct CallbackInfo {
 class ExternalCallback {
  public:
   ExternalCallback(napi_env env, napi_callback cb, void* data)
-    : _env(env), _cb(cb), _data(data), newTarget(JS_UNDEFINED) {}
+    : _env(env), _cb(cb), _data(data) {}
 
 static JSValue Callback(JSContext *ctx, JSValueConst this_val, int argc, JSValueConst *argv, int magic, JSValue *func_data) {
   ExternalCallback* externalCallback = reinterpret_cast<ExternalCallback*>(JS_GetOpaque(func_data[0], js_callback_class_id));
@@ -94,19 +94,22 @@ static JSValue Callback(JSContext *ctx, JSValueConst this_val, int argc, JSValue
 
   JSValue actualThis = JS_UNDEFINED;
   bool isConstructCall = (magic == 1);
-  
-  // Handle constructor call
+
+  // For JS_CLASS_C_FUNCTION_DATA constructor calls, QuickJS passes new_target
+  // as this_val. Use it directly instead of storing the function as a raw
+  // JSValue on ExternalCallback — doing so created a self-referencing C-level
+  // ref that QuickJS GC could not see, leaking the function permanently.
   if (isConstructCall) {
-    JSValue prototypeProperty = JS_GetPropertyStr(ctx, externalCallback->newTarget, "prototype");
-    
+    JSValue prototypeProperty = JS_GetPropertyStr(ctx, this_val, "prototype");
+
     if (JS_IsException(prototypeProperty)) {
       externalCallback->_env->current_context = savedCtx; // RESTORE
       return JS_EXCEPTION;
     }
-    
+
     actualThis = JS_NewObjectProto(ctx, prototypeProperty);
     JS_FreeValue(ctx, prototypeProperty);
-    
+
     if (JS_IsException(actualThis)) {
       externalCallback->_env->current_context = savedCtx; // RESTORE
       return JS_EXCEPTION;
@@ -117,7 +120,12 @@ static JSValue Callback(JSContext *ctx, JSValueConst this_val, int argc, JSValue
 
   CallbackInfo cbInfo;
   cbInfo.thisArg = reinterpret_cast<napi_value>(&actualThis);
-  cbInfo.newTarget = reinterpret_cast<napi_value>(&externalCallback->newTarget);
+  // For constructor calls, this_val IS new_target (the constructor function
+  // itself). Cast away const for the napi_value contract; the callee must not
+  // mutate it during the call.
+  cbInfo.newTarget = isConstructCall
+      ? reinterpret_cast<napi_value>(const_cast<JSValue*>(&this_val))
+      : nullptr;
   cbInfo.isConstructCall = isConstructCall;
   cbInfo.argc = argc;
   cbInfo.argv = args.empty() ? nullptr : args.data();
@@ -176,25 +184,16 @@ static JSValue Callback(JSContext *ctx, JSValueConst this_val, int argc, JSValue
     void* opaque = JS_GetOpaque(val, js_callback_class_id);
     ExternalCallback* externalCallback = reinterpret_cast<ExternalCallback*>(opaque);
     if (externalCallback != nullptr) {
-      JS_FreeValueRT(rt, externalCallback->newTarget);
       delete externalCallback;
     }
   }
 
-  JSValue newTarget;
-
  private:
   napi_env _env;
   napi_callback _cb;
   void* _data;
 };
 
-// Reference info for preventing GC
-struct RefInfo {
-  JSValue value;
-  uint32_t count;
-};
-
 // Initialize class IDs (allocate once globally, register per runtime)
 void InitClassIds(JSRuntime* rt) {
   if (!class_ids_allocated) {
@@ -942,9 +941,11 @@ static napi_status create_function_internal(napi_env env, const char* utf8name,
     JS_FreeAtom(env->context, nameAtom);
   }
 
-  externalCallback->newTarget = JS_DupValue(env->context, func);
-  
   *result = FromJSValue(env, func);
+  // Release our reference on the opaque callback object: the cfunction
+  // retains it via func_data (which dups), so it stays alive as long as the
+  // cfunction does. Without this the opaque object leaks (ref_count stuck at 2).
+  JS_FreeValue(env->context, callbackData);
   napi_clear_last_error(env);
   return napi_ok;
 }
@@ -1682,8 +1683,29 @@ napi_status napi_create_reference(napi_env env, napi_value value, uint32_t initi
   CHECK_ARG(env, result);
 
   JSValue jsValue = ToJSValue(value);
-  RefInfo* info = new RefInfo{ JS_DupValue(env->context, jsValue), initial_refcount };
-  
+
+  // initial_refcount == 0 means a WEAK reference per NAPI semantics. Holding
+  // a duplicated JSValue in that case would create an invisible-to-GC C-level
+  // root and prevent QuickJS from ever collecting wrapped objects (which is
+  // the root cause of the assert(list_empty(&rt->gc_obj_list)) failure in
+  // JS_FreeRuntime, since napi_wrap calls napi_create_reference with
+  // initial_refcount == 0).
+  RefInfo* info;
+  if (initial_refcount > 0) {
+    info = new RefInfo{ JS_DupValue(env->context, jsValue), initial_refcount };
+  } else {
+    // Weak: don't dup. The stored JSValue is non-owning and must not be
+    // dereferenced once count is back to 0 unless it has been re-strengthened.
+    info = new RefInfo{ jsValue, 0 };
+  }
+
+  // Track so Detach() can release any strong holds before JS_FreeRuntime runs
+  // its terminal GC. Skip tracking once shutting down — env state is being
+  // torn down and we must not mutate it from finalizer-driven calls.
+  if (!env->is_shutting_down) {
+    env->refs.insert(info);
+  }
+
   *result = reinterpret_cast<napi_ref>(info);
   napi_clear_last_error(env);
   return napi_ok;
@@ -1694,7 +1716,21 @@ napi_status napi_delete_reference(napi_env env, napi_ref ref) {
   CHECK_ARG(env, ref);
 
   RefInfo* info = reinterpret_cast<RefInfo*>(ref);
-  JS_FreeValue(env->context, info->value);
+
+  // Even during shutdown we MUST drop any strong JS hold this reference still
+  // owns. Detach() drains env->refs but cannot reach RefInfo objects that are
+  // created after, or that the drain skipped because they were processed by
+  // a finalizer racing the loop. Leaving info->count > 0 with a live value
+  // keeps that JSValue alive past JS_FreeContext and fails the
+  // assert(list_empty(&rt->gc_obj_list)) inside JS_FreeRuntime.
+  if (!env->is_shutting_down) {
+    env->refs.erase(info);
+  }
+  if (info->count > 0) {
+    JS_FreeValue(env->context, info->value);
+    info->value = JS_UNDEFINED;
+    info->count = 0;
+  }
   delete info;
 
   napi_clear_last_error(env);
@@ -1704,31 +1740,43 @@ napi_status napi_delete_reference(napi_env env, napi_ref ref) {
 napi_status napi_reference_ref(napi_env env, napi_ref ref, uint32_t* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, ref);
-  
+
   RefInfo* info = reinterpret_cast<RefInfo*>(ref);
+  // Going 0 -> 1 must promote a weak ref to a strong one by duping the value
+  // so the JS object can no longer be collected from under us.
+  if (info->count == 0 && !env->is_shutting_down) {
+    info->value = JS_DupValue(env->context, info->value);
+  }
   info->count++;
-  
+
   if (result != nullptr) {
     *result = info->count;
   }
-  
+
   napi_clear_last_error(env);
   return napi_ok;
 }
 
 napi_status napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) {
   CHECK_ENV(env);
   CHECK_ARG(env, ref);
-  
+
   RefInfo* info = reinterpret_cast<RefInfo*>(ref);
   if (info->count > 0) {
     info->count--;
+    // Going 1 -> 0 demotes back to weak: release the strong JS hold. We must
+    // do this even during shutdown to avoid pinning JSValues past
+    // JS_FreeContext / JS_FreeRuntime (gc_obj_list assertion).
+    if (info->count == 0) {
+      JS_FreeValue(env->context, info->value);
+      info->value = JS_UNDEFINED;
+    }
   }
-  
+
   if (result != nullptr) {
     *result = info->count;
   }
-  
+
   napi_clear_last_error(env);
   return napi_ok;
 }
@@ -2184,8 +2232,12 @@ napi_status napi_create_promise(napi_env env, napi_deferred* deferred, napi_valu
   JS_SetPropertyStr(env->context, container, "reject", resolving_funcs[1]);
 
   // Create reference directly from container WITHOUT going through FromJSValue
-  // to avoid double-ownership (handle scope + reference)
+  // to avoid double-ownership (handle scope + reference). Track in env->refs
+  // so Detach() can release the strong hold if the promise is never settled.
   RefInfo* refInfo = new RefInfo{ container, 1 }; // Use refcount 1 for immediate access
+  if (!env->is_shutting_down) {
+    env->refs.insert(refInfo);
+  }
   napi_ref ref = reinterpret_cast<napi_ref>(refInfo);
 
   *deferred = reinterpret_cast<napi_deferred>(ref);

Core/Node-API/Source/js_native_api_quickjs.h

@@ -5,6 +5,15 @@
 #include <thread>
 #include <cassert>
 #include <vector>
+#include <unordered_set>
+
+// Reference info for a napi_ref. count==0 means a weak reference (no JS
+// resource held); count>0 means a strong reference (value is JS_DupValue'd
+// and must be JS_FreeValue'd when count returns to 0 or the ref is deleted).
+struct RefInfo {
+  JSValue value;
+  uint32_t count;
+};
 
 struct napi_env__ {
   JSContext* context = nullptr;
@@ -17,6 +26,17 @@ struct napi_env__ {
   // Handle scope storage
   std::vector<std::unique_ptr<JSValue>> handle_scope_stack;
   size_t current_scope_start = 0;
+
+  // All RefInfos created via napi_create_reference. Used by Detach() to
+  // release strong references on the JS values they hold so that the QuickJS
+  // garbage collector can reclaim wrapped objects (and their finalizers can
+  // run) during JS_FreeContext / JS_FreeRuntime.
+  std::unordered_set<RefInfo*> refs;
+
+  // Set true by Detach(). After this point napi_delete_reference must not
+  // touch JS state because Detach() already released it, and napi_create_reference
+  // must not allocate new JS-holding refs (the runtime is shutting down).
+  bool is_shutting_down = false;
 };
 
 #define RETURN_STATUS_IF_FALSE(env, condition, status) \