Bugzilla – Bug 17713
Exceptions thrown from event handlers should not be propagated
Last modified: 2014-11-26 11:00:41 UTC
See <http://software.hixie.ch/utilities/js/live-dom-viewer/saved/1650>; this should at least be clarified.
In 4.7. of http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-event-listener-invoke we could write to not propagate them. But is it then still clear window.onerror does get them? Is there some way we can make it clear they run in their own microtask?
For posterity: <http://software.hixie.ch/utilities/js/live-dom-viewer/saved/1653> shows that Gecko doesn't trigger window.onerror; Chrome, Opera-next and IE10 do.
I think this is an IDL/HTML problem because this applies to all callbacks. heycam, Hixie?
I guess it's not safe to make them propagate? It seems like the more sensible behaviour. I am not sure that shunting off the exception to window.onerror is what you would always want -- I feel like it should be specific to whatever function that takes the callback.
It does not make sense for exceptions in asynchronous callbacks to propagate. And I do not think it makes sense for synchronous callbacks either. And also, having all callbacks implemented in the same way is probably a good thing.
I am reassigning this for now. heycam, let me know if you disagree.
The events algorithms should just use the "jump to a code entry point" algorithm in the HTML spec to trigger the setting of "entry script", and then wrap a prose "try-catch" block around that to catch the exceptions, and then if there are any invoke the "report the error" algorithm in HTML to report it to onerror.
But it is not just events, all callbacks are affected. Seems annoying to have to write that out everywhere (and Ms2ger would still like for DOM to not depend on HTML).
Surely most callbacks aren't affected; in most cases if a callback raises an exception you just want to keep propagating it, no? Maybe I don't follow what you mean exactly? I agree that other callbacks need to define some of this too, but I don't see why that changes the resolution here.
Whether DOM (or WebIDL) uses a hook that HTML then defines (as for mutation observers in DOM Core or global environment scopes in WebIDL) or calls into HTML directly (as XHR does) is a non-issue to me, this can be defined either way. (But I don't really understand why we're avoiding dependencies so much. The truth is this is just one platform.)
This really depends on what we mean by "propagated".
Should calling a callback not report an exception to error consoles and window.onerror if the caller is itself called from some code that has a try/catch? I would argue that it _should_ in fact do so regardless of what's going on with try/catch.... The alternative is that people start getting very confused as to why their exceptions are not being reported. We've run into this with Gecko a good bit, since we do have that exception-swallowing behavior in a bunch of cases.
Our (Gecko's) current implementation of WebIDL callbacks reports exceptions on return from the callback, and I have no plans to change that, honestly.
Developer tools are not web-exposed so not our territory (apart from window.console, which might be). I'm fine with encouraging reporting exceptions there.
Whether window.onerror is called is quite noticeable from the page though.
My bad, per the testcase from comment 0 Opera/Chrome/Safari/ invoke window.onerror, Gecko does not, dunno about Internet Explorer.
The fact that Gecko does not invoke onerror in that testcase is a longstanding Gecko but, not a design feature. As I said, we've had a large number of bug reports about it.
I still disagree that you always want to swallow the exception from the callback. I would say, in general, that if the function you're passing the callback to is going to synchronously call your callback, then the exception should propagate out. Like it does with Array.prototype.sort. But if the callback is going to be invoked asynchronously, then obviously there's no top-level JS stack frame to propagate it out to.
If I were designing dispatchEvent from scratch, I would make it propagate the exception. But since we're stuck with eating it, I think that should be done in the prose defining dispatchEvent.
Re-assigning back to Anne.
> I still disagree that you always want to swallow the exception from the
No one is talking about swallowing the exception.
The basic options for callback exceptions are catch-and-report and catch-and-rethrow. The latter is somewhat equivalent to "just abort and pretend that you got an exception that interrupts whatever you're doing" maybe.
For APIs that call the callback directly off a task, of course, catch-and-report is the only reasonable option. In my opinion. Note that for these APIs it's often not equivalent to catch-and-rethrow, even if we define that as reporting if there's no JS on stack (but of course interrupting processing steps).
For what it's worth, the desired behavior of current standardized APIs that we (Mozilla) have converted to WebIDL that use callbacks seems to be:
1) Promise: catch-and-do-something-complicated.
2) Window.setTimeout: catch-and-report (off a task, not equivalent to
3) EventHandler: catch-and-report.
4) EventListener: catch-and-report.
5) NodeFilter: catch-and-rethrow.
6) requestAnimationFrame: catch-and-report (off a task, not equivalent to
7) WebComponents: catch-and-report (haven't checked equivalence).
8) WebRTC success/error callbacks: catch-and-report (off a task, may be
equivalent to catch-and-rethrow).
9) Notificaion.requestPermission: catch-and-report (off a task, equivalent to
10) System message callbacks: Can't tell, because I can't find where anyone
invokes "fier a system message".
11) Mutation observers: catch-and-report.
12) Canvas.toBlob: catch-and-report (off a task, equivalent to
13) Geolocation: catch-and-report (off a task, equivalent to catch-and-rethrow).
14) UndoManager: Processing model not defined enough for me to be able to tell.
15) AudioContext.decodeAudioData: catch-and-report (off a task, equivalent to
I believe that the specs for all of these say nothing about exceptions except NodeFilter, which specifies the catch-and-rethrow behavior, and Promise which specifies the complicated behavior it has.
Fundamentally, having the default behavior be catch-and-report means that spec authors don't have to worry about calling a callback throwing an exception that interrupts their processing model (as currently they seem not to). Having the default behavior be catch-and-rethrow means that any callback invocation needs to explicitly consider the consequences of an exception... There are pros and cons to both. :(
My perception is that the thing people were mostly upset about with .dispatchEvent is that it does "magic". I.e. it does things that can't be replicated in JS.
An easy way to fix that would be to add something like
This would do exactly the same reporting as we currently do for errors that are thrown by event listeners (which I *think* is the same error reporting as we do for unhandled errors?).
Or we could add something like
which would both fire window.onerror as well as do the appropriate error reporting if that error went unhandled.
> (which I *think* is the same error reporting as we do for unhandled errors?)
Yes, it is.
I like the reportError idea; that would in fact allow the "catch-and-report" behavior to be implemented.
I guess I agree with comment 15 these days which means DOM should do what comment 7 suggests. Do we know any other places that behave similar to event listeners?
Anne, see comment 16?
I think the other thing we need to make clearer is when microtasks are run in relation to these callbacks. I guess that ends up requiring pretty deep integration into HTML.
What are you looking for from IDL here?
See the dependencies.
I have now addressed comment 7 partially to at least make it clear the exception is to be reported to window.onerror:
Once IDL adds a hook for invoking callbacks as requested in bug 25138 I would like to add that.
Not sure if something around "jump to a code entry point" is still needed at that point. If it is, please advice.
To address comment 16 partially:
* 1) handled by ES
* 2) bug 27442
* 3) bug 27443
* 4) see comment 24
* 5) handled by DOM
* 6) handled by HTML
* 7) bug 27440
* 9) https://github.com/whatwg/notifications/commit/87f285ebd4bcc5fa423f1480a6bcbd16c70d6d98
* 11) https://github.com/whatwg/dom/commit/2e567d3308724b29c9787bd069332775a3dbd8cc
* 12) bug 27441
This leaves 4) WebRTC, 10) ?, and 13) Geolocation, 14) UndoManager, and 15) Audio.