src: use per-type tags for cppgc wrappers

src/README.md

@@ -1120,6 +1120,9 @@ A `cppgc`-managed native wrapper should look something like this:
 class MyWrap final : CPPGC_MIXIN(MyWrap) {
  public:
   SET_CPPGC_NAME(MyWrap)  // Sets the heap snapshot name to "Node / MyWrap"
+  // Uses CppgcWrapperTag::kMyWrap from src/node_v8_embedder.h when wrapping
+  // and unwrapping. Each class needs its own tag.
+  SET_CPPGC_TAG(MyWrap)
 
   // The constructor can only be called by `cppgc::MakeGarbageCollected()`.
   MyWrap(Environment* env, v8::Local<v8::Object> object);

src/cppgc_helpers-inl.h

@@ -15,7 +15,7 @@ void CppgcMixin::Wrap(T* ptr, Realm* realm, v8::Local<v8::Object> obj) {
   ptr->traced_reference_ = v8::TracedReference<v8::Object>(isolate, obj);
   // Note that ptr must be of concrete type T in Wrap.
   auto* wrappable = static_cast<v8::Object::Wrappable*>(ptr);
-  v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(isolate, obj, wrappable);
+  v8::Object::Wrap<T::kCppHeapPointerTag>(isolate, obj, wrappable);
   // Keep the layout consistent with BaseObjects.
   obj->SetAlignedPointerInInternalField(
       kEmbedderType,
@@ -32,19 +32,13 @@ void CppgcMixin::Wrap(T* ptr, Environment* env, v8::Local<v8::Object> obj) {
 
 template <typename T>
 T* CppgcMixin::Unwrap(v8::Local<v8::Object> obj) {
-  // We are not using v8::Object::Unwrap currently because that requires
-  // access to isolate which the ASSIGN_OR_RETURN_UNWRAP macro that we'll shim
-  // with ASSIGN_OR_RETURN_UNWRAP_GC doesn't take, and we also want a
-  // signature consistent with BaseObject::Unwrap() to avoid churn. Since
-  // cppgc-managed objects share the same layout as BaseObjects, just unwrap
-  // from the pointer in the internal field, which should be valid as long as
-  // the object is still alive.
+  // The isolate is not passed in to keep the signature consistent with
+  // BaseObject::Unwrap() and the ASSIGN_OR_RETURN_UNWRAP_CPPGC macro.
   if (obj->InternalFieldCount() != T::kInternalFieldCount) {
     return nullptr;
   }
-  T* ptr = static_cast<T*>(obj->GetAlignedPointerFromInternalField(
-      T::kSlot, EmbedderDataTag::kDefault));
-  return ptr;
+  return v8::Object::Unwrap<T::kCppHeapPointerTag, T>(v8::Isolate::GetCurrent(),
+                                                      obj);
 }
 
 v8::Local<v8::Object> CppgcMixin::object() const {

src/cppgc_helpers.h

@@ -8,6 +8,7 @@
 #include "cppgc/name-provider.h"
 #include "cppgc/persistent.h"
 #include "memory_tracker.h"
+#include "node_v8_embedder.h"
 #include "util.h"
 #include "v8-cppgc.h"
 #include "v8-sandbox.h"
@@ -37,6 +38,7 @@ class CppgcWrapperListNode;
  * class MyWrap final : CPPGC_MIXIN(MyWrap) {
  *  public:
  *   SET_CPPGC_NAME(MyWrap)  // Sets the heap snapshot name to "Node / MyWrap"
+ *   SET_CPPGC_TAG(MyWrap)   // Uses CppgcWrapperTag::kMyWrap for Wrap/Unwrap
  *   void Trace(cppgc::Visitor* visitor) const final {
  *     CppgcMixin::Trace(visitor);
  *     visitor->Trace(...);  // Trace any additional owned traceable data
@@ -126,6 +128,12 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer {
   }                                                                            \
   inline const char* MemoryInfoName() const override { return #Klass; }
 
+// This macro sets the tag used to wrap and unwrap the class. Each class needs
+// its own entry in node::CppgcWrapperTag.
+#define SET_CPPGC_TAG(Klass)                                                   \
+  static constexpr v8::CppHeapPointerTag kCppHeapPointerTag =                  \
+      ToCppHeapPointerTag(CppgcWrapperTag::k##Klass);
+
 /**
  * Similar to ASSIGN_OR_RETURN_UNWRAP() but works on cppgc-managed types
  * inheriting CppgcMixin.

src/env.cc

@@ -668,7 +668,7 @@ IsolateData::~IsolateData() {
 void SetCppgcReference(Isolate* isolate,
                        Local<Object> object,
                        v8::Object::Wrappable* wrappable) {
-  v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(
+  v8::Object::Wrap<ToCppHeapPointerTag(CppgcWrapperTag::kSetCppgcReference)>(
       isolate, object, wrappable);
 }
 

src/node_contextify.h

@@ -76,6 +76,7 @@ struct ContextOptions {
 class ContextifyContext final : CPPGC_MIXIN(ContextifyContext) {
  public:
   SET_CPPGC_NAME(ContextifyContext)
+  SET_CPPGC_TAG(ContextifyContext)
   void Trace(cppgc::Visitor* visitor) const final;
   SET_NO_MEMORY_INFO()
 
@@ -192,6 +193,7 @@ class ContextifyContext final : CPPGC_MIXIN(ContextifyContext) {
 class ContextifyScript final : CPPGC_MIXIN(ContextifyScript) {
  public:
   SET_CPPGC_NAME(ContextifyScript)
+  SET_CPPGC_TAG(ContextifyScript)
   void Trace(cppgc::Visitor* visitor) const final;
   SET_NO_MEMORY_INFO()
 

src/node_v8_embedder.h

@@ -4,6 +4,7 @@
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #include <cstdint>
+#include "v8-sandbox.h"
 
 namespace node {
 
@@ -19,6 +20,27 @@ enum EmbedderDataTag : uint16_t {
   kPerContextData,
 };
 
+// Tags for cppgc-managed wrappers passed to v8::Object::Wrap(). Each wrapper
+// type needs its own tag so that, with the V8 sandbox enabled, unwrapping an
+// object as the wrong type yields nullptr.
+enum class CppgcWrapperTag : uint16_t {
+  // Used by the deprecated SetCppgcReference(), which wraps embedder types
+  // unknown to Node.js.
+  kSetCppgcReference =
+      static_cast<uint16_t>(v8::CppHeapPointerTag::kFirstEmbedderWrappableTag),
+  kContextifyContext,
+  kContextifyScript,
+  kLast = kContextifyScript,
+};
+
+static_assert(
+    static_cast<uint16_t>(CppgcWrapperTag::kLast) <=
+    static_cast<uint16_t>(v8::CppHeapPointerTag::kLastEmbedderWrappableTag));
+
+constexpr v8::CppHeapPointerTag ToCppHeapPointerTag(CppgcWrapperTag tag) {
+  return static_cast<v8::CppHeapPointerTag>(tag);
+}
+
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

test/addons/cppgc-object/binding.cc

@@ -10,7 +10,7 @@
 
 class CppGCed : public v8::Object::Wrappable {
  public:
-  static uint16_t states[2];
+  static uint16_t* states;
   static constexpr int kDestructCount = 0;
   static constexpr int kTraceCount = 1;
 
@@ -38,18 +38,20 @@ class CppGCed : public v8::Object::Wrappable {
   void Trace(cppgc::Visitor* visitor) const { states[kTraceCount]++; }
 };
 
-uint16_t CppGCed::states[] = {0, 0};
+uint16_t* CppGCed::states = nullptr;
 
 void InitModule(v8::Local<v8::Object> exports) {
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
   auto context = isolate->GetCurrentContext();
 
-  auto store = v8::ArrayBuffer::NewBackingStore(
-      CppGCed::states,
-      sizeof(uint16_t) * 2,
-      [](void*, size_t, void*) {},
-      nullptr);
-  auto ab = v8::ArrayBuffer::New(isolate, std::move(store));
+  // Let V8 allocate the counters, as the V8 sandbox does not allow external
+  // backing stores. The store is leaked so that the counters outlive the
+  // CppHeap, whose teardown runs the destructors.
+  static auto* store = new std::shared_ptr<v8::BackingStore>(
+      v8::ArrayBuffer::NewBackingStore(isolate, sizeof(uint16_t) * 2));
+  CppGCed::states = static_cast<uint16_t*>((*store)->Data());
+  std::fill_n(CppGCed::states, 2, 0);
+  auto ab = v8::ArrayBuffer::New(isolate, *store);
 
   exports
       ->Set(context,

test/addons/null-buffer-neuter/binding.cc

@@ -28,8 +28,12 @@ void Run(const v8::FunctionCallbackInfo<v8::Value>& args) {
           FreeCallback,
           nullptr).ToLocalChecked();
 
+    assert(node::Buffer::Length(buf) == 0);
+#ifndef V8_ENABLE_SANDBOX
+    // With the V8 sandbox, empty buffers point into the sandbox instead.
     char* data = node::Buffer::Data(buf);
     assert(data == nullptr);
+#endif
   }
 }
 

test/addons/worker-buffer-callback/test-free-called.js

@@ -14,6 +14,10 @@ assert.strictEqual(getFreeCallCount(), 0);
 
 const w = new Worker(`require(${JSON.stringify(binding)})`, { eval: true });
 
+// With the V8 sandbox, the data is copied into the sandbox and freed right
+// away, so the buffer created when loading the binding above is freed too.
+const expected = common.hasV8Sandbox ? 2 : 1;
+
 w.on('exit', common.mustCall(() => {
-  assert.strictEqual(getFreeCallCount(), 1);
+  assert.strictEqual(getFreeCallCount(), expected);
 }));

test/cctest/test_cppgc.cc

@@ -133,6 +133,8 @@ using node::CppgcMixin;
 class RealmBoundWrap final : CPPGC_MIXIN(RealmBoundWrap) {
  public:
   SET_CPPGC_NAME(RealmBoundWrap)
+  static constexpr v8::CppHeapPointerTag kCppHeapPointerTag =
+      v8::CppHeapPointerTag::kLastEmbedderWrappableTag;
   DEFAULT_CPPGC_TRACE()
   SET_NO_MEMORY_INFO()
 

test/cctest/test_environment_shared_isolate.cc

@@ -298,7 +298,7 @@ TEST_P(SharedIsolateTest, EnvironmentsComeAndGoWhileSiblingsRun) {
                                    ->InstanceTemplate()
                                    ->NewInstance(context)
                                    .ToLocalChecked();
-    v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(
+    v8::Object::Wrap<v8::CppHeapPointerTag::kFirstEmbedderWrappableTag>(
         isolate_,
         holder,
         cppgc::MakeGarbageCollected<EmbedderObject>(

test/embedding/shared_embedtest.cc

@@ -39,7 +39,7 @@ int main(int argc, char* argv[]) {
     v8::Local<v8::Object> obj = v8::Object::New(isolate);
     Wrappable* wrappable = cppgc::MakeGarbageCollected<Wrappable>(
         isolate->GetCppHeap()->GetAllocationHandle());
-    v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(
+    v8::Object::Wrap<v8::CppHeapPointerTag::kFirstEmbedderWrappableTag>(
         isolate, obj, wrappable);
     v8::Local<v8::String> source =
         v8::String::NewFromUtf8Literal(isolate, "'Hello' + ', World!'");