REGRESSION (r244995): Assertion failure when addEventListener to an SVGElement which has an. instance in shadow tree

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Created attachment 374784 [details]
test case

Open the attached test case. The following assertion will fire in debug builds:

0x000000012208cbde in ::WTFCrash()
0x000000010a37a9fb in WTFCrashWithInfo(int, char const*, char const*, int)
0x000000010c3d5df4 in WebCore::JSLazyEventListener::checkValidityForEventTarget(WebCore::EventTarget&)
0x000000010c9bd339 in WebCore::EventTarget::addEventListener(WTF::AtomString const&, WTF::Ref<WebCore::EventListener, WTF::DumbPtrTraits<WebCore::EventListener> >&&, WebCore::EventTarget::AddEventListenerOptions const&)
0x000000010ca2c09d in WebCore::tryAddEventListener(WebCore::Node*, WTF::AtomString const&, WTF::Ref<WebCore::EventListener, WTF::DumbPtrTraits<WebCore::EventListener> >&&, WebCore::EventTarget::AddEventListenerOptions const&)
0x000000010ca2c038 in WebCore::Node::addEventListener(WTF::AtomString const&, WTF::Ref<WebCore::EventListener, WTF::DumbPtrTraits<WebCore::EventListener> >&&, WebCore::EventTarget::AddEventListenerOptions const&)
0x000000010e01f1bf in WebCore::SVGElement::addEventListener(WTF::AtomString const&, WTF::Ref<WebCore::EventListener, WTF::DumbPtrTraits<WebCore::EventListener> >&&, WebCore::EventTarget::AddEventListenerOptions const&)
0x000000010c9bdfb7 in WebCore::EventTarget::setAttributeEventListener(WTF::AtomString const&, WTF::RefPtr<WebCore::EventListener, WTF::DumbPtrTraits<WebCore::EventListener> >&&, WebCore::DOMWrapperWorld&)
0x000000010c975d26 in WebCore::Element::setAttributeEventListener(WTF::AtomString const&, WebCore::QualifiedName const&, WTF::AtomString const&)
0x000000010e01edf5 in WebCore::SVGElement::parseAttribute(WebCore::QualifiedName const&, WTF::AtomString const&)
0x000000010e15eea0 in WebCore::SVGGraphicsElement::parseAttribute(WebCore::QualifiedName const&, WTF::AtomString const&)
0x000000010e2af04f in WebCore::SVGUseElement::parseAttribute(WebCore::QualifiedName const&, WTF::AtomString const&)
0x000000010c9712cd in WebCore::Element::attributeChanged(WebCore::QualifiedName const&, WTF::AtomString const&, WTF::AtomString const&, WebCore::Element::AttributeModificationReason)
0x000000010cac1e0c in WebCore::StyledElement::attributeChanged(WebCore::QualifiedName const&, WTF::AtomString const&, WTF::AtomString const&, WebCore::Element::AttributeModificationReason)
0x000000010e01fafb in WebCore::SVGElement::attributeChanged(WebCore::QualifiedName const&, WTF::AtomString const&, WTF::AtomString const&, WebCore::Element::AttributeModificationReason)
0x000000010c977702 in WebCore::Element::didAddAttribute(WebCore::QualifiedName const&, WTF::AtomString const&)
0x000000010c977643 in WebCore::Element::addAttributeInternal(WebCore::QualifiedName const&, WTF::AtomString const&, WebCore::Element::SynchronizationOfLazyAttribute)
0x000000010c970a71 in WebCore::Element::setAttributeInternal(unsigned int, WebCore::QualifiedName const&, WTF::AtomString const&, WebCore::Element::SynchronizationOfLazyAttribute)
0x000000010c970d2e in WebCore::Element::setAttribute(WTF::AtomString const&, WTF::AtomString const&)

[Said Abou-Hallawa]

Created attachment 374787 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/53502472>

[Ryosuke Niwa]

Comment on attachment 374787 [details]
Patch

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

> Source/WebCore/bindings/js/JSLazyEventListener.cpp:89
> +        ASSERT(m_originalNode.get() == &node || !m_originalNode->isInShadowTree() && node.isInShadowTree());

This assertion isn’t quite right. We need to restrict it to the shadow tree of SVG use element instead.

[Said Abou-Hallawa]

Comment on attachment 374787 [details]
Patch

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

>> Source/WebCore/bindings/js/JSLazyEventListener.cpp:89
>> +        ASSERT(m_originalNode.get() == &node || !m_originalNode->isInShadowTree() && node.isInShadowTree());
> 
> This assertion isn’t quite right. We need to restrict it to the shadow tree of SVG use element instead.

Why it is not quite right? What is the case that you want to catch and the new assertion is going to hide it? Is there a possibility that you addEventListener() to a node in a shadow tree without having to add the same event listener to its source node? Please explain this case so I can add a test case for it.

[Ryosuke Niwa]

(In reply to Said Abou-Hallawa from comment #4)
> Comment on attachment 374787 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374787&action=review
> 
> >> Source/WebCore/bindings/js/JSLazyEventListener.cpp:89
> >> +        ASSERT(m_originalNode.get() == &node || !m_originalNode->isInShadowTree() && node.isInShadowTree());
> > 
> > This assertion isn’t quite right. We need to restrict it to the shadow tree of SVG use element instead.
> 
> Why it is not quite right? What is the case that you want to catch and the
> new assertion is going to hide it? Is there a possibility that you
> addEventListener() to a node in a shadow tree without having to add the same
> event listener to its source node? Please explain this case so I can add a
> test case for it.

Yes. Note that shadow DOM is a Web exposed API. Anyone can add an event listener anywhere.

SVG use element is a very special case, and we shouldn't be weakening this assertion for all other event listeners in the shadow tress just for SVG use element.

[Said Abou-Hallawa]

Created attachment 374827 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 374827 [details]
Patch

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

> Source/WebCore/bindings/js/JSLazyEventListener.cpp:98
> +    if (is<SVGElement>(node)) {
> +        auto& element = downcast<SVGElement>(node);
> +
> +        if (element.correspondingElement()) {

We shouldn't be running this code in release builds when the assertion is disabled.
I suggest we wrap this into an inline helper function instead.
e.g. isCloneInShadowTreeOfSVGUseElement

[Said Abou-Hallawa]

Created attachment 374837 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374837 [details]
Patch

Clearing flags on attachment: 374837

Committed r247826: <https://trac.webkit.org/changeset/247826>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.