From 5bade478cb55dd99264f5b63c641fb92e6fd9fa8 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 1 Sep 2023 10:29:28 -0700 Subject: [PATCH 1/5] JSG Completion: add logUncaughtException variant taking JsValue --- src/workerd/io/io-context.c++ | 6 ++++++ src/workerd/io/io-context.h | 10 ++++++++-- src/workerd/io/worker.c++ | 6 ++++++ src/workerd/io/worker.h | 10 ++++++++-- src/workerd/jsg/jsg.h | 1 + src/workerd/jsg/jsvalue.h | 10 ++++++++++ 6 files changed, 39 insertions(+), 4 deletions(-) diff --git a/src/workerd/io/io-context.c++ b/src/workerd/io/io-context.c++ index fbf64b13837..9da26437017 100644 --- a/src/workerd/io/io-context.c++ +++ b/src/workerd/io/io-context.c++ @@ -365,6 +365,12 @@ void IoContext::logUncaughtException(UncaughtExceptionSource source, KJ_REQUIRE_NONNULL(currentLock).logUncaughtException(source, exception, message); } +void IoContext::logUncaughtException(UncaughtExceptionSource source, + const jsg::JsValue& exception, + const jsg::JsMessage& message) { + KJ_REQUIRE_NONNULL(currentLock).logUncaughtException(source, exception, message); +} + void IoContext::logUncaughtExceptionAsync(UncaughtExceptionSource source, kj::Exception&& exception) { if (getWorkerTracer() == nullptr && diff --git a/src/workerd/io/io-context.h b/src/workerd/io/io-context.h index 5021d62d0c0..239ae0510e9 100644 --- a/src/workerd/io/io-context.h +++ b/src/workerd/io/io-context.h @@ -379,8 +379,14 @@ 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, + v8::Local exception, + v8::Local message = {}) + KJ_DEPRECATED("Use the version that takes jsg::JsValue instead"); + + 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". diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index be5b41a1bf4..ac61cee4535 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -1796,6 +1796,12 @@ void Worker::Lock::logUncaughtException(kj::StringPtr description) { void Worker::Lock::logUncaughtException(UncaughtExceptionSource source, v8::Local exception, v8::Local message) { + logUncaughtException(source, jsg::JsValue(exception), jsg::JsMessage(message)); +} + +void Worker::Lock::logUncaughtException(UncaughtExceptionSource source, + 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(); diff --git a/src/workerd/io/worker.h b/src/workerd/io/worker.h index fb59e37cfbd..803ca5df169 100644 --- a/src/workerd/io/worker.h +++ b/src/workerd/io/worker.h @@ -584,8 +584,14 @@ 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, + v8::Local exception, + v8::Local message = {}) + KJ_DEPRECATED("Use the version that takes jsg::JsValue instead"); + + 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.h b/src/workerd/jsg/jsvalue.h index fcb5cf7756f..be290ddb84d 100644 --- a/src/workerd/jsg/jsvalue.h +++ b/src/workerd/jsg/jsvalue.h @@ -491,4 +491,14 @@ struct JsValueWrapper { } }; +class JsMessage final { +public: + explicit inline JsMessage() : inner(v8::Local()) {} + explicit inline JsMessage(v8::Local inner) : inner(inner) {} + operator v8::Local() const { return inner; } + +private: + v8::Local inner; +}; + } // namespace workerd::jsg From 87299daf9335422a656da307da600ce28c25a8b7 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 1 Sep 2023 10:33:10 -0700 Subject: [PATCH 2/5] JSG Completion: Use JsValue version of logUncaughtException --- src/workerd/io/io-context.c++ | 9 +++++---- src/workerd/io/io-context.h | 3 ++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/workerd/io/io-context.c++ b/src/workerd/io/io-context.c++ index 9da26437017..0198cfeda5f 100644 --- a/src/workerd/io/io-context.c++ +++ b/src/workerd/io/io-context.c++ @@ -362,7 +362,7 @@ void IoContext::logUncaughtException(kj::StringPtr description) { void IoContext::logUncaughtException(UncaughtExceptionSource source, v8::Local exception, v8::Local message) { - KJ_REQUIRE_NONNULL(currentLock).logUncaughtException(source, exception, message); + logUncaughtException(source, jsg::JsValue(exception), jsg::JsMessage(message)); } void IoContext::logUncaughtException(UncaughtExceptionSource source, @@ -397,7 +397,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)); } @@ -1214,8 +1214,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 239ae0510e9..035917d9e85 100644 --- a/src/workerd/io/io-context.h +++ b/src/workerd/io/io-context.h @@ -1462,7 +1462,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; From 7f77f6a772f21ded79ab4ca53bdf8d9c6acec2e5 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 1 Sep 2023 10:36:55 -0700 Subject: [PATCH 3/5] JSG Completion: remove deprecated logUncaughtException --- src/workerd/io/io-context.c++ | 6 ------ src/workerd/io/io-context.h | 5 ----- src/workerd/io/worker.c++ | 6 ------ src/workerd/io/worker.h | 5 ----- 4 files changed, 22 deletions(-) diff --git a/src/workerd/io/io-context.c++ b/src/workerd/io/io-context.c++ index 0198cfeda5f..c303d1c375c 100644 --- a/src/workerd/io/io-context.c++ +++ b/src/workerd/io/io-context.c++ @@ -359,12 +359,6 @@ void IoContext::logUncaughtException(kj::StringPtr description) { KJ_REQUIRE_NONNULL(currentLock).logUncaughtException(description); } -void IoContext::logUncaughtException(UncaughtExceptionSource source, - v8::Local exception, - v8::Local message) { - logUncaughtException(source, jsg::JsValue(exception), jsg::JsMessage(message)); -} - void IoContext::logUncaughtException(UncaughtExceptionSource source, const jsg::JsValue& exception, const jsg::JsMessage& message) { diff --git a/src/workerd/io/io-context.h b/src/workerd/io/io-context.h index 035917d9e85..37b2ccf9f5a 100644 --- a/src/workerd/io/io-context.h +++ b/src/workerd/io/io-context.h @@ -379,11 +379,6 @@ 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 = {}) - KJ_DEPRECATED("Use the version that takes jsg::JsValue instead"); - void logUncaughtException(UncaughtExceptionSource source, const jsg::JsValue& exception, const jsg::JsMessage& message = jsg::JsMessage()); diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index ac61cee4535..d9cc1e0ca4c 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -1793,12 +1793,6 @@ void Worker::Lock::logUncaughtException(kj::StringPtr description) { KJ_LOG(INFO, "uncaught exception", description); } -void Worker::Lock::logUncaughtException(UncaughtExceptionSource source, - v8::Local exception, - v8::Local message) { - logUncaughtException(source, jsg::JsValue(exception), jsg::JsMessage(message)); -} - void Worker::Lock::logUncaughtException(UncaughtExceptionSource source, const jsg::JsValue& exception, const jsg::JsMessage& message) { diff --git a/src/workerd/io/worker.h b/src/workerd/io/worker.h index 803ca5df169..336eaabbe1b 100644 --- a/src/workerd/io/worker.h +++ b/src/workerd/io/worker.h @@ -584,11 +584,6 @@ 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 = {}) - KJ_DEPRECATED("Use the version that takes jsg::JsValue instead"); - void logUncaughtException(UncaughtExceptionSource source, const jsg::JsValue& exception, const jsg::JsMessage& message = jsg::JsMessage()); From 53db19e2f311a230e231924c82ab184344f53c96 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 1 Sep 2023 11:09:49 -0700 Subject: [PATCH 4/5] JSG Completion: Use JsValue/JsMessage in worker.c++ --- src/workerd/io/worker.c++ | 150 ++++++++++++------------------------ src/workerd/jsg/jsvalue.c++ | 67 ++++++++++++++++ src/workerd/jsg/jsvalue.h | 5 ++ 3 files changed, 122 insertions(+), 100 deletions(-) diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index d9cc1e0ca4c..e41a332efd1 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -174,115 +174,63 @@ 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)); + KJ_DASSERT(message); } - auto stackTrace = message->GetStackTrace(); + // TODO(cleanup): Move the inspector stuff into a utility within jsg to better + // encapsulate + 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 +267,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 +285,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 +305,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 +1247,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 +1489,6 @@ Worker::Worker(kj::Own scriptParam, reportStartupError(script->id, lock, script->isolate->impl->inspector, - context, script->isolate->getLimitEnforcer(), kj::mv(maybeLimitError), catcher, @@ -1785,7 +1734,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); }); } @@ -1802,9 +1751,8 @@ void Worker::Lock::logUncaughtException(UncaughtExceptionSource source, 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 +1761,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 +2197,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/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 be290ddb84d..c95add43562 100644 --- a/src/workerd/jsg/jsvalue.h +++ b/src/workerd/jsg/jsvalue.h @@ -493,10 +493,15 @@ struct JsValueWrapper { class JsMessage final { public: + static JsMessage create(Lock& js, const JsValue& exception); explicit inline JsMessage() : inner(v8::Local()) {} explicit inline JsMessage(v8::Local inner) : inner(inner) {} operator v8::Local() const { return inner; } + operator bool() const { return inner.IsEmpty(); } + + void addJsStackTrace(Lock& js, kj::Vector& lines); + private: v8::Local inner; }; From 6b40aadd3d74cb99c6e5719951375732e213c521 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 1 Sep 2023 11:17:02 -0700 Subject: [PATCH 5/5] JSG Completion: add miscellaneous doc comments --- src/workerd/io/worker.c++ | 3 ++- src/workerd/jsg/jsvalue.h | 14 +++++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index e41a332efd1..a9bd36479f5 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -203,12 +203,13 @@ void sendExceptionToInspector(jsg::Lock& js, // latter case, V8 will create a Message based on the current stack trace, but it won't be // super meaningful. message = jsg::JsMessage::create(js, jsg::JsValue(exception)); - KJ_DASSERT(message); } // 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(); diff --git a/src/workerd/jsg/jsvalue.h b/src/workerd/jsg/jsvalue.h index c95add43562..6c6d8ad9e13 100644 --- a/src/workerd/jsg/jsvalue.h +++ b/src/workerd/jsg/jsvalue.h @@ -494,12 +494,20 @@ struct JsValueWrapper { class JsMessage final { public: static JsMessage create(Lock& js, const JsValue& exception); - explicit inline JsMessage() : inner(v8::Local()) {} - explicit inline JsMessage(v8::Local inner) : inner(inner) {} + explicit inline JsMessage() : inner(v8::Local()) { + requireOnStack(this); + } + explicit inline JsMessage(v8::Local inner) : inner(inner) { + requireOnStack(this); + } operator v8::Local() const { return inner; } - operator bool() const { return inner.IsEmpty(); } + // 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: