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

Description Doug Kelly 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.

Comment 1 Doug Kelly 2020-02-18 15:54:55 PST

<rdar://problem/58818176>

Comment 2 Doug Kelly 2020-02-18 15:59:39 PST

Created attachment 391112 [details]
Patch

Comment 3 Ryosuke Niwa 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>

Comment 4 Doug Kelly 2020-03-03 13:41:28 PST

Created attachment 392321 [details]
Patch

Comment 5 Ryosuke Niwa 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"');

Comment 6 Doug Kelly 2020-03-03 17:59:23 PST

Created attachment 392356 [details]
Patch

Comment 7 Ryosuke Niwa 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().

Comment 8 Ryosuke Niwa 2020-03-03 20:33:33 PST

Sorry, I keep finding new issues :(

Comment 9 Doug Kelly 2020-03-04 09:02:12 PST

Created attachment 392419 [details]
Patch

Comment 10 Doug Kelly 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.

Comment 11 Ryosuke Niwa 2020-03-04 18:49:17 PST

Comment on attachment 392419 [details]
Patch

Nice!

Comment 12 WebKit Commit Bot 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>

Comment 13 WebKit Commit Bot 2020-03-04 19:33:28 PST

All reviewed patches have been landed.  Closing bug.