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. PR-URL: #56522 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

Author: joyeecheung

src/env.cc

@@ -223,7 +223,12 @@ void AsyncHooks::InstallPromiseHooks(Local<Context> ctx){
  : PersistentToLocal::Strong(js_promise_hooks_[3]));
 }
 
+voidEnvironment::PurgeTrackedEmptyContexts(){
+std::erase_if(contexts_, [&](auto&& el){return el.IsEmpty()});
+}
+
 voidEnvironment::TrackContext(Local<Context> 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> context){
 
 voidEnvironment::UntrackContext(Local<Context> 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<Context> saved_context = PersistentToLocal::Weak(isolate_, *it);
  saved_context == context){

src/env.h

@@ -1093,6 +1093,7 @@ class Environment final : public MemoryRetainer{
 const char* errmsg);
 voidTrackContext(v8::Local<v8::Context> context);
 voidUntrackContext(v8::Local<v8::Context> context);
+voidPurgeTrackedEmptyContexts();
 
  std::list<binding::DLib> loaded_addons_;
  v8::Isolate* const isolate_;

src/node_contextify.cc

@@ -118,8 +118,9 @@ Local<Name> Uint32ToName(Local<Context> context, uint32_t index){
 
 } // anonymous namespace
 
-BaseObjectPtr<ContextifyContext> ContextifyContext::New(
- Environment* env, Local<Object> sandbox_obj, ContextOptions* options){
+ContextifyContext* ContextifyContext::New(Environment* env,
+ Local<Object> sandbox_obj,
+ ContextOptions* options){
  Local<ObjectTemplate> object_template;
  HandleScope scope(env->isolate());
 CHECK_IMPLIES(sandbox_obj.IsEmpty(), options->vanilla);
@@ -140,41 +141,32 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
 if (!(CreateV8Context(env->isolate(), object_template, snapshot_data, queue)
  .ToLocal(&v8_context))){
 // Allocation failure, maximum call stack size reached, termination, etc.
-returnBaseObjectPtr<ContextifyContext>();
+return{};
  }
 returnNew(v8_context, env, sandbox_obj, options);
 }
 
-voidContextifyContext::MemoryInfo(MemoryTracker* tracker) const{}
+voidContextifyContext::Trace(cppgc::Visitor* visitor) const{
+CppgcMixin::Trace(visitor);
+ visitor->Trace(context_);
+}
 
 ContextifyContext::ContextifyContext(Environment* env,
  Local<Object> wrapper,
  Local<Context> 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.
 DCHECK_NULL(v8_context->GetAlignedPointerFromEmbedderData(
  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();
 }
 
 voidContextifyContext::InitializeGlobalTemplates(IsolateData* isolate_data){
@@ -251,19 +243,18 @@ MaybeLocal<Context> ContextifyContext::CreateV8Context(
 return scope.Escape(ctx);
 }
 
-BaseObjectPtr<ContextifyContext> ContextifyContext::New(
- Local<Context> v8_context,
- Environment* env,
- Local<Object> sandbox_obj,
- ContextOptions* options){
+ContextifyContext* ContextifyContext::New(Local<Context> v8_context,
+ Environment* env,
+ Local<Object> 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
 // only initialized when needed because even deserializing them slows
 // things down significantly and they are only needed in rare occasions
 // in the vm contexts.
 if (InitializeContextRuntime(v8_context).IsNothing()){
-returnBaseObjectPtr<ContextifyContext>();
+return{};
  }
 
  Local<Context> main_context = env->context();
@@ -300,7 +291,7 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
  info.origin = *origin_val;
  }
 
-BaseObjectPtr<ContextifyContext> result;
+ ContextifyContext* result;
  Local<Object> wrapper;
 {
  Context::Scope context_scope(v8_context);
@@ -315,7 +306,7 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
  ctor_name,
 static_cast<v8::PropertyAttribute>(v8::DontEnum))
  .IsNothing()){
-returnBaseObjectPtr<ContextifyContext>();
+return{};
  }
  }
 
@@ -328,21 +319,23 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
  env->host_defined_option_symbol(),
  options->host_defined_options_id)
  .IsNothing()){
-returnBaseObjectPtr<ContextifyContext>();
+return{};
  }
 
  env->AssignToContext(v8_context, nullptr, info);
 
 if (!env->contextify_wrapper_template()
  ->NewInstance(v8_context)
  .ToLocal(&wrapper)){
-returnBaseObjectPtr<ContextifyContext>();
+return{};
  }
 
- result =
- MakeBaseObject<ContextifyContext>(env, wrapper, v8_context, options);
-// The only strong reference to the wrapper will come from the sandbox.
- result->MakeWeak();
+ result = cppgc::MakeGarbageCollected<ContextifyContext>(
+ env->isolate()->GetCppHeap()->GetAllocationHandle(),
+ env,
+ wrapper,
+ v8_context,
+ options);
  }
 
  Local<Object> wrapper_holder =
@@ -352,7 +345,7 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
  ->SetPrivate(
  v8_context, env->contextify_context_private_symbol(), wrapper)
  .IsNothing()){
-returnBaseObjectPtr<ContextifyContext>();
+return{};
  }
 
 // Assign host_defined_options_id to the sandbox object or the global object
@@ -364,7 +357,7 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
  env->host_defined_option_symbol(),
  options->host_defined_options_id)
  .IsNothing()){
-returnBaseObjectPtr<ContextifyContext>();
+return{};
  }
 return result;
 }
