From ece22105f16bd017d4f20892ddaa631cd80fa301 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 20 Jul 2019 23:12:41 +0200 Subject: [PATCH] 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 --- src/async_wrap.cc | 36 ++++++++++++--- src/async_wrap.h | 5 +- src/base_object.h | 3 ++ src/env.cc | 6 ++- .../test-heapdump-async-hooks-init-promise.js | 46 +++++++++++++++++++ 5 files changed, 86 insertions(+), 10 deletions(-) create mode 100644 test/parallel/test-heapdump-async-hooks-init-promise.js diff --git a/src/async_wrap.cc b/src/async_wrap.cc index b7da5565a3a8b1..83b661a12db0a8 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) - : 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 0fe41352234d06..546f5130e01fc6 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 53bd4b00d6044a..0b202cd3a51324 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -83,6 +83,9 @@ class BaseObject : public MemoryRetainer { v8::Local value, const v8::PropertyCallbackInfo& 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 38d5796f54d6e5..a009833f737c3c 100644 --- a/src/env.cc +++ b/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 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 00000000000000..c59cb89baa3d18 --- /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);