Implement onunhandledrejection / onrejectionhandled events

Implement onunhandledrejection / onrejectionhandled events

BUG=495801
R=yurys@chromium.org,domenic@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197255

[jochen (gone - plz use gerrit), 2015-06-15 08:46:10]

Yury, ptal

Domenic, should "preventDefault" on unhandledrejection also cause
rejectionhandled to not log to the console even if that event doesn't have an
handler?

Also, you promised me to translate the io.js test suite to a layout test :)

[yurys, 2015-06-15 09:10:13]

lgtm

[domenic, 2015-06-15 15:53:57]

On 2015/06/15 at 08:46:10, jochen wrote:
> Yury, ptal
> 
> Domenic, should "preventDefault" on unhandledrejection also cause
rejectionhandled to not log to the console even if that event doesn't have an
handler?

I'm not sure I quite understand this sentence? I think if you preventDefault on
unhandledrejection then nothing should show up in the console.

> Also, you promised me to translate the io.js test suite to a layout test :)

Yep, that's today's major work item.

[jochen (gone - plz use gerrit), 2015-06-15 15:57:54]

On 2015/06/15 at 15:53:57, domenic wrote:
> On 2015/06/15 at 08:46:10, jochen wrote:
> > Yury, ptal
> > 
> > Domenic, should "preventDefault" on unhandledrejection also cause
rejectionhandled to not log to the console even if that event doesn't have an
handler?
> 
> I'm not sure I quite understand this sentence? I think if you preventDefault
on unhandledrejection then nothing should show up in the console.
> 

Assume you only have a listener for unhandledrejection, and it invokes
preventDefault but then something handles the rejection.

Since there's no listener for rejectionhandled, should we log this to the
console?




> > Also, you promised me to translate the io.js test suite to a layout test :)
> 
> Yep, that's today's major work item.

[blink-reviews, 2015-06-15 16:01:39]

I do not think we should. The purpose of the logging is to alert developers
to unhandled rejections (especially developers who haven't put telemetry
stuff in place). There's no point logging a message to tell them "it was
handled, even though you didn't care enough to add a listener for that
event."

On Mon, Jun 15, 2015 at 11:58 AM <jochen@chromium.org> wrote:

> On 2015/06/15 at 15:53:57, domenic wrote:
> > On 2015/06/15 at 08:46:10, jochen wrote:
> > > Yury, ptal
> > >
> > > Domenic, should "preventDefault" on unhandledrejection also cause
> rejectionhandled to not log to the console even if that event doesn't have
> an
> handler?
>
> > I'm not sure I quite understand this sentence? I think if you
> > preventDefault
> on unhandledrejection then nothing should show up in the console.
>
>
> Assume you only have a listener for unhandledrejection, and it invokes
> preventDefault but then something handles the rejection.
>
> Since there's no listener for rejectionhandled, should we log this to the
> console?
>
>
>
>
> > > Also, you promised me to translate the io.js test suite to a layout
> > test :)
>
> > Yep, that's today's major work item.
>
>
>
> https://codereview.chromium.org/1179113007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[jochen (gone - plz use gerrit), 2015-06-16 12:16:34]

updated.

I also rewrote your tests a bit, and added some more.

any reason we don't want to expose this on ServiceWorker?

[jochen (gone - plz use gerrit), 2015-06-16 13:22:31]

jochen@chromium.org changed reviewers:
+ philipj@opera.com

[jochen (gone - plz use gerrit), 2015-06-16 13:22:32]

+philipj for webexposed stuff

[philipj_slow, 2015-06-16 14:58:04]

Took a quick peek, LGTM with some non-obviousness and IDL nitpickery.

https://codereview.chromium.org/1179113007/diff/120001/Source/bindings/core/v...
File Source/bindings/core/v8/RejectedPromises.cpp (right):

https://codereview.chromium.org/1179113007/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/RejectedPromises.cpp:67: m_shouldLogToConsole =
target->dispatchEvent(event);
Can you document this with "Log to console if event was not preventDefault()'ed"
or something? At least I always have to look up what the return value of
dispatchEvent() means.

https://codereview.chromium.org/1179113007/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/RejectedPromises.cpp:102: // Either collected or
https://crbug.com/450330
This issue is 403 for me, what does this comment mean?

