diff options
| -rw-r--r-- | src/async_wrap.cc | 36 | ||||
| -rw-r--r-- | src/async_wrap.h | 5 | ||||
| -rw-r--r-- | src/base_object.h | 3 | ||||
| -rw-r--r-- | src/env.cc | 6 | ||||
| -rw-r--r-- | test/parallel/test-heapdump-async-hooks-init-promise.js | 46 |
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); |