Changeset 275543 in webkit


Ignore:
Timestamp:
Apr 6, 2021 12:51:49 PM (3 years ago)
Author:
rniwa@webkit.org
Message:

Assert failure in isCloneInShadowTreeOfSVGUseElement
​https://bugs.webkit.org/show_bug.cgi?id=224174

Reviewed by Darin Adler and Antti Koivisto.

Source/WebCore:

The bug was caused by two related but distinct issues:

  1. An element can have an instance that had been removed from a use element's shadow tree but not yet deleted. Because SVGElement clears its correspondingElement in its destructor, when addEventListener is called on such an element, it can try to add an event listener on this instance which is in the process of getting disposed.
  2. DOM mutation events can be fired on the corresponding element of an instance inside a use element’s shadow tree with EventQueueScope in the stack when the event is schedueld via Node::dispatchScopedEvent, e.g. because use element's shadow tree was updated during a style update at the beginning of document.execComand. Because SVGUseElement::cloneTarget constructs the shadow tree by cloning the original tree while it's disconnected from the document, Node::dispatchSubtreeModifiedEvent sees isInShadowTree() to be false and happily tries to dispach DOMSubtreeModified event using Node::dispatchScopedEvent. This works fine when the event is dispatched synchronously since these elements had never been exposed to any scripts yet and they are still disconnected so no scripts have had an opportunity to attach an event listener. But when EventQueueScope in the stack, Node::dispatchScopedEvent will queue up the event and fire it later when those instance elements had been inserted into use element's shadow tree.

This patch addresses (1) by severing correspondingElement relationship as soon as an instance
is removed from its use element's shadow tree, and (2) by not dispatching a scheduled mutation
event if the target is inside a shadow tree. Note that this patch also addresses (2) for
a regular shadow tree attached by author scripts.

Tests: fast/shadow-dom/mutation-event-in-shadow-tree.html

svg/dom/mutate-symbol-subtree-referenced-by-use-during-execCommand.html
svg/dom/update-svg-use-shadow-tree-with-execCommand.html

  • dom/ScopedEventQueue.cpp:

(WebCore::ScopedEventQueue::dispatchEvent const):

  • svg/SVGElement.cpp:

(WebCore::SVGElement::~SVGElement):
(WebCore::SVGElement::removedFromAncestor):
(WebCore::SVGElement::addEventListener):
(WebCore::SVGElement::removeEventListener):

LayoutTests:

Added tests for mutating nodes which is later inserted into a shadow tree during execCommand
as well as forcing a SVG use element to update its shadow tree by mutating the corresponding
element tree during execCommand.

  • fast/shadow-dom/mutation-event-in-shadow-tree-expected.txt: Added.
  • fast/shadow-dom/mutation-event-in-shadow-tree.html: Added.
  • svg/dom/mutate-symbol-subtree-referenced-by-use-during-execCommand-expected.txt: Added.
  • svg/dom/mutate-symbol-subtree-referenced-by-use-during-execCommand.html: Added.
  • svg/dom/update-svg-use-shadow-tree-with-execCommand-expected.txt: Added.
  • svg/dom/update-svg-use-shadow-tree-with-execCommand.html: Added.
