Bug 17713 - Exceptions thrown from event handlers should not be propagated
Exceptions thrown from event handlers should not be propagated
Status: NEW
Product: WebAppsWG
Classification: Unclassified
Component: DOM
unspecified
All Windows 3.1
: P2 normal
: ---
Assigned To: Anne
public-webapps-bugzilla
blocked on IDL
:
Depends on: 24403 25138 27046
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-07 15:49 UTC by Ms2ger
Modified: 2014-11-26 11:00 UTC (History)
10 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ms2ger 2012-07-07 15:49:03 UTC
See <http://software.hixie.ch/utilities/js/live-dom-viewer/saved/1650>; this should at least be clarified.
Comment 1 Anne 2012-07-08 10:42:27 UTC
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?
Comment 2 Ms2ger 2012-07-08 15:16:56 UTC
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.
Comment 3 Anne 2012-07-09 19:20:34 UTC
I think this is an IDL/HTML problem because this applies to all callbacks. heycam, Hixie?
Comment 4 Cameron McCormack 2012-07-10 07:47:18 UTC
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.
Comment 5 Anne 2012-07-10 08:19:48 UTC
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.
Comment 6 Anne 2012-09-11 14:57:26 UTC
I am reassigning this for now. heycam, let me know if you disagree.
Comment 7 Ian 'Hixie' Hickson 2012-10-27 16:53:35 UTC
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.
Comment 8 Anne 2012-10-27 16:56:57 UTC
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).
Comment 9 Ian 'Hixie' Hickson 2012-10-29 22:09:30 UTC
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.)
Comment 10 Boris Zbarsky 2012-11-27 07:30:19 UTC
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.
Comment 11 Anne 2012-11-27 08:30:12 UTC
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.
Comment 12 Jonas Sicking 2012-11-27 09:57:25 UTC
Whether window.onerror is called is quite noticeable from the page though.
Comment 13 Anne 2012-11-27 10:08:33 UTC
My bad, per the testcase from comment 0 Opera/Chrome/Safari/ invoke window.onerror, Gecko does not, dunno about Internet Explorer.
Comment 14 Boris Zbarsky 2012-11-27 19:24:21 UTC
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.
Comment 15 Cameron McCormack 2013-08-03 06:21:41 UTC
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.
Comment 16 Boris Zbarsky 2013-08-04 02:00:36 UTC
> I still disagree that you always want to swallow the exception from the
> callback. 

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
   catch-and-rethrow).
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
   catch-and-rethrow).
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
   catch-and-rethrow).
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
    catch-and-rethrow).
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
    catch-and-rethrow).

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.  :(
Comment 17 Jonas Sicking 2013-08-04 03:51:23 UTC
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

console.reportError(exception);

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

window.reportError(exception);

which would both fire window.onerror as well as do the appropriate error reporting if that error went unhandled.
Comment 18 Boris Zbarsky 2013-08-04 04:01:46 UTC
> (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.
Comment 19 Anne 2013-08-08 13:46:51 UTC
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?
Comment 20 Boris Zbarsky 2013-08-09 19:34:08 UTC
Anne, see comment 16?
Comment 21 Anne 2013-09-09 15:55:52 UTC
Oops, right.

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.
Comment 22 Boris Zbarsky 2014-05-22 15:56:17 UTC
What are you looking for from IDL here?
Comment 23 Anne 2014-05-22 16:07:26 UTC
See the dependencies.
Comment 24 Anne 2014-11-26 10:25:20 UTC
I have now addressed comment 7 partially to at least make it clear the exception is to be reported to window.onerror:

https://github.com/whatwg/dom/commit/55f7dbf200bfb63a26f66a3875dc6f9d402c7728

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.
Comment 25 Anne 2014-11-26 11:00:41 UTC
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.