From 6656c7c7a3af7ea4940fe1a89bc8db24b0d2fd2d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 9 Aug 2023 15:53:22 -0700 Subject: [PATCH] JSG Completion: use getHandle(jsg::Lock&) consistently --- src/workerd/api/actor-state-test.c++ | 6 +++--- src/workerd/api/crypto-impl-aes-test.c++ | 2 +- src/workerd/api/gpu/gpu-buffer.c++ | 9 ++++----- src/workerd/api/http.c++ | 2 +- src/workerd/api/kv.c++ | 4 ++-- src/workerd/api/queue.c++ | 2 +- src/workerd/api/sockets.c++ | 2 +- src/workerd/api/sockets.h | 2 +- src/workerd/api/streams/queue-test.c++ | 2 +- src/workerd/api/streams/standard.c++ | 2 +- src/workerd/io/io-context.c++ | 2 +- src/workerd/io/io-context.h | 4 ++-- src/workerd/io/worker.c++ | 10 +++++----- src/workerd/jsg/buffersource.c++ | 2 +- src/workerd/jsg/buffersource.h | 2 +- src/workerd/jsg/function-test.c++ | 2 +- src/workerd/jsg/function.h | 4 ++-- src/workerd/jsg/jsg.h | 12 +++++++++--- src/workerd/jsg/promise-test.c++ | 4 ++-- src/workerd/jsg/promise.c++ | 2 +- src/workerd/jsg/promise.h | 4 ++-- src/workerd/jsg/setup-test.c++ | 4 ++-- 22 files changed, 45 insertions(+), 40 deletions(-) diff --git a/src/workerd/api/actor-state-test.c++ b/src/workerd/api/actor-state-test.c++ index f3ad335377b..5ad0444cb3f 100644 --- a/src/workerd/api/actor-state-test.c++ +++ b/src/workerd/api/actor-state-test.c++ @@ -36,7 +36,7 @@ KJ_TEST("v8 serialization version tag hasn't changed") { jsg::V8StackScope stackScope; ActorStateIsolate::Lock isolateLock(actorStateIsolate, stackScope); isolateLock.withinHandleScope([&] { - auto v8Context = isolateLock.newContext().getHandle(isolateLock.v8Isolate); + auto v8Context = isolateLock.newContext().getHandle(isolateLock); auto contextScope = isolateLock.enterContextScope(v8Context); auto trueVal = v8::True(isolateLock.v8Isolate); @@ -66,7 +66,7 @@ KJ_TEST("we support deserializing up to v15") { jsg::V8StackScope stackScope; ActorStateIsolate::Lock isolateLock(actorStateIsolate, stackScope); isolateLock.withinHandleScope([&] { - auto v8Context = isolateLock.newContext().getHandle(isolateLock.v8Isolate); + auto v8Context = isolateLock.newContext().getHandle(isolateLock); auto contextScope = isolateLock.enterContextScope(v8Context); kj::Vector testCases; @@ -115,7 +115,7 @@ KJ_TEST("wire format version does not change deserialization behavior on real da jsg::V8StackScope stackScope; ActorStateIsolate::Lock isolateLock(actorStateIsolate, stackScope); isolateLock.withinHandleScope([&] { - auto v8Context = isolateLock.newContext().getHandle(isolateLock.v8Isolate); + auto v8Context = isolateLock.newContext().getHandle(isolateLock); auto contextScope = isolateLock.enterContextScope(v8Context); // Read in data line by line and verify that it round trips (serializes and then deserializes) diff --git a/src/workerd/api/crypto-impl-aes-test.c++ b/src/workerd/api/crypto-impl-aes-test.c++ index c2069418a10..a9546fcfb3c 100644 --- a/src/workerd/api/crypto-impl-aes-test.c++ +++ b/src/workerd/api/crypto-impl-aes-test.c++ @@ -96,7 +96,7 @@ KJ_TEST("AES-CTR key wrap") { auto isolate = isolateLock.v8Isolate; isolateLock.withinHandleScope([&] { - auto context = isolateLock.newContext().getHandle(isolate); + auto context = isolateLock.newContext().getHandle(isolateLock); auto contextScope = isolateLock.enterContextScope(context); auto& js = jsg::Lock::from(isolate); diff --git a/src/workerd/api/gpu/gpu-buffer.c++ b/src/workerd/api/gpu/gpu-buffer.c++ index b30c21c78b9..a8cb376daf3 100644 --- a/src/workerd/api/gpu/gpu-buffer.c++ +++ b/src/workerd/api/gpu/gpu-buffer.c++ @@ -65,18 +65,17 @@ GPUBuffer::getMappedRange(jsg::Lock& js, jsg::Optional offset, v8::Local arrayBuffer = v8::ArrayBuffer::New(js.v8Isolate, backing); - arrayBuffer->SetDetachKey(detachKey_.getHandle(js.v8Isolate)); + arrayBuffer->SetDetachKey(detachKey_.getHandle(js)); - mapped_.add(Mapping{start, end, - jsg::V8Ref(js.v8Isolate, arrayBuffer)}); + mapped_.add(Mapping{ start, end, js.v8Ref(arrayBuffer) }); return arrayBuffer; } void GPUBuffer::DetachMappings(jsg::Lock& js) { for (auto& mapping : mapped_) { - auto ab = mapping.buffer.getHandle(js.v8Isolate); + auto ab = mapping.buffer.getHandle(js); - auto res = ab->Detach(detachKey_.getHandle(js.v8Isolate)); + auto res = ab->Detach(detachKey_.getHandle(js)); KJ_ASSERT(res.IsJust()); } mapped_.clear(); diff --git a/src/workerd/api/http.c++ b/src/workerd/api/http.c++ index 4c4153a96f4..df7dc842095 100644 --- a/src/workerd/api/http.c++ +++ b/src/workerd/api/http.c++ @@ -1408,7 +1408,7 @@ kj::Maybe> Response::getWebSocket(jsg::Lock& js) { jsg::Optional> Response::getCf( const v8::PropertyCallbackInfo& info) { return cf.map([&](jsg::V8Ref& handle) { - return handle.getHandle(info.GetIsolate()); + return handle.getHandle(jsg::Lock::from(info.GetIsolate())); }); } diff --git a/src/workerd/api/kv.c++ b/src/workerd/api/kv.c++ index 79ba3451ea2..7e45419209a 100644 --- a/src/workerd/api/kv.c++ +++ b/src/workerd/api/kv.c++ @@ -293,9 +293,9 @@ jsg::Promise KvNamespace::list(jsg::Lock& js, jsg::Optional v8::Local { - return cs.getHandle((js.v8Isolate)); + return cs.getHandle(js); })); return result; }); diff --git a/src/workerd/api/queue.c++ b/src/workerd/api/queue.c++ index 8776c2a3a50..f17691920df 100644 --- a/src/workerd/api/queue.c++ +++ b/src/workerd/api/queue.c++ @@ -433,7 +433,7 @@ jsg::Ref startQueueEvent( KJ_IF_MAYBE(h, exportedHandler) { auto queueHandler = KJ_ASSERT_NONNULL(handlerHandler.tryUnwrap( - lock, h->self.getHandle(lock.getIsolate()))); + lock, h->self.getHandle(lock))); KJ_IF_MAYBE(f, queueHandler.queue) { auto promise = (*f)(lock, jsg::alloc(event.addRef()), h->env.addRef(js), h->getCtx()); diff --git a/src/workerd/api/sockets.c++ b/src/workerd/api/sockets.c++ index bd4775c4c60..3c30fadd5b7 100644 --- a/src/workerd/api/sockets.c++ +++ b/src/workerd/api/sockets.c++ @@ -360,7 +360,7 @@ jsg::Promise Socket::maybeCloseWriteSide(jsg::Lock& js) { // been flushed. return writable->getController().close(js).catch_(js, JSG_VISITABLE_LAMBDA((ref=JSG_THIS), (ref), (jsg::Lock& js, jsg::Value&& exc) { - ref->closedResolver.reject(js, exc.getHandle(js.v8Isolate)); + ref->closedResolver.reject(js, exc.getHandle(js)); })).then(js, JSG_VISITABLE_LAMBDA((ref=JSG_THIS), (ref), (jsg::Lock& js) { ref->closedResolver.resolve(js); })); diff --git a/src/workerd/api/sockets.h b/src/workerd/api/sockets.h index 93e68757e5c..cb120bfddf5 100644 --- a/src/workerd/api/sockets.h +++ b/src/workerd/api/sockets.h @@ -119,7 +119,7 @@ class Socket: public jsg::Object { }; jsg::Promise errorHandler(jsg::Lock& js, jsg::Value err) { - auto jsException = err.getHandle(js.v8Isolate); + auto jsException = err.getHandle(js); resolveFulfiller(js, jsg::createTunneledException(js.v8Isolate, jsException)); return js.resolvedPromise(); }; diff --git a/src/workerd/api/streams/queue-test.c++ b/src/workerd/api/streams/queue-test.c++ index 79abd203bef..ed78d7d5598 100644 --- a/src/workerd/api/streams/queue-test.c++ +++ b/src/workerd/api/streams/queue-test.c++ @@ -27,7 +27,7 @@ struct Preamble { : isolate(v8System, kj::heap()), lock(isolate, stackScope), scope(lock.v8Isolate), - context(lock.newContext().getHandle(lock.v8Isolate)), + context(lock.newContext().getHandle(lock)), contextScope(context) {} jsg::Lock& getJs() { return jsg::Lock::from(lock.v8Isolate); } diff --git a/src/workerd/api/streams/standard.c++ b/src/workerd/api/streams/standard.c++ index 4ad1e099f59..0267d11be7b 100644 --- a/src/workerd/api/streams/standard.c++ +++ b/src/workerd/api/streams/standard.c++ @@ -3669,7 +3669,7 @@ jsg::Promise WritableStreamJsController::pipeLoop(jsg::Lock& js) { }; auto promise = write(js, result.value.map([&](jsg::Value& value) { - return value.getHandle(js.v8Isolate); + return value.getHandle(js); })); return maybeAddFunctor(js, kj::mv(promise), kj::mv(onSuccess), kj::mv(onFailure)); diff --git a/src/workerd/io/io-context.c++ b/src/workerd/io/io-context.c++ index 8eef3a04d8b..7512df03b34 100644 --- a/src/workerd/io/io-context.c++ +++ b/src/workerd/io/io-context.c++ @@ -392,7 +392,7 @@ void IoContext::logUncaughtExceptionAsync(UncaughtExceptionSource source, jsg::Lock& js = lock; auto error = js.exceptionToJs(kj::mv(exception)); // TODO(soon): Add logUncaughtException to jsg::Lock. - lock.logUncaughtException(source, error.getHandle(js.v8Isolate)); + lock.logUncaughtException(source, error.getHandle(js)); } }; diff --git a/src/workerd/io/io-context.h b/src/workerd/io/io-context.h index 80527942eaa..a4ae7ce36c9 100644 --- a/src/workerd/io/io-context.h +++ b/src/workerd/io/io-context.h @@ -1416,14 +1416,14 @@ kj::_::ReducePromises> IoContext::awaitJs(jsg::Lock& js, jsg::Pro auto errorHandler = [fulfiller = addObject(kj::addRef(*fulfiller))] - (jsg::Lock&, jsg::Value jsExceptionRef) mutable { + (jsg::Lock& js, jsg::Value jsExceptionRef) mutable { // Note: `context` can possibly be different than the one that started the wait, if the // promise resolved from a different context. In that case the use of `fulfiller` will // throw later on. But it's OK to use the wrong context up until that point. auto& context = IoContext::current(); auto isolate = context.getCurrentLock().getIsolate(); - auto jsException = jsExceptionRef.getHandle(isolate); + auto jsException = jsExceptionRef.getHandle(js); // TODO(someday): We log an "uncaught exception" here whenever a promise returned from JS to // C++ rejects. However, the C++ code waiting on the promise may do its own logging (e.g. diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index ed98b106845..01581a70c86 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -707,7 +707,7 @@ struct Worker::Isolate::Impl { lock->withinHandleScope([&] { context->clear(); KJ_IF_MAYBE(i, impl.inspector) { - i->get()->contextDestroyed(context.getHandle(lock->v8Isolate)); + i->get()->contextDestroyed(context.getHandle(*lock)); } { auto drop = kj::mv(context); } lock->v8Isolate->ContextDisposedNotification(false); @@ -1213,7 +1213,7 @@ Worker::Script::Script(kj::Own isolateParam, kj::StringPtr id, // Modules can't be compiled for multiple contexts. We need to create the real context now. auto& mContext = impl->moduleContext.emplace(isolate->apiIsolate->newContext(lock)); mContext->enableWarningOnSpecialEvents(); - context = mContext.getHandle(lock.v8Isolate); + context = mContext.getHandle(lock); recordedLock.setupContext(context); } else { // Although we're going to compile a script independent of context, V8 requires that there be @@ -1424,7 +1424,7 @@ Worker::Worker(kj::Own scriptParam, jsContext = &this->impl->context.emplace(script->isolate->apiIsolate->newContext(lock)); } - v8::Local context = KJ_REQUIRE_NONNULL(jsContext).getHandle(lock.v8Isolate); + v8::Local context = KJ_REQUIRE_NONNULL(jsContext).getHandle(lock); if (!script->modular) { recordedLock.setupContext(context); } @@ -1716,9 +1716,9 @@ v8::Isolate* Worker::Lock::getIsolate() { v8::Local Worker::Lock::getContext() { KJ_IF_MAYBE(c, worker.impl->context) { - return c->getHandle(impl->inner.v8Isolate); + return c->getHandle(impl->inner); } else KJ_IF_MAYBE(c, const_cast(*worker.script).impl->moduleContext) { - return c->getHandle(impl->inner.v8Isolate); + return c->getHandle(impl->inner); } else { KJ_UNREACHABLE; } diff --git a/src/workerd/jsg/buffersource.c++ b/src/workerd/jsg/buffersource.c++ index 308f36bc4da..153f289abb1 100644 --- a/src/workerd/jsg/buffersource.c++ +++ b/src/workerd/jsg/buffersource.c++ @@ -120,7 +120,7 @@ BackingStore BufferSource::detach(Lock& js) { bool BufferSource::canDetach(Lock& js) { if (isDetached()) return false; - return isDetachable(handle.getHandle(js.v8Isolate)); + return isDetachable(handle.getHandle(js)); } v8::Local BufferSource::getHandle(Lock& js) { diff --git a/src/workerd/jsg/buffersource.h b/src/workerd/jsg/buffersource.h index ce4714b8891..e27d299f767 100644 --- a/src/workerd/jsg/buffersource.h +++ b/src/workerd/jsg/buffersource.h @@ -317,7 +317,7 @@ class BufferSource { if (isDetached()) { return nullptr; } - auto h = handle.getHandle(js.v8Isolate); + auto h = getHandle(js); if (h->IsArrayBuffer()) { return h.As()->ByteLength(); } else if (h->IsArrayBufferView()) { diff --git a/src/workerd/jsg/function-test.c++ b/src/workerd/jsg/function-test.c++ index b7e216e36c8..31099522253 100644 --- a/src/workerd/jsg/function-test.c++ +++ b/src/workerd/jsg/function-test.c++ @@ -197,7 +197,7 @@ struct FunctionContext: public ContextGlobalObject { return js.tryCatch([&]() { return kj::str(thrower(js)); }, [&](Value exception) { - auto handle = exception.getHandle(js.v8Isolate); + auto handle = exception.getHandle(js); return kj::str("caught: ", handle); }); } diff --git a/src/workerd/jsg/function.h b/src/workerd/jsg/function.h index 644f4e7da15..733340d2da2 100644 --- a/src/workerd/jsg/function.h +++ b/src/workerd/jsg/function.h @@ -155,8 +155,8 @@ class Function { return (*native)(jsl, kj::fwd(args)...); } KJ_CASE_ONEOF(js, JsImpl) { - return (*js.wrapper)(jsl, js.receiver.getHandle(jsl.v8Isolate), - js.handle.getHandle(jsl.v8Isolate), kj::fwd(args)...); + return (*js.wrapper)(jsl, js.receiver.getHandle(jsl), + js.handle.getHandle(jsl), kj::fwd(args)...); } } __builtin_unreachable(); diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index b5144475f23..984bcfcd17c 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -1406,6 +1406,7 @@ class JsContext { v8::Local getHandle(v8::Isolate* isolate) { return handle.Get(isolate); } + v8::Local getHandle(Lock& js); private: v8::Global handle; @@ -2130,7 +2131,7 @@ inline v8::Local Lock::v8Null() { } inline Data Data::addRef(jsg::Lock& js) { - return Data(js.v8Isolate, getHandle(js.v8Isolate)); + return Data(js.v8Isolate, getHandle(js)); } template @@ -2140,7 +2141,7 @@ kj::Maybe> Ref::tryGetHandle(Lock& js) { template inline V8Ref V8Ref::addRef(jsg::Lock& js) { - return js.v8Ref(getHandle(js.v8Isolate)); + return js.v8Ref(getHandle(js)); } v8::Local deepClone(v8::Local context, v8::Local value); @@ -2154,7 +2155,7 @@ V8Ref V8Ref::deepClone(jsg::Lock& js) { template inline HashableV8Ref HashableV8Ref::addRef(jsg::Lock& js) { - return HashableV8Ref(js.v8Isolate, this->getHandle(js.v8Isolate), identityHash); + return HashableV8Ref(js.v8Isolate, this->getHandle(js), identityHash); } template @@ -2166,6 +2167,11 @@ inline v8::Local Data::getHandle(jsg::Lock& js) { return getHandle(js.v8Isolate); } +template +inline v8::Local JsContext::getHandle(Lock& js) { + return handle.Get(js.v8Isolate); +} + } // namespace workerd::jsg // These two includes are needed for the JSG type glue macros to work. diff --git a/src/workerd/jsg/promise-test.c++ b/src/workerd/jsg/promise-test.c++ index dc9f03d006d..d460041da4f 100644 --- a/src/workerd/jsg/promise-test.c++ +++ b/src/workerd/jsg/promise-test.c++ @@ -37,12 +37,12 @@ struct PromiseContext: public jsg::Object, public jsg::ContextGlobal { void catchIt(jsg::Lock& js, Promise promise) { promise.catch_(js, [](jsg::Lock& js, Value value) -> int { - JSG_FAIL_REQUIRE(Error, kj::str(value.getHandle(js.v8Isolate))); + JSG_FAIL_REQUIRE(Error, kj::str(value.getHandle(js))); }).then(js, [](jsg::Lock& js, int i) { KJ_FAIL_REQUIRE("shouldn't get here"); return kj::str("nope"); }, [](jsg::Lock& js, Value value) { - return kj::str(value.getHandle(js.v8Isolate)); + return kj::str(value.getHandle(js)); }).then(js, [](jsg::Lock&, kj::String s) { catchTestResult = kj::mv(s); }); diff --git a/src/workerd/jsg/promise.c++ b/src/workerd/jsg/promise.c++ index 4e639c746fd..5e9b92fdf8e 100644 --- a/src/workerd/jsg/promise.c++ +++ b/src/workerd/jsg/promise.c++ @@ -95,7 +95,7 @@ void UnhandledRejectionHandler::rejectedWithNoHandler( jsg::Lock& js, jsg::V8Ref promise, jsg::V8Ref value) { - auto message = v8::Exception::CreateMessage(js.v8Isolate, value.getHandle(js.v8Isolate)); + auto message = v8::Exception::CreateMessage(js.v8Isolate, value.getHandle(js)); // It's not yet clear under what conditions it happens, but this can be called // twice with the same promise. It really shouldn't happen in the regular cases diff --git a/src/workerd/jsg/promise.h b/src/workerd/jsg/promise.h index 6ee7a84118f..6137ce4bbf2 100644 --- a/src/workerd/jsg/promise.h +++ b/src/workerd/jsg/promise.h @@ -355,7 +355,7 @@ class Promise { } Resolver addRef(Lock& js) { - return { js.v8Isolate, v8Resolver.getHandle(js.v8Isolate) }; + return { js.v8Isolate, v8Resolver.getHandle(js) }; } void visitForGc(GcVisitor& visitor) { visitor.visit(v8Resolver); @@ -422,7 +422,7 @@ class Promise { v8::Local getInner(Lock& js) { return KJ_REQUIRE_NONNULL(v8Promise, "jsg::Promise can only be used once") - .getHandle(js.v8Isolate); + .getHandle(js); } template ()>()> diff --git a/src/workerd/jsg/setup-test.c++ b/src/workerd/jsg/setup-test.c++ index 7e200077cc8..a856efd795e 100644 --- a/src/workerd/jsg/setup-test.c++ +++ b/src/workerd/jsg/setup-test.c++ @@ -70,7 +70,7 @@ KJ_TEST("configuration values reach nested type declarations") { V8StackScope stackScope; ConfigIsolate::Lock lock(isolate, stackScope); v8::HandleScope handleScope(lock.v8Isolate); - lock.newContext().getHandle(lock.v8Isolate); + lock.newContext().getHandle(lock); } { KJ_EXPECT_LOG(ERROR, "failed: expected configuration == 123"); @@ -79,7 +79,7 @@ KJ_TEST("configuration values reach nested type declarations") { V8StackScope stackScope; ConfigIsolate::Lock lock(isolate, stackScope); v8::HandleScope handleScope(lock.v8Isolate); - lock.newContext().getHandle(lock.v8Isolate); + lock.newContext().getHandle(lock); } }