src: do not include partial AsyncWrap instances in heap dump

src/async_wrap.cc

@@ -577,22 +577,44 @@ AsyncWrap::AsyncWrap(Environment* env,
                      ProviderType provider,
                      double execution_async_id,
                      bool silent)
-    : BaseObject(env, object),
-      provider_type_(provider) {
+    : AsyncWrap(env, object) {
   CHECK_NE(provider, PROVIDER_NONE);
-  CHECK_GE(object->InternalFieldCount(), 1);
+  provider_type_ = provider;
 
   // Use AsyncReset() call to execute the init() callbacks.
   AsyncReset(execution_async_id, silent);
+  init_hook_ran_ = true;
 }
 
 AsyncWrap::AsyncWrap(Environment* env, Local<Object> object)
-    : BaseObject(env, object),
-      provider_type_(PROVIDER_NONE) {
-  CHECK_GE(object->InternalFieldCount(), 1);
+  : BaseObject(env, object) {
+}
+
+// This method is necessary to work around one specific problem:
+// Before the init() hook runs, if there is one, the BaseObject() constructor
+// registers this object with the Environment for finilization and debugging
+// purposes.
+// If the Environment decides to inspect this object for debugging, it tries to
+// call virtual methods on this object that are only (meaningfully) implemented
+// by the subclasses of AsyncWrap.
+// This could, with bad luck, happen during the AsyncWrap() constructor,
+// because we run JS code as part of it and that in turn can lead to a heapdump
+// being taken, either through the inspector or our programmatic API for it.
+// The object being initialized is not fully constructed at that point, and
+// in particular its virtual function table points to the AsyncWrap one
+// (as the subclass constructor has not yet begun execution at that point).
+// This means that the functions that are used for heap dump memory tracking
+// are not yet available, and trying to call them would crash the process.
+// We use this particular `IsDoneInitializing()` method to tell the Environment
+// that such debugging methods are not yet available.
+// This may be somewhat unreliable when it comes to future changes, because
+// at this point it *only* protects AsyncWrap subclasses, and *only* for cases
+// where heap dumps are being taken while the init() hook is on the call stack.
+// For now, it seems like the best solution, though.
+bool AsyncWrap::IsDoneInitializing() const {
+  return init_hook_ran_;
 }
 
-
 AsyncWrap::~AsyncWrap() {
   EmitTraceEventDestroy();
   EmitDestroy();

src/async_wrap.h

@@ -210,6 +210,8 @@ class AsyncWrap : public BaseObject {
     AsyncWrap* wrap_ = nullptr;
   };
 
+  bool IsDoneInitializing() const override;
+
  private:
   friend class PromiseWrap;
 
@@ -218,7 +220,8 @@ class AsyncWrap : public BaseObject {
             ProviderType provider,
             double execution_async_id,
             bool silent);
-  ProviderType provider_type_;
+  ProviderType provider_type_ = PROVIDER_NONE;
+  bool init_hook_ran_ = false;
   // Because the values may be Reset(), cannot be made const.
   double async_id_ = kInvalidAsyncId;
   double trigger_async_id_;

src/base_object.h

@@ -83,6 +83,9 @@ class BaseObject : public MemoryRetainer {
                                v8::Local<v8::Value> value,
                                const v8::PropertyCallbackInfo<void>& info);
 
+  // This is a bit of a hack. See the override in async_wrap.cc for details.
+  virtual bool IsDoneInitializing() const;
+
  protected:
   // Can be used to avoid the automatic object deletion when the Environment
   // exits, for example when this object is owned and deleted by another

src/env.cc

@@ -974,8 +974,8 @@ void MemoryTracker::TrackField(const char* edge_name,
   // identified and tracked here (based on their deleters),
   // but we may convert and track other known types here.
   BaseObject* obj = value.GetBaseObject();
-  if (obj != nullptr) {
-    this->TrackField("arg", obj);
+  if (obj != nullptr && obj->IsDoneInitializing()) {
+    TrackField("arg", obj);
   }
   CHECK_EQ(CurrentNode(), n);
   CHECK_NE(n->size_, 0);
@@ -1097,6 +1097,8 @@ void BaseObject::DeleteMe(void* data) {
   delete self;
 }
 
+bool BaseObject::IsDoneInitializing() const { return true; }
+
 Local<Object> BaseObject::WrappedObject() const {
   return object();
 }

test/parallel/test-heapdump-async-hooks-init-promise.js

@@ -0,0 +1,46 @@
+// Flags: --expose-gc
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const async_hooks = require('async_hooks');
+const v8 = require('v8');
+
+// Regression test for https://github.com/nodejs/node/issues/28786
+// Make sure that creating a heap snapshot inside an async_hooks hook
+// works for Promises.
+
+const createSnapshot = common.mustCall(() => {
+  v8.getHeapSnapshot().resume();
+}, 8);  // 2 × init + 2 × resolve + 1 × (after + before) + 2 × destroy = 8 calls
+
+const promiseIds = [];
+
+async_hooks.createHook({
+  init(id, type) {
+    if (type === 'PROMISE') {
+      createSnapshot();
+      promiseIds.push(id);
+    }
+  },
+
+  before(id) {
+    if (promiseIds.includes(id)) createSnapshot();
+  },
+
+  after(id) {
+    if (promiseIds.includes(id)) createSnapshot();
+  },
+
+  promiseResolve(id) {
+    assert(promiseIds.includes(id));
+    createSnapshot();
+  },
+
+  destroy(id) {
+    if (promiseIds.includes(id)) createSnapshot();
+  }
+}).enable();
+
+
+Promise.resolve().then(() => {});
+setImmediate(global.gc);