async_hooks: fix Promises with later enabled hooks

configure

@@ -954,6 +954,7 @@ def configure_v8(o):
   o['variables']['v8_no_strict_aliasing'] = 1  # Work around compiler bugs.
   o['variables']['v8_optimized_debug'] = 0  # Compile with -O0 in debug builds.
   o['variables']['v8_random_seed'] = 0  # Use a random seed for hash tables.
+  o['variables']['v8_promise_internal_field_count'] = 1 # Add internal field to promises for async hooks.
   o['variables']['v8_use_snapshot'] = 'false' if options.without_snapshot else 'true'
   o['variables']['node_use_v8_platform'] = b(not options.without_v8_platform)
   o['variables']['node_use_bundled_v8'] = b(not options.without_bundled_v8)

src/async-wrap.cc

@@ -283,8 +283,8 @@ bool AsyncWrap::EmitAfter(Environment* env, double async_id) {
 
 class PromiseWrap : public AsyncWrap {
  public:
-  PromiseWrap(Environment* env, Local<Object> object)
-    : AsyncWrap(env, object, PROVIDER_PROMISE) {}
+  PromiseWrap(Environment* env, Local<Object> object, bool silent)
+    : AsyncWrap(env, object, PROVIDER_PROMISE, silent) {}
   size_t self_size() const override { return sizeof(*this); }
 };
 
@@ -293,33 +293,14 @@ static void PromiseHook(PromiseHookType type, Local<Promise> promise,
                         Local<Value> parent, void* arg) {
   Local<Context> context = promise->CreationContext();
   Environment* env = Environment::GetCurrent(context);
-  if (type == PromiseHookType::kInit) {
-    // Unfortunately, promises don't have internal fields. Need a surrogate that
-    // async wrap can wrap.
-    Local<Object> obj =
-      env->async_hooks_promise_object()->NewInstance(context).ToLocalChecked();
-    PromiseWrap* wrap = new PromiseWrap(env, obj);
-    v8::PropertyAttribute hidden =
-      static_cast<v8::PropertyAttribute>(v8::ReadOnly
-                                         | v8::DontDelete
-                                         | v8::DontEnum);
-    promise->DefineOwnProperty(context,
-              env->promise_wrap(),
-              v8::External::New(env->isolate(), wrap),
-              hidden).FromJust();
-    // The async tag will be destroyed at the same time as the promise as the
-    // only reference to it is held by the promise. This allows the promise
-    // wrap instance to be notified when the promise is destroyed.
-    promise->DefineOwnProperty(context,
-              env->promise_async_tag(),
-              obj, hidden).FromJust();
+  PromiseWrap* wrap = Unwrap<PromiseWrap>(promise);
+  if (type == PromiseHookType::kInit || wrap == nullptr) {
+    bool silent = type != PromiseHookType::kInit;
+    wrap = new PromiseWrap(env, promise, silent);
+    wrap->MakeWeak(wrap);
   } else if (type == PromiseHookType::kResolve) {
     // TODO(matthewloring): need to expose this through the async hooks api.
   }
-  Local<v8::Value> external_wrap =
-      promise->Get(context, env->promise_wrap()).ToLocalChecked();
-  PromiseWrap* wrap =
-    static_cast<PromiseWrap*>(external_wrap.As<v8::External>()->Value());
   CHECK_NE(wrap, nullptr);
   if (type == PromiseHookType::kBefore) {
     PreCallbackExecution(wrap, false);
@@ -415,11 +396,6 @@ void AsyncWrap::Initialize(Local<Object> target,
   env->SetMethod(target, "clearIdStack", ClearIdStack);
   env->SetMethod(target, "addIdToDestroyList", QueueDestroyId);
 
-  Local<v8::ObjectTemplate> promise_object_template =
-    v8::ObjectTemplate::New(env->isolate());
-  promise_object_template->SetInternalFieldCount(1);
-  env->set_async_hooks_promise_object(promise_object_template);
-
   v8::PropertyAttribute ReadOnlyDontDelete =
       static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete);
 
@@ -516,7 +492,8 @@ void LoadAsyncWrapperInfo(Environment* env) {
 
 AsyncWrap::AsyncWrap(Environment* env,
                      Local<Object> object,
-                     ProviderType provider)
+                     ProviderType provider,
+                     bool silent)
     : BaseObject(env, object),
       provider_type_(provider) {
   CHECK_NE(provider, PROVIDER_NONE);
@@ -526,7 +503,7 @@ AsyncWrap::AsyncWrap(Environment* env,
   persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider);
 
   // Use AsyncReset() call to execute the init() callbacks.
-  AsyncReset();
+  AsyncReset(silent);
 }
 
 
@@ -538,10 +515,12 @@ AsyncWrap::~AsyncWrap() {
 // Generalized call for both the constructor and for handles that are pooled
 // and reused over their lifetime. This way a new uid can be assigned when
 // the resource is pulled out of the pool and put back into use.
-void AsyncWrap::AsyncReset() {
+void AsyncWrap::AsyncReset(bool silent) {
   async_id_ = env()->new_async_id();
   trigger_id_ = env()->get_init_trigger_id();
 
+  if (silent) return;
+
   EmitAsyncInit(env(), object(),
                 env()->async_hooks()->provider_string(provider_type()),
                 async_id_, trigger_id_);

src/async-wrap.h

@@ -86,7 +86,8 @@ class AsyncWrap : public BaseObject {
 
   AsyncWrap(Environment* env,
             v8::Local<v8::Object> object,
-            ProviderType provider);
+            ProviderType provider,
+            bool silent = false);
 
   virtual ~AsyncWrap();
 
@@ -116,7 +117,7 @@ class AsyncWrap : public BaseObject {
 
   inline double get_trigger_id() const;
 
-  void AsyncReset();
+  void AsyncReset(bool silent = false);
 
   // Only call these within a valid HandleScope.
   // TODO(trevnorris): These should return a MaybeLocal.

src/env.h

@@ -195,8 +195,6 @@ namespace node {
   V(preference_string, "preference")                                          \
   V(priority_string, "priority")                                              \
   V(produce_cached_data_string, "produceCachedData")                          \
-  V(promise_wrap, "_promise_async_wrap")                                      \
-  V(promise_async_tag, "_promise_async_wrap_tag")                             \
   V(raw_string, "raw")                                                        \
   V(read_host_object_string, "_readHostObject")                               \
   V(readable_string, "readable")                                              \
@@ -258,7 +256,6 @@ namespace node {
   V(async_hooks_init_function, v8::Function)                                  \
   V(async_hooks_before_function, v8::Function)                                \
   V(async_hooks_after_function, v8::Function)                                 \
-  V(async_hooks_promise_object, v8::ObjectTemplate)                           \
   V(binding_cache_object, v8::Object)                                         \
   V(buffer_constructor_function, v8::Function)                                \
   V(buffer_prototype_object, v8::Object)                                      \

test/parallel/test-async-wrap-promise-after-enabled.js

@@ -0,0 +1,34 @@
+'use strict';
+
+// Regression test for https://github.com/nodejs/node/issues/13237
+
+const common = require('../common');
+const assert = require('assert');
+
+const async_hooks = require('async_hooks');
+
+const seenEvents = [];
+
+const p = new Promise((resolve) => resolve(1));
+p.then(() => seenEvents.push('then'));
+
+const hooks = async_hooks.createHook({
+  init: common.mustNotCall(),
+
+  before: common.mustCall((id) => {
+    assert.ok(id > 1);
+    seenEvents.push('before');
+  }),
+
+  after: common.mustCall((id) => {
+    assert.ok(id > 1);
+    seenEvents.push('after');
+    hooks.disable();
+  })
+});
+
+setImmediate(() => {
+  assert.deepStrictEqual(seenEvents, ['before', 'then', 'after']);
+});
+
+hooks.enable(); // After `setImmediate` in order to not catch its init event.