Bug 200066 - Non-callable "handleEvent" property is silently ignored
Summary: Non-callable "handleEvent" property is silently ignored
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Nobody
URL: https://github.com/web-platform-tests...
Keywords: InRadar
Depends on: 200592 202179
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-23 17:29 PDT by Alexey Shvayka
Modified: 2019-09-26 13:05 PDT (History)
10 users (show)

See Also:


Attachments
Patch (49.81 KB, patch)
2019-07-23 17:43 PDT, Alexey Shvayka
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (3.40 MB, application/zip)
2019-07-23 19:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-highsierra (3.35 MB, application/zip)
2019-07-23 19:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (3.11 MB, application/zip)
2019-07-23 20:55 PDT, Build Bot
no flags Details
Patch (49.80 KB, patch)
2019-07-24 02:30 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (52.79 KB, patch)
2019-07-24 16:31 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (51.00 KB, patch)
2019-07-25 10:16 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (3.33 KB, patch)
2019-08-14 11:04 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (7.05 KB, patch)
2019-09-26 09:35 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2019-07-23 17:29:18 PDT
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.
Comment 1 Alexey Shvayka 2019-07-23 17:43:54 PDT
Created attachment 374743 [details]
Patch
Comment 2 Build Bot 2019-07-23 19:02:40 PDT
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
Comment 3 Build Bot 2019-07-23 19:02:42 PDT
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
Comment 4 Build Bot 2019-07-23 19:04:23 PDT
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
Comment 5 Build Bot 2019-07-23 19:04:25 PDT
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
Comment 6 Build Bot 2019-07-23 20:55:32 PDT
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
Comment 7 Build Bot 2019-07-23 20:55:34 PDT
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
Comment 8 Alexey Shvayka 2019-07-24 02:30:09 PDT
Created attachment 374768 [details]
Patch

Fix unrelated test failure.
Comment 9 Alexey Shvayka 2019-07-24 16:31:18 PDT
Created attachment 374829 [details]
Patch

Remove outdated expectations that caused iOS tests failure.
Comment 10 Alexey Shvayka 2019-07-25 10:16:26 PDT
Created attachment 374896 [details]
Patch

Bring back iOS expectations and adjust them.
Comment 11 Darin Adler 2019-08-07 17:21:04 PDT
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.
Comment 12 Ryosuke Niwa 2019-08-14 01:35:12 PDT
Comment on attachment 374896 [details]
Patch

Please update the patch now that the tests have been landed. Thanks!
Comment 13 Alexey Shvayka 2019-08-14 11:04:07 PDT
Created attachment 376283 [details]
Patch

Rebase patch.
Comment 14 Darin Adler 2019-08-14 11:21:25 PDT
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?
Comment 15 Alexey Shvayka 2019-08-14 13:12:33 PDT
(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.
Comment 16 Alexey Shvayka 2019-09-26 09:35:02 PDT
Created attachment 379647 [details]
Patch

Pass newly imported IndexedDB tests.
Comment 17 Darin Adler 2019-09-26 10:00:26 PDT
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.
Comment 18 Alexey Shvayka 2019-09-26 10:20:39 PDT
(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.
Comment 19 WebKit Commit Bot 2019-09-26 10:43:30 PDT
Comment on attachment 379647 [details]
Patch

Clearing flags on attachment: 379647

Committed r250385: <https://trac.webkit.org/changeset/250385>
Comment 20 WebKit Commit Bot 2019-09-26 10:43:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2019-09-26 10:44:25 PDT
<rdar://problem/55749496>
Comment 22 Truitt Savell 2019-09-26 12:30:57 PDT
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.
Comment 23 Truitt Savell 2019-09-26 13:05:29 PDT
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>