Summary: | Non-callable "handleEvent" property is silently ignored | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Shvayka <ashvayka> | ||||||||||||||||||||||||
Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Minor | CC: | alecflett, beidson, commit-queue, darin, ews-watchlist, jsbell, mkwst, rniwa, tsavell, webkit-bug-importer | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||
URL: | https://github.com/web-platform-tests/wpt/pull/15105 | ||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=200735 | ||||||||||||||||||||||||||
Bug Depends on: | 200592, 202179, 204814 | ||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||
Attachments: |
|
Description
Alexey Shvayka
2019-07-23 17:29:18 PDT
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. 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> All reviewed patches have been landed. Closing bug. |