src: do not include partial AsyncWrap instances in heap dump

Heap dumps can be taken either through the inspector or the public API
for it during an async_hooks init() hook, but at that point the
AsyncWrap in question is not done initializing yet and virtual methods
cannot be called on it.

Address this issue (somewhat hackily) by excluding `AsyncWrap`
instances which have not yet executed their `init()` hook fully
from heap dumps.

Fixes: https://github.com/nodejs/node/issues/28786

PR-URL: https://github.com/nodejs/node/pull/28789
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>

5 files changed, 86 insertions, 10 deletions

diff --git a/src/async_wrap.cc b/src/async_wrap.cc
index b7da5565a3..83b661a12d 100644
--- a/src/async_wrap.cc
+++ b/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();
diff --git a/src/async_wrap.h b/src/async_wrap.h
index 0fe4135223..546f5130e0 100644
--- a/src/async_wrap.h
+++ b/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_;
diff --git a/src/base_object.h b/src/base_object.h
index 53bd4b00d6..0b202cd3a5 100644
--- a/src/base_object.h
+++ b/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
diff --git a/src/env.cc b/src/env.cc
index bb78a0c1f3..936b9a1854 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -954,8 +954,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);
@@ -1077,6 +1077,8 @@ void BaseObject::DeleteMe(void* data) {
   delete self;
 }
 
+bool BaseObject::IsDoneInitializing() const { return true; }
+
 Local<Object> BaseObject::WrappedObject() const {
   return object();
 }
diff --git a/test/parallel/test-heapdump-async-hooks-init-promise.js b/test/parallel/test-heapdump-async-hooks-init-promise.js
new file mode 100644
index 0000000000..c59cb89baa
--- /dev/null
+++ b/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);