Skip to content

Conversation

bptato
Copy link
Contributor

@bptato bptato commented Apr 30, 2025

Implemented separately from the other errors because it is defined in terms of WebIDL, where members of an interface are getters on their prototype.

See the difference between
JSON.stringify(Object.getOwnPropertyDescriptors(new TypeError())) vs JSON.stringify(Object.getOwnPropertyDescriptors(new DOMException())).

(Required for btoa/atob and structuredClone; ref. #16, #1032)

function bjson_test_fuzz()
{
var corpus = [
"FBAAAAAABGA=",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this was complaining because of the bytecode version, but I don't know how the corpus was generated so I just incremented it manually...

@bptato bptato marked this pull request as draft April 30, 2025 16:49
@bptato
Copy link
Contributor Author

bptato commented Apr 30, 2025

It's still missing a stack trace when created with the constructor...

@bptato
Copy link
Contributor Author

bptato commented Apr 30, 2025

I was confused for a minute because browsers set stack as a getter on error prototypes, but we set it as a property on individual Error objects. I ended up following our existing practice.
(stack itself is non-standard; the spec only says that if regular Errors have it, DOMException should also have it.
I don't know if the difference matters anywhere in practice.)

@bptato bptato marked this pull request as ready for review April 30, 2025 17:51
@saghul
Copy link
Contributor

saghul commented Apr 30, 2025

Perhaps you could use Js_NewError and extract it?

@saghul
Copy link
Contributor

saghul commented Apr 30, 2025

I was confused for a minute because browsers set stack as a getter on error prototypes, but we set it as a property on individual Error objects. I ended up following our existing practice.

(stack itself is non-standard; the spec only says that if regular Errors have it, DOMException should also have it.

I don't know if the difference matters anywhere in practice.)

I think that should be fine.

Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a small comment, good work!

Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@saghul
Copy link
Contributor

saghul commented Jun 5, 2025

@bnoordhuis Any further comments on this one?

Implemented separately from the other errors because it is defined in
terms of WebIDL, where members of an interface are getters on their
prototype.

See the difference between
`JSON.stringify(Object.getOwnPropertyDescriptors(new TypeError()))` vs
`JSON.stringify(Object.getOwnPropertyDescriptors(new DOMException()))`.

Note: the standard doesn't specify where to put "stack".  We follow
existing practice which imitates node instead of browsers.
@saghul
Copy link
Contributor

saghul commented Aug 28, 2025

@bnoordhuis Shall I land this? atob / btoa relies on it so maybe it's time for it :-)

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments but mostly LGTM.

quickjs.c Outdated
Comment on lines 57927 to 57928
JS_CGETSET_DEF("name", js_domexception_get_name, NULL ),
JS_CGETSET_DEF("message", js_domexception_get_message, NULL ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't wrong but if you want to DRY the code, you can do:

Suggested change
JS_CGETSET_DEF("name", js_domexception_get_name, NULL ),
JS_CGETSET_DEF("message", js_domexception_get_message, NULL ),
JS_CGETSET_MAGIC_DEF("name", js_domexception_getfield, NULL,
offsetof(JSDOMExceptionData, name)),
JS_CGETSET_MAGIC_DEF("message", js_domexception_getfield, NULL,
offsetof(JSDOMExceptionData, message)),

See js_callsite_getfield for an example:

static JSValue js_callsite_getfield(JSContext *ctx, JSValueConst this_val, int argc, JSValueConst *argv, int magic)
{
    JSCallSiteData *csd = JS_GetOpaque2(ctx, this_val, JS_CLASS_CALL_SITE);
    if (!csd)
        return JS_EXCEPTION;
    JSValue *field = (void *)((char *)csd + magic);
    return js_dup(*field);
}

This is another one of those things we could generalize more because ultimately it just computes:

JSValue *field = (void *)((char *)JS_VALUE_GET_OBJ(this_val)->u.opaque + magic);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, done. (At this scale though, it seems a bit closer to golfing than DRY :P)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not even code golfing, I'm afraid... I tried adding a generalized getter to the whole code base and it added about as many lines of code as it shaved off. Not worth it at the moment, I guess.

JS_EXTERN JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowReferenceError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...);
JS_EXTERN JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowSyntaxError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...);
JS_EXTERN JSValue JS_PRINTF_FORMAT_ATTR(2, 3) JS_ThrowTypeError(JSContext *ctx, JS_PRINTF_FORMAT const char *fmt, ...);
JS_EXTERN JSValue JS_PRINTF_FORMAT_ATTR(3, 4) JS_ThrowDOMException(JSContext *ctx, const char *name, JS_PRINTF_FORMAT const char *fmt, ...);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this function do if I don't call JS_AddIntrinsicDOMException first? Maybe it should have an "is initialized?" check?

(My guess is it throws a prototype-less object but I'm not 100% sure.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a user error. I've added an assertion.

@saghul saghul merged commit eb996c5 into quickjs-ng:master Sep 1, 2025
127 checks passed
@saghul
Copy link
Contributor

saghul commented Sep 1, 2025

Great work @bptato !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants