Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce self.reportError() #1196

Merged
merged 1 commit into from
Jul 27, 2021
Merged

Introduce self.reportError() #1196

merged 1 commit into from
Jul 27, 2021

Conversation

annevk
Copy link
Member

@annevk annevk commented May 5, 2016

@domenic domenic added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest labels May 5, 2016
@domenic
Copy link
Member

domenic commented May 5, 2016

So based on following the links it looks like @bzbarsky is in favor of implementing this in Gecko. Any other implementers interested?

@terinjokes
Copy link

Will the be hooks for scripts to hook into this (ie, for New Relic or Sentry error reporting?), or will they still have to trampoline through window.onerror?

@domenic
Copy link
Member

domenic commented May 6, 2016

What do you mean by "trampoline through window.onerror?" What is the use case that window.addEventListener("error", ...) currently doesn't solve?

@terinjokes
Copy link

I didn't know that window.addEventListener("error", …) was a thing, tbqh.

source Outdated
<div w-nodev>

<p>The <dfn data-x="dom-reportException"><code>reportException(<var>exception</var>)</code></dfn>
method, when invoked, must <span>report an exception</span> with <var>exception</var>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Should this do nothing if it's called inside window.onerror (like runtime script errors in onerror are no-ops)? (Can still report something to devtools though.)

Copy link
Member Author

Choose a reason for hiding this comment

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

It does nothing, right? Target would be in error reporting mode.

Copy link
Member

Choose a reason for hiding this comment

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

Right, sorry. I misremembered the logic.

@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Jun 24, 2016
@annevk
Copy link
Member Author

annevk commented Jul 7, 2016

To be clear, the reason we want this is for JavaScript to be able to emulate the behavior of dispatchEvent(), the behavior of custom element reactions, and maybe a few other features I'm forgetting.

@annevk
Copy link
Member Author

annevk commented Oct 18, 2016

It seems this is still the right approach even after "report an exception" is fixed in #958. We don't handle muting here, but I don't think we need to.

@rniwa
Copy link

rniwa commented Oct 29, 2016

Here's one feedback: have we considered adding this on console instead?

@domenic
Copy link
Member

domenic commented Oct 29, 2016

@rniwa we did; that was actually the original suggestion. However, it fires an Error event at self, so it seems better to put it there.

@annevk
Copy link
Member Author

annevk commented Feb 20, 2017

Surely Blink's "explain the platform" folks are interested in this?

@annevk
Copy link
Member Author

annevk commented Jan 11, 2019

@rniwa any more thoughts? Shall we do this? @mathiasbynens perhaps you can figure out Chrome support for this?

@annevk
Copy link
Member Author

annevk commented Oct 14, 2019

@rniwa @mathiasbynens ping.

@foolip
Copy link
Member

foolip commented Oct 14, 2019

@clelland can you take a look at this for Chrome? I suspect there is a connection to https://w3c.github.io/reporting/ in that things which are reported through self.reportException() should feed into the Reporting API as well. Right?

Base automatically changed from master to main January 15, 2021 07:56
@annevk annevk added the agenda+ To be discussed at a triage meeting label Apr 28, 2021
@domenic
Copy link
Member

domenic commented Jun 3, 2021

I found https://github.com/ReactiveX/rxjs/blob/master/src/internal/util/reportUnhandledError.ts showing how at least one popular library is doing this today: indeed, it's with setTimeout(() => { throw e; }, 0) hacks. @benlesh, would you be interested in a native self.reportException(e) which would allow you to do this without setTimeout hacks, in the same way that the browser does internally for implementing EventTarget?

https://github.com/kriskowal/asap seems to do something similar, except hidden behind a few layers of abstraction. Any thoughts, @kriskowal?

@gaearon
Copy link

gaearon commented Jun 3, 2021

Not sure if this is the same thing as you're asking, but we use this trick in React:

https://github.com/facebook/react/blob/1b7b3592f4713b3a53709554aefd1d20e9a0a6a5/packages/shared/invokeGuardedCallbackImpl.js#L33-L237

Essentially, when we catch a user error in our try / catch (which we need to keep), we want to rethrow it, but we want the browser to present it as unhandled ("pause on uncaught" should break on it, the error message should say "Uncaught", and so on.)

Our trick is to call user code inside the context of a fake DOM event dispatch, and listen to the error event to see if it errored. This way it's treated by the browser as a top-level error even though technically there's a try / catch higher up in the stack.

@kriskowal
Copy link

ASAP would certainly have benefited from a mechanism to surface an error as if it had been thrown but without throwing. Much of the complexity in ASAP flows from preserving execution order and amortizing timers. Any time it needs to report an error, ASAP stops flushing its event queue, schedules a flush, and throws the error. With, reportError or some such, that would just be a for loop.

@benlesh
Copy link

benlesh commented Jun 4, 2021

would you be interested in a native self.reportException(e) which would allow you to do this without setTimeout hacks, in the same way that the browser does internally for implementing EventTarget?

@domenic Absolutely! Whenever we implemented this, I was honestly curious why something like that didn't already exist.

Although, we would probably still end up using setTimeout and throwing, because RxJS has to run in node.js as well. Unless this same mechanism existed in node.js, I'm not sure we would use it unless there were tangible benefits to doing so. Maybe @MylesBorins or @benjamingr have thoughts about an API like this existing in node.js? (If so I guess a discussion should start there, as it's off topic here)

@domenic domenic force-pushed the reportexception branch 2 times, most recently from 755c57a to 0dae6cc Compare June 4, 2021 13:06
@benjamingr
Copy link
Member

Opened a discussion in Node.js :) nodejs/node#38947

@annevk
Copy link
Member Author

annevk commented Jun 17, 2021

This seems like something we should also expose in worklets, right? cc @padenot

@domenic
Copy link
Member

domenic commented Jun 17, 2021

Since nobody has had further strong opinions on the name I'm going to change it to reportError().

@domenic domenic changed the title Introducing self.reportException() Introduce self.reportError() Jun 17, 2021
@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Jun 17, 2021
@padenot
Copy link

padenot commented Jun 21, 2021

This seems like something we should also expose in worklets, right? cc @padenot

Each worklet implement their own error reporting, if any, and for the AudioWorklet, the error reporting is outside the scope, so it's fairly different.

Copy link
Member Author

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I cannot approve my own PR, but I still think this is okay. I'll update OP with the PR template as I don't think we have tests for this yet.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Jul 22, 2021
@annevk
Copy link
Member Author

annevk commented Jul 22, 2021

While writing tests I briefly wondered if we wanted to expose whether the exception was handled in some way, but I could not think of a reason why a library would want control over that. Especially as the use case is emulating what happens if an exception is thrown in certain contexts. (If an exception is not handled, the browser will report it in the console.)

@domenic
Copy link
Member

domenic commented Jul 22, 2021

The tests made me realize we probably want optional any instead of any because try { x(); } catch (e) { reportError(e); } could end up with e === undefined.

@domenic
Copy link
Member

domenic commented Jul 22, 2021

Oh, hmm, re-reading https://heycam.github.io/webidl/#es-any it looks like undefined works fine for an any value, so self.reportError(undefined) works fine.

I guess the difference between optional any and any is that self.reportError() would only work with optional any? Hmmmm.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Jul 27, 2021
@annevk annevk merged commit 97a1e58 into main Jul 27, 2021
@annevk annevk deleted the reportexception branch July 27, 2021 06:15
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 30, 2021
Automatic update from web-platform-tests
HTML: self.reportError()

For whatwg/html#1196.
--

wpt-commits: c3cb1109a30e82ceccdf4202d26f6c6648bbef70
wpt-pr: 29738
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 31, 2021
Automatic update from web-platform-tests
HTML: self.reportError()

For whatwg/html#1196.
--

wpt-commits: c3cb1109a30e82ceccdf4202d26f6c6648bbef70
wpt-pr: 29738
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Aug 4, 2021
Automatic update from web-platform-tests
HTML: self.reportError()

