RESOLVED FIXED 200083
REGRESSION (r244995): Assertion failure when addEventListener to an SVGElement which has an. instance in shadow tree
https://bugs.webkit.org/show_bug.cgi?id=200083
Summary REGRESSION (r244995): Assertion failure when addEventListener to an SVGElemen...
Said Abou-Hallawa
Reported 2019-07-24 09:54:54 PDT
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&)
Attachments
test case (540 bytes, text/html)
2019-07-24 09:54 PDT, Said Abou-Hallawa
no flags
Patch (4.39 KB, patch)
2019-07-24 10:26 PDT, Said Abou-Hallawa
no flags
Patch (5.20 KB, patch)
2019-07-24 16:25 PDT, Said Abou-Hallawa
no flags
Patch (5.17 KB, patch)
2019-07-24 17:18 PDT, Said Abou-Hallawa
no flags
Said Abou-Hallawa
Comment 1 2019-07-24 10:26:49 PDT
Radar WebKit Bug Importer
Comment 2 2019-07-24 10:27:39 PDT
Ryosuke Niwa
Comment 3 2019-07-24 11:06:23 PDT
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 4 2019-07-24 11:26:01 PDT
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
Comment 5 2019-07-24 13:12:18 PDT
(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
Comment 6 2019-07-24 16:25:42 PDT
Ryosuke Niwa
Comment 7 2019-07-24 16:35:03 PDT
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
Comment 8 2019-07-24 17:18:39 PDT
WebKit Commit Bot
Comment 9 2019-07-25 10:56:52 PDT
Comment on attachment 374837 [details] Patch Clearing flags on attachment: 374837 Committed r247826: <https://trac.webkit.org/changeset/247826>
WebKit Commit Bot
Comment 10 2019-07-25 10:56:53 PDT
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.