| Summary: | Assert failure in isCloneInShadowTreeOfSVGUseElement | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
| Component: | SVG | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | cdumez, darin, dino, esprehn+autocc, ews-watchlist, fmalita, gyuyoung.kim, kangil.han, koivisto, pdr, sabouhallawa, schenney, sergio, zalan, zimmermann | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Ryosuke Niwa
2021-04-04 17:24:16 PDT
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. Created attachment 425137 [details]
Patch
Created attachment 425138 [details]
Fixed change logs
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. (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. Created attachment 425234 [details]
Added a test for regular shadow tree
Created attachment 425235 [details]
Added a test for regular shadow tree
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" Committed r275543 (236199@main): <https://commits.webkit.org/236199@main> (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. |