For whatwg/html#1196.
--

wpt-commits: c3cb1109a30e82ceccdf4202d26f6c6648bbef70
wpt-pr: 29738
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Aug 4, 2021
Automatic update from web-platform-tests
HTML: self.reportError()

For whatwg/html#1196.
--

wpt-commits: c3cb1109a30e82ceccdf4202d26f6c6648bbef70
wpt-pr: 29738
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this pull request Aug 30, 2021
https://bugs.webkit.org/show_bug.cgi?id=228316
<rdar://problem/81446162>

Reviewed by Sam Weinig.

LayoutTests/imported/w3c:

Import test coverage from:
- web-platform-tests/wpt#29738

* web-platform-tests/html/webappapis/scripting/reporterror.any-expected.txt: Added.
* web-platform-tests/html/webappapis/scripting/reporterror.any.html: Added.
* web-platform-tests/html/webappapis/scripting/reporterror.any.js: Added.
(undefined.forEach.throwable.test.t.assert_equals):
(test):
* web-platform-tests/html/webappapis/scripting/reporterror.any.worker-expected.txt: Added.
* web-platform-tests/html/webappapis/scripting/reporterror.any.worker.html: Added.
* web-platform-tests/html/webappapis/scripting/w3c-import.log: Added.

Source/WebCore:

Implement self.reportError() as per:
- whatwg/html#1196

Firefox already shipped this and Chrome will do so soon too.

Tests: imported/w3c/web-platform-tests/html/webappapis/scripting/reporterror.any.html
       imported/w3c/web-platform-tests/html/webappapis/scripting/reporterror.any.worker.html

* page/DOMWindow.cpp:
(WebCore::DOMWindow::reportError):
* page/DOMWindow.h:
* page/WindowOrWorkerGlobalScope.idl:
* workers/WorkerGlobalScope.cpp:
(WebCore::WorkerGlobalScope::reportError):
* workers/WorkerGlobalScope.h:


Canonical link: https://commits.webkit.org/241098@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@281756 268f45cc-cd09-0410-ab3c-d52691b4dbfc
bertogg pushed a commit to Igalia/webkit that referenced this pull request Sep 6, 2021
https://bugs.webkit.org/show_bug.cgi?id=228316
<rdar://problem/81446162>

Reviewed by Sam Weinig.

LayoutTests/imported/w3c:

Import test coverage from:
- web-platform-tests/wpt#29738

* web-platform-tests/html/webappapis/scripting/reporterror.any-expected.txt: Added.
* web-platform-tests/html/webappapis/scripting/reporterror.any.html: Added.
* web-platform-tests/html/webappapis/scripting/reporterror.any.js: Added.
(undefined.forEach.throwable.test.t.assert_equals):
(test):
* web-platform-tests/html/webappapis/scripting/reporterror.any.worker-expected.txt: Added.
* web-platform-tests/html/webappapis/scripting/reporterror.any.worker.html: Added.
* web-platform-tests/html/webappapis/scripting/w3c-import.log: Added.

Source/WebCore:

Implement self.reportError() as per:
- whatwg/html#1196

Firefox already shipped this and Chrome will do so soon too.

Tests: imported/w3c/web-platform-tests/html/webappapis/scripting/reporterror.any.html
       imported/w3c/web-platform-tests/html/webappapis/scripting/reporterror.any.worker.html

* page/DOMWindow.cpp:
(WebCore::DOMWindow::reportError):
* page/DOMWindow.h:
* page/WindowOrWorkerGlobalScope.idl:
* workers/WorkerGlobalScope.cpp:
(WebCore::WorkerGlobalScope::reportError):
* workers/WorkerGlobalScope.h:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@281756 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements agenda+ To be discussed at a triage meeting needs implementer interest Moving the issue forward requires implementers to express interest topic: error reporting topic: script
Development

Successfully merging this pull request may close these issues.

Add a console.* API that invokes the HTML "report the error" algorithm (which calls window.onerror)