quickjs napi: track napi_refs in env and release at Detach

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>

Author: CedricGuillemet

Core/Node-API/Source/env_quickjs.cc

@@ -49,6 +49,32 @@ namespace Napi
         napi_env env_ptr{env};
         if (env_ptr)
         {
+            // Release every strong napi_ref still outstanding. This mirrors
+            // the V8 impl (napi_env__::DeleteMe) and is essential on QuickJS:
+            // any surviving strong ref pins a JS value from outside the GC
+            // graph, which prevents the teardown cascade in JS_FreeContext
+            // from running and triggers list_empty(gc_obj_list) assert in
+            // JS_FreeRuntime.
+            //
+            // We only release the JS side here. The RefInfo allocations
+            // themselves are owned by their C++ holders (e.g. an
+            // Napi::FunctionReference embedded in a polyfill object) and
+            // will be freed later when those holders are destroyed. We
+            // zero-out count/value so the subsequent napi_delete_reference
+            // is a no-op and does not touch the already freed context.
+            for (void* p : env_ptr->refs_list)
+            {
+                auto* info = reinterpret_cast<RefInfo*>(p);
+                if (info->count > 0)
+                {
+                    JS_FreeValue(env_ptr->context, info->value);
+                }
+                info->count = 0;
+                info->value = JS_UNDEFINED;
+            }
+            env_ptr->refs_list.clear();
+            env_ptr->detached = true;
+
             if (!JS_IsUndefined(env_ptr->has_own_property_function))
             {
                 JS_FreeValue(env_ptr->context, env_ptr->has_own_property_function);
@@ -62,7 +88,23 @@ namespace Napi
             }
             env_ptr->handle_scope_stack.clear();
 
-            delete env_ptr;
+            // Run the cycle collector so napi_wrap finalizers (which
+            // destroy C++ wrapper objects and release any embedded
+            // napi_refs) get a chance to execute while the env is still
+            // valid. A second pass picks up anything unpinned by the
+            // first pass's finalizers.
+            JSRuntime* rt = JS_GetRuntime(env_ptr->context);
+            JS_RunGC(rt);
+            JS_RunGC(rt);
+
+            // NOTE: we intentionally do NOT `delete env_ptr` here. Some
+            // native objects (e.g. ObjectWrap instances) have their
+            // destructors run as a side effect of JS_FreeContext's
+            // teardown cascade (via napi_wrap finalizers) which happens
+            // *after* Detach returns. Those destructors reach
+            // napi_delete_reference on an env pointer that must remain
+            // valid. The env is leaked, but this is bounded (one per
+            // AppRuntime environment tier).
         }
     }
 

Core/Node-API/Source/js_native_api_quickjs.cc

@@ -176,12 +176,23 @@ 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) {
-      // newTarget is NOT a strong reference (we do not dup when assigning),
-      // so do NOT JS_FreeValueRT it here. See create_function_internal.
+      JS_FreeValueRT(rt, externalCallback->newTarget);
       delete externalCallback;
     }
   }
 
+  // Expose newTarget (a strong JSValue held by this class) to QuickJS's cycle
+  // collector. This is what allows the cycle
+  //   func(cid=15) -> func_data[callbackData(cid=66)] -> opaque -> newTarget -> func
+  // to be detected and broken at teardown.
+  static void GcMark(JSRuntime *rt, JSValueConst val, JS_MarkFunc *mark_func) {
+    void* opaque = JS_GetOpaque(val, js_callback_class_id);
+    ExternalCallback* externalCallback = reinterpret_cast<ExternalCallback*>(opaque);
+    if (externalCallback != nullptr) {
+      JS_MarkValue(rt, externalCallback->newTarget, mark_func);
+    }
+  }
+
   JSValue newTarget;
 
  private:
@@ -190,11 +201,7 @@ static JSValue Callback(JSContext *ctx, JSValueConst this_val, int argc, JSValue
   void* _data;
 };
 
