Repository navigation
Check for a pending exception in every Rust JSG shim that enters V8 - #8009
Conversation
9870093 to
53c2d46
Compare
|
Follow-up idea, not for this PR: make the rule part of the types instead of a runtime check in each shim. Today a call that can run JavaScript returns
Calling into V8 again after a failure would then be a compile error, not something reviewers have to catch. The guard from this PR would stay as a backstop. A termination requested from another thread only takes effect at V8's next stack check, so it can start inside any call, and a pending exception can also come from a C++ caller. This fits best in the rusty_v8-based bindings, where the borrow checker already tracks scopes. For comparison, rusty_v8 and Deno have neither a guard nor a type like this. Deno checks the |
53c2d46 to
33d83e0
Compare
piscisaureus
changed the title
33d83e0 to
726ff5f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8009 +/- ##
==========================================
- Coverage 68.67% 68.66% -0.01%
==========================================
Files 725 725
Lines 188585 188631 +46
Branches 29628 29653 +25
==========================================
+ Hits 129513 129531 +18
- Misses 41704 41728 +24
- Partials 17368 17372 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…8039) Rust JSG's `v8` module takes handles and returns handles. Converting between a Rust value and a JavaScript value is the job of `FromJS` and `ToJS`, not of a `v8` method. A `v8` method returns a Rust value only where V8 itself returns a C scalar, from a typed handle (`value`, `length`, `value_of`, `is_*`) or from one of V8's coercions of any value (`int32_value`, `uint32_value`, `integer_value`), or copies memory out (`to_vec`, `to_string`), and takes raw Rust data only where V8 takes C data: constructor input, indices, times and `&str` property keys. The one method that breaks the rule is `Local<Function>::call`, which converts its result with `FromJS`; two thirds of its callers ask for `Local<Value>`, that is, for no conversion. A survey of the module also found a bug. Three C++ shims are declared infallible but can throw a KJ exception: `local_new_string`, `exception_create_from_bytes` and `isolate_throw_error`. All three build their string through `newUtf8String`, which fails a `KJ_REQUIRE` for input longer than `v8::String::kMaxLength`. An exception that crosses a cxx bridge function not declared `Result` becomes a Rust panic, so a string longer than V8's limit (about 512 MiB) returns `None` from `v8::String::new_from_str` but panics through `"…".to_js(lock)`, although `to_js` returns a `Result`. Strings: * `ToJS` for `&str` and `String` goes through `v8::String::new_from_str` and fails with `Error::invalid_string_length()`, a `RangeError` with V8's own message, "Invalid string length". The error is not pending and nothing has entered V8. The `local_new_string` shim is removed. * A `#[jsg_struct]` converts its fields with `ToJS` (since #8014), so an overlong `String` field gives the same `RangeError`. * The `v8::String` constructors decide in Rust whether the input fits `MAX_LENGTH`, so `None` means exactly that, and no length above `i32::MAX` reaches V8's `int` parameter. V8's `NewFromUtf8` measures its input against `kMaxLength` in bytes too, so the Rust check refuses exactly what V8 refuses. `new_from_utf8_literal` keeps its panic, as V8's `NewFromUtf8Literal` fails a `static_assert`. Error messages: * An error message longer than the limit is cut to fit rather than refused. `error_message_for_v8` cuts it in Rust at a UTF-8 boundary; `exception_create_from_bytes` takes a slice, clamps as a last defence and uses `ToLocalChecked()`, so it is total. A message that long is a bug or embeds user data, and its start is what a reader wants. Returning a `Result` would make `Lock::error`, `Lock::throw_exception` and the fast API error path fallible for a case no caller can act on, and a fixed replacement message would lose the information. * `isolate_throw_error` is removed. `Lock::throw_error` creates the error with `Lock::error` and throws it through `isolate_throw_exception`, after the same termination check as before. * `newUtf8String` returns a `MaybeLocal` and never throws; each caller decides. `makeInternedStr` keeps its `KJ_REQUIRE`: the property shims that intern a `&str` key and `object_new_with_data_properties` (used by `#[jsg_struct]` and, since #7998, `jsg::Dict`) are declared fallible, and resource template names come from Rust source. `Function::call`: `call` returns `Result<Local<Value>, Error>`. A caller that wants a Rust value converts the handle itself: let result = f.call(lock, None, &[])?; let n = f64::from_js(lock, result)?; Every caller, the doc examples in `lib.rs`, `v8.rs` and the README, the compile tests and the benchmark fixture are converted. With #8054 (no `jsg::Number`) and #8072 (integer `FromJS`/`ToJS`), the converted tests read their results as `u32`, `i32` or `f64`. The only caller outside the jsg crates, the inspector in `src/rust/api` (#7986), ignores the result. Conventions: The README gets a section "Conventions of the `v8` module", and `src/rust/AGENTS.md` a matching bullet: when a method returns or takes a Rust value, when it returns `Result` or `Option`, that no method has an infallible signature over a C++ shim that can throw, and the `as_*`/`to_*`/`value`/`*_value` naming. It covers the APIs from #8054 and #8072: `Local<Number>::value` and the other typed-handle reads, `Number::new` and `Integer::new` as constructors taking C data, and the `int32_value`/`uint32_value`/`integer_value` coercions, which can run JavaScript and so return `Result`. Related: * #8009 adds a pending-exception guard to every shim; it touches `ToJS` for strings and the error shims, and this change is written to fit either order. Not done here: * Renames that the conventions suggest: `Local<String>::to_string` (a lossy copy to a Rust `String`) and `Value::type_of`, which returns a Rust `String` rather than a handle. * `v8::String::MAX_LENGTH` is a copy of `kMaxLength`, checked against a hard-coded number. The C++ clamp and V8's own check keep a drift from crashing anything, but the `RangeError` threshold would be off. * A smaller cap for error messages than V8's string limit.
726ff5f to
6df0340
Compare
|
This pull request's reviews are posted as pull request reviews: see the review of |
440b093 to
6b5f4e4
Compare
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Nice work tracking down all the V8 entry paths. I have one suggestion on the condition the guard checks (inline on exceptionPending()): it also treats an exception that a C++ v8::TryCatch has already caught as pending, which V8 doesn't require. When that misfires the failure is silent, since the Rust API just returns undefined, and it is the reason handleLog() and IoContext::runImpl() need their resets. It also puts the "reset before calling into Rust JSG" rule into AGENTS.md, the README and the checklist.
I tested it locally at 6b5f4e4:
| Setup | jsg-test_test |
caught-exception-reset-test@ |
|---|---|---|
| PR as is | pass | pass |
probe TryCatch in exceptionPending(), both C++ resets removed |
pass | pass |
| no probe, both C++ resets removed (control) | both subtests fail: domainToASCII() returns undefined |
With the probe, all of the guard tests in jsg-test still pass, so a pending exception or a termination still blocks entry. One rule is left: C++ that has caught an exception and calls Rust JSG directly, with no JavaScript in between, still needs to reset first. That one is V8's own rule (the internal exception is still set at call depth > 0), so the docs could say that instead of the broader rule.
I'd keep the two resets as hygiene either way. Not blocking, but I think it's worth doing in this PR, since it changes what the docs say.
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
LGTM. The guard in each shim is the right backstop, and I confirmed locally that both caught-exception-reset-test cases fail with the two Reset()s reverted.
Two things worth following up:
- Script startup in
worker.c++has the same pattern as the two sites fixed here: aftercatcher.HasCaught(),js.exceptionToKj(...)and the error reporter and inspector paths read the exception's properties, so getters can run, before theTryCatchis reset. I couldn't find a way to observe it from a test, since the worker is dead by then, but it seems worth aReset()like the one inrunImpl(). - This interacts with #8086. Its
callCpp()makes sure nothing is thrown on the way out while execution is terminating, but it doesn't guard against entering while an exception is already pending. So a Rust caller with a pending exception can run C++ JSG code that enters V8 with a context, e.g.cppRefWrap→TypeHandler::wrap→ObjectTemplate::NewInstance. Whichever of the two lands second should probably startcallCpp()withif (exceptionPending(isolate)) return false;.
|
Your reading of V8 seems right. Once an API that opens a I would agree with you if we had no plans for this codebase. Rephrasing the argument in favor of your suggestion:
However, I'm thinking a bit more about where we want to be: C++ gone, everything in Rust. When we get there, it makes more sense to keep the broader check:
But I undersand with your worry, let me see if I can add a structural fix on the C++ side. I think JSG_TRY already does the right thing so we're really jsut looking for a few places where TryCatch is used directly. |
|
Thanks for clarifying, yes if Rust is the outermost caller then the probe isn't a requirement, perhaps there is some way we can ensure that during the transition? For example, asserting in debug builds that no Rust JSG callback is entered with HasPendingException() true would make a missed Reset() fail loudly in tests rather than return undefined. That stays true in a Rust-only world too, since try_catch's own TryCatch hasn't caught anything when a callback runs. Plus the startup site in worker.c++ from my other comment. |
6b5f4e4 to
e273ee4
Compare
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
9272c9b7.
This review, of e273ee41
💬 Approved with comments — One WARNING that affects tests only: the new raw-v8-trycatch-test is not excluded on linux_aarch64, where the custom clang-tidy cannot run. There is also one optional documentation SUGGESTION. The runtime changes are correct.
Findings: 1 warning · 1 suggestion (1 is an inline comment on the diff)
Not opened by the correctness review: 3 changed files it was meant to read
src/workerd/api/unsafe.c++src/workerd/api/worker-rpc.c++src/workerd/io/io-context.c++
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #8009.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] The new `raw-v8-trycatch-test` is missing the `linux_aarch64` incompatible entry that `iocontext-run-manual-capture-test` has (`tools/clang-tidy/BUILD.bazel:266`). The custom clang-tidy cannot run on the arm64 Linux CI image, so the test will probably fail in that CI job. This affects tests only.
- **File:** tools/clang-tidy/BUILD.bazel:290
## SUGGESTION (1)
### [AGENTS.md] The rule text in the AGENTS files could also mention V8 APIs that the shims check before even though they take no context argument, such as `ArrayBuffer::Detach`.
- **File:** src/rust/AGENTS.md:48
📖 AGENTS.md: 💬 1 suggestion
One optional wording note on the scope of the rule.
Recommendation: 💬 Optional Nits
- [SUGGESTION] The rule text in the AGENTS files could also mention V8 APIs that the shims check before even though they take no context argument, such as
ArrayBuffer::Detach. (src/rust/AGENTS.md:48)
View full review
AGENTS.md:305 and src/rust/AGENTS.md:48 state the rule in terms of APIs that take a v8::Local<v8::Context>. The shims also check before ArrayBuffer::Detach, which takes no context argument. The guidance could mention that case.
📋 Code Quality: ⚠️ 1 warning
The new clang-tidy sh_test is missing the linux_aarch64 exclusion that the test next to it has. The other changes are correct.
Recommendation: 📝 Minor Comments
- [WARNING] The new
raw-v8-trycatch-testis missing thelinux_aarch64incompatible entry thatiocontext-run-manual-capture-testhas (tools/clang-tidy/BUILD.bazel:266). The custom clang-tidy cannot run on the arm64 Linux CI image, so the test will probably fail in that CI job. This affects tests only. (tools/clang-tidy/BUILD.bazel:290)
View full review
raw-v8-trycatch-test will be scheduled on arm64 Linux (tools/clang-tidy/BUILD.bazel:288-293)
On aarch64 Linux, //tools:clang-tidy resolves to @clang_tidy_linux_arm64 (tools/BUILD.bazel:65). The test next to it, iocontext-run-manual-capture-test, marks linux_aarch64 as incompatible (tools/clang-tidy/BUILD.bazel:266). The TODO above that line says why: the custom clang-tidy needs a newer glibc than the arm64 CI runner image has. The new target copies the rest of that select() but leaves out this entry, so it will run, and probably fail, in the arm64 Linux CI job. This affects tests only and has no effect on production.
Checked, no issues
- Every context-taking shim in
ffi.c++andser.c++checksexceptionPendingfirst.local_type_ofis exempt, and its comment says why. - In
wrap_resource, the check for a wrapper in another isolate still runs first, and the new test inisolates.rscovers that order. utf8_value_newreturns a null pointer on failure, and Rust turns that into anErr.new_functionstreats an error fromwrappable_attach_opaque_wrapperas a pending exception only when V8 actually has one pending. Other KJ errors are passed on.- The assertion in
scope.rscovers only callbacks that V8 enters, and it allows a termination. reportStartupError(),handleLog()andIoContext::runImpl()takeException()andMessage()beforeReset(), and they reset only whenCanContinue()is true, so a termination is never cleared.- Each raw
v8::TryCatchhas aNOLINTNEXTLINE(custom-raw-v8-trycatch)comment directly above it.
✅ All other checks passed
🔒 Security: ✅ LGTM
The pending-exception checks make the shims stricter and add no new attack surface. The new TryCatch resets never clear a termination: each one runs only when CanContinue() is true. The new callback-entry check in scope.rs is a debug_assert!, so release builds behave as before. The change to unsafe.c++ is only a comment on a code path used for fuzzing.
⚡ Performance: ✅ LGTM
Each shim's check reads one isolate flag. It replaces the extra FFI call that call_into_js() used to make before every operation, so each operation now costs less.
📝 Documentation: ✅ LGTM
The README, the review checklist and the comments in ffi.h describe the pending-exception rule and the TryCatch reset requirement accurately.
Reviewed commit: e273ee41
I've added a clang-tidy lint,
Agreed and done. With debug assertions on,
I agree too. On #8086: if it lands before this PR, I'll add |
A `v8::TryCatch` that has caught an exception keeps it pending, as `v8::Isolate::HasPendingException()` reports it, until it is reset or destroyed. Rust JSG's promise shims already fail while an exception is pending, and a later commit makes every Rust JSG shim that enters V8 do the same. A few places caught an exception and then ran JavaScript that can call into Rust JSG: * `handleLog()` stringifies `console.log()` arguments for the trace under one `TryCatch`. After one argument threw, or a `toJSON` getter read by `GetRealNamedProperty()` threw, the next `toJSON()` or `toString()` ran with that exception still caught. * `IoContext::runImpl()` logged and tunneled an exception while its `TryCatch` still held it. * `reportStartupError()` converted and reported the startup exception while the caller's `TryCatch` held it. This was safe only because the function opens a `TryCatch` of its own first, which is then the innermost one. Reading the exception's properties can run getters. Each now copies what it needs and resets the `TryCatch` first, unless execution is terminating. The destructor resets it anyway, so nothing else changes. `caught-exception-reset-test` calls into `node:url`, which is implemented with Rust JSG, from the `handleLog()` and `runImpl()` paths, and checks the results.
A `v8::TryCatch` that has caught an exception keeps it pending until it is reset or destroyed, and Rust JSG fails while an exception is pending. `jsg::Lock::tryCatch()` and `JSG_TRY`/`JSG_CATCH` destroy their TryCatch before the handler runs; code with a TryCatch of its own has to reset it before running more JavaScript. The query-based check `custom-raw-v8-trycatch` flags every variable or member of type `v8::TryCatch`. Each existing one gets a `NOLINTNEXTLINE(custom-raw-v8-trycatch)` comment that says why it is safe. `//tools/clang-tidy:raw-v8-trycatch-test` checks what the query matches. Like the test of the other query-based check, it doesn't run on arm64 Linux, whose CI runners have too old a glibc for clang-tidy.
A V8 API that takes a v8::Local<v8::Context> must not be entered while an exception or a termination is pending. Some of these APIs (those that open a PrepareForExecutionScope, in src/api/api-inl.h) clear the pending exception on entry, a termination included, so it is lost and JavaScript runs on as if nothing had happened. Others (EnterV8Scope) fail a V8 debug check, and in a release build run on with the exception still pending. Creating strings, numbers, objects, templates and error objects takes no context and is fine at any time, also while execution is terminating. Every C++ shim that calls such an API now first checks exceptionPending(), which the promise shims already used, and fails with an empty result or false, leaving the exception pending. Rust turns that into a pending jsg::Error as before. This covers Function::Call, property get, set and has, Object::keys, defining a new object's data properties, the coercions (including Int32Value, Uint32Value and IntegerValue), Array::Iterate, Array::set, serialization and deserialization, Date::New, Function::New, the opaque wrapper, FunctionTemplate::GetFunction and String::Utf8Value. In wrap_resource, the check replaces one that looked only for a termination. It comes after the check for a wrapper in another isolate, so that the caller leaks its reference then, as it does without an exception pending, rather than releasing it without that isolate's lock. Utf8Value::new can't report the failure yet and gives an empty value; the next commit changes that. The check replaces the isolate_is_execution_terminating pre-check that call_into_js() made before every such call. That cost an extra FFI call per operation and missed an ordinary pending exception. The pre-check in Function::new goes the same way. exceptionPending() moves to ffi.h as an inline function. Operations that would not run JavaScript for a given value now also fail while an ordinary exception is pending, not only while execution is terminating: converting a string or number primitive, and wrapping a resource that already has a wrapper. They fail the same way whatever the value, which is simpler to reason about than a check per value. "Pending" includes an exception that the innermost v8::TryCatch has caught and not yet reset, as v8::Isolate::HasPendingException() reports it. C++ code that catches with its own v8::TryCatch must reset it before it calls into Rust JSG; jsg::Lock::tryCatch() and JSG_CATCH leave the TryCatch's scope first. API changes: function_template_of and Global<FunctionTemplate>::as_local_function now return a Result, so that an empty result from GetFunction becomes a pending error rather than a panic through jsg::check. With that, ffi.c++ uses jsg::check no more, and its allowlist entry goes. Creating the data of a function from a Rust closure passes on a C++ exception, such as a failed KJ_REQUIRE, instead of reporting it as a pending exception when none is pending. The docs of ToJS, resource::wrap, Date::new, ser and the conversions say that they also fail while an ordinary exception is pending. Comments that said creating an error object while terminating fails a V8 debug check are corrected: it does not. Tests: getting a constructor and Utf8Value while terminating keep the termination and run no JavaScript. With an ordinary exception pending, a call, a coercion, a property read, creating a Date, wrapping a new or an existing resource, getting a constructor and Utf8Value run nothing, and the exception reaches JavaScript unchanged. Wrapping a resource that has a wrapper in another isolate panics, and leaks the reference, also with an exception pending.
ArrayBuffer::detach and Utf8Value::new now check for a pending exception before they enter V8, as the other shims do, and return a Result. That removes the last exceptions to the rule that every shim passing a context to V8 checks first. * ArrayBuffer::detach runs no JavaScript, but V8 enters it with the current context and does not allow that while an exception or a termination is pending. * Utf8Value::new converts with ToString itself and leaves what it throws pending. v8::String::Utf8Value would catch and discard it, and read as an empty string. A Symbol now fails with V8's TypeError rather than giving an empty value. API changes: ArrayBuffer::detach and Utf8Value::new return a Result; callers pass the error on with `?`. The README's "Errors from JavaScript" now lists Utf8Value::new among the operations that leave an exception pending. Tests: detaching while terminating or with an exception pending detaches nothing; Utf8Value of a Symbol or of a value whose toString throws fails with the exception pending.
Rule 1 was "Never drop or replace a pending termination": don't throw while execution is terminating, and don't discard an error that may be the termination. It didn't say which V8 APIs may be entered while an exception or a termination is pending, which is what the shims now check. A V8 API that takes a context must not be entered then: some clear the exception on entry, so it is lost and JavaScript runs on, and others fail a debug check and in a release build run on with it pending. Creating strings, numbers, objects, templates and error objects takes no context and is fine at any time. Rule 1 in the README and the AGENTS files, the review checklist and the review prompts now state that rule. They say that every shim that passes a context to V8 first checks exceptionPending(), or says why it needn't, that the check also sees an exception a v8::TryCatch has caught, so C++ code resets its TryCatch before calling into Rust JSG (the README points at the custom-raw-v8-trycatch clang-tidy check), and that a pending error from Object::set or ToJS::to_js must be passed on like any other. They tell reviewers not to flag creating values while execution is terminating.
A Rust JSG callback that V8 enters while an exception is pending can only fail: every operation it tries fails with a pending error, and JavaScript sees `undefined`. That happens when C++ runs JavaScript while its `v8::TryCatch` still holds a caught exception, a missed `Reset()` that is otherwise easy to overlook. With debug assertions, `CallbackScope::open()` now panics in that case for the callbacks V8 enters (functions, accessors and interceptors), unless the exception is a termination. As for any panic in a callback, that throws an internal error and terminates execution, so the missed reset fails a test instead of producing a wrong value. Scopes that C++ opens with an isolate pointer are not checked. A new jsg-test case, run only with debug assertions, enters a callback while a C++ TryCatch holds a caught exception.
e273ee4 to
9272c9b
Compare
There was a problem hiding this comment.
💬 Approved with comments — The earlier WARNING is fixed: raw-v8-trycatch-test now marks linux_aarch64 incompatible. The runtime changes are correct, and one optional SUGGESTION about the AGENTS wording remains.
Findings: 1 suggestion
Not opened by the correctness review: 2 changed files it was meant to read
src/workerd/api/unsafe.c++src/workerd/api/worker-rpc.c++
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📖 AGENTS.md: 💬 1 suggestion
One optional wording suggestion for the Rust JSG safety rule.
Recommendation: 💬 Optional Nits
- [SUGGESTION] The Rust JSG safety rule could also name the V8 APIs the shims check even though they take no context argument, such as
ArrayBuffer::Detach. (src/rust/AGENTS.md:48)
View full review
AGENTS.md:305 and src/rust/AGENTS.md:48 state the rule in terms of V8 APIs that take a v8::Local<v8::Context>. The shims also check before ArrayBuffer::Detach, which takes no context argument. Naming that case would stop contributors from concluding that such calls are safe while an exception is pending.
✅ All other checks passed
🔒 Security: ✅ LGTM
The pending-exception checks make the shims stricter and add no new attack surface. Each new TryCatch reset runs only when execution can continue, so none of them clears a termination. The callback-entry check is a debug_assert!, so release builds behave as before.
📋 Code Quality: ✅ LGTM
🔄 Since last review: 1 finding resolved, 1 preserved, 0 new.
- Earlier finding, now fixed:
tools/clang-tidy/BUILD.bazelnow marks@bazel_tools//src/conditions:linux_aarch64incompatible forraw-v8-trycatch-test. This matchesiocontext-run-manual-capture-test. - Shim guards: in
src/rust/jsg/ffi.c++andser.c++, every shim that passes a context to V8 checksexceptionPending(isolate)first. Inwrap_resourcethe check runs after the other-isolate check, so that path still leaks the reference instead of releasing it without the other isolate's lock.local_type_ofskips the check, and its comment explains why. - Signature changes:
utf8_value_newreturns a null pointer on failure, and Rust maps that to anErr.function_template_get_functionandwrappable_attach_opaque_wrapperreturn an empty value on failure, and the Rust callers handle it.new_functionstreats an FFI error as a pending exception only when V8 actually has one pending; other KJ errors are passed on. - TryCatch resets:
IoContext::runImpl()takesException()andMessage()before it callsReset(), and resets only in the branch where execution is not terminating.reportStartupError()resets only whenCanContinue()is true.handleLog()resets throughforgetCaughtException()after atoJSONlookup fails and after stringifying fails.- None of these resets can clear a termination.
- Other C++ files:
unsafe.c++andworker-rpc.c++only gain aNOLINTNEXTLINE(custom-raw-v8-trycatch)comment, and the reason given in each is correct. Every remaining rawv8::TryCatchundersrchas that comment. The positive and negative test files match what the test script expects. - Debug assertion: the check in
scope.rscovers only callbacks that V8 enters, allows a termination, and runs only underdebug_assert!. - Tests: the new and updated tests in
pending_exception.rs,termination.rs,coercion_safety.rs,isolates.rs,string.rs,buffer_types.rsandcaught-exception-reset-testcover these guards and resets.
⚡ Performance: ✅ LGTM
Each shim now reads one isolate flag before it enters V8. That replaces the extra FFI call call_into_js() used to make before every operation, so each operation costs less. The new TryCatch resets add negligible overhead.
📝 Documentation: ✅ LGTM
The README, the review checklist and the comments in ffi.h correctly describe the pending-exception rule and the requirement to reset a TryCatch.
Reviewed commit: 9272c9b7
This builds on #7997, which made Rust JSG's
ToJSfallible and gavewrap_resourcea check for a termination. This change applies thatcheck to every shim that enters V8 with a context, and to any pending
exception, not only a termination.
What V8 does: an API that takes a
v8::Local<v8::Context>must not beentered while an exception or a termination is pending. How it goes
wrong depends on how the API enters V8 (
src/api/api-inl.h):PrepareForExecutionScopeclears the pending exception on entry, atermination included. The exception is lost and JavaScript runs on
as if nothing had happened.
Object::Get,ToStringof anon-string,
ObjectTemplate::NewInstance,FunctionTemplate::GetFunction,Function::New,Date::Newand thepromise APIs enter this way.
EnterV8ScopeandEnterV8NoScriptScopedon't clear it. They faila debug check, and in a release build run on with the exception
still pending. Through
EnterV8Scopethat can run user JavaScriptwhile an exception is in flight.
Function::Call,Object::Has,Object::Set,CreateDataPropertyandNumberValueenter thisway, and
ArrayBuffer::DetachthroughEnterV8NoScriptScope.arrays, typed arrays, error objects and templates takes no context
and is fine at any time, also while execution is terminating.
Before this change,
call_into_js()checkedisolate_is_execution_terminatingfrom Rust before each shim. Thatcost an extra FFI call per operation, missed an ordinary pending
exception, and didn't cover the shims that don't go through
call_into_js().ArrayBuffer::detachandUtf8Value::newcould notreport a pending exception at all.
exceptionPending(isolate)(a termination orHasPendingException(), which the promise shims already used) andfails with an empty result or
falsewithout entering V8. Rustturns that into a pending
jsg::Error, as it does for an exceptionthe call itself throws. That includes the shims added since this
was first written: the
Int32Value/Uint32Value/IntegerValuecoercions,
Object::keysandget_by_name,Object::with_data_properties, serialization andto_js_string.A shim that skips the check says why in a comment (
local_type_of).In
wrap_resourcethe check comes after the one for a wrapper inanother isolate, so that
resource::wrap()still leaks itsreference then instead of releasing it without that isolate's lock.
The Rust pre-check in
call_into_js()and inFunction::newisremoved.
ArrayBuffer::detachandUtf8Value::newnow return aResult.Utf8Value::newconverts withToStringitself, becausev8::String::Utf8Valuecatches and discards whattoStringthrows;a Symbol now fails with V8's
TypeErrorinstead of reading as anempty string.
function_template_ofandas_local_functionreturna
Resulttoo, soffi.c++no longer usesjsg::check.prompt state the rule as V8 requires it, and tell reviewers not to
flag creating values while execution is terminating. Comments that
said creating an error object while terminating fails a debug check
are corrected.
pending_exception.rsand intermination.rs,coercion_safety.rs,resource_callback.rs,string.rs,isolates.rsandbuffer_types.rs: with an exception or atermination pending, each guarded operation runs no JavaScript and
the exception reaches JavaScript unchanged; what
toStringthrows inUtf8Value::newis passed on. Removing any guard exceptfunction_new's fails a test.function_newis called only afterthe opaque wrapper, whose shim already checks.
Design choice: the check lives in each C++ shim, next to the V8 call
it protects, and it fails whatever the value. Converting a string
primitive or wrapping a resource that already has a wrapper would run
no JavaScript, but these fail too while an exception is pending. One
rule per shim is easier to review than a check that depends on the
value, and these calls have no reason to run while an exception is
propagating.
"Pending" includes an exception that the innermost
v8::TryCatchhascaught and not yet reset, because that is what
HasPendingException()reports. So C++ code that catches with its own
v8::TryCatchmustreset it before it calls into Rust JSG, or runs JavaScript that might.
jsg::Lock::tryCatch()andJSG_TRY/JSG_CATCHalready leave theTryCatch's scope first. Three places did not, and now reset first:handleLog(), which kept stringifyingconsole.log()arguments forthe trace after one threw, or after a
toJSONgetter threw.IoContext::runImpl(), which logged and tunneled an exception whileits
TryCatchstill held it.reportStartupError(), which was safe only because it opens aTryCatchof its own first.Reading the exception's properties can run getters. Tests in
caught-exception-reset-testcallnode:urlfrom the first two.To keep it that way:
custom-raw-v8-trycatch, flags every rawv8::TryCatch. Each existing one has aNOLINTNEXTLINE(custom-raw-v8-trycatch)comment saying why it issafe.
exception other than a termination is pending panics, which throws
an internal error and terminates execution. A missed reset then
fails a test instead of producing
undefined. CI runs Rust debugassertions in the arm64 Linux debug job.
Alternatives considered:
call_into_js(), widened to any pendingexception. It costs an FFI call per operation, and the shims that
don't go through
call_into_js()stay unguarded.wrap_resource.That still lets JavaScript run with an ordinary exception pending,
which V8 allows in release builds and which can replace or hide the
first exception.
v8::TryCatch. That catches what the callthrows, but does nothing about an exception that is already
pending, and would hide exceptions that must keep propagating.
v8::TryCatchinexceptionPending(), so that an exception a C++TryCatchhascaught doesn't count. V8 tolerates that state, so the C++ resets
would not be needed. But once Rust drives V8 with no JavaScript on
the stack, V8 clears its internal exception as soon as the throwing
call returns, and the probe would let Rust run more JavaScript after
a failure inside
try_catch. It would also give Rust JSG twodefinitions of "pending".
The commits each stand on their own:
v8::TryCatchArrayBuffer::detachandUtf8Value::newfallibleIf #8086 lands first,
callCpp()gets the sameexceptionPending()check here.