diff --git a/src/README.md b/src/README.md index 7cdf099ff819..7120013236a1 100644 --- a/src/README.md +++ b/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 object); diff --git a/src/cppgc_helpers-inl.h b/src/cppgc_helpers-inl.h index 4db197d694f8..ae9c4d9a5cb8 100644 --- a/src/cppgc_helpers-inl.h +++ b/src/cppgc_helpers-inl.h @@ -15,7 +15,7 @@ void CppgcMixin::Wrap(T* ptr, Realm* realm, v8::Local obj) { ptr->traced_reference_ = v8::TracedReference(isolate, obj); // Note that ptr must be of concrete type T in Wrap. auto* wrappable = static_cast(ptr); - v8::Object::Wrap(isolate, obj, wrappable); + v8::Object::Wrap(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 obj) { template T* CppgcMixin::Unwrap(v8::Local 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(obj->GetAlignedPointerFromInternalField( - T::kSlot, EmbedderDataTag::kDefault)); - return ptr; + return v8::Object::Unwrap(v8::Isolate::GetCurrent(), + obj); } v8::Local CppgcMixin::object() const { diff --git a/src/cppgc_helpers.h b/src/cppgc_helpers.h index 3dfd31bcb29e..a78a483decbf 100644 --- a/src/cppgc_helpers.h +++ b/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. diff --git a/src/env.cc b/src/env.cc index c30b6e2f5049..45a810821ed5 100644 --- a/src/env.cc +++ b/src/env.cc @@ -668,7 +668,7 @@ IsolateData::~IsolateData() { void SetCppgcReference(Isolate* isolate, Local object, v8::Object::Wrappable* wrappable) { - v8::Object::Wrap( + v8::Object::Wrap( isolate, object, wrappable); } diff --git a/src/node_contextify.h b/src/node_contextify.h index be3034da39ff..41c276c308e3 100644 --- a/src/node_contextify.h +++ b/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() diff --git a/src/node_v8_embedder.h b/src/node_v8_embedder.h index 26a98bdb706f..05e6047f75a7 100644 --- a/src/node_v8_embedder.h +++ b/src/node_v8_embedder.h @@ -4,6 +4,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include +#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(v8::CppHeapPointerTag::kFirstEmbedderWrappableTag), + kContextifyContext, + kContextifyScript, + kLast = kContextifyScript, +}; + +static_assert( + static_cast(CppgcWrapperTag::kLast) <= + static_cast(v8::CppHeapPointerTag::kLastEmbedderWrappableTag)); + +constexpr v8::CppHeapPointerTag ToCppHeapPointerTag(CppgcWrapperTag tag) { + return static_cast(tag); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/test/addons/cppgc-object/binding.cc b/test/addons/cppgc-object/binding.cc index 90be16147483..5c84a5218a39 100644 --- a/test/addons/cppgc-object/binding.cc +++ b/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 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::ArrayBuffer::NewBackingStore(isolate, sizeof(uint16_t) * 2)); + CppGCed::states = static_cast((*store)->Data()); + std::fill_n(CppGCed::states, 2, 0); + auto ab = v8::ArrayBuffer::New(isolate, *store); exports ->Set(context, diff --git a/test/addons/null-buffer-neuter/binding.cc b/test/addons/null-buffer-neuter/binding.cc index 7575e29fa850..576f547063f8 100644 --- a/test/addons/null-buffer-neuter/binding.cc +++ b/test/addons/null-buffer-neuter/binding.cc @@ -28,8 +28,12 @@ void Run(const v8::FunctionCallbackInfo& 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 } } diff --git a/test/addons/worker-buffer-callback/test-free-called.js b/test/addons/worker-buffer-callback/test-free-called.js index c15cdde432cd..aef04ced1daf 100644 --- a/test/addons/worker-buffer-callback/test-free-called.js +++ b/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); })); diff --git a/test/cctest/test_cppgc.cc b/test/cctest/test_cppgc.cc index 478098154665..f072c0c450b1 100644 --- a/test/cctest/test_cppgc.cc +++ b/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() diff --git a/test/cctest/test_environment_shared_isolate.cc b/test/cctest/test_environment_shared_isolate.cc index b3807b7bb709..f017c6d3260e 100644 --- a/test/cctest/test_environment_shared_isolate.cc +++ b/test/cctest/test_environment_shared_isolate.cc @@ -298,7 +298,7 @@ TEST_P(SharedIsolateTest, EnvironmentsComeAndGoWhileSiblingsRun) { ->InstanceTemplate() ->NewInstance(context) .ToLocalChecked(); - v8::Object::Wrap( + v8::Object::Wrap( isolate_, holder, cppgc::MakeGarbageCollected( diff --git a/test/embedding/shared_embedtest.cc b/test/embedding/shared_embedtest.cc index f071f43c5b76..e5434a0b1bfc 100644 --- a/test/embedding/shared_embedtest.cc +++ b/test/embedding/shared_embedtest.cc @@ -39,7 +39,7 @@ int main(int argc, char* argv[]) { v8::Local obj = v8::Object::New(isolate); Wrappable* wrappable = cppgc::MakeGarbageCollected( isolate->GetCppHeap()->GetAllocationHandle()); - v8::Object::Wrap( + v8::Object::Wrap( isolate, obj, wrappable); v8::Local source = v8::String::NewFromUtf8Literal(isolate, "'Hello' + ', World!'");