https://codereview.chromium.org/1179113007/diff/120001/Source/core/events/Pro...
File Source/core/events/PromiseRejectionEvent.h (right):

https://codereview.chromium.org/1179113007/diff/120001/Source/core/events/Pro...
Source/core/events/PromiseRejectionEvent.h:29: initializer.setCancelable(true);
https://github.com/domenic/unhandled-rejections-browser-spec says "Let event be
a new trusted PromiseRejectionEvent object that does not bubble and is not
cancelable, and which has the event name unhandledrejection" but also "If event
was canceled, then the error is handled. Otherwise, the error is not handled."
which doesn't seem to add up.

https://codereview.chromium.org/1179113007/diff/120001/Source/core/frame/Wind...
File Source/core/frame/WindowEventHandlers.idl (right):

https://codereview.chromium.org/1179113007/diff/120001/Source/core/frame/Wind...
Source/core/frame/WindowEventHandlers.idl:49:
[RuntimeEnabled=PromiseRejectionEvent] attribute EventHandler
onrejectionhandled;
Here and in WorkerGlobalScope.idl, can you put the added things in a section at
the bottom called "Unhandled Rejection Tracking" or similar that links to
https://github.com/domenic/unhandled-rejections-browser-spec? (To make it easier
to track which bits come from which spec.)

[domenic, 2015-06-16 15:31:47]

On 2015/06/16 at 14:58:04, philipj wrote:

>
https://codereview.chromium.org/1179113007/diff/120001/Source/core/events/Pro...
> Source/core/events/PromiseRejectionEvent.h:29:
initializer.setCancelable(true);
> https://github.com/domenic/unhandled-rejections-browser-spec says "Let event
be a new trusted PromiseRejectionEvent object that does not bubble and is not
cancelable, and which has the event name unhandledrejection" but also "If event
was canceled, then the error is handled. Otherwise, the error is not handled."
which doesn't seem to add up.

Thanks for the find, fixed in the spec to be cancelable
https://github.com/domenic/unhandled-rejections-browser-spec/commit/c8a5f9247...

[philipj_slow, 2015-06-16 15:36:24]

On 2015/06/16 15:31:47, domenic wrote:
> On 2015/06/16 at 14:58:04, philipj wrote:
> 
> >
>
https://codereview.chromium.org/1179113007/diff/120001/Source/core/events/Pro...
> > Source/core/events/PromiseRejectionEvent.h:29:
> initializer.setCancelable(true);
> > https://github.com/domenic/unhandled-rejections-browser-spec says "Let event
> be a new trusted PromiseRejectionEvent object that does not bubble and is not
> cancelable, and which has the event name unhandledrejection" but also "If
event
> was canceled, then the error is handled. Otherwise, the error is not handled."
> which doesn't seem to add up.
> 
> Thanks for the find, fixed in the spec to be cancelable
>
https://github.com/domenic/unhandled-rejections-browser-spec/commit/c8a5f9247...

Jochen, can test that Event.cancelable has the right state for the two events,
especially given that they're not the same? Also for script-created |new
PromiseRejectionEvent()| so that this doesn't fall into the wrong bucket.

[domenic, 2015-06-16 15:42:36]

https://codereview.chromium.org/1179113007/diff/120001/LayoutTests/fast/dom/r...
File LayoutTests/fast/dom/resources/promise-rejection-events.js (right):

