From ad3fdedd0bdb64f5d834d36514aabb90fb955b4b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 14 Jan 2025 13:35:54 +0100 Subject: [PATCH 1/4] test: add maxCount and gcOptions to gcUntil() --- test/common/gc.js | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/test/common/gc.js b/test/common/gc.js index 82cc4c79edc3dd..4f11d0e1bd49e2 100644 --- a/test/common/gc.js +++ b/test/common/gc.js @@ -3,6 +3,7 @@ const wait = require('timers/promises').setTimeout; const assert = require('assert'); const common = require('../common'); +const { setImmediate } = require('timers/promises'); const gcTrackerMap = new WeakMap(); const gcTrackerTag = 'NODE_TEST_COMMON_GC_TRACKER'; @@ -40,32 +41,26 @@ function onGC(obj, gcListener) { /** * Repeatedly triggers garbage collection until a specified condition is met or a maximum number of attempts is reached. + * This utillity must be run in a Node.js instance that enables --expose-gc. * @param {string|Function} [name] - Optional name, used in the rejection message if the condition is not met. * @param {Function} condition - A function that returns true when the desired condition is met. + * @param {number} maxCount - Maximum number of garbage collections that should be tried. + * @param {object} gcOptions - Options to pass into the global gc() function. * @returns {Promise} A promise that resolves when the condition is met, or rejects after 10 failed attempts. */ -function gcUntil(name, condition) { - if (typeof name === 'function') { - condition = name; - name = undefined; - } - return new Promise((resolve, reject) => { - let count = 0; - function gcAndCheck() { - setImmediate(() => { - count++; - global.gc(); - if (condition()) { - resolve(); - } else if (count < 10) { - gcAndCheck(); - } else { - reject(name === undefined ? undefined : 'Test ' + name + ' failed'); - } - }); +async function gcUntil(name, condition, maxCount = 10, gcOptions) { + for (let count = 0; count < maxCount; ++count) { + await setImmediate(); + if (gcOptions) { + await global.gc(gcOptions); + } else { + await global.gc(); // Passing in undefined is not the same as empty. } - gcAndCheck(); - }); + if (condition()) { + return; + } + } + throw new Error(`Test ${name} failed`); } // This function can be used to check if an object factor leaks or not, From e67c128c08666e66a2d0f3726c31be957a56b105 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 9 Jan 2025 01:21:23 +0100 Subject: [PATCH 2/4] src: use cppgc to manage ContextifyContext This simplifies the memory management of ContextifyContext, making all references visible to V8. The destructors don't need to do anything because when the wrapper is going away, the context is already going away or otherwise it would've been holding the wrapper alive, so there's no need to reset the pointers in the context. Also, any global handles to the context would've been empty at this point, and the per-Environment context tracking code is capable of dealing with empty handles from contexts purged elsewhere. To this end, the context tracking code also purges empty handles from the list now, to prevent keeping too many empty handles around. --- src/env.cc | 7 +- src/env.h | 1 + src/node_contextify.cc | 69 ++++++++++--------- src/node_contextify.h | 84 ++++++++++++++++++++---- test/parallel/test-inspector-contexts.js | 14 ++-- 5 files changed, 115 insertions(+), 60 deletions(-) diff --git a/src/env.cc b/src/env.cc index f0f97244fdef63..0eda889802710d 100644 --- a/src/env.cc +++ b/src/env.cc @@ -223,7 +223,12 @@ void AsyncHooks::InstallPromiseHooks(Local ctx) { : PersistentToLocal::Strong(js_promise_hooks_[3])); } +void Environment::PurgeTrackedEmptyContexts() { + std::erase_if(contexts_, [&](auto&& el) { return el.IsEmpty(); }); +} + void Environment::TrackContext(Local context) { + PurgeTrackedEmptyContexts(); size_t id = contexts_.size(); contexts_.resize(id + 1); contexts_[id].Reset(isolate_, context); @@ -232,7 +237,7 @@ void Environment::TrackContext(Local context) { void Environment::UntrackContext(Local context) { HandleScope handle_scope(isolate_); - std::erase_if(contexts_, [&](auto&& el) { return el.IsEmpty(); }); + PurgeTrackedEmptyContexts(); for (auto it = contexts_.begin(); it != contexts_.end(); it++) { if (Local saved_context = PersistentToLocal::Weak(isolate_, *it); saved_context == context) { diff --git a/src/env.h b/src/env.h index ec5b608cede6a1..1929450b8fe393 100644 --- a/src/env.h +++ b/src/env.h @@ -1085,6 +1085,7 @@ class Environment final : public MemoryRetainer { const char* errmsg); void TrackContext(v8::Local context); void UntrackContext(v8::Local context); + void PurgeTrackedEmptyContexts(); std::list loaded_addons_; v8::Isolate* const isolate_; diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 77d35675827c67..ab6659d8cdccc6 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -118,8 +118,9 @@ Local Uint32ToName(Local context, uint32_t index) { } // anonymous namespace -BaseObjectPtr ContextifyContext::New( - Environment* env, Local sandbox_obj, ContextOptions* options) { +ContextifyContext* ContextifyContext::New(Environment* env, + Local sandbox_obj, + ContextOptions* options) { Local object_template; HandleScope scope(env->isolate()); CHECK_IMPLIES(sandbox_obj.IsEmpty(), options->vanilla); @@ -140,21 +141,25 @@ BaseObjectPtr ContextifyContext::New( if (!(CreateV8Context(env->isolate(), object_template, snapshot_data, queue) .ToLocal(&v8_context))) { // Allocation failure, maximum call stack size reached, termination, etc. - return BaseObjectPtr(); + return {}; } return New(v8_context, env, sandbox_obj, options); } -void ContextifyContext::MemoryInfo(MemoryTracker* tracker) const {} +void ContextifyContext::Trace(cppgc::Visitor* visitor) const { + CppgcMixin::Trace(visitor); + visitor->Trace(context_); +} ContextifyContext::ContextifyContext(Environment* env, Local wrapper, Local v8_context, ContextOptions* options) - : BaseObject(env, wrapper), - microtask_queue_(options->own_microtask_queue + : microtask_queue_(options->own_microtask_queue ? options->own_microtask_queue.release() : nullptr) { + CppgcMixin::Wrap(this, env, wrapper); + context_.Reset(env->isolate(), v8_context); // This should only be done after the initial initializations of the context // global object is finished. @@ -162,19 +167,6 @@ ContextifyContext::ContextifyContext(Environment* env, ContextEmbedderIndex::kContextifyContext)); v8_context->SetAlignedPointerInEmbedderData( ContextEmbedderIndex::kContextifyContext, this); - // It's okay to make this reference weak - V8 would create an internal - // reference to this context via the constructor of the wrapper. - // As long as the wrapper is alive, it's constructor is alive, and so - // is the context. - context_.SetWeak(); -} - -ContextifyContext::~ContextifyContext() { - Isolate* isolate = env()->isolate(); - HandleScope scope(isolate); - - env()->UnassignFromContext(PersistentToLocal::Weak(isolate, context_)); - context_.Reset(); } void ContextifyContext::InitializeGlobalTemplates(IsolateData* isolate_data) { @@ -251,11 +243,10 @@ MaybeLocal ContextifyContext::CreateV8Context( return scope.Escape(ctx); } -BaseObjectPtr ContextifyContext::New( - Local v8_context, - Environment* env, - Local sandbox_obj, - ContextOptions* options) { +ContextifyContext* ContextifyContext::New(Local v8_context, + Environment* env, + Local sandbox_obj, + ContextOptions* options) { HandleScope scope(env->isolate()); CHECK_IMPLIES(sandbox_obj.IsEmpty(), options->vanilla); // This only initializes part of the context. The primordials are @@ -263,7 +254,7 @@ BaseObjectPtr ContextifyContext::New( // things down significantly and they are only needed in rare occasions // in the vm contexts. if (InitializeContextRuntime(v8_context).IsNothing()) { - return BaseObjectPtr(); + return {}; } Local main_context = env->context(); @@ -300,7 +291,7 @@ BaseObjectPtr ContextifyContext::New( info.origin = *origin_val; } - BaseObjectPtr result; + ContextifyContext* result; Local wrapper; { Context::Scope context_scope(v8_context); @@ -315,7 +306,7 @@ BaseObjectPtr ContextifyContext::New( ctor_name, static_cast(v8::DontEnum)) .IsNothing()) { - return BaseObjectPtr(); + return {}; } } @@ -328,7 +319,7 @@ BaseObjectPtr ContextifyContext::New( env->host_defined_option_symbol(), options->host_defined_options_id) .IsNothing()) { - return BaseObjectPtr(); + return {}; } env->AssignToContext(v8_context, nullptr, info); @@ -336,13 +327,15 @@ BaseObjectPtr ContextifyContext::New( if (!env->contextify_wrapper_template() ->NewInstance(v8_context) .ToLocal(&wrapper)) { - return BaseObjectPtr(); + return {}; } - result = - MakeBaseObject(env, wrapper, v8_context, options); - // The only strong reference to the wrapper will come from the sandbox. - result->MakeWeak(); + result = cppgc::MakeGarbageCollected( + env->isolate()->GetCppHeap()->GetAllocationHandle(), + env, + wrapper, + v8_context, + options); } Local wrapper_holder = @@ -352,7 +345,7 @@ BaseObjectPtr ContextifyContext::New( ->SetPrivate( v8_context, env->contextify_context_private_symbol(), wrapper) .IsNothing()) { - return BaseObjectPtr(); + return {}; } // Assign host_defined_options_id to the sandbox object or the global object @@ -364,7 +357,7 @@ BaseObjectPtr ContextifyContext::New( env->host_defined_option_symbol(), options->host_defined_options_id) .IsNothing()) { - return BaseObjectPtr(); + return {}; } return result; } @@ -438,7 +431,7 @@ void ContextifyContext::MakeContext(const FunctionCallbackInfo& args) { options.host_defined_options_id = args[6].As(); TryCatchScope try_catch(env); - BaseObjectPtr context_ptr = + ContextifyContext* context_ptr = ContextifyContext::New(env, sandbox, &options); if (try_catch.HasCaught()) { @@ -469,6 +462,10 @@ ContextifyContext* ContextifyContext::ContextFromContextifiedSandbox( template ContextifyContext* ContextifyContext::Get(const PropertyCallbackInfo& args) { + // TODO(joyeecheung): it should be fine to simply use + // args.GetIsolate()->GetCurrentContext() and take the pointer at + // ContextEmbedderIndex::kContextifyContext, as V8 is supposed to + // push the creation context before invoking these callbacks. return Get(args.This()); } diff --git a/src/node_contextify.h b/src/node_contextify.h index d67968406d7b74..9ca611319ce1c5 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -23,17 +23,73 @@ struct ContextOptions { bool vanilla = false; }; -class ContextifyContext : public BaseObject { +/** + * The memory management of a vm context is as follows: + * + * user code + * │ + * As global proxy or ▼ + * ┌──────────────┐ kSandboxObject embedder data ┌────────────────┐ + * ┌─► │ V8 Context │────────────────────────────────►│ Wrapper holder │ + * │ └──────────────┘ └───────┬────────┘ + * │ ▲ Object constructor/creation context │ + * │ │ │ + * │ ┌──────┴────────────┐ contextify_context_private_symbol │ + * │ │ ContextifyContext │◄────────────────────────────────────┘ + * │ │ JS Wrapper │◄──────────► ┌─────────────────────────┐ + * │ └───────────────────┘ cppgc │ node::ContextifyContext │ + * │ │ C++ Object │ + * └──────────────────────────────────► └─────────────────────────┘ + * v8::TracedReference / ContextEmbedderIndex::kContextifyContext + * + * There are two possibilities for the "wrapper holder": + * + * 1. When vm.constants.DONT_CONTEXTIFY is used, the wrapper holder is the V8 + * context's global proxy object + * 2. Otherwise it's the arbitrary "sandbox object" that users pass into + * vm.createContext() or a new empty object created internally if they pass + * undefined. + * + * In 2, the global object of the new V8 context is created using + * global_object_template with interceptors that perform any requested + * operations on the global object in the context first on the sandbox object + * living outside of the new context, then fall back to the global proxy of the + * new context. + * + * It's critical for the user-accessible wrapper holder to keep the + * ContextifyContext wrapper alive via contextify_context_private_symbol + * so that the V8 context is always available to the user while they still + * hold the vm "context" object alive. + * + * It's also critical for the V8 context to keep the wrapper holder + * (specifically, the "sandbox object") as well as the node::ContextifyContext + * C++ object alive, so that when the code runs inside the object and accesses + * the global object, the interceptors can still access the "sandbox object" + * as well as and perform operations + * on them, even if users already relinquish access to the outer + * "sandbox object". + * + * The v8::TracedReference and the ContextEmbedderIndex::kContextifyContext + * slot in the context act as shortcuts from the node::ContextifyContext + * C++ object to the V8 context. + */ +class ContextifyContext final : CPPGC_MIXIN(ContextifyContext) { public: + SET_CPPGC_NAME(ContextifyContext) + void Trace(cppgc::Visitor* visitor) const final; + ContextifyContext(Environment* env, v8::Local wrapper, v8::Local v8_context, ContextOptions* options); - ~ContextifyContext(); - void MemoryInfo(MemoryTracker* tracker) const override; - SET_MEMORY_INFO_NAME(ContextifyContext) - SET_SELF_SIZE(ContextifyContext) + // The destructors don't need to do anything because when the wrapper is + // going away, the context is already going away or otherwise it would've + // been holding the wrapper alive, so there's no need to reset the pointers + // in the context. Also, any global handles to the context would've been + // empty at this point, and the per-Environment context tracking code is + // capable of dealing with empty handles from contexts purged elsewhere. + ~ContextifyContext() {} static v8::MaybeLocal CreateV8Context( v8::Isolate* isolate, @@ -48,7 +104,7 @@ class ContextifyContext : public BaseObject { Environment* env, const v8::Local& wrapper_holder); inline v8::Local context() const { - return PersistentToLocal::Default(env()->isolate(), context_); + return context_.Get(env()->isolate()); } inline v8::Local global_proxy() const { @@ -75,14 +131,14 @@ class ContextifyContext : public BaseObject { static void InitializeGlobalTemplates(IsolateData* isolate_data); private: - static BaseObjectPtr New(Environment* env, - v8::Local sandbox_obj, - ContextOptions* options); + static ContextifyContext* New(Environment* env, + v8::Local sandbox_obj, + ContextOptions* options); // Initialize a context created from CreateV8Context() - static BaseObjectPtr New(v8::Local ctx, - Environment* env, - v8::Local sandbox_obj, - ContextOptions* options); + static ContextifyContext* New(v8::Local ctx, + Environment* env, + v8::Local sandbox_obj, + ContextOptions* options); static bool IsStillInitializing(const ContextifyContext* ctx); static void MakeContext(const v8::FunctionCallbackInfo& args); @@ -140,7 +196,7 @@ class ContextifyContext : public BaseObject { static void IndexedPropertyEnumeratorCallback( const v8::PropertyCallbackInfo& args); - v8::Global context_; + v8::TracedReference context_; std::unique_ptr microtask_queue_; }; diff --git a/test/parallel/test-inspector-contexts.js b/test/parallel/test-inspector-contexts.js index 9cdf2d0017c4be..3d6ee4d460e863 100644 --- a/test/parallel/test-inspector-contexts.js +++ b/test/parallel/test-inspector-contexts.js @@ -8,7 +8,7 @@ common.skipIfInspectorDisabled(); const assert = require('assert'); const vm = require('vm'); const { Session } = require('inspector'); - +const { gcUntil } = require('../common/gc'); const session = new Session(); session.connect(); @@ -66,8 +66,7 @@ async function testContextCreatedAndDestroyed() { // GC is unpredictable... console.log('Checking/waiting for GC.'); - while (!contextDestroyed) - global.gc(); + await gcUntil('context destruction', () => contextDestroyed, Infinity, { type: 'major', execution: 'async' }); console.log('Context destroyed.'); assert.strictEqual(contextDestroyed.params.executionContextId, id, @@ -98,8 +97,7 @@ async function testContextCreatedAndDestroyed() { // GC is unpredictable... console.log('Checking/waiting for GC again.'); - while (!contextDestroyed) - global.gc(); + await gcUntil('context destruction', () => contextDestroyed, Infinity, { type: 'major', execution: 'async' }); console.log('Other context destroyed.'); } @@ -124,8 +122,7 @@ async function testContextCreatedAndDestroyed() { // GC is unpredictable... console.log('Checking/waiting for GC a third time.'); - while (!contextDestroyed) - global.gc(); + await gcUntil('context destruction', () => contextDestroyed, Infinity, { type: 'major', execution: 'async' }); console.log('Context destroyed once again.'); } @@ -148,8 +145,7 @@ async function testContextCreatedAndDestroyed() { // GC is unpredictable... console.log('Checking/waiting for GC a fourth time.'); - while (!contextDestroyed) - global.gc(); + await gcUntil('context destruction', () => contextDestroyed, Infinity, { type: 'major', execution: 'async' }); console.log('Context destroyed a fourth time.'); } } From ed02ac170cb846f8ea66a1ef4a55746df9bbd911 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 15 Jan 2025 15:25:13 +0100 Subject: [PATCH 3/4] fixup! test: add maxCount and gcOptions to gcUntil() --- test/common/gc.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/common/gc.js b/test/common/gc.js index 4f11d0e1bd49e2..87625068c2cbca 100644 --- a/test/common/gc.js +++ b/test/common/gc.js @@ -3,7 +3,8 @@ const wait = require('timers/promises').setTimeout; const assert = require('assert'); const common = require('../common'); -const { setImmediate } = require('timers/promises'); +// TODO(joyeecheung): rewrite checkIfCollectable to use this too. +const { setImmediate: setImmediatePromisified } = require('timers/promises'); const gcTrackerMap = new WeakMap(); const gcTrackerTag = 'NODE_TEST_COMMON_GC_TRACKER'; @@ -50,7 +51,7 @@ function onGC(obj, gcListener) { */ async function gcUntil(name, condition, maxCount = 10, gcOptions) { for (let count = 0; count < maxCount; ++count) { - await setImmediate(); + await setImmediatePromisified(); if (gcOptions) { await global.gc(gcOptions); } else { From b18f41a01ecd13da51527ba633d3317a726055fb Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 15 Jan 2025 15:27:19 +0100 Subject: [PATCH 4/4] fixup! src: use cppgc to manage ContextifyContext --- src/node_contextify.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/node_contextify.h b/src/node_contextify.h index 9ca611319ce1c5..de69c22b0ebaed 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -62,16 +62,16 @@ struct ContextOptions { * hold the vm "context" object alive. * * It's also critical for the V8 context to keep the wrapper holder - * (specifically, the "sandbox object") as well as the node::ContextifyContext - * C++ object alive, so that when the code runs inside the object and accesses - * the global object, the interceptors can still access the "sandbox object" - * as well as and perform operations + * (specifically, the "sandbox object" if users pass one) as well as the + * node::ContextifyContext C++ object alive, so that when the code + * runs inside the object and accesses the global object, the interceptors + * can still access the "sandbox object" and perform operations * on them, even if users already relinquish access to the outer * "sandbox object". * * The v8::TracedReference and the ContextEmbedderIndex::kContextifyContext - * slot in the context act as shortcuts from the node::ContextifyContext - * C++ object to the V8 context. + * slot in the context only act as shortcuts between + * the node::ContextifyContext C++ object and the V8 context. */ class ContextifyContext final : CPPGC_MIXIN(ContextifyContext) { public: @@ -89,7 +89,7 @@ class ContextifyContext final : CPPGC_MIXIN(ContextifyContext) { // in the context. Also, any global handles to the context would've been // empty at this point, and the per-Environment context tracking code is // capable of dealing with empty handles from contexts purged elsewhere. - ~ContextifyContext() {} + ~ContextifyContext() = default; static v8::MaybeLocal CreateV8Context( v8::Isolate* isolate,