src: convert context_frame field in AsyncWrap to internal field

Using an internal field instead of a `v8::Global<>` removes
an unnecessary memory leak footgun.

This includes a test that demonstrates the issue, albeit
using internal APIs.

It's worth noting that if this PR is not accepted, we'd still
be missing memory tracking for the `context_frame_` field,
and we'd need to add it through our memory tracking API.

PR-URL: #62103
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Author: addaleax

src/async_wrap-inl.h

@@ -50,7 +50,25 @@ inline double AsyncWrap::get_trigger_async_id() const {
 }
 
 inline v8::Local<v8::Value> AsyncWrap::context_frame() const {
-  return context_frame_.Get(env()->isolate());
+  auto as_data = object()->GetInternalField(kAsyncContextFrame);
+  DCHECK_IMPLIES(!as_data.IsEmpty(),
+                 as_data->IsValue() || as_data->IsPrivate());
+  if (as_data->IsPrivate()) {
+    DCHECK(as_data.As<v8::Private>()->Name()->SameValue(
+        env()->empty_context_frame_sentinel_symbol()->Name()));
+    return {};
+  }
+  return as_data.As<v8::Value>();
+}
+
+inline void AsyncWrap::set_context_frame(v8::Local<v8::Value> value) {
+  if (value.IsEmpty()) {
+    // Empty values are not allowed in internal fields
+    object()->SetInternalField(kAsyncContextFrame,
+                               env()->empty_context_frame_sentinel_symbol());
+  } else {
+    object()->SetInternalField(kAsyncContextFrame, value);
+  }
 }
 
 inline v8::MaybeLocal<v8::Value> AsyncWrap::MakeCallback(

src/async_wrap.cc

@@ -348,7 +348,7 @@ void AsyncWrap::EmitDestroy(bool from_gc) {
       HandleScope handle_scope(env()->isolate());
       USE(object()->Set(env()->context(), env()->resource_symbol(), object()));
     }
-    context_frame_.Reset();
+    set_context_frame({});
   }
 }
 
@@ -530,9 +530,9 @@ AsyncWrap::AsyncWrap(Environment* env,
 }
 
 AsyncWrap::AsyncWrap(Environment* env, Local<Object> object)
-    : BaseObject(env, object),
-      context_frame_(env->isolate(),
-                     async_context_frame::current(env->isolate())) {}
+    : BaseObject(env, object) {
+  set_context_frame(async_context_frame::current(env->isolate()));
+}
 
 // This method is necessary to work around one specific problem:
 // Before the init() hook runs, if there is one, the BaseObject() constructor
@@ -635,7 +635,7 @@ void AsyncWrap::AsyncReset(Local<Object> resource, double execution_async_id) {
 
   EmitTraceAsyncStart();
 
-  context_frame_.Reset(isolate, async_context_frame::current(isolate));
+  set_context_frame(async_context_frame::current(isolate));
 
   EmitAsyncInit(env(), resource,
                 env()->async_hooks()->provider_string(provider_type()),
@@ -678,7 +678,7 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
   EmitTraceEventBefore();
 
 #ifdef DEBUG
-  if (context_frame_.IsEmpty()) {
+  if (context_frame().IsEmpty()) {
     ProcessEmitWarning(env(),
                        "MakeCallback() called without context_frame, "
                        "likely use after destroy of AsyncWrap.");
@@ -687,15 +687,8 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
 
   ProviderType provider = provider_type();
   async_context context { get_async_id(), get_trigger_async_id() };
-  MaybeLocal<Value> ret =
-      InternalMakeCallback(env(),
-                           object(),
-                           object(),
-                           cb,
-                           argc,
-                           argv,
-                           context,
-                           context_frame_.Get(env()->isolate()));
+  MaybeLocal<Value> ret = InternalMakeCallback(
+      env(), object(), object(), cb, argc, argv, context, context_frame());
 
   // This is a static call with cached values because the `this` object may
   // no longer be alive at this point.

src/async_wrap.h

@@ -119,7 +119,8 @@ class ExternalReferenceRegistry;
 class AsyncWrap : public BaseObject {
  public:
   enum InternalFields {
-    kInternalFieldCount = BaseObject::kInternalFieldCount,
+    kAsyncContextFrame = BaseObject::kInternalFieldCount,
+    kInternalFieldCount,
   };
 
   enum ProviderType {
@@ -201,6 +202,7 @@ class AsyncWrap : public BaseObject {
   inline double get_trigger_async_id() const;
 
   inline v8::Local<v8::Value> context_frame() const;
+  inline void set_context_frame(v8::Local<v8::Value> value);
 
   void AsyncReset(v8::Local<v8::Object> resource,
                   double execution_async_id = kInvalidAsyncId);
@@ -244,8 +246,6 @@ class AsyncWrap : public BaseObject {
   // Because the values may be Reset(), cannot be made const.
   double async_id_ = kInvalidAsyncId;
   double trigger_async_id_ = kInvalidAsyncId;
-
-  v8::Global<v8::Value> context_frame_;
 };
 
 }  // namespace node

src/env_properties.h

@@ -21,6 +21,7 @@
   V(arrow_message_private_symbol, "node:arrowMessage")                         \
   V(contextify_context_private_symbol, "node:contextify:context")              \
   V(decorated_private_symbol, "node:decorated")                                \
+  V(empty_context_frame_sentinel_symbol, "node:empty_context_frame_sentinel")  \
   V(transfer_mode_private_symbol, "node:transfer_mode")                        \
   V(host_defined_option_symbol, "node:host_defined_option_symbol")             \
   V(js_transferable_wrapper_private_symbol, "node:js_transferable_wrapper")    \

test/parallel/test-async-local-storage-weak-asyncwrap-leak.js

@@ -0,0 +1,49 @@
+// Flags: --expose-gc
+'use strict';
+const common = require('../common');
+const assert = require('node:assert');
+const zlib = require('node:zlib');
+const v8 = require('node:v8');
+const { AsyncLocalStorage } = require('node:async_hooks');
+
+// This test verifies that referencing an AsyncLocalStorage store from
+// a weak AsyncWrap does not prevent the store from being garbage collected.
+// We use zlib streams as examples of weak AsyncWraps here, but the
+// issue is not specific to zlib.
+
+class Store {}
+
+let zlibDone = false;
+
+// Use immediates to ensure no accidental async context propagation occurs
+setImmediate(common.mustCall(() => {
+  const asyncLocalStorage = new AsyncLocalStorage();
+  const store = new Store();
+  asyncLocalStorage.run(store, common.mustCall(() => {
+    (async () => {
+      const zlibStream = zlib.createGzip();
+      // (Make sure this test does not break if _handle is renamed
+      // to something else)
+      assert.strictEqual(typeof zlibStream._handle, 'object');
+      // Create backreference from AsyncWrap to store
+      store.zlibStream = zlibStream._handle;
+      // Let the zlib stream finish (not strictly necessary)
+      zlibStream.end('hello world');
+      await zlibStream.toArray();
+      zlibDone = true;
+    })().then(common.mustCall());
+  }));
+}));
+
+const finish = common.mustCall(() => {
+  global.gc();
+  // Make sure the ALS instance has been garbage-collected
+  assert.strictEqual(v8.queryObjects(Store), 0);
+});
+
+const interval = setInterval(() => {
+  if (zlibDone) {
+    clearInterval(interval);
+    finish();
+  }
+}, 5);