src: use symbol to store AsyncWrap resource

Use a symbol on the bindings object to store the public resource object,
rather than a `v8::Global` Persistent. This has several advantages:

- It’s harder to inadvertently create memory leaks this way.
  The garbage collector sees the `AsyncWrap` →  resource link like
  a regular JS property, and can collect the objects as a group,
  even if the resource object should happen to point back to the
  `AsyncWrap` object.
- This will make it easier in the future to use `owner_symbol` for
  this purpose, which is generally the direction we should be moving
  the `async_hooks` API into (i.e. using more public objects instead
  of letting internal wires stick out).

PR-URL: #31745
Backport-PR-URL: #33962
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>

Author: addaleax

lib/internal/async_hooks.js

@@ -36,8 +36,7 @@ const async_wrap = internalBinding('async_wrap');
 const {
   async_hook_fields,
   async_id_fields,
-  execution_async_resources,
-  owner_symbol
+  execution_async_resources
 } = async_wrap;
 // Store the pair executionAsyncId and triggerAsyncId in a std::stack on
 // Environment::AsyncHooks::async_ids_stack_ tracks the resource responsible for
@@ -78,6 +77,7 @@ const active_hooks = {
 
 const { registerDestroyHook } = async_wrap;
 const { enqueueMicrotask } = internalBinding('task_queue');
+const { resource_symbol, owner_symbol } = internalBinding('symbols');
 
 // Each constant tracks how many callbacks there are for any given step of
 // async execution. These are tracked so if the user didn't include callbacks
@@ -106,7 +106,7 @@ function executionAsyncResource() {
   const index = async_hook_fields[kStackLength] - 1;
   if (index === -1) return topLevelResource;
   const resource = execution_async_resources[index];
-  return resource;
+  return lookupPublicResource(resource);
 }
 
 // Used to fatally abort the process if a callback throws.
@@ -127,6 +127,15 @@ function fatalError(e) {
   process.exit(1);
 }
 
+function lookupPublicResource(resource) {
+  if (typeof resource !== 'object' || resource === null) return resource;
+  // TODO(addaleax): Merge this with owner_symbol and use it across all
+  // AsyncWrap instances.
+  const publicResource = resource[resource_symbol];
+  if (publicResource !== undefined)
+    return publicResource;
+  return resource;
+}
 
 // Emit From Native //
 
@@ -135,6 +144,7 @@ function fatalError(e) {
 // emitInitScript.
 function emitInitNative(asyncId, type, triggerAsyncId, resource) {
   active_hooks.call_depth += 1;
+  resource = lookupPublicResource(resource);
   // Use a single try/catch for all hooks to avoid setting up one per iteration.
   try {
     // Using var here instead of let because "for (var ...)" is faster than let.

src/api/callback.cc

@@ -36,7 +36,7 @@ CallbackScope::~CallbackScope() {
 
 InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap, int flags)
     : InternalCallbackScope(async_wrap->env(),
-                            async_wrap->GetResource(),
+                            async_wrap->object(),
                             { async_wrap->get_async_id(),
                               async_wrap->get_trigger_async_id() },
                             flags) {}

src/async_wrap.cc

@@ -451,11 +451,15 @@ void AsyncWrap::GetProviderType(const FunctionCallbackInfo<Value>& args) {
 }
 
 
-void AsyncWrap::EmitDestroy() {
+void AsyncWrap::EmitDestroy(bool from_gc) {
   AsyncWrap::EmitDestroy(env(), async_id_);
   // Ensure no double destroy is emitted via AsyncReset().
   async_id_ = kInvalidAsyncId;
-  resource_.Reset();
+
+  if (!persistent().IsEmpty() && !from_gc) {
+    HandleScope handle_scope(env()->isolate());
+    USE(object()->Set(env()->context(), env()->resource_symbol(), object()));
+  }
 }
 
 void AsyncWrap::QueueDestroyAsyncId(const FunctionCallbackInfo<Value>& args) {
@@ -533,10 +537,6 @@ void AsyncWrap::Initialize(Local<Object> target,
               env->async_ids_stack_string(),
               env->async_hooks()->async_ids_stack().GetJSArray()).Check();
 
-  target->Set(context,
-              FIXED_ONE_BYTE_STRING(env->isolate(), "owner_symbol"),
-              env->owner_symbol()).Check();
-
   Local<Object> constants = Object::New(isolate);
 #define SET_HOOKS_CONSTANT(name)                                              \
   FORCE_SET_TARGET_FIELD(                                                     \
@@ -632,7 +632,7 @@ bool AsyncWrap::IsDoneInitializing() const {
 
 AsyncWrap::~AsyncWrap() {
   EmitTraceEventDestroy();
-  EmitDestroy();
+  EmitDestroy(true /* from gc */);
 }
 
 void AsyncWrap::EmitTraceEventDestroy() {
@@ -682,10 +682,13 @@ void AsyncWrap::AsyncReset(Local<Object> resource, double execution_async_id,
                                                      : execution_async_id;
   trigger_async_id_ = env()->get_default_trigger_async_id();
 
-  if (resource != object()) {
-    // TODO(addaleax): Using a strong reference here makes it very easy to
-    // introduce memory leaks. Move away from using a strong reference.
-    resource_.Reset(env()->isolate(), resource);
+  {
+    HandleScope handle_scope(env()->isolate());
+    Local<Object> obj = object();
+    CHECK(!obj.IsEmpty());
+    if (resource != obj) {
+      USE(obj->Set(env()->context(), env()->resource_symbol(), resource));
+    }
   }
 
   switch (provider_type()) {
@@ -755,7 +758,7 @@ 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(), GetResource(), object(), cb, argc, argv, context);
+      env(), object(), object(), cb, argc, argv, context);
 
   // This is a static call with cached values because the `this` object may
   // no longer be alive at this point.
@@ -794,14 +797,6 @@ Local<Object> AsyncWrap::GetOwner(Environment* env, Local<Object> obj) {
   }
 }
 
-Local<Object> AsyncWrap::GetResource() {
-  if (resource_.IsEmpty()) {
-    return object();
-  }
-
-  return resource_.Get(env()->isolate());
-}
-
 }  // namespace node
 
 NODE_MODULE_CONTEXT_AWARE_INTERNAL(async_wrap, node::AsyncWrap::Initialize)

src/async_wrap.h

@@ -152,7 +152,7 @@ class AsyncWrap : public BaseObject {
   static void EmitAfter(Environment* env, double async_id);
   static void EmitPromiseResolve(Environment* env, double async_id);
 
-  void EmitDestroy();
+  void EmitDestroy(bool from_gc = false);
 
   void EmitTraceEventBefore();
   static void EmitTraceEventAfter(ProviderType type, double async_id);
@@ -199,7 +199,6 @@ class AsyncWrap : public BaseObject {
                                         v8::Local<v8::Object> obj);
 
   bool IsDoneInitializing() const override;
-  v8::Local<v8::Object> GetResource();
 
  private:
   friend class PromiseWrap;
@@ -214,7 +213,6 @@ class AsyncWrap : public BaseObject {
   // Because the values may be Reset(), cannot be made const.
   double async_id_ = kInvalidAsyncId;
   double trigger_async_id_;
-  v8::Global<v8::Object> resource_;
 };
 
 }  // namespace node

src/env.h

@@ -167,8 +167,10 @@ constexpr size_t kFsStatsBufferLength =
   V(handle_onclose_symbol, "handle_onclose")                                   \
   V(no_message_symbol, "no_message_symbol")                                    \
   V(oninit_symbol, "oninit")                                                   \
-  V(owner_symbol, "owner")                                                     \
+  V(owner_symbol, "owner_symbol")                                              \
   V(onpskexchange_symbol, "onpskexchange")                                     \
+  V(resource_symbol, "resource_symbol")                                        \
+  V(trigger_async_id_symbol, "trigger_async_id_symbol")                        \
 
 // Strings are per-isolate primitives but Environment proxies them
 // for the sake of convenience.  Strings should be ASCII-only.

test/parallel/test-bootstrap-modules.js

@@ -24,6 +24,7 @@ const expectedModules = new Set([
   'Internal Binding process_methods',
   'Internal Binding report',
   'Internal Binding string_decoder',
+  'Internal Binding symbols',
   'Internal Binding task_queue',
   'Internal Binding timers',
   'Internal Binding trace_events',