Bug 200066 – Non-callable "handleEvent" property is silently ignored

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Alexey Shvayka]

Test case:
let et = document.createElement("div")
et.addEventListener("foo", {handleEvent: "str"})
et.dispatchEvent(new Event("foo")) // => true

Actual:
No errors reported in DevTools or `window.onerror`

Expected:
TypeError should be reported

WPT: https://github.com/web-platform-tests/wpt/pull/15105
WebIDL: https://heycam.github.io/webidl/#call-a-user-objects-operation (step 10.4)

Firefox 68 reports TypeError, Chrome 75 doesn't.

[Alexey Shvayka]

Created attachment 374743 [details]
Patch

[EWS Watchlist]

Comment on attachment 374743 [details]
Patch

Attachment 374743 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12798284

New failing tests:
imported/w3c/web-platform-tests/dom/events/event-global-extra.window.html

[EWS Watchlist]

Created attachment 374751 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 374743 [details]
Patch

Attachment 374743 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12798301

New failing tests:
imported/w3c/web-platform-tests/dom/events/event-global-extra.window.html

[EWS Watchlist]

Created attachment 374752 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 374743 [details]
Patch

Attachment 374743 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12798762

New failing tests:
imported/w3c/web-platform-tests/dom/events/event-global-extra.window.html

[EWS Watchlist]

Created attachment 374759 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Alexey Shvayka]

Created attachment 374768 [details]
Patch

Fix unrelated test failure.

[Alexey Shvayka]

Created attachment 374829 [details]
Patch

Remove outdated expectations that caused iOS tests failure.

[Alexey Shvayka]

Created attachment 374896 [details]
Patch

Bring back iOS expectations and adjust them.

[Darin Adler]

Comment on attachment 374896 [details]
Patch

Thanks for contributing this fix.

It’s not ideal to have a single patch with many test changes, along with a bug fix and the new expectations that show the bug is fixed.

Instead, we should first re-sync the tests in a first patch, and then make sure that this second patch has the behavior change and the test result changes that go with it only.

[Ryosuke Niwa]

Comment on attachment 374896 [details]
Patch

Please update the patch now that the tests have been landed. Thanks!

[Alexey Shvayka]

Created attachment 376283 [details]
Patch

Rebase patch.

[Darin Adler]

Comment on attachment 376283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376283&action=review

Thanks for fixing this!

> Source/WebCore/bindings/js/JSEventListener.cpp:152
> +            event.target()->uncaughtExceptionInEventHandler();

I can’t find anything in that tests that checks this. I assume this is needed here for the same reason it’s needed above, but how can we test that it’s working properly?

[Alexey Shvayka]

(In reply to Darin Adler from comment #14)
> Comment on attachment 376283 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376283&action=review
> 
> Thanks for fixing this!

Thank you for reviewing my patches!

> 
> > Source/WebCore/bindings/js/JSEventListener.cpp:152
> > +            event.target()->uncaughtExceptionInEventHandler();
> 
> I can’t find anything in that tests that checks this. I assume this is
> needed here for the same reason it’s needed above, but how can we test that
> it’s working properly?

Nice catch, we don't have test coverage for this. It implements legacyOutputDidListenersThrowFlag (please see https://dom.spec.whatwg.org/#dispatching-events and https://w3c.github.io/IndexedDB) via IDBRequest::uncaughtExceptionInEventHandler.

I will make a PR to web-platform-tests, submit another patch to re-sync, and update this one to pass new tests.

[Alexey Shvayka]

Created attachment 379647 [details]
Patch

Pass newly imported IndexedDB tests.

[Darin Adler]

Comment on attachment 379647 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379647&action=review

> Source/WebCore/bindings/js/JSEventListener.cpp:153
> +            reportException(exec, createTypeError(exec, "'handleEvent' property of event listener should be callable"_s));

Not sure if we really need to create an exception object just to do the logging, but seems fine for now at least.

[Alexey Shvayka]

(In reply to Darin Adler from comment #17)
> Not sure if we really need to create an exception object just to do the
> logging, but seems fine for now at least.

Please note that it is observable in `window.onerror`: https://github.com/web-platform-tests/wpt/blob/master/dom/events/EventListener-handleEvent.html#L102.

[WebKit Commit Bot]

Comment on attachment 379647 [details]
Patch

Clearing flags on attachment: 379647

Committed r250385: <https://trac.webkit.org/changeset/250385>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55749496>

[Truitt Savell]

Looks like the changes in https://trac.webkit.org/changeset/250385/webkit

broke: imported/w3c/web-platform-tests/svg/animations/syncbase-remove-add-while-running.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fsvg%2Fanimations%2Fsyncbase-remove-add-while-running.html

Diff:
--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/svg/animations/syncbase-remove-add-while-running-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/svg/animations/syncbase-remove-add-while-running-actual.txt
@@ -1,3 +1,6 @@
+CONSOLE MESSAGE: TypeError: 'handleEvent' property of event listener should be callable
+
+Harness Error (FAIL), message = Script error.
 
 FAIL Remove/Add syncbase while animation is running assert_equals: Sync base triggered expected 100 but got 0
 

This issue was identified by EWS, so we will need to roll this change back.

[Truitt Savell]

Reverted r250385 for reason:

Broke imported/w3c/web-platform-tests/svg/animations/syncbase-remove-add-while-running.html on all platforms. Which was caught by EWS

Committed r250395: <https://trac.webkit.org/changeset/250395>

[Alexey Shvayka]

web-platform-tests/svg/animations/syncbase-remove-add-while-running.html failed because a) it had incorrect test harness usage (fixed in https://github.com/web-platform-tests/wpt/commit/786fc058fd83cbf002be3a35100ab2a3b1f98d58#diff-30a693f22699fe784f08efb22d0743ee) and b) WebKit gets "handleEvent" property from event handlers (on* attributes) and throws if it is non-callable.

https://bugs.webkit.org/show_bug.cgi?id=204814 syncs web-platform-tests so I can update this patch with a fix and new tests passes.

[Alexey Shvayka]

Created attachment 384829 [details]
Patch

Add m_isAttribute check.

[Alexey Shvayka]

Created attachment 384836 [details]
Patch

Adjust 3 more tests.

[Ryosuke Niwa]

Comment on attachment 384836 [details]
Patch

Seems reasonable.

[WebKit Commit Bot]

Comment on attachment 384836 [details]
Patch

Clearing flags on attachment: 384836

Committed r253123: <https://trac.webkit.org/changeset/253123>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.