src: track async resources via pointers to stack-allocated handles

This addresses an existing `TODO` to remove the need for a separate
`LocalVector`. Since all relevant handles are already present on the
stack, we can track their addresses instead of storing the objects
in a separate container.

PR-URL: #59704
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>

Author: addaleax

src/api/callback.cc

@@ -48,17 +48,26 @@ InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap, int flags)
           flags,
           async_wrap->context_frame()) {}
 
-InternalCallbackScope::InternalCallbackScope(Environment* env,
-                                             Local<Object> object,
-                                             const async_context& asyncContext,
-                                             int flags,
-                                             Local<Value> context_frame)
+InternalCallbackScope::InternalCallbackScope(
+    Environment* env,
+    std::variant<Local<Object>, Local<Object>*> object,
+    const async_context& asyncContext,
+    int flags,
+    Local<Value> context_frame)
     : env_(env),
       async_context_(asyncContext),
-      object_(object),
       skip_hooks_(flags & kSkipAsyncHooks),
       skip_task_queues_(flags & kSkipTaskQueues) {
   CHECK_NOT_NULL(env);
+
+  if (std::holds_alternative<Local<Object>>(object)) {
+    object_storage_ = std::get<Local<Object>>(object);
+    object_ = &object_storage_;
+  } else {
+    object_ = std::get<Local<Object>*>(object);
+    CHECK_NOT_NULL(object_);
+  }
+
   env->PushAsyncCallbackScope();
 
   if (!env->can_call_into_js()) {
@@ -85,7 +94,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
       isolate, async_context_frame::exchange(isolate, context_frame));
 
   env->async_hooks()->push_async_context(
-    async_context_.async_id, async_context_.trigger_async_id, object);
+      async_context_.async_id, async_context_.trigger_async_id, object_);
 
   pushed_ids_ = true;
 

src/async_wrap.cc

@@ -278,7 +278,7 @@ void AsyncWrap::PushAsyncContext(const FunctionCallbackInfo<Value>& args) {
   // then the checks in push_async_ids() and pop_async_id() will.
   double async_id = args[0]->NumberValue(env->context()).FromJust();
   double trigger_async_id = args[1]->NumberValue(env->context()).FromJust();
-  env->async_hooks()->push_async_context(async_id, trigger_async_id, {});
+  env->async_hooks()->push_async_context(async_id, trigger_async_id, nullptr);
 }
 
 

src/env-inl.h

