diff --git a/src/workerd/io/io-context.c++ b/src/workerd/io/io-context.c++ index fbf64b13837..c303d1c375c 100644 --- a/src/workerd/io/io-context.c++ +++ b/src/workerd/io/io-context.c++ @@ -360,8 +360,8 @@ void IoContext::logUncaughtException(kj::StringPtr description) { } void IoContext::logUncaughtException(UncaughtExceptionSource source, - v8::Local exception, - v8::Local message) { + const jsg::JsValue& exception, + const jsg::JsMessage& message) { KJ_REQUIRE_NONNULL(currentLock).logUncaughtException(source, exception, message); } @@ -391,7 +391,7 @@ void IoContext::logUncaughtExceptionAsync(UncaughtExceptionSource source, : source(source), exception(kj::mv(exception)) {} void run(Worker::Lock& lock) override { jsg::Lock& js = lock; - auto error = js.exceptionToJs(kj::mv(exception)); + auto error = js.exceptionToJsValue(kj::mv(exception)); // TODO(soon): Add logUncaughtException to jsg::Lock. lock.logUncaughtException(source, error.getHandle(js)); } @@ -1208,8 +1208,9 @@ void IoContext::runImpl(Runnable& runnable, bool takePendingEvent, // exception has been tunneled into a KJ exception, so the later logging won't be as // useful. We should improve the tunneling to include stack traces and ensure that all // consumers do in fact log exceptions, then we can remove this. - workerLock.logUncaughtException(UncaughtExceptionSource::INTERNAL, jsException, - tryCatch.Message()); + workerLock.logUncaughtException(UncaughtExceptionSource::INTERNAL, + jsg::JsValue(jsException), + jsg::JsMessage(tryCatch.Message())); jsg::throwTunneledException(workerLock.getIsolate(), jsException); } diff --git a/src/workerd/io/io-context.h b/src/workerd/io/io-context.h index 6732f3edffa..f4f02f58dec 100644 --- a/src/workerd/io/io-context.h +++ b/src/workerd/io/io-context.h @@ -379,8 +379,9 @@ class IoContext final: public kj::Refcounted, private kj::TaskSet::ErrorHandler void logErrorOnce(kj::StringPtr description); void logUncaughtException(kj::StringPtr description); - void logUncaughtException(UncaughtExceptionSource source, v8::Local exception, - v8::Local message = {}); + void logUncaughtException(UncaughtExceptionSource source, + const jsg::JsValue& exception, + const jsg::JsMessage& message = jsg::JsMessage()); // Log an uncaught exception from an asynchronous context, i.e. when the IoContext is not // "current". @@ -1458,7 +1459,8 @@ kj::_::ReducePromises> IoContext::awaitJs(jsg::Lock& js, jsg::Pro // once the exception has been tunneled into a KJ exception, so the later logging won't be // as useful. We should improve the tunneling to include stack traces and ensure that all // consumers do in fact log exceptions, then we can remove this. - context.logUncaughtException(UncaughtExceptionSource::INTERNAL_ASYNC, jsException); + context.logUncaughtException(UncaughtExceptionSource::INTERNAL_ASYNC, + jsg::JsValue(jsException)); fulfiller->fulfiller->reject(jsg::createTunneledException(isolate, jsException)); fulfiller->isDone = true; diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index be5b41a1bf4..a9bd36479f5 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -174,115 +174,64 @@ kj::StringPtr KJ_STRINGIFY(UncaughtExceptionSource value) { } namespace { - -void addJsStackTrace(v8::Local context, - kj::Vector& lines, v8::Local message) { - // TODO(someday): Relying on v8::Message to pass around source locations means - // we can't provide the module name for errors like compiling wasm modules. We - // should have our own type, but it requires a refactor of how we pass around errors - // for script startup. - - auto addLineCol = [](kj::StringTree str, int line, int col) { - if (line != v8::Message::kNoLineNumberInfo) { - str = kj::strTree(kj::mv(str), ":", line); - if (col != v8::Message::kNoColumnInfo) { - str = kj::strTree(kj::mv(str), ":", col); - } - } - return str; - }; - - if (!message.IsEmpty()) { - auto trace = message->GetStackTrace(); - if (trace.IsEmpty() || trace->GetFrameCount() == 0) { - kj::StringTree locationStr; - - auto resourceNameVal = message->GetScriptResourceName(); - if (resourceNameVal->IsString()) { - auto resourceName = resourceNameVal.As(); - if (!resourceName.IsEmpty() && resourceName->Length() != 0) { - locationStr = kj::strTree(" at ", resourceName); - } - } - - auto lineNumber = jsg::check(message->GetLineNumber(context)); - auto columnNumber = jsg::check(message->GetStartColumn(context)); - locationStr = addLineCol(kj::mv(locationStr), lineNumber, columnNumber); - - if (locationStr.size() > 0) { - lines.add(locationStr.flatten()); - } - } else { - for (auto i: kj::zeroTo(trace->GetFrameCount())) { - auto frame = trace->GetFrame(context->GetIsolate(), i); - kj::StringTree locationStr; - - auto scriptName = frame->GetScriptName(); - if (!scriptName.IsEmpty() && scriptName->Length() != 0) { - locationStr = kj::strTree(" at ", scriptName); - } else { - locationStr = kj::strTree(" at worker.js"); - } - - auto lineNumber = frame->GetLineNumber(); - auto columnNumber = frame->GetColumn(); - locationStr = addLineCol(kj::mv(locationStr), lineNumber, columnNumber); - - auto func = frame->GetFunctionName(); - if (!func.IsEmpty() && func->Length() != 0) { - locationStr = kj::strTree(kj::mv(locationStr), " in ", func); - } - - lines.add(locationStr.flatten()); - } - } - } -} - // Inform the inspector of a problem not associated with any particular exception object. // // Passes `description` as the exception's detailed message, dummy values for everything else. -void sendExceptionToInspector(v8_inspector::V8Inspector& inspector, v8::Local context, +void sendExceptionToInspector(jsg::Lock& js, + v8_inspector::V8Inspector& inspector, kj::StringPtr description) { - inspector.exceptionThrown(context, v8_inspector::StringView(), v8::Local(), - toStringView(description), v8_inspector::StringView(), - 0, 0, nullptr, 0); + inspector.exceptionThrown(js.v8Context(), + v8_inspector::StringView(), + v8::Local(), + toStringView(description), + v8_inspector::StringView(), + 0, 0, nullptr, 0); } // Inform the inspector of an exception thrown. // // Passes `source` as the exception's short message. Reconstructs `message` from `exception` if // `message` is empty. -void sendExceptionToInspector(v8_inspector::V8Inspector& inspector, v8::Local context, - UncaughtExceptionSource source, v8::Local exception, - v8::Local message) { - if (message.IsEmpty()) { +void sendExceptionToInspector(jsg::Lock& js, + v8_inspector::V8Inspector& inspector, + UncaughtExceptionSource source, + const jsg::JsValue& exception, + jsg::JsMessage message) { + if (!message) { // This exception didn't come with a Message. This can happen for exceptions delivered via // v8::Promise::Catch(), or for exceptions which were tunneled through C++ promises. In the // latter case, V8 will create a Message based on the current stack trace, but it won't be // super meaningful. - message = v8::Exception::CreateMessage(context->GetIsolate(), exception); - KJ_ASSERT(!message.IsEmpty()); + message = jsg::JsMessage::create(js, jsg::JsValue(exception)); } - auto stackTrace = message->GetStackTrace(); + // TODO(cleanup): Move the inspector stuff into a utility within jsg to better + // encapsulate + KJ_ASSERT(message); + v8::Local msg = message; + + auto context = js.v8Context(); + + auto stackTrace = msg->GetStackTrace(); // The resource name is whatever we set in the Script ctor, e.g. "worker.js". - auto scriptResourceName = message->GetScriptResourceName(); + auto scriptResourceName = msg->GetScriptResourceName(); - auto lineNumber = message->GetLineNumber(context).FromMaybe(0); - auto startColumn = message->GetStartColumn(context).FromMaybe(0); + auto lineNumber = msg->GetLineNumber(context).FromMaybe(0); + auto startColumn = msg->GetStartColumn(context).FromMaybe(0); // TODO(soon): EW-2636 Pass a real "script ID" as the last parameter instead of 0. I suspect this // has something to do with the incorrect links in the console when it logs uncaught exceptions. inspector.exceptionThrown(context, toStringView(kj::str(source)), exception, - toStringView(kj::str(message->Get())), toStringView(kj::str(scriptResourceName)), + toStringView(kj::str(msg->Get())), toStringView(kj::str(scriptResourceName)), lineNumber, startColumn, inspector.createStackTrace(stackTrace), 0); } -void addExceptionToTrace(jsg::Lock& js, IoContext &ioContext, WorkerTracer& tracer, - v8::Local context, UncaughtExceptionSource source, - v8::Local exception, +void addExceptionToTrace(jsg::Lock& js, + IoContext &ioContext, + WorkerTracer& tracer, + UncaughtExceptionSource source, + const jsg::JsValue& exception, const jsg::TypeHandler& errorTypeHandler) { if (source == UncaughtExceptionSource::INTERNAL || @@ -319,7 +268,6 @@ void reportStartupError( kj::StringPtr id, jsg::Lock& js, const kj::Maybe>& inspector, - v8::Local context, const IsolateLimitEnforcer& limitEnforcer, kj::Maybe maybeLimitError, v8::TryCatch& catcher, @@ -338,7 +286,7 @@ void reportStartupError( // We want to extend just enough cpu time as is necessary to report the exception // to the inspector here. 10 milliseconds should be more than enough. auto limitScope = limitEnforcer.enterLoggingJs(js, maybeLimitError2); - sendExceptionToInspector(*i->get(), context, description); + sendExceptionToInspector(js, *i->get(), description); // When the inspector is active, we don't want to throw here because then the inspector // won't be able to connect and the developer will never know what happened. } else { @@ -358,19 +306,23 @@ void reportStartupError( kj::Vector lines; lines.add(kj::str("Uncaught ", jsg::extractTunneledExceptionDescription( KJ_ASSERT_NONNULL(permanentException).getDescription()))); - addJsStackTrace(context, lines, catcher.Message()); + jsg::JsMessage message(catcher.Message()); + message.addJsStackTrace(js, lines); e->addError(kj::strArray(lines, "\n")); } else KJ_IF_MAYBE(i, inspector) { auto limitScope = limitEnforcer.enterLoggingJs(js, maybeLimitError2); - sendExceptionToInspector(*i->get(), context, UncaughtExceptionSource::INTERNAL, - exception, catcher.Message()); + sendExceptionToInspector(js, *i->get(), + UncaughtExceptionSource::INTERNAL, + jsg::JsValue(exception), + jsg::JsMessage(catcher.Message())); // When the inspector is active, we don't want to throw here because then the inspector // won't be able to connect and the developer will never know what happened. } else { // We should never get here in production if we've validated scripts before deployment. kj::Vector lines; - addJsStackTrace(context, lines, catcher.Message()); + jsg::JsMessage message(catcher.Message()); + message.addJsStackTrace(js, lines); auto trace = kj::strArray(lines, "; "); auto description = KJ_ASSERT_NONNULL(permanentException).getDescription(); if (description == "jsg.SyntaxError: \\8 and \\9 are not allowed in template strings.") { @@ -1296,7 +1248,6 @@ Worker::Script::Script(kj::Own isolateParam, kj::StringPtr id, reportStartupError(id, lock, isolate->impl->inspector, - context, isolate->getLimitEnforcer(), kj::mv(maybeLimitError), catcher, @@ -1539,7 +1490,6 @@ Worker::Worker(kj::Own scriptParam, reportStartupError(script->id, lock, script->isolate->impl->inspector, - context, script->isolate->getLimitEnforcer(), kj::mv(maybeLimitError), catcher, @@ -1785,7 +1735,7 @@ void Worker::Lock::logUncaughtException(kj::StringPtr description) { js.withinHandleScope([&] { auto context = getContext(); auto contextScope = js.enterContextScope(context); - sendExceptionToInspector(*i->get(), context, description); + sendExceptionToInspector(js, *i->get(), description); }); } @@ -1794,17 +1744,16 @@ void Worker::Lock::logUncaughtException(kj::StringPtr description) { } void Worker::Lock::logUncaughtException(UncaughtExceptionSource source, - v8::Local exception, - v8::Local message) { + const jsg::JsValue& exception, + const jsg::JsMessage& message) { // Only add exception to trace when running within an I/O context with a tracer. if (IoContext::hasCurrent()) { auto& ioContext = IoContext::current(); KJ_IF_MAYBE(tracer, ioContext.getWorkerTracer()) { jsg::Lock& js = *this; js.withinHandleScope([&] { - auto context = getContext(); - auto contextScope = js.enterContextScope(context); - addExceptionToTrace(impl->inner, ioContext, *tracer, context, source, exception, + auto contextScope = js.enterContextScope(getContext()); + addExceptionToTrace(impl->inner, ioContext, *tracer, source, exception, worker.getIsolate().apiIsolate->getErrorInterfaceTypeHandler(*this)); }); } @@ -1813,9 +1762,8 @@ void Worker::Lock::logUncaughtException(UncaughtExceptionSource source, KJ_IF_MAYBE(i, worker.script->isolate->impl->inspector) { jsg::Lock& js = *this; js.withinHandleScope([&] { - auto context = getContext(); - auto contextScope = js.enterContextScope(context); - sendExceptionToInspector(*i->get(), context, source, exception, message); + auto contextScope = js.enterContextScope(getContext()); + sendExceptionToInspector(js, *i->get(), source, exception, message); }); } @@ -2250,8 +2198,11 @@ public: inspector.contextCreated( v8_inspector::V8ContextInfo(dummyContext, 1, v8_inspector::StringView( reinterpret_cast("Worker"), 6))); - sendExceptionToInspector(inspector, dummyContext, - jsg::extractTunneledExceptionDescription(limitError->getDescription())); + { + auto contextScope = lock->enterContextScope(dummyContext); + sendExceptionToInspector(*lock, inspector, + jsg::extractTunneledExceptionDescription(limitError->getDescription())); + } inspector.contextDestroyed(dummyContext); }); } diff --git a/src/workerd/io/worker.h b/src/workerd/io/worker.h index fb59e37cfbd..336eaabbe1b 100644 --- a/src/workerd/io/worker.h +++ b/src/workerd/io/worker.h @@ -584,8 +584,9 @@ class Worker::Lock { void logUncaughtException(kj::StringPtr description); // Logs an exception to the debug console or trace, if active. - void logUncaughtException(UncaughtExceptionSource source, v8::Local exception, - v8::Local message = {}); + void logUncaughtException(UncaughtExceptionSource source, + const jsg::JsValue& exception, + const jsg::JsMessage& message = jsg::JsMessage()); void reportPromiseRejectEvent(v8::PromiseRejectMessage& message); diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index 9afe2c8a074..f68b908af36 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -1806,6 +1806,7 @@ template class JsRef; V(Unscopables) class JsValue; +class JsMessage; #define JS_TYPE_CLASSES(V) \ V(Object) \ V(Boolean) \ diff --git a/src/workerd/jsg/jsvalue.c++ b/src/workerd/jsg/jsvalue.c++ index fb125de6347..c604d156cdf 100644 --- a/src/workerd/jsg/jsvalue.c++ +++ b/src/workerd/jsg/jsvalue.c++ @@ -454,5 +454,72 @@ JsValue JsValue::structuredClone(Lock& js, kj::Maybe> maybeTr return jsg::structuredClone(js, *this, kj::mv(maybeTransfers)); } +JsMessage JsMessage::create(Lock& js, const JsValue& exception) { + return JsMessage(v8::Exception::CreateMessage(js.v8Isolate, exception)); +} + +void JsMessage::addJsStackTrace(Lock& js, kj::Vector& lines) { + if (inner.IsEmpty()) return; + + // TODO(someday): Relying on v8::Message to pass around source locations means + // we can't provide the module name for errors like compiling wasm modules. We + // should have our own type, but it requires a refactor of how we pass around errors + // for script startup. + + static constexpr auto addLineCol = [](kj::StringTree str, int line, int col) { + if (line != v8::Message::kNoLineNumberInfo) { + str = kj::strTree(kj::mv(str), ":", line); + if (col != v8::Message::kNoColumnInfo) { + str = kj::strTree(kj::mv(str), ":", col); + } + } + return str; + }; + + auto context = js.v8Context(); + auto trace = inner->GetStackTrace(); + if (trace.IsEmpty() || trace->GetFrameCount() == 0) { + kj::StringTree locationStr; + + auto resourceNameVal = inner->GetScriptResourceName(); + if (resourceNameVal->IsString()) { + auto resourceName = resourceNameVal.As(); + if (!resourceName.IsEmpty() && resourceName->Length() != 0) { + locationStr = kj::strTree(" at ", resourceName); + } + } + + auto lineNumber = jsg::check(inner->GetLineNumber(context)); + auto columnNumber = jsg::check(inner->GetStartColumn(context)); + locationStr = addLineCol(kj::mv(locationStr), lineNumber, columnNumber); + + if (locationStr.size() > 0) { + lines.add(locationStr.flatten()); + } + } else { + for (auto i: kj::zeroTo(trace->GetFrameCount())) { + auto frame = trace->GetFrame(context->GetIsolate(), i); + kj::StringTree locationStr; + + auto scriptName = frame->GetScriptName(); + if (!scriptName.IsEmpty() && scriptName->Length() != 0) { + locationStr = kj::strTree(" at ", scriptName); + } else { + locationStr = kj::strTree(" at worker.js"); + } + + auto lineNumber = frame->GetLineNumber(); + auto columnNumber = frame->GetColumn(); + locationStr = addLineCol(kj::mv(locationStr), lineNumber, columnNumber); + + auto func = frame->GetFunctionName(); + if (!func.IsEmpty() && func->Length() != 0) { + locationStr = kj::strTree(kj::mv(locationStr), " in ", func); + } + + lines.add(locationStr.flatten()); + } + } +} } // namespace workerd::jsg diff --git a/src/workerd/jsg/jsvalue.h b/src/workerd/jsg/jsvalue.h index fcb5cf7756f..6c6d8ad9e13 100644 --- a/src/workerd/jsg/jsvalue.h +++ b/src/workerd/jsg/jsvalue.h @@ -491,4 +491,27 @@ struct JsValueWrapper { } }; +class JsMessage final { +public: + static JsMessage create(Lock& js, const JsValue& exception); + explicit inline JsMessage() : inner(v8::Local()) { + requireOnStack(this); + } + explicit inline JsMessage(v8::Local inner) : inner(inner) { + requireOnStack(this); + } + operator v8::Local() const { return inner; } + + // Is it possible for the underlying v8::Local to be + // empty, in which case the bool() operator will return false. + operator bool() const { return !inner.IsEmpty(); } + + // Adds the JS Stack associated with this JsMessage to the given + // kj::Vector. + void addJsStackTrace(Lock& js, kj::Vector& lines); + +private: + v8::Local inner; +}; + } // namespace workerd::jsg