web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
4.98k stars 3.09k forks source link

`reportError(undefined)` and `ErrorEvent.prototype.error` #33206

Open nayeemrmn opened 2 years ago

nayeemrmn commented 2 years ago

In the test for reportError(undefined): https://github.com/web-platform-tests/wpt/blob/93068ae96dda375b249ec97434033077fba7c5a7/html/webappapis/scripting/reporterror.any.js#L3-L21 I'm not sure the assertion which checks that e.error === undefined is correct. https://github.com/web-platform-tests/wpt/blob/93068ae96dda375b249ec97434033077fba7c5a7/html/webappapis/scripting/reporterror.any.js#L15

If you dispatch an error event with { error: undefined }, the ErrorEvent::error is changed to null. This is the case in Chrome, Firefox and Deno. I assume it's compliant.

addEventListener("error", e => {
  console.log(e.error);
});
dispatchEvent(new ErrorEvent("error", { error: undefined }));
// Outputs `null`.

I think the error event dispatched as an effect of reportError(undefined) should match that and set ErrorEvent::error to null. Firefox seems to make an exception to comply with the test, but it does in Chrome already:

addEventListener("error", e => {
  console.log(e.error);
});
reportError(undefined);
// Should output `null`.

Should we change this?

cc @annevk

annevk commented 2 years ago

So currently the error dictionary member defaults to null, which is why if you construct an ErrorEvent through JS/IDL you cannot ever get its error member to return undefined. The HTML Standard doesn't construct its ErrorEvent that way so Firefox is compliant, but it's a reasonable question whether anything ought to change here.

Options:

  1. The most logical change to me would be that we no longer default the error dictionary member to null, but that is also a riskier change.
  2. Mapping undefined to null as Chrome reportedly does is a change we could make.
  3. We could keep the status quo whereby reportError() is the only way to get ErrorEvent's error member to return undefined in userland.

cc @domenic @mfreed7 @evilpie

domenic commented 2 years ago

I am happy with either (1) or (3). I don't like (2) because I think code such as catch (e) { reportError(e) } should report e exactly.

Another scenario worth testing is the simple

window.addEventListener("error", ev => {
  console.assert(ev.error, undefined);
});

throw undefined;

which should pass per the HTML spec (and IMO should not be changed).

evilpie commented 2 years ago

Option 3 sounds good to me, but I can't really judge the tradeoffs with option 1. throw undefined and reportError(undefined) should behave identical.