From abb88ce36c55655d7897bf846e1b0a9e9de828b0 Mon Sep 17 00:00:00 2001 From: Dan Lapid Date: Wed, 20 Nov 2024 11:06:38 +0000 Subject: [PATCH] Enable using context-specific type wrapper instances. Sometimes we'd like to have multiple contexts with different typewrapper that were instantiated with different configuration objects. To do that this commit extends the Isolate class, wrapper is now a vector of wrappers, the 0 indexed wrapper is the default wrapper but additional wrappers can be created with `instantiateWrapper` which receives a new Configuration object. To associate a context with its wrapper we use `context->SetAlignedPointerInEmbedderData(3, wrapper)`. --- src/workerd/io/worker.c++ | 8 ++ src/workerd/jsg/resource.h | 4 + src/workerd/jsg/setup.c++ | 27 ------- src/workerd/jsg/setup.h | 161 +++++++++++++++++++++++++++++-------- 4 files changed, 141 insertions(+), 59 deletions(-) diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index 92a56e0730a..fe9524f6ab6 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -1279,6 +1279,10 @@ Worker::Script::Script(kj::Own isolateParam, // (Undocumented, as usual.) context = v8::Context::New(lock.v8Isolate, nullptr, v8::ObjectTemplate::New(lock.v8Isolate)); + // 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. + context->SetAlignedPointerInEmbedderData(3, nullptr); } JSG_WITHIN_CONTEXT_SCOPE(lock, context, [&](jsg::Lock& js) { @@ -2594,6 +2598,10 @@ class Worker::Isolate::InspectorChannelImpl final: public v8_inspector::V8Inspec // We don't know which contexts exist in this isolate, so I guess we have to // create one. Ugh. auto dummyContext = v8::Context::New(lock->v8Isolate); + // 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. + dummyContext->SetAlignedPointerInEmbedderData(3, nullptr); auto& inspector = *KJ_ASSERT_NONNULL(isolate.impl->inspector); inspector.contextCreated(v8_inspector::V8ContextInfo(dummyContext, 1, v8_inspector::StringView(reinterpret_cast("Worker"), 6))); diff --git a/src/workerd/jsg/resource.h b/src/workerd/jsg/resource.h index 898a069f7e8..cfc022ce873 100644 --- a/src/workerd/jsg/resource.h +++ b/src/workerd/jsg/resource.h @@ -1435,6 +1435,10 @@ class ResourceWrapper { // Store a pointer to this object in slot 1, to be extracted in callbacks. context->SetAlignedPointerInEmbedderData(1, 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. + context->SetAlignedPointerInEmbedderData(3, 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 diff --git a/src/workerd/jsg/setup.c++ b/src/workerd/jsg/setup.c++ index dcce75aa10d..10577e659f7 100644 --- a/src/workerd/jsg/setup.c++ +++ b/src/workerd/jsg/setup.c++ @@ -404,33 +404,6 @@ v8::Local IsolateBase::getOpaqueTemplate(v8::Isolate* isol return static_cast(isolate->GetData(0))->opaqueTemplate.Get(isolate); } -void IsolateBase::dropWrappers(kj::Own typeWrapperInstance) { - // Delete all wrappers. - jsg::runInV8Stack([&](jsg::V8StackScope& stackScope) { - v8::Locker lock(ptr); - v8::Isolate::Scope isolateScope(ptr); - - // Make sure everything in the deferred destruction queue is dropped. - clearDestructionQueue(); - clearPendingExternalMemoryDecrement(); - - // We MUST call heapTracer.destroy(), but we can't do it yet because destroying other handles - // may call into the heap tracer. - KJ_DEFER(heapTracer.destroy()); - - // Make sure v8::Globals are destroyed under lock (but not until later). - KJ_DEFER(symbolAsyncDispose.Reset()); - KJ_DEFER(opaqueTemplate.Reset()); - - // Make sure the TypeWrapper is destroyed under lock by declaring a new copy of the variable - // that is destroyed before the lock is released. - kj::Own typeWrapperInstanceInner = kj::mv(typeWrapperInstance); - - // Destroy all wrappers. - heapTracer.clearWrappers(); - }); -} - void IsolateBase::fatalError(const char* location, const char* message) { reportV8FatalError(location, message); } diff --git a/src/workerd/jsg/setup.h b/src/workerd/jsg/setup.h index 82ef29aa1bd..db719775471 100644 --- a/src/workerd/jsg/setup.h +++ b/src/workerd/jsg/setup.h @@ -17,6 +17,7 @@ #include #include +#include namespace workerd::jsg { @@ -295,7 +296,31 @@ class IsolateBase { ~IsolateBase() noexcept(false); KJ_DISALLOW_COPY_AND_MOVE(IsolateBase); - void dropWrappers(kj::Own typeWrapperInstance); + void dropWrappers(auto typeWrapperInstance) { + jsg::runInV8Stack([&](jsg::V8StackScope& stackScope) { + v8::Locker lock(ptr); + v8::Isolate::Scope isolateScope(ptr); + + // Make sure everything in the deferred destruction queue is dropped. + clearDestructionQueue(); + clearPendingExternalMemoryDecrement(); + + // We MUST call heapTracer.destroy(), but we can't do it yet because destroying other handles + // may call into the heap tracer. + KJ_DEFER(heapTracer.destroy()); + + // Make sure v8::Globals are destroyed under lock (but not until later). + KJ_DEFER(symbolAsyncDispose.Reset()); + KJ_DEFER(opaqueTemplate.Reset()); + + // Make sure the TypeWrapper is destroyed under lock by declaring a new copy of the variable + // that is destroyed before the lock is released. + auto typeWrapperInstanceInner = kj::mv(typeWrapperInstance); + + // Destroy all wrappers. + heapTracer.clearWrappers(); + }); + } bool getCaptureThrowsAsRejections() const { return captureThrowsAsRejections; @@ -398,14 +423,20 @@ class Isolate: public IsolateBase { // If different wrappers use different types, then `MetaConfiguration` should be some value that // inherits or defines conversion operators to each required type -- or the individual // configuration types must declare constructors from `MetaConfiguration`. + // If `instantiateTypeWrapper` is false, then the default wrapper will not be instantiated + // and should be instantiated with `instantiateTypeWrapper` before `newContext` is called on + // a jsg::Lock of this Isolate. template explicit Isolate(const V8System& system, MetaConfiguration&& configuration, kj::Own observer, - v8::Isolate::CreateParams createParams = {}) - : IsolateBase(system, kj::mv(createParams), kj::mv(observer)), - wrapper(wrapperSpace.construct(ptr, kj::fwd(configuration))) { - wrapper->initTypeWrapper(); + v8::Isolate::CreateParams createParams = {}, + bool instantiateTypeWrapper = true) + : IsolateBase(system, kj::mv(createParams), kj::mv(observer)) { + wrappers.resize(1); + if (instantiateTypeWrapper) { + instantiateDefaultWrapper(kj::fwd(configuration)); + } } // Use this constructor when no wrappers have any required configuration. @@ -414,23 +445,39 @@ class Isolate: public IsolateBase { v8::Isolate::CreateParams createParams = {}) : Isolate(system, nullptr, kj::mv(observer), kj::mv(createParams)) {} + template + void instantiateDefaultWrapper(MetaConfiguration&& configuration) { + KJ_DASSERT(wrappers[0].get() == nullptr); + auto wrapper = wrapperSpace.construct(ptr, kj::fwd(configuration)); + wrapper->initTypeWrapper(); + wrappers[0] = kj::mv(wrapper); + } + + template + kj::Own& instantiateWrapper(MetaConfiguration&& configuration) { + hasExtraWrappers = true; + return wrappers.add(kj::heap(ptr, kj::fwd(configuration))); + } + ~Isolate() noexcept(false) { - dropWrappers(kj::mv(wrapper)); + dropWrappers(wrappers.releaseAsArray()); } + public: kj::Exception unwrapException( v8::Local context, v8::Local exception) override { - return wrapper->template unwrap( + return getWrapperByContext(context)->template unwrap( context, exception, jsg::TypeErrorContext::other()); } v8::Local wrapException( v8::Local context, kj::Exception&& exception) override { - return wrapper->wrap(context, kj::none, kj::fwd(exception)); + return getWrapperByContext(context)->wrap(context, kj::none, kj::fwd(exception)); } bool serialize( Lock& js, std::type_index type, jsg::Object& instance, Serializer& serializer) override { + auto* wrapper = getWrapperByContext(js.v8Context()); KJ_IF_SOME(func, wrapper->serializerMap.find(type)) { func(*wrapper, js, instance, serializer); return true; @@ -440,6 +487,7 @@ class Isolate: public IsolateBase { } kj::Maybe> deserialize( Lock& js, uint tag, Deserializer& deserializer) override { + auto* wrapper = getWrapperByContext(js.v8Context()); KJ_IF_SOME(func, wrapper->deserializerMap.find(tag)) { return func(*wrapper, js, tag, deserializer); } else { @@ -475,21 +523,22 @@ class Isolate: public IsolateBase { // Wrap a C++ value, returning a v8::Local (possibly of a specific type). template auto wrap(v8::Local context, T&& value) { - return jsgIsolate.wrapper->wrap(context, kj::none, kj::fwd(value)); + return jsgIsolate.getWrapperByContext(context)->wrap(context, kj::none, kj::fwd(value)); } // Wrap a context-independent value. Only a few built-in types, like numbers and strings, // can be wrapped without a context. template auto wrapNoContext(T&& value) { - return jsgIsolate.wrapper->wrap(v8Isolate, kj::none, kj::fwd(value)); + return jsgIsolate.getWrapperByContext(v8Context()) + ->wrap(v8Isolate, kj::none, kj::fwd(value)); } // Convert a JavaScript value to a C++ value, or throw a JS exception if the type doesn't // match. template auto unwrap(v8::Local context, v8::Local handle) { - return jsgIsolate.wrapper->template unwrap( + return jsgIsolate.getWrapperByContext(context)->template unwrap( context, handle, jsg::TypeErrorContext::other()); } @@ -497,7 +546,8 @@ class Isolate: public IsolateBase { kj::String name, kj::String message, kj::Maybe maybeStack) override { return withinHandleScope([&] { v8::Local tmpl = - jsgIsolate.wrapper->getTemplate(v8Isolate, static_cast(nullptr)); + jsgIsolate.getWrapperByContext(v8Context()) + ->getTemplate(v8Isolate, static_cast(nullptr)); KJ_DASSERT(!tmpl.IsEmpty()); v8::Local obj = check(tmpl->InstanceTemplate()->NewInstance(v8Context())); v8::Local stackName = str("stack"_kjc); @@ -533,47 +583,68 @@ class Isolate: public IsolateBase { template jsg::JsObject getConstructor(v8::Local context) { v8::EscapableHandleScope scope(v8Isolate); - v8::Local tpl = jsgIsolate.wrapper->getTemplate(v8Isolate, (T*)nullptr); + v8::Local tpl = + jsgIsolate.getWrapperByContext(context)->getTemplate(v8Isolate, (T*)nullptr); v8::Local prototype = check(tpl->GetFunction(context)); return jsg::JsObject(scope.Escape(prototype)); } v8::Local wrapBytes(kj::Array data) override { - return jsgIsolate.wrapper->wrap(v8Isolate, kj::none, kj::mv(data)); + return jsgIsolate.getWrapperByContext(v8Context())->wrap(v8Isolate, kj::none, kj::mv(data)); } v8::Local wrapSimpleFunction(v8::Local context, jsg::Function& info)> simpleFunction) override { - return jsgIsolate.wrapper->wrap(context, kj::none, kj::mv(simpleFunction)); + return jsgIsolate.getWrapperByContext(context)->wrap( + context, kj::none, kj::mv(simpleFunction)); } v8::Local wrapReturningFunction(v8::Local context, jsg::Function(const v8::FunctionCallbackInfo& info)> returningFunction) override { - return jsgIsolate.wrapper->wrap(context, kj::none, kj::mv(returningFunction)); + return jsgIsolate.getWrapperByContext(context)->wrap( + context, kj::none, kj::mv(returningFunction)); } v8::Local wrapPromiseReturningFunction(v8::Local context, jsg::Function(const v8::FunctionCallbackInfo& info)> returningFunction) override { - return jsgIsolate.wrapper->wrap(context, kj::none, kj::mv(returningFunction)); + return jsgIsolate.getWrapperByContext(context)->wrap( + context, kj::none, kj::mv(returningFunction)); } kj::String toString(v8::Local value) override { - return jsgIsolate.wrapper->template unwrap( - v8Isolate->GetCurrentContext(), value, jsg::TypeErrorContext::other()); + return jsgIsolate.getWrapperByContext(v8Context()) + ->template unwrap( + v8Isolate->GetCurrentContext(), value, jsg::TypeErrorContext::other()); } jsg::Dict> toDict(v8::Local value) override { - return jsgIsolate.wrapper->template unwrap>>( - v8Isolate->GetCurrentContext(), value, jsg::TypeErrorContext::other()); + return jsgIsolate.getWrapperByContext(v8Context()) + ->template unwrap>>( + v8Isolate->GetCurrentContext(), value, jsg::TypeErrorContext::other()); } jsg::Dict toDict(const jsg::JsValue& value) override { - return jsgIsolate.wrapper->template unwrap>( - v8Isolate->GetCurrentContext(), value, jsg::TypeErrorContext::other()); + return jsgIsolate.getWrapperByContext(v8Context()) + ->template unwrap>( + v8Isolate->GetCurrentContext(), value, jsg::TypeErrorContext::other()); } v8::Local wrapSimplePromise(jsg::Promise promise) override { - return jsgIsolate.wrapper->wrap(v8Context(), kj::none, kj::mv(promise)); + return jsgIsolate.getWrapperByContext(v8Context()) + ->wrap(v8Context(), kj::none, kj::mv(promise)); } jsg::Promise toPromise(v8::Local promise) override { - return jsgIsolate.wrapper->template unwrap>( - v8Isolate->GetCurrentContext(), promise, jsg::TypeErrorContext::other()); + return jsgIsolate.getWrapperByContext(v8Context()) + ->template unwrap>( + v8Isolate->GetCurrentContext(), promise, jsg::TypeErrorContext::other()); + } + + template + JsContext newContextWithWrapper( + kj::Own& wrapper, NewContextOptions options, Args&&... args) { + // TODO(soon): Requiring move semantics for the global object is awkward. This should instead + // allocate the object (forwarding arguments to the constructor) and return something like + // a Ref. + auto context = wrapper->newContext(*this, options, jsgIsolate.getObserver(), + static_cast(nullptr), kj::fwd(args)...); + context.getHandle(v8Isolate)->SetAlignedPointerInEmbedderData(3, wrapper.get()); + return context; } // Creates a new JavaScript "context", i.e. the global object. This is the first step to @@ -584,8 +655,9 @@ class Isolate: public IsolateBase { // TODO(soon): Requiring move semantics for the global object is awkward. This should instead // allocate the object (forwarding arguments to the constructor) and return something like // a Ref. - return jsgIsolate.wrapper->newContext(*this, options, jsgIsolate.getObserver(), - static_cast(nullptr), kj::fwd(args)...); + KJ_DASSERT(!jsgIsolate.wrappers.empty()); + KJ_DASSERT(jsgIsolate.wrappers[0].get() != nullptr); + return newContextWithWrapper(jsgIsolate.wrappers[0], options, kj::fwd(args)...); } // Creates a new JavaScript "context", i.e. the global object. This is the first step to @@ -596,10 +668,17 @@ class Isolate: public IsolateBase { return newContext(NewContextOptions{}, kj::fwd(args)...); } + template + JsContext newContextWithConfiguration( + MetaConfiguration&& configuration, NewContextOptions options, Args&&... args) { + auto& wrapper = jsgIsolate.instantiateWrapper(kj::fwd(configuration)); + return newContextWithWrapper(wrapper, options, kj::fwd(args)...); + } + void reportError(const JsValue& value) override { KJ_IF_SOME(domException, - jsgIsolate.wrapper->tryUnwrap( - v8Context(), value, static_cast(nullptr), kj::none)) { + jsgIsolate.getWrapperByContext(v8Context()) + ->tryUnwrap(v8Context(), value, static_cast(nullptr), kj::none)) { auto desc = kj::str("DOMException(", domException.getName(), "): ", domException.getMessage()); jsgIsolate.reportError(*this, kj::mv(desc), value, JsMessage::create(*this, value)); @@ -615,7 +694,7 @@ class Isolate: public IsolateBase { virtual kj::Maybe getInstance( v8::Local obj, const std::type_info& type) override { auto instance = v8::Local(obj)->FindInstanceInPrototypeChain( - jsgIsolate.wrapper->getDynamicTypeInfo(v8Isolate, type).tmpl); + jsgIsolate.getWrapperByContext(v8Context())->getDynamicTypeInfo(v8Isolate, type).tmpl); if (instance.IsEmpty()) { return kj::none; } else { @@ -634,9 +713,27 @@ class Isolate: public IsolateBase { }); } + protected: + inline TypeWrapper* getWrapperByContext(auto context) { + if (KJ_LIKELY(!hasExtraWrappers)) { + return wrappers[0].get(); + } else { + auto ptr = context->GetAlignedPointerFromEmbedderData(3); + if (KJ_LIKELY(ptr != nullptr)) { + return static_cast(ptr); + } else { + // This can happen when we create dummy contexts such as in worker.c++. + return wrappers[0].get(); + } + } + } + private: kj::SpaceFor wrapperSpace; - kj::Own wrapper; // Needs to be destroyed under lock... + kj::Vector> wrappers; // Needs to be destroyed under lock... + // This is just an optimization boolean, when we only have one wrapper we can skip calling + // GetAlignedPointerFromEmbedderData and just return wrappers[0]. + bool hasExtraWrappers = false; }; // This macro helps cut down on template spam in error messages. Instead of instantiating Isolate