RESOLVED FIXED 207920
Crash in SVGElement::removeEventListener with symbol element 
https://bugs.webkit.org/show_bug.cgi?id=207920
Summary Crash in SVGElement::removeEventListener with symbol element
Doug Kelly
Reported 2020-02-18 15:54:20 PST
When an event handler is set on a symbol element, then referenced with a use tag, and finally unset, WebKit crashes in removeEventListener.
Attachments
Patch (4.86 KB, patch)
2020-02-18 15:59 PST, Doug Kelly 		no flags
DetailsFormatted DiffDiff
Patch (7.46 KB, patch)
2020-03-03 13:41 PST, Doug Kelly 		no flags
DetailsFormatted DiffDiff
Patch (7.96 KB, patch)
2020-03-03 17:59 PST, Doug Kelly 		no flags
DetailsFormatted DiffDiff
Patch (7.85 KB, patch)
2020-03-04 09:02 PST, Doug Kelly 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Doug Kelly
Comment 1 2020-02-18 15:54:55 PST
<rdar://problem/58818176>
Doug Kelly
Comment 2 2020-02-18 15:59:39 PST
Created attachment 391112 [details] Patch
Ryosuke Niwa
Comment 3 2020-03-02 14:30:48 PST
Comment on attachment 391112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391112&action=review > Source/WebCore/svg/SVGSVGElement.cpp:146 > - if (!nearestViewportElement()) { > + if (!nearestViewportElement() && isConnected()) { Can we also add a new test testing that resize event handler etc... on a newly created SVG svg element won't be added to the window? e.g. you can do something like this: <!DOCTYPE html> <html> <body> <script src="../../resources/js-test.js"></script> <script> description('This tests creating a disconnected SVG element with resize event handler. The event handler should not get dispatched unless the element is connected'); const iframe = document.createElement('iframe'); iframe.style.width = '100px'; iframe.style.height = '100px'; document.body.appendChild(iframe); didFireResize = false; iframe.contentWindow.requestAnimationFrame(() => { const svg = iframe.contentDocument.createElementNS('http://www.w3.org/2000/svg', 'svg'); svg.setAttribute('onresize', 'top.didFireResize = true'); iframe.style.width = '200px'; iframe.contentWindow.requestAnimationFrame(() => { shouldBeFalse('didFireResize'); }); }); </script> </body>
Doug Kelly
Comment 4 2020-03-03 13:41:28 PST
Created attachment 392321 [details] Patch
Ryosuke Niwa
Comment 5 2020-03-03 13:54:36 PST
Comment on attachment 392321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392321&action=review > Source/WebCore/ChangeLog:13 > + Note that Chrome shares the behavior of a symbol element + use element being transformed into a svg element in the shadow DOM, but Firefox > + does not. This means that Chrome's behavior is slightly different than the WebKit behavior here (they apply event handlers to the parent This is not really relevant. What's important is whether a setting event handler attribute on a disconnected SVG element transfers the event handler to window or not. Firefox doesn't whilst Chrome does. > LayoutTests/fast/events/onresize-svg-parent-window.html:11 > + document.body.appendChild(iframe); > + didFireResize = false; This test doesn't work because Firefox doesn't synchronously load iframe. Let's fix that by putting iframe.contentWindow.requestAnimationFrame inside iframe.onload as in: didFireResize = false; iframe.onload = () { iframe.contentWindow.requestAnimationFrame(() => { .. } document.body.appendChild(iframe); > LayoutTests/fast/events/onresize-svg-parent-window.html:14 > + svg.setAttribute('onresize', 'top.didFireResize = true'); Let's also add a check for on error like this: svg.setAttribute('onerror', 'top.didFireOnError = true'); Declare didFireOnError next to didFireResize. > LayoutTests/fast/events/onresize-svg-parent-window.html:17 > + shouldBeFalse('didFireResize'); Assert also that shouldBeFalse('didFireOnError'); > LayoutTests/fast/events/onresize-svg-parent-window.html:18 > + }); Then after this requestAnimationFrame, do something like this: iframe.contentWindow.eval('throw "error"');
Doug Kelly
Comment 6 2020-03-03 17:59:23 PST
Created attachment 392356 [details] Patch
Ryosuke Niwa
Comment 7 2020-03-03 20:33:13 PST
Comment on attachment 392356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392356&action=review > LayoutTests/fast/events/detached-svg-parent-window-events-expected.txt:9 > +TEST COMPLETE Ugh... we need to define jsTestIsAsync = true. > LayoutTests/fast/events/detached-svg-parent-window-events.html:11 > + didFireOnError = false; Do this here: jsTestIsAsync = true > LayoutTests/fast/events/detached-svg-parent-window-events.html:22 > + if (window.testRunner) > + testRunner.notifyDone(); And call finishJSTest instead of this if (~) testRunner.notifyDone().
Ryosuke Niwa
Comment 8 2020-03-03 20:33:33 PST
Sorry, I keep finding new issues :(
Doug Kelly
Comment 9 2020-03-04 09:02:12 PST
Created attachment 392419 [details] Patch
Doug Kelly
Comment 10 2020-03-04 09:03:47 PST
(In reply to Ryosuke Niwa from comment #8) > Sorry, I keep finding new issues :( Hey, I'd rather find them now when they're easy to fix! Thanks for pointing them out.
Ryosuke Niwa
Comment 11 2020-03-04 18:49:17 PST
Comment on attachment 392419 [details] Patch Nice!
WebKit Commit Bot
Comment 12 2020-03-04 19:33:27 PST
Comment on attachment 392419 [details] Patch Clearing flags on attachment: 392419 Committed r257897: <https://trac.webkit.org/changeset/257897>
WebKit Commit Bot
Comment 13 2020-03-04 19:33:28 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.