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..de69c22b0ebaed 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" 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 only act as shortcuts between + * the node::ContextifyContext C++ object and 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() = default; 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.'); } }