Bug 223302

Summary: [JSC] Implement Error#cause
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: New BugsAssignee: Ross Kirsling <ross.kirsling>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, hi, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=223404
https://bugs.webkit.org/show_bug.cgi?id=225535
Bug Depends on:    
Bug Blocks: 223407    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

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.