@@ -438,7 +431,7 @@ void ContextifyContext::MakeContext(const FunctionCallbackInfo<Value>& args){
  options.host_defined_options_id = args[6].As<Symbol>();
 
  TryCatchScope try_catch(env);
-BaseObjectPtr<ContextifyContext> context_ptr =
+ ContextifyContext* context_ptr =
 ContextifyContext::New(env, sandbox, &options);
 
 if (try_catch.HasCaught()){
@@ -469,6 +462,10 @@ ContextifyContext* ContextifyContext::ContextFromContextifiedSandbox(
 
 template <typename T>
 ContextifyContext* ContextifyContext::Get(const PropertyCallbackInfo<T>& 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.
 returnGet(args.This());
 }
 

src/node_contextify.h

@@ -23,17 +23,73 @@ struct ContextOptions{
 bool vanilla = false;
 };
 
-classContextifyContext : publicBaseObject{
+/**
+ * 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.
+*/
+classContextifyContextfinal : CPPGC_MIXIN(ContextifyContext){
 public:
+SET_CPPGC_NAME(ContextifyContext)
+voidTrace(cppgc::Visitor* visitor) constfinal;
+
 ContextifyContext(Environment* env,
  v8::Local<v8::Object> wrapper,
  v8::Local<v8::Context> v8_context,
  ContextOptions* options);
-~ContextifyContext();
 
-voidMemoryInfo(MemoryTracker* tracker) constoverride;
-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<v8::Context> CreateV8Context(
  v8::Isolate* isolate,
@@ -48,7 +104,7 @@ class ContextifyContext : public BaseObject{
  Environment* env, const v8::Local<v8::Object>& wrapper_holder);
 
 inline v8::Local<v8::Context> context() const{
-returnPersistentToLocal::Default(env()->isolate(), context_);
+returncontext_.Get(env()->isolate());
  }
 
 inline v8::Local<v8::Object> global_proxy() const{
@@ -75,14 +131,14 @@ class ContextifyContext : public BaseObject{
 staticvoidInitializeGlobalTemplates(IsolateData* isolate_data);
 
 private:
-staticBaseObjectPtr<ContextifyContext>New(Environment* env,
-v8::Local<v8::Object> sandbox_obj,
-ContextOptions* options);
+static ContextifyContext*New(Environment* env,
+ v8::Local<v8::Object> sandbox_obj,
+ ContextOptions* options);
 // Initialize a context created from CreateV8Context()
-staticBaseObjectPtr<ContextifyContext>New(v8::Local<v8::Context> ctx,
-Environment* env,
-v8::Local<v8::Object> sandbox_obj,
-ContextOptions* options);
+static ContextifyContext*New(v8::Local<v8::Context> ctx,
+ Environment* env,
+ v8::Local<v8::Object> sandbox_obj,
+ ContextOptions* options);
 
 staticboolIsStillInitializing(const ContextifyContext* ctx);
 staticvoidMakeContext(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -140,7 +196,7 @@ class ContextifyContext : public BaseObject{
 staticvoidIndexedPropertyEnumeratorCallback(
 const v8::PropertyCallbackInfo<v8::Array>& args);
 
- v8::Global<v8::Context> context_;
+ v8::TracedReference<v8::Context> context_;
  std::unique_ptr<v8::MicrotaskQueue> microtask_queue_;
 };
 

test/parallel/test-inspector-contexts.js

@@ -8,7 +8,7 @@ common.skipIfInspectorDisabled();
 constassert=require('assert');
 constvm=require('vm');
 const{ Session }=require('inspector');
-
+const{ gcUntil }=require('../common/gc');
 constsession=newSession();
 session.connect();
 
@@ -66,8 +66,7 @@ async function testContextCreatedAndDestroyed(){
 
 // GC is unpredictable...
 console.log('Checking/waiting for GC.');
-while(!contextDestroyed)
-global.gc();
+awaitgcUntil('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();
+awaitgcUntil('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();
+awaitgcUntil('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();
+awaitgcUntil('context destruction',()=>contextDestroyed,Infinity,{type: 'major',execution: 'async'});
 console.log('Context destroyed a fourth time.');
 }
 }