@@ -141,8 +141,10 @@ v8::Local<v8::Array> AsyncHooks::js_execution_async_resources() {
 }
 
 v8::Local<v8::Object> AsyncHooks::native_execution_async_resource(size_t i) {
-  if (i >= native_execution_async_resources_.size()) return {};
-  return native_execution_async_resources_[i];
+  if (i >= native_execution_async_resources_.size() ||
+      native_execution_async_resources_[i] == nullptr)
+    return {};
+  return *native_execution_async_resources_[i];
 }
 
 inline v8::Local<v8::String> AsyncHooks::provider_string(int idx) {

src/env.cc

@@ -120,7 +120,8 @@ void Environment::ResetPromiseHooks(Local<Function> init,
 // Remember to keep this code aligned with pushAsyncContext() in JS.
 void AsyncHooks::push_async_context(double async_id,
                                     double trigger_async_id,
-                                    Local<Object> resource) {
+                                    Local<Object>* resource) {
+  CHECK_IMPLIES(resource != nullptr, !resource->IsEmpty());
   // Since async_hooks is experimental, do only perform the check
   // when async_hooks is enabled.
   if (fields_[kCheck] > 0) {
@@ -138,14 +139,14 @@ void AsyncHooks::push_async_context(double async_id,
 
 #ifdef DEBUG
   for (uint32_t i = offset; i < native_execution_async_resources_.size(); i++)
-    CHECK(native_execution_async_resources_[i].IsEmpty());
+    CHECK_NULL(native_execution_async_resources_[i]);
 #endif
 
   // When this call comes from JS (as a way of increasing the stack size),
   // `resource` will be empty, because JS caches these values anyway.
-  if (!resource.IsEmpty()) {
+  if (resource != nullptr) {
     native_execution_async_resources_.resize(offset + 1);
-    // Caveat: This is a v8::Local<> assignment, we do not keep a v8::Global<>!
+    // Caveat: This is a v8::Local<>* assignment, we do not keep a v8::Global<>!
     native_execution_async_resources_[offset] = resource;
   }
 }
@@ -170,11 +171,11 @@ bool AsyncHooks::pop_async_context(double async_id) {
   fields_[kStackLength] = offset;
 
   if (offset < native_execution_async_resources_.size() &&
-      !native_execution_async_resources_[offset].IsEmpty()) [[likely]] {
+      native_execution_async_resources_[offset] != nullptr) [[likely]] {
 #ifdef DEBUG
     for (uint32_t i = offset + 1; i < native_execution_async_resources_.size();
          i++) {
-      CHECK(native_execution_async_resources_[i].IsEmpty());
+      CHECK_NULL(native_execution_async_resources_[i]);
     }
 #endif
     native_execution_async_resources_.resize(offset);
@@ -1740,7 +1741,6 @@ AsyncHooks::AsyncHooks(Isolate* isolate, const SerializeInfo* info)
       fields_(isolate, kFieldsCount, MAYBE_FIELD_PTR(info, fields)),
       async_id_fields_(
           isolate, kUidFieldsCount, MAYBE_FIELD_PTR(info, async_id_fields)),
-      native_execution_async_resources_(isolate),
       info_(info) {
   HandleScope handle_scope(isolate);
   if (info == nullptr) {
@@ -1829,10 +1829,9 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local<Context> context,
       native_execution_async_resources_.size());
   for (size_t i = 0; i < native_execution_async_resources_.size(); i++) {
     info.native_execution_async_resources[i] =
-        native_execution_async_resources_[i].IsEmpty() ? SIZE_MAX :
-            creator->AddData(
-                context,
-                native_execution_async_resources_[i]);
+        native_execution_async_resources_[i] == nullptr
+            ? SIZE_MAX
+            : creator->AddData(context, *native_execution_async_resources_[i]);
   }
 
   // At the moment, promise hooks are not supported in the startup snapshot.

src/env.h

@@ -58,6 +58,7 @@
 #include <array>
 #include <atomic>
 #include <cstdint>
+#include <deque>
 #include <functional>
 #include <list>
 #include <memory>
@@ -345,7 +346,7 @@ class AsyncHooks : public MemoryRetainer {
   // `pop_async_context()` or `clear_async_id_stack()` are called.
   void push_async_context(double async_id,
                           double trigger_async_id,
-                          v8::Local<v8::Object> execution_async_resource);
+                          v8::Local<v8::Object>* execution_async_resource);
   bool pop_async_context(double async_id);
   void clear_async_id_stack();  // Used in fatal exceptions.
 
@@ -407,15 +408,9 @@ class AsyncHooks : public MemoryRetainer {
 
   v8::Global<v8::Array> js_execution_async_resources_;
 
-  // TODO(@jasnell): Note that this is technically illegal use of
-  // v8::Locals which should be kept on the stack. Here, the entries
-  // in this object grows and shrinks with the C stack, and entries
-  // will be in the right handle scopes, but v8::Locals are supposed
-  // to remain on the stack and not the heap. For general purposes
-  // this *should* be ok but may need to be looked at further should
-  // v8 become stricter in the future about v8::Locals being held in
-  // the stack.
-  v8::LocalVector<v8::Object> native_execution_async_resources_;
+  // We avoid storing the handles directly here, because they are already
+  // properly allocated on the stack, we just need access to them here.
+  std::deque<v8::Local<v8::Object>*> native_execution_async_resources_;
 
   // Non-empty during deserialization
   const SerializeInfo* info_ = nullptr;

src/node_internals.h

@@ -37,6 +37,7 @@
 #include <cstdlib>
 
 #include <string>
+#include <variant>
 #include <vector>
 
 struct sockaddr;
@@ -245,9 +246,14 @@ class InternalCallbackScope {
     // compatibility issues, but it shouldn't.)
     kSkipTaskQueues = 2
   };
+  // You need to either guarantee that this `InternalCallbackScope` is
+  // stack-allocated itself, OR that `object` is a pointer to a stack-allocated
+  // `v8::Local<v8::Object>` which outlives this scope (e.g. for the
+  // public `CallbackScope` which indirectly allocates an instance of
+  // this class for ABI stability purposes).
   InternalCallbackScope(
       Environment* env,
-      v8::Local<v8::Object> object,
+      std::variant<v8::Local<v8::Object>, v8::Local<v8::Object>*> object,
       const async_context& asyncContext,
       int flags = kNoFlags,
       v8::Local<v8::Value> context_frame = v8::Local<v8::Value>());
@@ -263,7 +269,8 @@ class InternalCallbackScope {
  private:
   Environment* env_;
   async_context async_context_;
-  v8::Local<v8::Object> object_;
+  v8::Local<v8::Object> object_storage_;
+  v8::Local<v8::Object>* object_;
   bool skip_hooks_;
   bool skip_task_queues_;
   bool failed_ = false;

src/node_task_queue.cc

@@ -100,10 +100,11 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
   if (!GetAssignedPromiseAsyncId(env, promise, env->trigger_async_id_symbol())
           .To(&trigger_async_id)) return;
 
+  Local<Object> promise_as_obj = promise;
   if (async_id != AsyncWrap::kInvalidAsyncId &&
       trigger_async_id != AsyncWrap::kInvalidAsyncId) {
     env->async_hooks()->push_async_context(
-        async_id, trigger_async_id, promise);
+        async_id, trigger_async_id, &promise_as_obj);
   }
 
   USE(callback->Call(