https://codereview.chromium.org/1179113007/diff/120001/LayoutTests/fast/dom/r...
LayoutTests/fast/dom/resources/promise-rejection-events.js:5: //
You killed all my nice separating newlines :(

https://codereview.chromium.org/1179113007/diff/120001/LayoutTests/fast/dom/r...
LayoutTests/fast/dom/resources/promise-rejection-events.js:393:
setTimeout(function() { f(); }, 0);
Should use MessageChannel in workers.
https://github.com/YuzuJS/setImmediate/blob/master/setImmediate.js#L107-L119

https://codereview.chromium.org/1179113007/diff/120001/LayoutTests/fast/dom/r...
LayoutTests/fast/dom/resources/promise-rejection-events.js:397:
Promise.resolve().then(function() { f(); });
Kind of the point of the microtask tests is to test interaction of other
microtasks with promise microtasks. So this should stick with mutation observers
where possible. I guess in workers there are no other microtask sources so
Promise.resolve is fine.

https://codereview.chromium.org/1179113007/diff/120001/LayoutTests/fast/dom/r...
LayoutTests/fast/dom/resources/promise-rejection-events.js:401: if (ev.promise
=== expectedPromiseGetter()) {
This if seems bad. If they mismatch, then the test should fail. Same here and
below.

[jochen (gone - plz use gerrit), 2015-06-16 18:08:18]

https://codereview.chromium.org/1179113007/diff/120001/LayoutTests/fast/dom/r...
File LayoutTests/fast/dom/resources/promise-rejection-events.js (right):

https://codereview.chromium.org/1179113007/diff/120001/LayoutTests/fast/dom/r...
LayoutTests/fast/dom/resources/promise-rejection-events.js:5: //
On 2015/06/16 at 15:42:36, domenic wrote:
> You killed all my nice separating newlines :(

I blame this on paste... I'll put them back in

https://codereview.chromium.org/1179113007/diff/120001/LayoutTests/fast/dom/r...
LayoutTests/fast/dom/resources/promise-rejection-events.js:397:
Promise.resolve().then(function() { f(); });
On 2015/06/16 at 15:42:36, domenic wrote:
> Kind of the point of the microtask tests is to test interaction of other
microtasks with promise microtasks. So this should stick with mutation observers
where possible. I guess in workers there are no other microtask sources so
Promise.resolve is fine.

i'll put them back.

from an implementation point of view, that's a boring distinction, it's the same
microtask queue :)

https://codereview.chromium.org/1179113007/diff/120001/LayoutTests/fast/dom/r...
LayoutTests/fast/dom/resources/promise-rejection-events.js:401: if (ev.promise
=== expectedPromiseGetter()) {
On 2015/06/16 at 15:42:36, domenic wrote:
> This if seems bad. If they mismatch, then the test should fail. Same here and
below.

without this, none of the tests will work, as they all run in a single microtask
run.

if there's a promise that doesn't match any of the listeners, the resp. test
will just timeout.

[jochen (gone - plz use gerrit), 2015-06-17 07:57:53]

all comments addressed

https://codereview.chromium.org/1179113007/diff/120001/LayoutTests/fast/dom/r...
File LayoutTests/fast/dom/resources/promise-rejection-events.js (right):

https://codereview.chromium.org/1179113007/diff/120001/LayoutTests/fast/dom/r...
LayoutTests/fast/dom/resources/promise-rejection-events.js:393:
setTimeout(function() { f(); }, 0);
On 2015/06/16 at 15:42:36, domenic wrote:
> Should use MessageChannel in workers.
https://github.com/YuzuJS/setImmediate/blob/master/setImmediate.js#L107-L119

done

https://codereview.chromium.org/1179113007/diff/120001/Source/bindings/core/v...
File Source/bindings/core/v8/RejectedPromises.cpp (right):

https://codereview.chromium.org/1179113007/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/RejectedPromises.cpp:67: m_shouldLogToConsole =
target->dispatchEvent(event);
On 2015/06/16 at 14:58:03, philipj wrote:
> Can you document this with "Log to console if event was not
preventDefault()'ed" or something? At least I always have to look up what the
return value of dispatchEvent() means.

done

https://codereview.chromium.org/1179113007/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/RejectedPromises.cpp:102: // Either collected or
https://crbug.com/450330
On 2015/06/16 at 14:58:03, philipj wrote:
> This issue is 403 for me, what does this comment mean?

I copied it from ::report()

cc'd you on the bug, I haven't looked into what is happening here

https://codereview.chromium.org/1179113007/diff/120001/Source/core/events/Pro...
File Source/core/events/PromiseRejectionEvent.h (right):

https://codereview.chromium.org/1179113007/diff/120001/Source/core/events/Pro...
Source/core/events/PromiseRejectionEvent.h:29: initializer.setCancelable(true);
On 2015/06/16 at 14:58:03, philipj wrote:
> https://github.com/domenic/unhandled-rejections-browser-spec says "Let event
be a new trusted PromiseRejectionEvent object that does not bubble and is not
cancelable, and which has the event name unhandledrejection" but also "If event
was canceled, then the error is handled. Otherwise, the error is not handled."
which doesn't seem to add up.

spec was updated

https://codereview.chromium.org/1179113007/diff/120001/Source/core/frame/Wind...
File Source/core/frame/WindowEventHandlers.idl (right):

https://codereview.chromium.org/1179113007/diff/120001/Source/core/frame/Wind...
Source/core/frame/WindowEventHandlers.idl:49:
[RuntimeEnabled=PromiseRejectionEvent] attribute EventHandler
onrejectionhandled;
On 2015/06/16 at 14:58:03, philipj wrote:
> Here and in WorkerGlobalScope.idl, can you put the added things in a section
at the bottom called "Unhandled Rejection Tracking" or similar that links to
https://github.com/domenic/unhandled-rejections-browser-spec? (To make it easier
to track which bits come from which spec.)

done

[philipj_slow, 2015-06-17 08:31:00]

LGTM for me, but I've focused on the web-facing half and not so much the
semantics and timing of these events.

https://codereview.chromium.org/1179113007/diff/120001/Source/bindings/core/v...
File Source/bindings/core/v8/RejectedPromises.cpp (right):

https://codereview.chromium.org/1179113007/diff/120001/Source/bindings/core/v...
Source/bindings/core/v8/RejectedPromises.cpp:102: // Either collected or
https://crbug.com/450330
On 2015/06/17 07:57:53, jochen wrote:
> On 2015/06/16 at 14:58:03, philipj wrote:
> > This issue is 403 for me, what does this comment mean?
> 
> I copied it from ::report()
> 
> cc'd you on the bug, I haven't looked into what is happening here

Thanks.

[jochen (gone - plz use gerrit), 2015-06-17 08:38:30]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-06-17 08:38:30]

The patchset sent to the CQ was uploaded after l-g-t-m from yurys@chromium.org,
philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1179113007/#ps160001
(title: "updates")

[commit-bot: I haz the power, 2015-06-17 08:38:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1179113007/160001

[philipj_slow, 2015-06-17 08:49:16]

Maybe a follow-up if these comments are too late.

https://codereview.chromium.org/1179113007/diff/160001/LayoutTests/fast/event...
File
LayoutTests/fast/events/constructors/promise-rejection-event-constructor.html
(right):

https://codereview.chromium.org/1179113007/diff/160001/LayoutTests/fast/event...
LayoutTests/fast/events/constructors/promise-rejection-event-constructor.html:11:
assert_equals(new PromiseRejectionEvent('eventType').cancelable, true);
Sorry, I neglected to check the new tests, this seems strange given that
EventInit.cancelable is false by default and it isn't overridden here.

https://codereview.chromium.org/1179113007/diff/160001/LayoutTests/fast/event...
LayoutTests/fast/events/constructors/promise-rejection-event-constructor.html:20:
assert_equals(new PromiseRejectionEvent('eventType', { cancelable: false
}).cancelable, true);
This also seems wrong.

[jochen (gone - plz use gerrit), 2015-06-17 09:09:37]

The CQ bit was unchecked by jochen@chromium.org

[philipj_slow, 2015-06-17 09:48:28]

LGTM again, all things cancelable seem to be in order now, tested by
fast/dom/promise-rejection-events-console.html and
fast/events/constructors/promise-rejection-event-constructor.html

[jochen (gone - plz use gerrit), 2015-06-17 09:49:04]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-06-17 09:49:04]

The patchset sent to the CQ was uploaded after l-g-t-m from yurys@chromium.org
Link to the patchset: https://codereview.chromium.org/1179113007/#ps200001
(title: "updates")

[commit-bot: I haz the power, 2015-06-17 09:49:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1179113007/200001

[jochen (gone - plz use gerrit), 2015-06-17 11:00:02]

The CQ bit was unchecked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-06-17 11:11:14]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-06-17 11:11:14]

The patchset sent to the CQ was uploaded after l-g-t-m from yurys@chromium.org,
philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1179113007/#ps210001
(title: "updates")

[commit-bot: I haz the power, 2015-06-17 11:11:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1179113007/210001

[commit-bot: I haz the power, 2015-06-17 12:12:47]

Committed patchset #12 (id:210001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197255