Bug 224174 – Assert failure in isCloneInShadowTreeOfSVGUseElement

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Ryosuke Niwa]

e.g.

frame #0: JavaScriptCore`WTFCrash+14
frame #1: WebCore`WTFCrashWithInfo(int, char const*, char const*, int)+27
frame #2: WebCore`WebCore::isCloneInShadowTreeOfSVGUseElement(WebCore::Node&, WebCore::EventTarget&)+215
frame #3: WebCore`WebCore::JSLazyEventListener::checkValidityForEventTarget(WebCore::EventTarget&)+215
frame #4: WebCore`WebCore::EventTarget::addEventListener(WTF::AtomString const&, WTF::Ref<WebCore::EventListener, WTF::RawPtrTraits<WebCore::EventListener> >&&, WebCore::AddEventListenerOptions const&)+57
frame #5: WebCore`WebCore::tryAddEventListener(WebCore::Node*, WTF::AtomString const&, WTF::Ref<WebCore::EventListener, WTF::RawPtrTraits<WebCore::EventListener> >&&, WebCore::AddEventListenerOptions const&)+74
frame #6: WebCore`WebCore::Node::addEventListener(WTF::AtomString const&, WTF::Ref<WebCore::EventListener, WTF::RawPtrTraits<WebCore::EventListener> >&&, WebCore::AddEventListenerOptions const&)+69
frame #7: WebCore`WebCore::SVGElement::addEventListener(WTF::AtomString const&, WTF::Ref<WebCore::EventListener, WTF::RawPtrTraits<WebCore::EventListener> >&&, WebCore::AddEventListenerOptions const&)+611
frame #8: WebCore`WebCore::EventTarget::setAttributeEventListener(WTF::AtomString const&, WTF::RefPtr<WebCore::EventListener, WTF::RawPtrTraits<WebCore::EventListener>, WTF::DefaultRefDerefTraits<WebCore::EventListener> >&&, WebCore::DOMWrapperWorld&)+502
frame #9: WebCore`WebCore::Element::setAttributeEventListener(WTF::AtomString const&, WebCore::QualifiedName const&, WTF::AtomString const&)+98
frame #10: WebCore`WebCore::SVGElement::parseAttribute(WebCore::QualifiedName const&, WTF::AtomString const&)+393
frame #11: WebCore`WebCore::SVGGraphicsElement::parseAttribute(WebCore::QualifiedName const&, WTF::AtomString const&)+169
frame #12: WebCore`WebCore::SVGTextContentElement::parseAttribute(WebCore::QualifiedName const&, WTF::AtomString const&)+315
frame #13: WebCore`WebCore::SVGTextPathElement::parseAttribute(WebCore::QualifiedName const&, WTF::AtomString const&)+430
frame #14: WebCore`WebCore::Element::attributeChanged(WebCore::QualifiedName const&, WTF::AtomString const&, WTF::AtomString const&, WebCore::Element::AttributeModificationReason)+1131
frame #15: WebCore`WebCore::StyledElement::attributeChanged(WebCore::QualifiedName const&, WTF::AtomString const&, WTF::AtomString const&, WebCore::Element::AttributeModificationReason)+224
frame #16: WebCore`WebCore::SVGElement::attributeChanged(WebCore::QualifiedName const&, WTF::AtomString const&, WTF::AtomString const&, WebCore::Element::AttributeModificationReason)+58
frame #17: WebCore`WebCore::Element::didAddAttribute(WebCore::QualifiedName const&, WTF::AtomString const&)+68
frame #18: WebCore`WebCore::Element::addAttributeInternal(WebCore::QualifiedName const&, WTF::AtomString const&, WebCore::Element::SynchronizationOfLazyAttribute)+192
frame #19: WebCore`WebCore::Element::setAttributeInternal(unsigned int, WebCore::QualifiedName const&, WTF::AtomString const&, WebCore::Element::SynchronizationOfLazyAttribute)+204
frame #20: WebCore`WebCore::Element::setAttribute(WTF::AtomString const&, WTF::AtomString const&)+389

<rdar://75970108>

[Ryosuke Niwa]

There are two problems here.
1. addEventListener is called on a SVG instance that had already been disconnected from use element’s shadow tree but not yet deleted so its correspondingElement hasn’t been cleared yet.
2. DOM mutation events are fired on the corresponding elements of instances inside use element’s shadow tree when there is an EventQueueScope in the stack as in the case of document.execComand. This is because when the instants are cloned and inserted under its parent, isInShadowTree() is false since the nodes had not been inserted into the shadow tree at that moment.

[Ryosuke Niwa]

Created attachment 425137 [details]
Patch

[Ryosuke Niwa]

Created attachment 425138 [details]
Fixed change logs

[Darin Adler]

Comment on attachment 425138 [details]
Fixed change logs

View in context: https://bugs.webkit.org/attachment.cgi?id=425138&action=review

> Source/WebCore/dom/ScopedEventQueue.cpp:59
> +    if (event.event->eventInterface() == MutationEventInterfaceType && event.target->isInShadowTree())

All shadow trees? The other half of this patch is specific to user agent shadow trees. Do we have tests to verify these events don’t get dispatched for the various types of shadow trees? Should we? Is this just an optimization or an important correctness fix?

This seems to have an effect beyond what is mentioned in the change log.

> Source/WebCore/svg/SVGElement.cpp:255
> +        if (downcast<ShadowRoot>(oldParentOfRemovedTree).mode() == ShadowRootMode::UserAgent)

I would have used just one longer if statement instead of two nested if statements.

Separately, a helper function named isUserAgenrShadowRoot would make this easier to read and would be consistent with functions like isInUserAgentShadowTree.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #4)
> Comment on attachment 425138 [details]
> Fixed change logs
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425138&action=review
> 
> > Source/WebCore/dom/ScopedEventQueue.cpp:59
> > +    if (event.event->eventInterface() == MutationEventInterfaceType && event.target->isInShadowTree())
> 
> All shadow trees? The other half of this patch is specific to user agent
> shadow trees. Do we have tests to verify these events don’t get dispatched
> for the various types of shadow trees? Should we? Is this just an
> optimization or an important correctness fix?

We do for mutation events in general (we don't fire any DOM mutation events inside a shadow tree regardless of its mode). I guess we can add a test for non-UA shadow root as well.

> > Source/WebCore/svg/SVGElement.cpp:255
> > +        if (downcast<ShadowRoot>(oldParentOfRemovedTree).mode() == ShadowRootMode::UserAgent)
> 
> I would have used just one longer if statement instead of two nested if
> statements.
> 
> Separately, a helper function named isUserAgenrShadowRoot would make this
> easier to read and would be consistent with functions like
> isInUserAgentShadowTree.

Good point. Done.

[Ryosuke Niwa]

Created attachment 425234 [details]
Added a test for regular shadow tree

[Ryosuke Niwa]

Created attachment 425235 [details]
Added a test for regular shadow tree

[Darin Adler]

Comment on attachment 425235 [details]
Added a test for regular shadow tree

View in context: https://bugs.webkit.org/attachment.cgi?id=425235&action=review

> Source/WebCore/ChangeLog:8
> +        The bug was caused by two related but disticnt issues:

Typo in comment here: "distinct"

[Ryosuke Niwa]

Committed r275543 (236199@main): <https://commits.webkit.org/236199@main>

[Ryosuke Niwa]

(In reply to Darin Adler from comment #8)
> Comment on attachment 425235 [details]
> Added a test for regular shadow tree
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425235&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        The bug was caused by two related but disticnt issues:
> 
> Typo in comment here: "distinct"

Fixed.