From 1bcae2264aaf02fd92b755b0013ec3180f787724 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 22 Jan 2024 23:06:01 -0800 Subject: [PATCH] Refactor `js.enterContextScope()` to JSG_WITHIN_CONTEXT_SCOPE macro Helps to eliminate repetitive boilerplate in multiple locations. --- src/workerd/api/actor-state-test.c++ | 31 +- src/workerd/api/crypto-impl-aes-test.c++ | 124 ++++--- src/workerd/io/hibernation-manager.c++ | 6 +- src/workerd/io/worker.c++ | 437 +++++++++++------------ src/workerd/jsg/jsg.c++ | 5 - src/workerd/jsg/jsg.h | 23 +- 6 files changed, 292 insertions(+), 334 deletions(-) diff --git a/src/workerd/api/actor-state-test.c++ b/src/workerd/api/actor-state-test.c++ index 6adbe905250..3db232d3c45 100644 --- a/src/workerd/api/actor-state-test.c++ +++ b/src/workerd/api/actor-state-test.c++ @@ -33,16 +33,15 @@ JSG_DECLARE_ISOLATE_TYPE(ActorStateIsolate, ActorStateContext); KJ_TEST("v8 serialization version tag hasn't changed") { jsg::test::Evaluator e(v8System); e.getIsolate().runInLockScope([&](ActorStateIsolate::Lock& isolateLock) { - isolateLock.withinHandleScope([&] { - auto v8Context = isolateLock.newContext().getHandle(isolateLock); - auto contextScope = isolateLock.enterContextScope(v8Context); - + JSG_WITHIN_CONTEXT_SCOPE(isolateLock, + isolateLock.newContext().getHandle(isolateLock), + [&](jsg::Lock& js) { auto buf = serializeV8Value(isolateLock, isolateLock.boolean(true)); // Confirm that a version header is appropriately written and that it contains the expected - // current version. When the version increases, we need to write a v8 patch that allows it to - // continue writing data at the old version so that we can do a rolling upgrade without any - // bugs caused by old processes failing to read data written by new ones. + // current version. When the version increases, we need to write a v8 patch that allows it + // to continue writing data at the old version so that we can do a rolling upgrade without + // any bugs caused by old processes failing to read data written by new ones. KJ_EXPECT(buf[0] == 0xFF); KJ_EXPECT(buf[1] == 0x0F); // v8 serializer version @@ -61,10 +60,9 @@ KJ_TEST("v8 serialization version tag hasn't changed") { KJ_TEST("we support deserializing up to v15") { jsg::test::Evaluator e(v8System); e.getIsolate().runInLockScope([&](ActorStateIsolate::Lock& isolateLock) { - isolateLock.withinHandleScope([&] { - auto v8Context = isolateLock.newContext().getHandle(isolateLock); - auto contextScope = isolateLock.enterContextScope(v8Context); - + JSG_WITHIN_CONTEXT_SCOPE(isolateLock, + isolateLock.newContext().getHandle(isolateLock), + [&](jsg::Lock& js) { kj::Vector testCases; testCases.add("54"); testCases.add("FF0D54"); @@ -105,12 +103,11 @@ KJ_TEST("wire format version does not change deserialization behavior on real da jsg::test::Evaluator e(v8System); e.getIsolate().runInLockScope([&](ActorStateIsolate::Lock& isolateLock) { - isolateLock.withinHandleScope([&] { - 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) - // back to the exact same data as the input. + JSG_WITHIN_CONTEXT_SCOPE(isolateLock, + isolateLock.newContext().getHandle(isolateLock), + [&](jsg::Lock& js) { + // Read in data line by line and verify that it round trips (serializes and + // then deserializes) back to the exact same data as the input. std::string hexStr; const auto key = "some-key"_kj; while (std::getline(file, hexStr)) { diff --git a/src/workerd/api/crypto-impl-aes-test.c++ b/src/workerd/api/crypto-impl-aes-test.c++ index 9fa50d24701..14b4b724375 100644 --- a/src/workerd/api/crypto-impl-aes-test.c++ +++ b/src/workerd/api/crypto-impl-aes-test.c++ @@ -93,84 +93,82 @@ KJ_TEST("AES-CTR key wrap") { // Basic test that let me repro an issue where using an AES key that's not AES-KW would fail to // wrap if it didn't have "encrypt" in its usages when created. - jsg::test::Evaluator e(v8System); - e.getIsolate().runInLockScope([&](CryptoIsolate::Lock& isolateLock) { - auto isolate = isolateLock.v8Isolate; - - isolateLock.withinHandleScope([&] { - auto context = isolateLock.newContext().getHandle(isolateLock); - auto contextScope = isolateLock.enterContextScope(context); - - jsg::Lock& js = isolateLock; - - SubtleCrypto subtle; - - auto wrappingKey = [&] { - auto keyMaterial = kj::heapArray( - {0x52, 0x4b, 0x67, 0x25, 0xe3, 0x56, 0xaa, 0xce, 0x7e, 0x76, 0x9b, - 0x48, 0x92, 0x55, 0x49, 0x06, 0x12, 0x5e, 0xf5, 0xae, 0xce, 0x39, - 0xde, 0xc2, 0x5b, 0x27, 0x33, 0x4e, 0x6e, 0x52, 0x32, 0x4e}); - - SubtleCrypto::ImportKeyAlgorithm algorithm = { - .name = kj::str("AES-CTR"), - }; - bool extractable = false; - - return subtle.importKeySync(jsg::Lock::from(isolate), - "raw", kj::mv(keyMaterial), kj::mv(algorithm), extractable, - {kj::str("wrapKey"), kj::str("unwrapKey")}); - }(); - - const auto unwrappedKeyMaterial = kj::heapArray( - {0x52, 0x4b, 0x67, 0x25, 0xe3, 0x56, 0xaa, 0xce, 0x7e, 0x76, 0x9b, - 0x48, 0x92, 0x55, 0x49, 0x06, 0x12, 0x5e, 0xf5, 0xae, 0xce, 0x39, - 0xde, 0xc2, 0x5b, 0x27, 0x33, 0x4e, 0x6e, 0x52, 0x32, 0x4e}); - SubtleCrypto::ImportKeyAlgorithm importAlgorithm; - importAlgorithm.length = 256; - importAlgorithm.name = kj::str("AES-CBC"); + const jsg::TypeHandler* jwkHandler = nullptr; + // Not testing JWK here, so valid value isn't needed. + + static constexpr kj::byte RAW_KEY_DATA[] = { + 0x52, 0x4b, 0x67, 0x25, 0xe3, 0x56, 0xaa, 0xce, + 0x7e, 0x76, 0x9b, 0x48, 0x92, 0x55, 0x49, 0x06, + 0x12, 0x5e, 0xf5, 0xae, 0xce, 0x39, 0xde, 0xc2, + 0x5b, 0x27, 0x33, 0x4e, 0x6e, 0x52, 0x32, 0x4e + }; + + static constexpr kj::ArrayPtr KEY_DATA(RAW_KEY_DATA, 32); + + SubtleCrypto subtle; + + static constexpr auto getWrappingKey = [](jsg::Lock& js, SubtleCrypto& subtle) { + return subtle.importKeySync(js, "raw", + kj::heapArray(KEY_DATA), + SubtleCrypto::ImportKeyAlgorithm { + .name = kj::str("AES-CTR") + }, + false /* extractable */, + { kj::str("wrapKey"), kj::str("unwrapKey") }); + }; + + static constexpr auto getEnc = [] { + return SubtleCrypto::EncryptAlgorithm { + .name = kj::str("AES-CTR"), + .counter = kj::arr(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16), + .length = 5, + }; + }; - const jsg::TypeHandler* jwkHandler = nullptr; - // Not testing JWK here, so valid value isn't needed. + static constexpr auto getImportKeyAlg = [] { + return SubtleCrypto::ImportKeyAlgorithm { + .name = kj::str("AES-CBC"), + .length = 256, + }; + }; - bool completed = false; + jsg::test::Evaluator e(v8System); + bool completed = false; + e.getIsolate().runInLockScope([&](CryptoIsolate::Lock& isolateLock) { + JSG_WITHIN_CONTEXT_SCOPE(isolateLock, + isolateLock.newContext().getHandle(isolateLock), + [&](jsg::Lock& js) { + auto wrappingKey = getWrappingKey(js, subtle); subtle.importKey( - jsg::Lock::from(isolate), - kj::str("raw"), - kj::heapArray(unwrappedKeyMaterial.asPtr()), - kj::mv(importAlgorithm), + js, kj::str("raw"), + kj::heapArray(KEY_DATA), + getImportKeyAlg(), true, kj::arr(kj::str("decrypt"))) .then(js, [&] (jsg::Lock&, jsg::Ref toWrap) { - SubtleCrypto::EncryptAlgorithm enc; - enc.name = kj::str("AES-CTR"); - enc.counter = kj::arr(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16); - enc.length = 5; - return subtle.wrapKey(isolateLock, kj::str("raw"), *toWrap, *wrappingKey, - kj::mv(enc), *jwkHandler); + return subtle.wrapKey( + js, kj::str("raw"), *toWrap, *wrappingKey, + getEnc(), *jwkHandler); }).then(js, [&] (jsg::Lock&, kj::Array wrapped) { - SubtleCrypto::EncryptAlgorithm enc; - enc.name = kj::str("AES-CTR"); - enc.counter = kj::arr(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16); - enc.length = 5; - - SubtleCrypto::ImportKeyAlgorithm importAlgorithm; - importAlgorithm.length = 256; - importAlgorithm.name = kj::str("AES-CBC"); - - return subtle.unwrapKey(isolateLock, kj::str("raw"), kj::mv(wrapped), *wrappingKey, - kj::mv(enc), kj::mv(importAlgorithm), true, - kj::arr(kj::str("encrypt")), *jwkHandler); + return subtle.unwrapKey(js, + kj::str("raw"), + kj::mv(wrapped), + *wrappingKey, + getEnc(), + getImportKeyAlg(), + true, kj::arr(kj::str("encrypt")), *jwkHandler); }).then(js, [&] (jsg::Lock& js, jsg::Ref unwrapped) { return subtle.exportKey(js, kj::str("raw"), *unwrapped); }).then(js, [&] (jsg::Lock&, api::SubtleCrypto::ExportKeyData roundTrippedKeyMaterial) { - KJ_ASSERT(roundTrippedKeyMaterial.get>() == unwrappedKeyMaterial); + KJ_ASSERT(roundTrippedKeyMaterial.get>() == KEY_DATA); completed = true; }); - e.runMicrotasks(isolateLock); - KJ_ASSERT(completed, "Microtasks did not run fully."); + js.runMicrotasks(); }); }); + + KJ_ASSERT(completed, "Microtasks did not run fully."); } #if __clang__ && __has_feature(undefined_behavior_sanitizer) #pragma clang attribute pop // __attribute__((no_sanitize("null")) diff --git a/src/workerd/io/hibernation-manager.c++ b/src/workerd/io/hibernation-manager.c++ index b4587d07abb..0248d71e892 100644 --- a/src/workerd/io/hibernation-manager.c++ +++ b/src/workerd/io/hibernation-manager.c++ @@ -209,16 +209,14 @@ void HibernationManagerImpl::setTimerChannel(TimerChannel& timerChannel) { } void HibernationManagerImpl::hibernateWebSockets(Worker::Lock& lock) { - jsg::Lock& js(lock); - js.withinHandleScope([&] { - js.enterContextScope(lock.getContext()); + JSG_WITHIN_CONTEXT_SCOPE(lock, lock.getContext(), [&](jsg::Lock& js) { for (auto& ws : allWs) { KJ_IF_SOME(active, ws->activeOrPackage.tryGet>()) { // Transfers ownership of properties from api::WebSocket to HibernatableWebSocket via the // HibernationPackage. ws->activeOrPackage.init( active.get()->buildPackageForHibernation()); - } + } else {} // Here to quash compiler warning } }); } diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index 0e84e8ce2fe..ee43e2c55ed 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -762,52 +762,11 @@ struct Worker::Script::Impl { struct DynamicImportResult { jsg::Value value; bool isException = false; + DynamicImportResult(jsg::Value value, bool isException = false) + : value(kj::mv(value)), isException(isException) {} }; using DynamicImportHandler = kj::Function; - static DynamicImportResult getDynamicImportResult( - Worker::AsyncLock& asyncLock, - const Worker& worker, - kj::Maybe>& asyncContext, - DynamicImportHandler handler) { - return worker.runInLockScope(asyncLock, [&](Worker::Lock& lock) { - jsg::Lock& js = lock; - - return js.withinHandleScope([&]() -> DynamicImportResult { - // Cannot use js.v8Context() here because that assumes we've already entered - // the v8::Context::Scope... - auto contextScope = js.enterContextScope(lock.getContext()); - jsg::AsyncContextFrame::Scope asyncContextScope(js, asyncContext); - - // We have to wrap the call to handler in a try catch here because - // we have to tunnel any jsg::JsExceptionThrown instances back. - v8::TryCatch tryCatch(js.v8Isolate); - kj::Maybe maybeLimitError; - try { - auto limitScope = worker.getIsolate().getLimitEnforcer() - .enterDynamicImportJs(lock, maybeLimitError); - return { .value = handler() }; - } catch (jsg::JsExceptionThrown&) { - // Handled below... - } catch (kj::Exception& ex) { - kj::throwFatalException(kj::mv(ex)); - } - - KJ_ASSERT(tryCatch.HasCaught()); - if (!tryCatch.CanContinue() || tryCatch.Exception().IsEmpty()) { - // There's nothing else we can do here but throw a generic fatal exception. - KJ_IF_SOME(limitError, maybeLimitError) { - kj::throwFatalException(kj::mv(limitError)); - } else { - kj::throwFatalException(JSG_KJ_EXCEPTION(FAILED, Error, - "Failed to load dynamic module.")); - } - } - return { .value = js.v8Ref(tryCatch.Exception()), .isException = true }; - }); - }); - } - void configureDynamicImports(jsg::Lock& js, jsg::ModuleRegistry& modules) { static auto constexpr handleDynamicImport = [](kj::Own worker, @@ -817,7 +776,38 @@ struct Worker::Script::Impl { co_await kj::evalLater([] {}); auto asyncLock = co_await worker->takeAsyncLockWithoutRequest(nullptr); - co_return getDynamicImportResult(asyncLock, *worker, asyncContext, kj::mv(handler)); + co_return worker->runInLockScope(asyncLock, [&](Worker::Lock& lock) { + return JSG_WITHIN_CONTEXT_SCOPE(lock, lock.getContext(), + [&](jsg::Lock& js) { + jsg::AsyncContextFrame::Scope asyncContextScope(js, asyncContext); + + // We have to wrap the call to handler in a try catch here because + // we have to tunnel any jsg::JsExceptionThrown instances back. + v8::TryCatch tryCatch(js.v8Isolate); + kj::Maybe maybeLimitError; + try { + auto limitScope = worker->getIsolate().getLimitEnforcer() + .enterDynamicImportJs(lock, maybeLimitError); + return DynamicImportResult(handler()); + } catch (jsg::JsExceptionThrown&) { + // Handled below... + } catch (kj::Exception& ex) { + kj::throwFatalException(kj::mv(ex)); + } + + KJ_ASSERT(tryCatch.HasCaught()); + if (!tryCatch.CanContinue() || tryCatch.Exception().IsEmpty()) { + // There's nothing else we can do here but throw a generic fatal exception. + KJ_IF_SOME(limitError, maybeLimitError) { + kj::throwFatalException(kj::mv(limitError)); + } else { + kj::throwFatalException(JSG_KJ_EXCEPTION(FAILED, Error, + "Failed to load dynamic module.")); + } + } + return DynamicImportResult(js.v8Ref(tryCatch.Exception()), true); + }); + }); }; modules.setDynamicImportCallback([](jsg::Lock& js, DynamicImportHandler handler) mutable { @@ -1126,89 +1116,89 @@ Worker::Script::Script(kj::Own isolateParam, kj::StringPtr id, lock.v8Isolate, nullptr, v8::ObjectTemplate::New(lock.v8Isolate)); } - auto contextScope = lock.enterContextScope(context); - - // const_cast OK because we hold the isolate lock. - Worker::Isolate& lockedWorkerIsolate = const_cast(*isolate); - - if (logNewScript) { - // HACK: Log a message indicating that a new script was loaded. This is used only when the - // inspector is enabled. We want to do this immediately after the context is created, - // before the user gets a chance to modify the behavior of the console, which if they did, - // we'd then need to be more careful to apply time limits and such. - lockedWorkerIsolate.logMessage(lock, - static_cast(cdp::LogType::WARNING), "Script modified; context reset."); - } + JSG_WITHIN_CONTEXT_SCOPE(lock, context, [&](jsg::Lock& js) { + // const_cast OK because we hold the isolate lock. + Worker::Isolate& lockedWorkerIsolate = const_cast(*isolate); - // We need to register this context with the inspector, otherwise errors won't be reported. But - // we want it to be un-registered as soon as the script has been compiled, otherwise the - // inspector will end up with multiple contexts active which is very confusing for the user - // (since they'll have to select from the drop-down which context to use). - // - // (For modules, the context was already registered by `setupContext()`, above. - KJ_IF_SOME(i, isolate->impl->inspector) { - if (!source.is()) { - i.get()->contextCreated(v8_inspector::V8ContextInfo(context, - 1, jsg::toInspectorStringView("Compiler"))); + if (logNewScript) { + // HACK: Log a message indicating that a new script was loaded. This is used only when the + // inspector is enabled. We want to do this immediately after the context is created, + // before the user gets a chance to modify the behavior of the console, which if they did, + // we'd then need to be more careful to apply time limits and such. + lockedWorkerIsolate.logMessage(lock, + static_cast(cdp::LogType::WARNING), "Script modified; context reset."); } - } - KJ_DEFER({ - if (!source.is()) { - KJ_IF_SOME(i, isolate->impl->inspector) { - i.get()->contextDestroyed(context); - } else { - // Else block to avoid dangling else clang warning. + + // We need to register this context with the inspector, otherwise errors won't be reported. But + // we want it to be un-registered as soon as the script has been compiled, otherwise the + // inspector will end up with multiple contexts active which is very confusing for the user + // (since they'll have to select from the drop-down which context to use). + // + // (For modules, the context was already registered by `setupContext()`, above. + KJ_IF_SOME(i, isolate->impl->inspector) { + if (!source.is()) { + i.get()->contextCreated(v8_inspector::V8ContextInfo(context, + 1, jsg::toInspectorStringView("Compiler"))); } - } - }); + } else {} // Here to squash a compiler warning + KJ_DEFER({ + if (!source.is()) { + KJ_IF_SOME(i, isolate->impl->inspector) { + i.get()->contextDestroyed(context); + } else { + // Else block to avoid dangling else clang warning. + } + } + }); - v8::TryCatch catcher(lock.v8Isolate); - kj::Maybe maybeLimitError; + v8::TryCatch catcher(lock.v8Isolate); + kj::Maybe maybeLimitError; - try { try { - KJ_SWITCH_ONEOF(source) { - KJ_CASE_ONEOF(script, ScriptSource) { - impl->globals = script.compileGlobals(lock, isolate->getApi(), isolate->impl->metrics); - - { - // It's unclear to me if CompileUnboundScript() can get trapped in any infinite loops or - // excessively-expensive computation requiring a time limit. We'll go ahead and apply a time - // limit just to be safe. Don't add it to the rollover bank, though. - auto limitScope = isolate->getLimitEnforcer().enterStartupJs(lock, maybeLimitError); - impl->unboundScriptOrMainModule = - jsg::NonModuleScript::compile(script.mainScript, lock, script.mainScriptName); + try { + KJ_SWITCH_ONEOF(source) { + KJ_CASE_ONEOF(script, ScriptSource) { + impl->globals = script.compileGlobals(lock, isolate->getApi(), isolate->impl->metrics); + + { + // It's unclear to me if CompileUnboundScript() can get trapped in any infinite loops or + // excessively-expensive computation requiring a time limit. We'll go ahead and apply a time + // limit just to be safe. Don't add it to the rollover bank, though. + auto limitScope = isolate->getLimitEnforcer().enterStartupJs(lock, maybeLimitError); + impl->unboundScriptOrMainModule = + jsg::NonModuleScript::compile(script.mainScript, lock, script.mainScriptName); + } + + break; } - break; + KJ_CASE_ONEOF(modulesSource, ModulesSource) { + auto limitScope = isolate->getLimitEnforcer().enterStartupJs(lock, maybeLimitError); + auto& modules = KJ_ASSERT_NONNULL(impl->moduleContext)->getModuleRegistry(); + impl->configureDynamicImports(lock, modules); + modulesSource.compileModules(lock, isolate->getApi()); + impl->unboundScriptOrMainModule = kj::Path::parse(modulesSource.mainModule); + break; + } } - KJ_CASE_ONEOF(modulesSource, ModulesSource) { - auto limitScope = isolate->getLimitEnforcer().enterStartupJs(lock, maybeLimitError); - auto& modules = KJ_ASSERT_NONNULL(impl->moduleContext)->getModuleRegistry(); - impl->configureDynamicImports(lock, modules); - modulesSource.compileModules(lock, isolate->getApi()); - impl->unboundScriptOrMainModule = kj::Path::parse(modulesSource.mainModule); - break; - } + parseMetrics->done(); + } catch (const kj::Exception& e) { + lock.throwException(kj::cp(e)); + // lock.throwException() here will throw a jsg::JsExceptionThrown which we catch + // in the outer try/catch. } - - parseMetrics->done(); - } catch (const kj::Exception& e) { - lock.throwException(kj::cp(e)); - // lock.throwException() here will throw a jsg::JsExceptionThrown which we catch - // in the outer try/catch. + } catch (const jsg::JsExceptionThrown&) { + reportStartupError(id, + lock, + isolate->impl->inspector, + isolate->getLimitEnforcer(), + kj::mv(maybeLimitError), + catcher, + errorReporter, + impl->permanentException); } - } catch (const jsg::JsExceptionThrown&) { - reportStartupError(id, - lock, - isolate->impl->inspector, - isolate->getLimitEnforcer(), - kj::mv(maybeLimitError), - catcher, - errorReporter, - impl->permanentException); - } + }); }); }); } @@ -1356,117 +1346,117 @@ Worker::Worker(kj::Own scriptParam, } // Enter the context for compiling and running the script. - auto contextScope = lock.enterContextScope(context); - - v8::TryCatch catcher(lock.v8Isolate); - kj::Maybe maybeLimitError; + JSG_WITHIN_CONTEXT_SCOPE(lock, context, [&](jsg::Lock& js) { + v8::TryCatch catcher(lock.v8Isolate); + kj::Maybe maybeLimitError; - try { try { - currentSpan = maybeMakeSpan("lw:globals_instantiation"_kjc); - - v8::Local bindingsScope; - if (script->isModular()) { - // Use `env` variable. - bindingsScope = v8::Object::New(lock.v8Isolate); - } else { - // Use global-scope bindings. - bindingsScope = context->Global(); - } - - // Load globals. - // const_cast OK because we hold the lock. - for (auto& global: const_cast(*script).impl->globals) { - lock.v8Set(bindingsScope, global.name, global.value); - } + try { + currentSpan = maybeMakeSpan("lw:globals_instantiation"_kjc); - compileBindings(lock, script->isolate->getApi(), bindingsScope); - - // Execute script. - currentSpan = maybeMakeSpan("lw:top_level_execution"_kjc); + v8::Local bindingsScope; + if (script->isModular()) { + // Use `env` variable. + bindingsScope = v8::Object::New(lock.v8Isolate); + } else { + // Use global-scope bindings. + bindingsScope = context->Global(); + } - KJ_SWITCH_ONEOF(script->impl->unboundScriptOrMainModule) { - KJ_CASE_ONEOF(unboundScript, jsg::NonModuleScript) { - auto limitScope = script->isolate->getLimitEnforcer().enterStartupJs(lock, maybeLimitError); - unboundScript.run(lock.v8Context()); + // Load globals. + // const_cast OK because we hold the lock. + for (auto& global: const_cast(*script).impl->globals) { + lock.v8Set(bindingsScope, global.name, global.value); } - KJ_CASE_ONEOF(mainModule, kj::Path) { - auto& registry = (*jsContext)->getModuleRegistry(); - KJ_IF_SOME(entry, registry.resolve(lock, mainModule, kj::none)) { - JSG_REQUIRE(entry.maybeSynthetic == kj::none, TypeError, - "Main module must be an ES module."); - auto module = entry.module.getHandle(lock); - { - auto limitScope = script->isolate->getLimitEnforcer() - .enterStartupJs(lock, maybeLimitError); + compileBindings(lock, script->isolate->getApi(), bindingsScope); - jsg::instantiateModule(lock, module); - } + // Execute script. + currentSpan = maybeMakeSpan("lw:top_level_execution"_kjc); - if (maybeLimitError != kj::none) { - // If we hit the limit in PerformMicrotaskCheckpoint() we may not have actually - // thrown an exception. - throw jsg::JsExceptionThrown(); - } + KJ_SWITCH_ONEOF(script->impl->unboundScriptOrMainModule) { + KJ_CASE_ONEOF(unboundScript, jsg::NonModuleScript) { + auto limitScope = script->isolate->getLimitEnforcer().enterStartupJs(lock, maybeLimitError); + unboundScript.run(lock.v8Context()); + } + KJ_CASE_ONEOF(mainModule, kj::Path) { + auto& registry = (*jsContext)->getModuleRegistry(); + KJ_IF_SOME(entry, registry.resolve(lock, mainModule, kj::none)) { + JSG_REQUIRE(entry.maybeSynthetic == kj::none, TypeError, + "Main module must be an ES module."); + auto module = entry.module.getHandle(lock); + + { + auto limitScope = script->isolate->getLimitEnforcer() + .enterStartupJs(lock, maybeLimitError); + + jsg::instantiateModule(lock, module); + } - v8::Local ns = module->GetModuleNamespace(); + if (maybeLimitError != kj::none) { + // If we hit the limit in PerformMicrotaskCheckpoint() we may not have actually + // thrown an exception. + throw jsg::JsExceptionThrown(); + } - { - // The V8 module API is weird. Only the first call to Evaluate() will evaluate the - // module, even if subsequent calls pass a different context. Verify that we didn't - // switch contexts. - auto creationContext = jsg::check(ns.As()->GetCreationContext()); - KJ_ASSERT(creationContext == context, - "module was originally instantiated in a different context"); - } + v8::Local ns = module->GetModuleNamespace(); - impl->env = lock.v8Ref(bindingsScope.As()); + { + // The V8 module API is weird. Only the first call to Evaluate() will evaluate the + // module, even if subsequent calls pass a different context. Verify that we didn't + // switch contexts. + auto creationContext = jsg::check(ns.As()->GetCreationContext()); + KJ_ASSERT(creationContext == context, + "module was originally instantiated in a different context"); + } + + impl->env = lock.v8Ref(bindingsScope.As()); - auto handlers = script->isolate->getApi().unwrapExports(lock, ns); + auto handlers = script->isolate->getApi().unwrapExports(lock, ns); - for (auto& handler: handlers.fields) { - KJ_SWITCH_ONEOF(handler.value) { - KJ_CASE_ONEOF(obj, api::ExportedHandler) { - obj.env = lock.v8Ref(bindingsScope.As()); - obj.ctx = jsg::alloc(); + for (auto& handler: handlers.fields) { + KJ_SWITCH_ONEOF(handler.value) { + KJ_CASE_ONEOF(obj, api::ExportedHandler) { + obj.env = lock.v8Ref(bindingsScope.As()); + obj.ctx = jsg::alloc(); - if (handler.name == "default") { - // The default export is given the string name "default". I guess that means that - // you can't actually name an export "default"? Anyway, this is our default - // handler. - impl->defaultHandler = kj::mv(obj); - } else { - impl->namedHandlers.insert(kj::mv(handler.name), kj::mv(obj)); + if (handler.name == "default") { + // The default export is given the string name "default". I guess that means that + // you can't actually name an export "default"? Anyway, this is our default + // handler. + impl->defaultHandler = kj::mv(obj); + } else { + impl->namedHandlers.insert(kj::mv(handler.name), kj::mv(obj)); + } + } + KJ_CASE_ONEOF(cls, DurableObjectConstructor) { + impl->actorClasses.insert(kj::mv(handler.name), kj::mv(cls)); } - } - KJ_CASE_ONEOF(cls, DurableObjectConstructor) { - impl->actorClasses.insert(kj::mv(handler.name), kj::mv(cls)); } } + } else { + JSG_FAIL_REQUIRE(TypeError, "Main module name is not present in bundle."); } - } else { - JSG_FAIL_REQUIRE(TypeError, "Main module name is not present in bundle."); } } - } - startupMetrics->done(); - } catch (const kj::Exception& e) { - lock.throwException(kj::cp(e)); - // lock.throwException() here will throw a jsg::JsExceptionThrown which we catch - // in the outer try/catch. + startupMetrics->done(); + } catch (const kj::Exception& e) { + lock.throwException(kj::cp(e)); + // lock.throwException() here will throw a jsg::JsExceptionThrown which we catch + // in the outer try/catch. + } + } catch (const jsg::JsExceptionThrown&) { + reportStartupError(script->id, + lock, + script->isolate->impl->inspector, + script->isolate->getLimitEnforcer(), + kj::mv(maybeLimitError), + catcher, + errorReporter, + impl->permanentException); } - } catch (const jsg::JsExceptionThrown&) { - reportStartupError(script->id, - lock, - script->isolate->impl->inspector, - script->isolate->getLimitEnforcer(), - kj::mv(maybeLimitError), - catcher, - errorReporter, - impl->permanentException); - } + }); }); }); } @@ -1740,12 +1730,8 @@ void Worker::Lock::logErrorOnce(kj::StringPtr description) { void Worker::Lock::logUncaughtException(kj::StringPtr description) { // We don't add the exception to traces here, since it turns out that this path only gets hit by // intermediate exception handling. - KJ_IF_SOME(i, worker.script->isolate->impl->inspector) { - jsg::Lock& js = *this; - js.withinHandleScope([&] { - auto context = getContext(); - auto contextScope = js.enterContextScope(context); + JSG_WITHIN_CONTEXT_SCOPE(*this, getContext(), [&](jsg::Lock& js) { jsg::sendExceptionToInspector(js, *i.get(), description); }); } @@ -1761,9 +1747,7 @@ void Worker::Lock::logUncaughtException(UncaughtExceptionSource source, if (IoContext::hasCurrent()) { auto& ioContext = IoContext::current(); KJ_IF_SOME(tracer, ioContext.getWorkerTracer()) { - jsg::Lock& js = *this; - js.withinHandleScope([&] { - auto contextScope = js.enterContextScope(getContext()); + JSG_WITHIN_CONTEXT_SCOPE(*this, getContext(), [&](jsg::Lock& js) { addExceptionToTrace(impl->inner, ioContext, tracer, source, exception, worker.getIsolate().getApi().getErrorInterfaceTypeHandler(*this)); }); @@ -1771,9 +1755,7 @@ void Worker::Lock::logUncaughtException(UncaughtExceptionSource source, } KJ_IF_SOME(i, worker.script->isolate->impl->inspector) { - jsg::Lock& js = *this; - js.withinHandleScope([&] { - auto contextScope = js.enterContextScope(getContext()); + JSG_WITHIN_CONTEXT_SCOPE(*this, getContext(), [&](jsg::Lock& js) { sendExceptionToInspector(js, *i.get(), source, exception, message); }); } @@ -1790,10 +1772,7 @@ void Worker::Lock::reportPromiseRejectEvent(v8::PromiseRejectMessage& message) { } void Worker::Lock::validateHandlers(ValidationErrorReporter& errorReporter) { - jsg::Lock& js = *this; - js.withinHandleScope([&] { - auto contextScope = js.enterContextScope(getContext()); - // Ignore event types that represent internally-generated events. + JSG_WITHIN_CONTEXT_SCOPE(*this, getContext(), [&](jsg::Lock& js) { kj::HashSet ignoredHandlers; ignoredHandlers.insert("alarm"_kj); ignoredHandlers.insert("unhandledrejection"_kj); @@ -1836,7 +1815,7 @@ void Worker::Lock::validateHandlers(ValidationErrorReporter& errorReporter) { KJ_IF_SOME(h, worker.impl->defaultHandler) { report(kj::none, h); - } + } else {} // Here to squash a compiler warning. for (auto& entry: worker.impl->namedHandlers) { report(kj::StringPtr(entry.key), entry.value); } @@ -2216,11 +2195,10 @@ public: inspector.contextCreated( v8_inspector::V8ContextInfo(dummyContext, 1, v8_inspector::StringView( reinterpret_cast("Worker"), 6))); - { - auto contextScope = lock->enterContextScope(dummyContext); - jsg::sendExceptionToInspector(*lock, inspector, + JSG_WITHIN_CONTEXT_SCOPE(*lock, dummyContext, [&](jsg::Lock& js) { + jsg::sendExceptionToInspector(js, inspector, jsg::extractTunneledExceptionDescription(limitError.getDescription())); - } + }); inspector.contextDestroyed(dummyContext); }); } @@ -2651,9 +2629,7 @@ void Worker::Isolate::disconnectInspector() { void Worker::Isolate::logWarning(kj::StringPtr description, Lock& lock) { if (impl->inspector != kj::none) { - jsg::Lock& js = lock; - js.withinHandleScope([&] { - auto contextScope = js.enterContextScope(lock.getContext()); + JSG_WITHIN_CONTEXT_SCOPE(lock, lock.getContext(), [&](jsg::Lock& js) { logMessage(js, static_cast(cdp::LogType::WARNING), description); }); } @@ -2879,14 +2855,13 @@ struct Worker::Actor::Impl { shutdownFulfiller(kj::mv(paf.fulfiller)), hibernationManager(kj::mv(manager)), hibernationEventType(kj::mv(hibernationEventType)) { - jsg::Lock& js = lock; - js.withinHandleScope([&] { - auto contextScope = js.enterContextScope(lock.getContext()); + JSG_WITHIN_CONTEXT_SCOPE(lock, lock.getContext(), [&](jsg::Lock& js) { if (hasTransient) { transient.emplace(js, js.obj()); } - actorCache = makeActorCache(self.worker->getIsolate().impl->actorCacheLru, outputGate, hooks); + actorCache = makeActorCache(self.worker->getIsolate().impl->actorCacheLru, + outputGate, hooks); }); } }; diff --git a/src/workerd/jsg/jsg.c++ b/src/workerd/jsg/jsg.c++ index 1d7c22f971e..3ff47239df9 100644 --- a/src/workerd/jsg/jsg.c++ +++ b/src/workerd/jsg/jsg.c++ @@ -243,11 +243,6 @@ kj::StringPtr Lock::getUuid() const { return IsolateBase::from(v8Isolate).getUuid(); } -Lock::ContextScope Lock::enterContextScope(v8::Local context) { - KJ_ASSERT(!context.IsEmpty(), "unable to enter invalid v8::Context"); - return ContextScope(context); -} - void Lock::runMicrotasks() { v8Isolate->PerformMicrotaskCheckpoint(); } diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index a4bd2c4791c..28b5dcc4f07 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -2158,20 +2158,6 @@ class Lock { } } - // An opaque RAII type that encapsulates the v8::Context::Scope. The purpose - // here is to abstract away (somewhat) direct use of the v8 API. - class ContextScope { - public: - ContextScope(v8::Local context) : scope(context) {} - KJ_DISALLOW_COPY_AND_MOVE(ContextScope); - private: - v8::Context::Scope scope; - friend class Lock; - }; - - // Ensures that we are currently within the scope of the given v8::Context - ContextScope enterContextScope(v8::Local context); - // ==================================================================================== JsObject global() KJ_WARN_UNUSED_RESULT; JsValue undefined() KJ_WARN_UNUSED_RESULT; @@ -2271,6 +2257,15 @@ class Lock { bool warningsLogged; }; +// Ensures that the given fn is run within both a handlescope and the context scope. +// The lock must be assignable to a jsg::Lock, and the context must be or be assignable +// to a v8::Local. The context will be evaluated within the handle scope. +#define JSG_WITHIN_CONTEXT_SCOPE(lock, context, fn) \ + ((jsg::Lock&)lock).withinHandleScope([&]() -> auto { \ + v8::Local ctx = context; \ + KJ_ASSERT(!ctx.IsEmpty(), "unable to enter invalid v8::Context"); \ + v8::Context::Scope scope(ctx); \ + return fn((jsg::Lock&)lock); }) // The V8StackScope is used only as a marker to prove that we are running in the V8 stack // established by calling runInV8Stack(...)