diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index f2729d34c12..aaecd8cb4f3 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -2450,8 +2450,9 @@ kj::Maybe> Worker::Lock::getExportedHandler( } api::ServiceWorkerGlobalScope& Worker::Lock::getGlobalScope() { - return KJ_ASSERT_NONNULL(jsg::getAlignedPointerFromEmbedderData( - getContext(), jsg::ContextPointerSlot::GLOBAL_WRAPPER)); + auto context = getContext(); + return jsg::extractInternalPointer( + context, context->Global()); } TimeoutId::Generator& Worker::Lock::getTimeoutIdGenerator() { diff --git a/src/workerd/jsg/README.md b/src/workerd/jsg/README.md index 92e7f6d5f7d..1760eeabbda 100644 --- a/src/workerd/jsg/README.md +++ b/src/workerd/jsg/README.md @@ -480,11 +480,10 @@ Both may take additional `TypeHandler&` trailing parameters. | Slot | Enum | Purpose | | ---- | -------------------------- | --------------------------------- | | 0 | `RESERVED` | **Never use** — reserved by V8 | -| 1 | `GLOBAL_WRAPPER` | Pointer to global object wrapper | -| 2 | `MODULE_REGISTRY` | Pointer to module registry | -| 3 | `EXTENDED_CONTEXT_WRAPPER` | Extended type wrapper for context | -| 4 | `VIRTUAL_FILE_SYSTEM` | Virtual file system | -| 5 | `RUST_REALM` | Rust realm pointer | +| 1 | `MODULE_REGISTRY` | Pointer to module registry | +| 2 | `EXTENDED_CONTEXT_WRAPPER` | Extended type wrapper for context | +| 3 | `VIRTUAL_FILE_SYSTEM` | Virtual file system | +| 4 | `RUST_REALM` | Rust realm pointer | ## Wrappable Lifecycle diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index f96a240e93b..3353d416d5a 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -1521,8 +1521,10 @@ class Ref { // // It is an error to attach a wrapper when another wrapper is already attached. Hence, // typically this should only be called on a newly-allocated object. - void attachWrapper(v8::Isolate* isolate, v8::Local object) { - inner->Wrappable::attachWrapper(isolate, object, resourceNeedsGcTracing()); + void attachWrapper(v8::Isolate* isolate, + v8::Local object, + IsContextGlobal isContextGlobal = IsContextGlobal::NO) { + inner->Wrappable::attachWrapper(isolate, object, resourceNeedsGcTracing(), isContextGlobal); } // Obtain a weak reference to the referenced object. The weak reference does not keep the @@ -3208,10 +3210,8 @@ class Lock { // Returns the capnp::SchemaLoader for this isolate/context template const capnp::SchemaLoader& getCapnpSchemaLoader() const { - return KJ_ASSERT_NONNULL( - jsg::getAlignedPointerFromEmbedderData( - v8Isolate->GetCurrentContext(), ContextPointerSlot::GLOBAL_WRAPPER)) - .getSchemaLoader(); + auto context = v8Isolate->GetCurrentContext(); + return extractInternalPointer(context, context->Global()).getSchemaLoader(); } private: diff --git a/src/workerd/jsg/resource.h b/src/workerd/jsg/resource.h index 73ab5b2b9d0..40c13d78407 100644 --- a/src/workerd/jsg/resource.h +++ b/src/workerd/jsg/resource.h @@ -1882,18 +1882,7 @@ class ResourceWrapper { T*, Args&&... args) { // Construct an instance of this type to be used as the Javascript global object, creating - // a new JavaScript context. Unfortunately, we have to do some things differently in this - // case, because of quirks in how V8 handles the global object. There appear to be bugs - // that prevent it from being treated uniformly for callback purposes. See: - // - // https://groups.google.com/d/msg/v8-users/RET5b3KOa5E/3EvpRBzwAQAJ - // - // Because of this, our entire type registration system threads through an extra template - // parameter `bool isContext`. When the application decides to create a context using this - // type as the global, we instantiate this separate branch specifically for that type. - // Fortunately, for types that are never used as the global object, we never have to - // instantiate the `isContext = true` branch. - + // a new JavaScript context. auto isolate = js.v8Isolate; auto tmpl = getTemplate(isolate, nullptr)->InstanceTemplate(); v8::Local context = v8::Context::New(isolate, nullptr, tmpl); @@ -1903,7 +1892,8 @@ class ResourceWrapper { if constexpr (T::jsgHasReflection) { ptr->jsgInitReflection(static_cast(*this)); } - ptr.attachWrapper(isolate, global); + ptr.attachWrapper(isolate, global, IsContextGlobal::YES); + KJ_DASSERT((&extractInternalPointer(context, global) == ptr.get())); // Disable `eval(code)` and `new Function(code)`. (Actually, setting this to `false` really // means "call the callback registered on the isolate to check" -- setting it to `true` means @@ -1925,20 +1915,12 @@ class ResourceWrapper { deleteWeakRefGlobals(isolate, context); } - // Store a pointer to this object in slot 1, to be extracted in callbacks. - jsg::setAlignedPointerInEmbedderData( - context, jsg::ContextPointerSlot::GLOBAL_WRAPPER, ptr.get()); // We need to set the highest used index in every context we create to be a nullptr // This is because we might later on call GetAlignedPointerFromEmbedderData which fails with // a fatal error if the array is smaller than the given index. jsg::setAlignedPointerInEmbedderData( context, jsg::ContextPointerSlot::MAX_POINTER_SLOT, nullptr); - // (Note: V8 docs say: "Note that index 0 currently has a special meaning for Chrome's - // debugger." We aren't Chrome, but it does appear that some versions of V8 will mess with - // slot 0, causing us to segfault if we try to put anything there. So we avoid it and use slot - // 1, which seems to work just fine.) - // Expose the type of the global scope in the global scope itself. exposeGlobalScopeType(isolate, context); diff --git a/src/workerd/jsg/wrappable.c++ b/src/workerd/jsg/wrappable.c++ index 1eb5b63b3e6..b26a83e5f3f 100644 --- a/src/workerd/jsg/wrappable.c++ +++ b/src/workerd/jsg/wrappable.c++ @@ -330,8 +330,10 @@ void Wrappable::traceFromV8(cppgc::Visitor& cppgcVisitor) { jsgVisitForGc(visitor); } -void Wrappable::attachWrapper( - v8::Isolate* isolate, v8::Local object, bool needsGcTracing) { +void Wrappable::attachWrapper(v8::Isolate* isolate, + v8::Local object, + bool needsGcTracing, + IsContextGlobal isContextGlobal) { auto& tracer = HeapTracer::getTracer(isolate); KJ_REQUIRE(wrapper == kj::none); @@ -382,7 +384,17 @@ void Wrappable::attachWrapper( object->SetAlignedPointerInInternalField(WRAPPED_OBJECT_FIELD_INDEX, this, static_cast(WRAPPED_OBJECT_FIELD_INDEX)); - v8::Object::Wrap(isolate, object, tracer.allocateShim(*this)); + auto* shim = tracer.allocateShim(*this); + if (isContextGlobal) { + // Sets the CppHeapPointer on both the JSGlobalProxy (`object`) and its hidden prototype + // (the JSGlobalObject), so unwrapping works whichever of the two V8 hands us as a + // receiver. CHECK-fails inside V8 if `object` is not a JSGlobalProxy. + v8::Object::WrapGlobal(isolate, object, shim, GLOBAL_WRAPPABLE_TAG); + KJ_DASSERT(v8::Object::CheckGlobalWrappable( + isolate, object, v8::CppHeapPointerTagRange(GLOBAL_WRAPPABLE_TAG, GLOBAL_WRAPPABLE_TAG))); + } else { + v8::Object::Wrap(isolate, object, shim); + } if (strongRefcount > 0) { strongWrapper.Reset(isolate, object); @@ -412,6 +424,20 @@ v8::Local Wrappable::attachOpaqueWrapper( return object; } +kj::Maybe Wrappable::tryUnwrapGlobalReceiver( + v8::Isolate* isolate, v8::Local object) { + // In V8 fast API calls the nominal Local receiver may actually hold undefined. + if (object.IsEmpty() || !object.As()->IsObject()) return kj::none; + + auto* shimBase = v8::Object::Unwrap(isolate, object); + if (shimBase == nullptr) return kj::none; + auto* shim = static_cast(shimBase); + KJ_IF_SOME(active, shim->state.tryGet()) { + return *active.wrappable; + } + return kj::none; +} + kj::Maybe Wrappable::tryUnwrapOpaque( v8::Isolate* isolate, v8::Local handle) { if (handle->IsObject()) { diff --git a/src/workerd/jsg/wrappable.h b/src/workerd/jsg/wrappable.h index 849f596b554..a69188f87d2 100644 --- a/src/workerd/jsg/wrappable.h +++ b/src/workerd/jsg/wrappable.h @@ -8,6 +8,8 @@ // This file defines basic helpers involved in wrapping C++ objects for JavaScript consumption, // including garbage-collecting those objects. +#include + #include #include #include @@ -41,16 +43,17 @@ class Visitor; namespace workerd::jsg { +WD_STRONG_BOOL(IsContextGlobal); + // The ContextPointerSlot enum defines the embedder slots we use in v8::Context for // storing pointers to various important objects. enum class ContextPointerSlot : int { // Pointer slot 0 is special and should never be used by us. RESERVED = 0, - GLOBAL_WRAPPER = 1, - MODULE_REGISTRY = 2, - EXTENDED_CONTEXT_WRAPPER = 3, - VIRTUAL_FILE_SYSTEM = 4, - BOOTSTRAP_STATE = 5, + MODULE_REGISTRY = 1, + EXTENDED_CONTEXT_WRAPPER = 2, + VIRTUAL_FILE_SYSTEM = 3, + BOOTSTRAP_STATE = 4, // Keep the MAX_POINTER_SLOT as the last entry and always set to // to the highest value of the other entries. We use this to // ensure that the highest used index is always initialized in @@ -157,6 +160,13 @@ class Wrappable: public kj::Refcounted { static constexpr v8::CppHeapPointerTag WRAPPABLE_TAG = v8::CppHeapPointerTag::kDefaultTag; + // CppHeapPointer tag used for context globals (set via v8::Object::WrapGlobal() on both the + // JSGlobalProxy and the hidden JSGlobalObject). Distinct from WRAPPABLE_TAG so that + // Unwrap(receiver) with this tag matches *only* context globals, never ordinary jsg wrappers. + // Any value in the embedder range works; 0 is kNullTag, so use 1. + static constexpr v8::CppHeapPointerTag GLOBAL_WRAPPABLE_TAG = + static_cast(1); + // The value pointed to by the internal field field `WRAPPABLE_TAG_FIELD_INDEX`. // // This value was chosen randomly. @@ -203,7 +213,20 @@ class Wrappable: public kj::Refcounted { // If `needsGcTracing` is true, then the virtual method jsgVisitForGc() will be called to // perform GC tracing. If false, the method is never called (may be more efficient, if the // method does nothing anyway). - void attachWrapper(v8::Isolate* isolate, v8::Local object, bool needsGcTracing); + // + // If `isContextGlobal` is IsContextGlobal::YES, `object` must be a context's JSGlobalProxy + // (context->Global()); the cppgc wrappable is then set via v8::Object::WrapGlobal() on both + // the proxy and its hidden prototype (the JSGlobalObject), under GLOBAL_WRAPPABLE_TAG. + void attachWrapper(v8::Isolate* isolate, + v8::Local object, + bool needsGcTracing, + IsContextGlobal isContextGlobal = IsContextGlobal::NO); + + // If `object` is a live context global (the JSGlobalProxy or its hidden JSGlobalObject) that + // was attached via attachWrapper(..., IsContextGlobal::YES), returns the attached Wrappable. + // Returns kj::none for any other object. + static kj::Maybe tryUnwrapGlobalReceiver( + v8::Isolate* isolate, v8::Local object); // Attach an empty, null-prototype object as the wrapper. v8::Local attachOpaqueWrapper(v8::Local context, bool needsGcTracing); @@ -379,16 +402,14 @@ class HeapTracer: public v8::EmbedderRootsHandler { template T& extractInternalPointer( const v8::Local& context, const v8::Local& object) { - // Due to bugs in V8, we can't use internal fields on the global object: - // https://groups.google.com/d/msg/v8-users/RET5b3KOa5E/3EvpRBzwAQAJ - // - // So, when wrapping a global object, we store the pointer in the "embedder data" of the context - // instead of the internal fields of the object. - if constexpr (isContext) { - // V8 docs say EmbedderData slot 0 is special, so we use slot 1. (See comments in newContext().) - return KJ_ASSERT_NONNULL( - getAlignedPointerFromEmbedderData(context, ContextPointerSlot::GLOBAL_WRAPPER)); + auto* isolate = v8::Isolate::GetCurrent(); + KJ_IF_SOME(w, Wrappable::tryUnwrapGlobalReceiver(isolate, object)) { + return *reinterpret_cast(&w); + } + // Fall back to the current context's global for the Fast API and instance properties callbacks. + return *reinterpret_cast( + &KJ_ASSERT_NONNULL(Wrappable::tryUnwrapGlobalReceiver(isolate, context->Global()))); } else { KJ_ASSERT(object->InternalFieldCount() == Wrappable::INTERNAL_FIELD_COUNT); auto* ptr = object->GetAlignedPointerFromInternalField(Wrappable::WRAPPED_OBJECT_FIELD_INDEX,