-// Reference info for preventing GC
-struct RefInfo {
-  JSValue value;
-  uint32_t count;
-};
+// Reference info for preventing GC - defined in js_native_api_quickjs.h.
 
 // Initialize class IDs (allocate once globally, register per runtime)
 void InitClassIds(JSRuntime* rt) {
@@ -218,7 +225,7 @@ void InitClassIds(JSRuntime* rt) {
   JSClassDef callback_class_def = {
     "NapiCallback",
     ExternalCallback::Finalize,
-    nullptr,
+    ExternalCallback::GcMark,
     nullptr,
     nullptr
   };
@@ -943,14 +950,15 @@ static napi_status create_function_internal(napi_env env, const char* utf8name,
     JS_FreeAtom(env->context, nameAtom);
   }
 
-  // newTarget is set to the function itself so that constructor callbacks
-  // can read `func.prototype`. We intentionally store it as a RAW JSValue
-  // (no JS_DupValue) to avoid a self-cycle: `func -> func_data[callbackData]
-  // -> opaque ExternalCallback -> newTarget -> func` (the NapiCallback class
-  // has gc_mark=nullptr so the cycle collector cannot break this). This is
-  // safe because `newTarget` is only read during ExternalCallback::Callback,
-  // during which QuickJS is itself holding `func` alive.
-  externalCallback->newTarget = func;
+  // Release our local ref on callbackData. JS_NewCFunctionData has already
+  // dup'd it into func's data array, so it stays alive as long as func does.
+  JS_FreeValue(env->context, callbackData);
+
+  // newTarget is a strong dup of the function. This creates a cycle
+  //   func -> func_data[callbackData] -> opaque ExternalCallback -> newTarget -> func
+  // which is safe because NapiCallback has a gc_mark (see ExternalCallback::GcMark)
+  // that allows QuickJS's cycle collector to detect and break it.
+  externalCallback->newTarget = JS_DupValue(env->context, func);
 
   *result = FromJSValue(env, func);
   napi_clear_last_error(env);
@@ -1699,6 +1707,9 @@ napi_status napi_create_reference(napi_env env, napi_value value, uint32_t initi
                        : JS_DupValue(env->context, jsValue);
   RefInfo* info = new RefInfo{ stored, initial_refcount };
 
+  // Track for env-scoped cleanup (see Napi::Detach).
+  env->refs_list.push_back(info);
+
   *result = reinterpret_cast<napi_ref>(info);
   napi_clear_last_error(env);
   return napi_ok;
@@ -1709,12 +1720,26 @@ napi_status napi_delete_reference(napi_env env, napi_ref ref) {
   CHECK_ARG(env, ref);
 
   RefInfo* info = reinterpret_cast<RefInfo*>(ref);
+  if (env->detached) {
+    // Detach already released the JS side and cleared the tracking
+    // list; all we have to do is free the RefInfo allocation itself.
+    delete info;
+    return napi_ok;
+  }
   // Only a strong reference (count > 0) owns a dup that must be freed.
   // A weak reference does not own the JS value; freeing it here would be a
   // spurious decrement and could even touch an already-finalized value.
   if (info->count > 0) {
     JS_FreeValue(env->context, info->value);
   }
+  // Remove from env tracking.
+  auto& list = env->refs_list;
+  for (auto it = list.begin(); it != list.end(); ++it) {
+    if (*it == info) {
+      list.erase(it);
+      break;
+    }
+  }
   delete info;
 
   napi_clear_last_error(env);

Core/Node-API/Source/js_native_api_quickjs.h

@@ -6,6 +6,13 @@
 #include <cassert>
 #include <vector>
 
+// Reference info for preventing GC. Defined in the header so that both
+// the NAPI implementation and env teardown (env_quickjs.cc) can touch it.
+struct RefInfo {
+  JSValue value;
+  uint32_t count;
+};
+
 struct napi_env__ {
   JSContext* context = nullptr;
   JSContext* current_context = nullptr;
@@ -17,6 +24,20 @@ struct napi_env__ {
   // Handle scope storage
   std::vector<std::unique_ptr<JSValue>> handle_scope_stack;
   size_t current_scope_start = 0;
+
+  // Tracks every RefInfo* created by napi_create_reference so that
+  // pending strong references can be released during Detach. Without
+  // this, any napi_ref held by a native object (e.g. a polyfill's
+  // Napi::FunctionReference) pins JS values, preventing QuickJS from
+  // freeing them during teardown and causing an assertion failure in
+  // JS_FreeRuntime.
+  std::vector<void*> refs_list;
+
+  // Set to true once Detach has run. Subsequent napi_delete_reference
+  // calls (from native destructors running during the JS teardown
+  // cascade) must not touch the context or the (already emptied)
+  // refs_list.
+  bool detached = false;
 };
 
 #define RETURN_STATUS_IF_FALSE(env, condition, status) \