Location:
trunk
Files:
6 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r275535 r275543  
     12021-04-06  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Assert failure in isCloneInShadowTreeOfSVGUseElement
     4        https://bugs.webkit.org/show_bug.cgi?id=224174
     5
     6        Reviewed by Darin Adler and Antti Koivisto.
     7
     8        Added tests for mutating nodes which is later inserted into a shadow tree during execCommand
     9        as well as forcing a SVG use element to update its shadow tree by mutating the corresponding
     10        element tree during execCommand.
     11
     12        * fast/shadow-dom/mutation-event-in-shadow-tree-expected.txt: Added.
     13        * fast/shadow-dom/mutation-event-in-shadow-tree.html: Added.
     14        * svg/dom/mutate-symbol-subtree-referenced-by-use-during-execCommand-expected.txt: Added.
     15        * svg/dom/mutate-symbol-subtree-referenced-by-use-during-execCommand.html: Added.
     16        * svg/dom/update-svg-use-shadow-tree-with-execCommand-expected.txt: Added.
     17        * svg/dom/update-svg-use-shadow-tree-with-execCommand.html: Added.
     18
    1192021-04-06  Jiewen Tan  <jiewen_tan@apple.com>
    220

  • trunk/Source/WebCore/ChangeLog

    r275538 r275543  
     12021-04-06  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Assert failure in isCloneInShadowTreeOfSVGUseElement
     4        https://bugs.webkit.org/show_bug.cgi?id=224174
     5
     6        Reviewed by Darin Adler and Antti Koivisto.
     7
     8        The bug was caused by two related but distinct issues:
     9        1. An element can have an instance that had been removed from a use element's shadow tree
     10           but not yet deleted. Because SVGElement clears its correspondingElement in its destructor,
     11           when addEventListener is called on such an element, it can try to add an event listener
     12           on this instance which is in the process of getting disposed.
     13        2. DOM mutation events can be fired on the corresponding element of an instance inside
     14           a use element’s shadow tree with EventQueueScope in the stack when the event is schedueld
     15           via Node::dispatchScopedEvent, e.g. because use element's shadow tree was updated during
     16           a style update at the beginning of document.execComand. Because SVGUseElement::cloneTarget
     17           constructs the shadow tree by cloning the original tree while it's disconnected from the
     18           document, Node::dispatchSubtreeModifiedEvent sees isInShadowTree() to be false and happily
     19           tries to dispach DOMSubtreeModified event using Node::dispatchScopedEvent. This works fine
     20           when the event is dispatched synchronously since these elements had never been exposed to
     21           any scripts yet and they are still disconnected so no scripts have had an opportunity to
     22           attach an event listener. But when EventQueueScope in the stack, Node::dispatchScopedEvent
     23           will queue up the event and fire it later when those instance elements had been inserted
     24           into use element's shadow tree.
     25
     26        This patch addresses (1) by severing correspondingElement relationship as soon as an instance
     27        is removed from its use element's shadow tree, and (2) by not dispatching a scheduled mutation
     28        event if the target is inside a shadow tree. Note that this patch also addresses (2) for
     29        a regular shadow tree attached by author scripts.
     30
     31        Tests: fast/shadow-dom/mutation-event-in-shadow-tree.html
     32               svg/dom/mutate-symbol-subtree-referenced-by-use-during-execCommand.html
     33               svg/dom/update-svg-use-shadow-tree-with-execCommand.html
     34
     35        * dom/ScopedEventQueue.cpp:
     36        (WebCore::ScopedEventQueue::dispatchEvent const):
     37        * svg/SVGElement.cpp:
     38        (WebCore::SVGElement::~SVGElement):
     39        (WebCore::SVGElement::removedFromAncestor):
     40        (WebCore::SVGElement::addEventListener):
     41        (WebCore::SVGElement::removeEventListener):
     42
    1432021-04-06  Eric Carlson  <eric.carlson@apple.com>
    244

  • trunk/Source/WebCore/dom/Node.h

    r274818 r275543  
    217217    bool isDocumentFragment() const { return hasNodeFlag(NodeFlag::IsDocumentFragment); }
    218218    bool isShadowRoot() const { return hasNodeFlag(NodeFlag::IsShadowRoot); }
     219    bool isUserAgentShadowRoot() const; // Defined in ShadowRoot.h
    219220
    220221    bool hasCustomStyleResolveCallbacks() const { return hasNodeFlag(NodeFlag::HasCustomStyleResolveCallbacks); }
    … …  
    226227    WEBCORE_EXPORT Element* shadowHost() const;
    227228    ShadowRoot* containingShadowRoot() const;
    228     ShadowRoot* shadowRoot() const;
     229    ShadowRoot* shadowRoot() const; // Defined in ShadowRoot.h
    229230    bool isClosedShadowHidden(const Node&) const;
    230231
    … …  
    241242
    242243    // Node's parent or shadow tree host.
    243     ContainerNode* parentOrShadowHostNode() const;
     244    ContainerNode* parentOrShadowHostNode() const; // Defined in ShadowRoot.h
    244245    ContainerNode* parentInComposedTree() const;
    245246    Element* parentElementInComposedTree() const;

  • trunk/Source/WebCore/dom/ScopedEventQueue.cpp

    r262016 r275543  
    5757void ScopedEventQueue::dispatchEvent(const ScopedEvent& event) const
    5858{
     59    if (event.event->eventInterface() == MutationEventInterfaceType && event.target->isInShadowTree())
     60        return;
    5961    event.target->dispatchEvent(event.event);
    6062}

  • trunk/Source/WebCore/dom/ShadowRoot.h

    r266212 r275543  
    154154}
    155155
     156inline bool Node::isUserAgentShadowRoot() const
     157{
     158    return isShadowRoot() && downcast<ShadowRoot>(*this).mode() == ShadowRootMode::UserAgent;
     159}
     160
    156161inline ContainerNode* Node::parentOrShadowHostNode() const
    157162{

  • trunk/Source/WebCore/svg/SVGElement.cpp

    r275410 r275543  
    172172        for (SVGElement* instance : m_svgRareData->instances())
    173173            instance->m_svgRareData->setCorrespondingElement(nullptr);
    174         if (auto correspondingElement = makeRefPtr(m_svgRareData->correspondingElement()))
    175             correspondingElement->m_svgRareData->instances().remove(this);
    176 
     174        RELEASE_ASSERT(!m_svgRareData->correspondingElement());
    177175        m_svgRareData = nullptr;
    178176    }
    … …  
    254252    }
    255253    invalidateInstances();
     254
     255    if (removalType.treeScopeChanged && oldParentOfRemovedTree.isUserAgentShadowRoot())
     256        setCorrespondingElement(nullptr);
    256257}
    257258
    … …  
    376377    for (auto* instance : instances()) {
    377378        ASSERT(instance->correspondingElement() == this);
     379        ASSERT(instance->isInUserAgentShadowTree());
    378380        bool result = instance->Node::addEventListener(eventType, listener.copyRef(), options);
    379381        ASSERT_UNUSED(result, result);
    … …  
    403405    for (auto& instance : instances()) {
    404406        ASSERT(instance->correspondingElement() == this);
     407        ASSERT(instance->isInUserAgentShadowTree());
    405408
    406409        if (instance->Node::removeEventListener(eventType, listener, options))
Note: See TracChangeset for help on using the changeset viewer.