Bug 207920

Summary:

Crash in SVGElement::removeEventListener with symbol element

Product:

WebKit

Reporter:

Doug Kelly <dougk>

Component:

SVG

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, dino, ews-watchlist, fmalita, gyuyoung.kim, pdr, rniwa, sabouhallawa, schenney, sergio, webkit-bug-importer, zimmermann

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Mac

OS:

macOS 10.15

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (7.46 KB, patch) 2020-03-03 13:41 PST, Doug Kelly	no flags	Details Formatted Diff Diff
Patch (7.96 KB, patch) 2020-03-03 17:59 PST, Doug Kelly	no flags	Details Formatted Diff Diff
Patch (7.85 KB, patch) 2020-03-04 09:02 PST, Doug Kelly	no flags	Details Formatted Diff Diff
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.