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.
Created attachment 374743 [details] Patch
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
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 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
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 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
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
Created attachment 374768 [details] Patch Fix unrelated test failure.
Created attachment 374829 [details] Patch Remove outdated expectations that caused iOS tests failure.
Created attachment 374896 [details] Patch Bring back iOS expectations and adjust them.
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 on attachment 374896 [details] Patch Please update the patch now that the tests have been landed. Thanks!
Created attachment 376283 [details] Patch Rebase patch.
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?
(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.
Created attachment 379647 [details] Patch Pass newly imported IndexedDB tests.
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.
(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 on attachment 379647 [details] Patch Clearing flags on attachment: 379647 Committed r250385: <https://trac.webkit.org/changeset/250385>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55749496>
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.
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>
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.
Created attachment 384829 [details] Patch Add m_isAttribute check.
Created attachment 384836 [details] Patch Adjust 3 more tests.
Comment on attachment 384836 [details] Patch Seems reasonable.
Comment on attachment 384836 [details] Patch Clearing flags on attachment: 384836 Committed r253123: <https://trac.webkit.org/changeset/253123>