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&)
Created attachment 374787 [details] Patch
<rdar://problem/53502472>
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.
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.
(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.
Created attachment 374827 [details] Patch
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
Created attachment 374837 [details] Patch
Comment on attachment 374837 [details] Patch Clearing flags on attachment: 374837 Committed r247826: <https://trac.webkit.org/changeset/247826>
All reviewed patches have been landed. Closing bug.