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 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
nodejs · addaleax · Feb 11, 2020 · Jun 23, 2020 · Jun 19, 2020 · d2cb12a720d473d2275a6d90026d7dc1318e5237
commit d2cb12a720d473d2275a6d90026d7dc1318e5237
diff --git a/lib/internal/async_hooks.js b/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.

diff --git a/src/api/callback.cc b/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) {}

diff --git a/src/async_wrap.cc b/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)
diff --git a/src/async_wrap.h b/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

diff --git a/src/env.h b/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.