src: integrate ContextifyContext to cppgc

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") 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<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() {}
 
   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/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,