Bug 223302 - [JSC] Implement Error#cause
Summary: [JSC] Implement Error#cause
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks: 223407
  Show dependency treegraph
 
Reported: 2021-03-16 20:15 PDT by Ross Kirsling
Modified: 2021-05-07 12:23 PDT (History)
9 users (show)

See Also:


Attachments
Patch (34.88 KB, patch)
2021-03-16 20:29 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (35.02 KB, patch)
2021-03-16 20:58 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (34.88 KB, patch)
2021-03-16 22:17 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2021-03-16 20:15:34 PDT
[JSC] Implement Error#cause
Comment 1 Ross Kirsling 2021-03-16 20:29:22 PDT
Created attachment 423427 [details]
Patch
Comment 2 Ross Kirsling 2021-03-16 20:33:36 PDT
Comment on attachment 423427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423427&action=review

> Source/JavaScriptCore/API/JSObjectRef.cpp:236
>      JSValue message = argumentCount ? toJS(globalObject, arguments[0]) : jsUndefined();
> +    JSValue options = argumentCount > 1 ? toJS(globalObject, arguments[1]) : jsUndefined();
>      Structure* errorStructure = globalObject->errorStructure();
> -    JSObject* result = ErrorInstance::create(globalObject, errorStructure, message);
> +    JSObject* result = ErrorInstance::create(globalObject, errorStructure, message, options);

Note: We don't need to add support to JSObjectMakeError, but it's literally no extra effort to do so.
Comment 3 Yusuke Suzuki 2021-03-16 20:39:45 PDT
Comment on attachment 423427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423427&action=review

r=me

> JSTests/stress/error-cause.js:6
> +const errorConstructors = [Error, EvalError, RangeError, ReferenceError, SyntaxError, TypeError, URIError, AggregateError];

Can we add WebAssembly Errors if `typeof WebAssembly !== "undefined"`?
Comment 4 Ross Kirsling 2021-03-16 20:58:58 PDT
Created attachment 423430 [details]
Patch
Comment 5 Ross Kirsling 2021-03-16 22:05:37 PDT
(In reply to Yusuke Suzuki from comment #3)
> > JSTests/stress/error-cause.js:6
> > +const errorConstructors = [Error, EvalError, RangeError, ReferenceError, SyntaxError, TypeError, URIError, AggregateError];
> 
> Can we add WebAssembly Errors if `typeof WebAssembly !== "undefined"`?

Oops, we actually don't want to test these; their behavior hasn't changed (since they're not part of ECMA-262), it's just that they depend on ErrorInstance, whose interface changed.
Comment 6 EWS 2021-03-16 22:07:24 PDT Comment hidden (obsolete)
Comment 7 Ross Kirsling 2021-03-16 22:17:21 PDT
Created attachment 423435 [details]
Patch for landing
Comment 8 EWS 2021-03-16 23:02:47 PDT
Committed r274552: <https://commits.webkit.org/r274552>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423435 [details].
Comment 9 Radar WebKit Bug Importer 2021-03-16 23:03:15 PDT
<rdar://problem/75512624>
Comment 10 Keith Miller 2021-03-17 12:31:01 PDT
Comment on attachment 423435 [details]
Patch for landing

This should probably be behind a JSC flag (it can be on by default though) since it's technically a feature, albeit a simple one.
Comment 11 Devin Rousso 2021-03-17 13:00:17 PDT
Out of curiosity what does this look like in Web Inspector?  Should it have special treatment there given that one of the motivations for this feature was for developer tooling/debugging/etc.?
Comment 12 Ross Kirsling 2021-03-17 16:37:51 PDT
(In reply to Keith Miller from comment #10)
> Comment on attachment 423435 [details]
> Patch for landing
> 
> This should probably be behind a JSC flag (it can be on by default though)
> since it's technically a feature, albeit a simple one.

Alright, I can add that to the WASM-related follow-up patch.

(In reply to Devin Rousso from comment #11)
> Out of curiosity what does this look like in Web Inspector?  Should it have
> special treatment there given that one of the motivations for this feature
> was for developer tooling/debugging/etc.?

It should look the same as before for now. It would make sense to have a helpful display for this information, though of course it's possible to use this information for non-debugging purposes too -- to give better error messages, to do different things based on the first error in the cascade, etc.