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

src/env.cc

@@ -223,7 +223,12 @@ void AsyncHooks::InstallPromiseHooks(Local<Context> ctx) {
                            : PersistentToLocal::Strong(js_promise_hooks_[3]));
 }
 
+void Environment::PurgeTrackedEmptyContexts() {
+  std::erase_if(contexts_, [&](auto&& el) { return el.IsEmpty(); });
+}
+
 void Environment::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) {
 
 void Environment::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

@@ -1085,6 +1085,7 @@ class Environment final : public MemoryRetainer {
                          const char* errmsg);
   void TrackContext(v8::Local<v8::Context> context);
   void UntrackContext(v8::Local<v8::Context> context);
+  void PurgeTrackedEmptyContexts();
 
   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.
-    return BaseObjectPtr<ContextifyContext>();
+    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<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();
 }
 
 void ContextifyContext::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()) {
-    return BaseObjectPtr<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()) {
-        return BaseObjectPtr<ContextifyContext>();
+        return {};
       }
     }
 
@@ -328,21 +319,23 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
                          env->host_defined_option_symbol(),
                          options->host_defined_options_id)
             .IsNothing()) {
-      return BaseObjectPtr<ContextifyContext>();
+      return {};
     }
 
     env->AssignToContext(v8_context, nullptr, info);
 
     if (!env->contextify_wrapper_template()
              ->NewInstance(v8_context)
              .ToLocal(&wrapper)) {
-      return BaseObjectPtr<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()) {
-    return BaseObjectPtr<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()) {
-    return BaseObjectPtr<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.
   return Get(args.This());
 }
 

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<v8::Object> wrapper,
                     v8::Local<v8::Context> 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<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 {
-    return PersistentToLocal::Default(env()->isolate(), context_);
+    return context_.Get(env()->isolate());
   }
 
   inline v8::Local<v8::Object> global_proxy() const {
@@ -75,14 +131,14 @@ class ContextifyContext : public BaseObject {
   static void InitializeGlobalTemplates(IsolateData* isolate_data);
 
  private:
-  static BaseObjectPtr<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()
-  static BaseObjectPtr<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);
 
   static bool IsStillInitializing(const ContextifyContext* ctx);
   static void MakeContext(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -140,7 +196,7 @@ class ContextifyContext : public BaseObject {
   static void IndexedPropertyEnumeratorCallback(
       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();
 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.');
   }
 }