Repository navigation
Throw DOMExceptions from Rust JSG - #7951
bartlomieju wants to merge 16 commits into
Conversation
Rust JSG offered DOM error types such as AbortError and NotSupportedError but threw them all as a plain Error, and could not express DOMException(SyntaxError) because SyntaxError meant the JS class. Rename the DOM variants to mirror the C++ JSG_REQUIRE error types (DOMSyntaxError, DOMAbortError, ...), throw real DOMExceptions for them, and recognize DOMExceptions in tunneled KJ errors and in FromJS for jsg::Error. No shipped Rust API throws these types, so observable JS behaviour is unchanged.
…mexception # Conflicts: # src/rust/jsg-test/tests/mod.rs # src/rust/jsg/README.md # src/rust/jsg/ffi.c++ # src/rust/jsg/lib.rs
Creating a DOMException clears V8's pending exception, so a DOM-typed error returned after a termination replaced it with an exception that JavaScript could catch. Throwing now does nothing while terminating, and a failed creation lets V8's pending exception propagate instead of logging an internal error. DOMException names without a DOM-prefixed type are kept, and internal DOM errors throw a DOMException with the redacted message, as C++ JSG does.
piscisaureus
left a comment
piscisaureus
left a comment
There was a problem hiding this comment.
Direction is right. The two new shims add jsg::Lock/jsg::Ref, which rule 3 normally forbids, but I'm OK with that as an allowlisted exception: DOMException is a C++ JSG resource and Rust has to produce the same class.
Please rebase onto main (#7965), bump the allowlist explicitly, and keep the TODO pointing at moving DOMException out of C++ JSG. Expect a conflict with #7961 in FromJS for Error. Nit: log_internal_error builds the "internal error; reference = " text by hand instead of using renderInternalError.
| /// Each DOM-prefixed type with the name and legacy `code` the thrown | ||
| /// `DOMException` has (`src/workerd/jsg/dom-exception.h`). | ||
| #[rustfmt::skip] | ||
| const DOM_TYPES: [(ExceptionType, &str, u16); 13] = [ |
There was a problem hiding this comment.
We should likely just move DOMException to per-isolate TS. On the native side we just need a function to create a DE instance.
|
Pausing this for now, will wait until per-isolate autogate is gone and then try to do minimal V8 bindings + TS impl |
|
A follow-up for this PR: the Rust port of When a fallback produces a value that can't be serialized, C++
The port's trace parity test ( |
…mexception # Conflicts: # src/rust/jsg-test/ffi.c++ # src/rust/jsg-test/ffi.h # src/rust/jsg-test/lib.rs # src/rust/jsg/ffi.c++ # src/rust/jsg/ffi.h # src/rust/jsg/lib.rs # src/rust/jsg/v8.rs
Every caller only wanted the JS handle, and the filesystem API took a TypeHandler<Ref<DOMException>> on 16 methods just to wrap the Ref. With a JsObject the DOMException no longer has to be a C++ JSG resource, which lets it move to the per-isolate TypeScript bootstrap. No behaviour change.
A DOMException now carries the name and message it was created with in V8 private properties. Native code recognizes one by them and creates one as a plain object over the context's DOMException prototype, held in a new context data slot, with the own `code`, `name`, `message` and `stack` properties a DOMException has always had. Neither runs JavaScript, and neither needs the C++ JSG resource, so the TypeScript class the per-isolate bootstrap will install can share them. The serializers write a DOMException by its brand. Rust JSG creates and recognizes DOMExceptions through the same V8-only functions, so its shims no longer use jsg::Lock or jsg::Ref, and the allowlist is back to its counts on main. Only an object's own brand counts. C++ JSG also unwrapped an object that had a DOMException on its prototype chain, such as `Object.create(new DOMException())`; nothing depends on that. kj::str() of a natively created DOMException now gives "Name: message" rather than an empty string. MemoryCache.read() relied on the empty string to report a value that cannot be serialized as "failed to serialize <type>", so it now checks for a DOMException explicitly.
The per-isolate bootstrap replaces the C++ DOMException with a TypeScript one (src/per_isolate/dom-exception.ts), and hands native code its prototype, so the DOMExceptions native code and Rust JSG create are instances of it. Its constructor brands an instance as native code does, through a new utils.initDOMException(), so both kinds are recognized alike. The class copies the C++ class's shape: the global lives on the global object's prototype with the same attributes; instances have own read-only `code`, `name` and `message` and an enumerable `stack` in that order; the legacy constants are on the constructor and the prototype; and the attributes that compatibility flags choose (set_tostring_tag, immutable_api_prototypes, spec_compliant_property_attributes) are copied from the C++ class rather than recomputed. The stack leaves out the constructor's own frame. dom-exception-test pins the shape, and passes against the C++ class too. Compatibility: this changes the standard API surface in one way, which we accept: Function.prototype.toString() of DOMException shows its source rather than `[native code]`. It is ungated. The webfs bootstrap recognizes a DOMException by its brand rather than with instanceof, and the primordials no longer capture DOMException.
|
The generated output of |
|
Not ready for review yet, I'm still iterating on this one |
|
|
||
| // The message and name are converted as Web IDL converts a DOMString, which a template literal | ||
| // does too: a Symbol throws a TypeError. | ||
| function DOMException(this: object, ...args: unknown[]): void { |
There was a problem hiding this comment.
I know you mentioned that you're still iterating.. but I'm curious... why a function and not a class DOMException?
There was a problem hiding this comment.
To match the prototype property of the JSG class it replaces. A class's prototype is non-writable and non-configurable, so it can't be redefined afterwards. A JSG class's prototype is read-only only under immutable_api_prototypes, which has no enable date, so today DOMException.prototype is writable for every worker. setUpClass() copies the attribute from the C++ class, and a function is what lets it match both cases.
Using a class here would make DOMException.prototype read-only for every worker, are we fine with it being an ungated change? That said, once immutable_api_prototypes gets an enable date, this can become a class without any behavior changes.
I added a TODO in code for that
There was a problem hiding this comment.
I'm not convinced we need to strictly match the current JSG behavior without a gate. It is exceedingly unlikely that anyone is actually depending on DOMException.prototype being writable.
There was a problem hiding this comment.
Thanks, moved to a class when autogate is active
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Really nice direction, branding DOMExceptions so native code can create and recognize them with V8 alone makes the TS move clean.
Not for this PR, but it might be worth adding a serializable property on Rust resources later, encoded in the wrapper's field-0 marker. DOMException could then be a host object through V8's default internal-field check, instead of every serializer needing HasCustomHostObject() and the brand lookups.
…mexception Conflicts are additions on both sides, except for these adaptations to main: - exception_create_from_bytes takes a slice and never fails (main); Error::to_local keeps returning a Result, since creating a DOMException can fail, and creates other errors with Lock::create_error_from_bytes. - dom_exception_create and local_try_unwrap_dom_exception check exceptionPending() first, as main requires of every shim that passes a context to V8 (defining properties, capturing the stack and reading private properties all do). The unwrap shim reports that through its return value, so FromJS for jsg::Error fails with a pending error then, as it does for a native error; to_local drops its own termination check. A DOMException's name and message longer than V8's string limit are cut to fit, as main cuts an error message. - The DOMException tests use f64 for jsg::Number and the Result that function_template_of now returns, and Function::call is not generic. - ExceptionWrapper::tryUnwrap no longer needs the TypeWrapper.
A DOMException that native code or the TypeScript class creates is now an API object with three internal fields, from an ObjectTemplate that IsolateBase holds: a brand (an aligned pointer to a static tag) and the name and message it was created with. The TypeScript constructor returns such an object, created by utils.createDOMException() with the prototype that `new.target` gave `this`, in place of `this`, so subclasses and Reflect.construct() work as before. An instance of the C++ class, the global DOMException of isolates without the bootstrap, is recognized as the C++ object it wraps. As Guy suggested in review, this makes a DOMException a host object through V8's default internal-field check, so neither serializer looks up the brand in IsHostObject() any more. Both keep HasCustomHostObject() for native errors and class instances. Recognizing a DOMException reads internal fields, which takes no context, so it works while an exception or a termination is pending: local_try_unwrap_dom_exception no longer checks exceptionPending() or reports it, and FromJS for jsg::Error reads a DOMException's name and message then too. A context snapshot writes an internal field that holds an aligned pointer only through the embedder's callbacks, and workerd registered none, so the brand of a DOMException created while preparing a startup snapshot would have deserialized as null. setSnapshotDefaultContext() now passes a SerializeInternalFieldsCallback that writes the brand, and every v8::Context::New() passes newContextInternalFieldsDeserializer(), which restores it; V8 requires a deserializer for any context deserialized from a snapshot that has such fields. Other aligned pointers, such as the markers of JSG wrappers, whose C++ objects createSnapshotBlob() destroys, still deserialize as null. No change to the standard API surface: a DOMException has the same own properties, prototype and attributes. Tests: a DOMException is recognized while an exception is pending, and one created in a snapshot's default context keeps its brand, name and message in an isolate started from the snapshot. The latter fails without the serializer callback.
The reason was in the file's header comment, where a reviewer missed it. Add a TODO to make it a class once immutable_api_prototypes is enabled by default.
Cast Local<Data> through Value before narrowing it; V8_ENABLE_CHECKS builds reject the direct cast. Move the snapshot round trip into its own test binary. An isolate group shares one read-only heap, set up from the first isolate's snapshot and dropped with the last isolate. The Evaluator in dom-exception-test keeps an isolate alive, so the restored isolate reused the built-in snapshot's read-only heap, which debug V8 rejects by checksum. Mark the raw v8::TryCatch in the pending-exception test as intended.
…mexception Conflicts are additions on both sides, except for one adaptation to main: - main sizes a context's embedder-data array at build time (v8_embedder_data_array_size), and asserts that every ContextDataSlot fits. DOM_EXCEPTION_PROTOTYPE becomes the highest slot, index 10, so the array grows from 10 to 11.
The TypeScript DOMException is a class rather than a function. A class's `prototype` is non-writable and non-configurable, which Web IDL specifies, while the C++ class's is writable unless immutable_api_prototypes is set. As James suggested in review, nothing should depend on writing it, so the TypeScript class no longer copies that attribute. Every other property keeps the C++ class's attributes and order. Calling it without `new` throws V8's TypeError for a class constructor, with a different message. The per-isolate bootstrap installs it only under the new TS_DOM_EXCEPTION autogate. Without the gate the C++ class stays the global, as it does in isolates without the bootstrap, and native code creates instances of it. Either way, DOMExceptions are created and recognized by their brand. dom-exception-test runs against the C++ class, and dom-exception-ts-test, which enables the gate, runs the same module against the TypeScript class. The C++ cell has no @all-autogates variant, since that would enable the gate.
…ot brand HasCustomHostObject() returned true unconditionally since DOMExceptions were branded with private properties, so that IsHostObject() could find them. Now that they have internal fields, V8's default check already treats them as host objects, so both serializers return to the conditions on main, sparing a delegate call per object. restoreDOMExceptionBrand() runs inside V8's context deserializer, so a corrupt snapshot aborts rather than throwing through it, as deserializeInternalField() already does. docs/jsg.md says to create a DOMException with js.domException() and not to take one as a Ref<DOMException>.
JSG calls a C++ method that returns a promise through LiftKj_<v8::Local<v8::Promise>>: under capture_async_api_throws (on by default since 2022-10-31) what the method throws rejects the promise, and otherwise it is thrown. The Rust read() rejected in both cases. It now throws unless the flag is on, and under the flag catches what it throws, a JavaScript exception included, and rejects with it, as LiftKj_ does. A key that is too large still rejects either way, since memory-cache.c++ rejects it explicitly. memory-cache-throws-test.js checks, for a key that is not a string, a fallback that is not a function and a read in the global scope, that read() throws or rejects as the flag says. It also checks that a key that is too large rejects and that delete() throws whatever the flag. memory-cache-test.wd-test runs it in two services, one with the flag as the compatibility date sets it and one that enables it, so the @ and @all-autogates variants cover both settings against both implementations. Without this change, the Rust implementation fails it with the flag off. Also restores the TODO(#7951) in memory-cache-test.c++ to its original form.
Rust JSG offered DOM error types such as AbortError and NotSupportedError but threw them all as a plain Error. It also could not express DOMException(SyntaxError), because SyntaxError meant the JS class. Rename the DOM variants to mirror the C++ JSG_REQUIRE error types (DOMSyntaxError, DOMAbortError, ...), throw real DOMExceptions for them, and recognize DOMExceptions in tunneled KJ errors and in FromJS for jsg::Error.
Rust JSG's shims may not use C++ JSG, so native code now creates and recognizes DOMExceptions with V8 alone. This also lets the global DOMException move to the per-isolate TypeScript bootstrap, under the new TS_DOM_EXCEPTION autogate.
Why land this before any Rust API needs it:
new_abort_error,jsg_require!(..., NotSupportedError, ...)) that silently throw a plainError. That is a trap for every API ported to Rust JSG.jsg.DOMException(SyntaxError)from C++ decodes in Rust to the JSSyntaxError, the wrong type.AbortSignal. The Rust port ofMemoryCache.read()needsDOMDataCloneErrorfor parity.Design
tryGetDOMExceptionFields()recognizes a DOMException by its brand. It reads only internal fields and takes no context, so it also works while an exception or a termination is pending. An instance of the C++ class is recognized as the C++ object it wraps.newDOMException()creates one over the prototype in a newContextDataSlot::DOM_EXCEPTION_PROTOTYPE, without running JavaScript, so it works underDisallowJavascriptExecution(for example, in the deserializer).ExceptionWrapper,reportError, the webfs bootstrap and Rust JSG all use it. Having internal fields, a DOMException is a host object through V8's default check. Rust JSG's shims no longer usejsg::Lock/jsg::Ref, so the allowlist matches main.domException()return type.jsg::Lock::domException()returns aJsObject. This drops the 16TypeHandler<Ref<DOMException>>parameters from the filesystem API.setSnapshotDefaultContext()passes a serializer that writes the brand. Everyv8::Context::New()passesnewContextInternalFieldsDeserializer(), which restores it; V8 requires a deserializer for any context deserialized from such a snapshot.DOM_EXCEPTION_PROTOTYPEis context embedder-data slot 10, sov8_embedder_data_array_sizegrows from 10 to 11.src/per_isolate/dom-exception.tsreplaces the C++ class as the global, and hands native code its prototype, so the DOMExceptions native code creates are instances of it.utils.createDOMException(), with the prototype thatnew.targetgavethis, so subclasses andReflect.construct()work.set_tostring_tag,spec_compliant_property_attributes) are copied from the C++ class rather than recomputed.prototype, which is read-only as for any class.Behaviour
Rust JSG now matches C++ JSG:
Lock::throw_exceptionthrows nothing while execution is terminating. If creating a DOMException fails with an exception pending, that exception propagates instead of an internal error.Error::new_dom_exception(name, message)accepts any DOMException name, asjsg::Lock::domException()does. A name without a DOM-prefixed type, such as the legacyIndexSizeError, keeps its name (Error::dom_exception_name()) throughFromJS, tunneled errors andDisplay.jsg-internal.DOMException(<Name>)is thrown as a DOMException with that name and the redactedinternal error; reference = <id>message, logged asmakeInternalError()logs it.Compatibility:
DOMException.prototypeis read-only for every Worker, as Web IDL specifies. A JSG class's is read-only only underimmutable_api_prototypes, which has no enable date. Nothing is expected to depend on writing it.Function.prototype.toString()ofDOMExceptionshows its source instead of[native code].DOMException()withoutnewthrows V8's class-constructor TypeError, whose message differs.src/workerd/api/tests/dom-exception-testpins the shape and runs against both classes.kj::str()of one givesName: messageinstead of an empty string.MemoryCache.read()relied on the empty string to report an unserializable value as "failed to serialize ", so it now checks for a DOMException explicitly. Itsfallback_errorspan tag for that case becomesDataCloneError: failed to serialize <type>instead of"".Object.create(new DOMException())); nothing depends on that.Error, or a JSSyntaxErrorforDOMException(SyntaxError). No shipped API reaches this:UrlUtilandDnsUtilonly receive values their TypeScript wrappers have already converted.Resulthas no fast callback, so a fast callback only throwsTypeErrors from its receiver and argument checks (now checked with adebug_assert!).Not covered
worker-factory.c++). Under TS_DOM_EXCEPTION, the bootstrap that runs again in a restored context will need to reuse the snapshotted class rather than install a second one over it.