Changeset 224053 in webkit


Ignore:
Timestamp:
Oct 26, 2017 3:02:38 PM (6 years ago)
Author:
rniwa@webkit.org
Message:

DidMoveToNewDocumentAssertionScope shouldn't be necessary
https://bugs.webkit.org/show_bug.cgi?id=178836
<rdar://problem/35008876>

Reviewed by Antti Koivisto.

DidMoveToNewDocumentAssertionScope was introduced in r217972 to replace an existing assertion to make sure
Node::didMoveToNewDocument is always called by its overrides in Node's subclasses. However, we can ensure
better Node::didMoveToNewDocument is always called if we called it directly in Node::moveTreeToNewScope.

Because only subclasses of Element and ShadowRoot override Node::didMoveToNewDocument and we already have
a specialized code path to adopt a ShadowRoot to a new document, this refactoring eliminates the need for
having a virtual function on Node at all.

Hence this patch names Node::didMoveToNewDocument to Node::moveToNewDocument and makes it non-virtual,
splits ShadowRoot::didMoveToNewDocument into moveShadowRootToNewParentScope and moveShadowRootToNewDocument,
and removes DidMoveToNewDocumentAssertionScope completely.

No new tests since there should be no behavioral change.

  • dom/Document.cpp:

(WebCore::Document::moveNodeIteratorsToNewDocumentSlowCase): Renamed from moveNodeIteratorsToNewDocument.

  • dom/Document.h:

(WebCore::Document::moveNodeIteratorsToNewDocument): Inlined the check for emptiness of m_nodeIterators to
avoid keep calling moveNodeIteratorsToNewDocumentSlowCase on every single node getting moved.

  • dom/Element.cpp:

(WebCore::Element::didMoveToNewDocument): Removed the call to Node::didMoveToNewDocument since this is the
base virtual function now.

  • dom/Element.h:
  • dom/Node.cpp:

(WebCore::DidMoveToNewDocumentAssertionScope::DidMoveToNewDocumentAssertionScope): Deleted.
(WebCore::DidMoveToNewDocumentAssertionScope::~DidMoveToNewDocumentAssertionScope): Deleted.
(WebCore::DidMoveToNewDocumentAssertionScope::didRecieveCall): Deleted.
(WebCore::moveNodeToNewDocument): Deleted.
(WebCore::Node::moveShadowTreeToNewDocument): Made this a member function of Node since it needs to call
moveNodeToNewDocument, which is private to Node.
(WebCore::Node::moveTreeToNewScope): Removed the release assert for the root node since the same check
exists inside traverseSubtreeToUpdateTreeScope. Also removed the release assertion for checking that
node's old document matches the old document since document() simply calls treeScope().documentScope()
and we're already release-asserting that the old scope of a node matches the old scope we know of.
We release-assert that the old tree scope's document didn't change after the traversal instead. Finally,
replaced a bunch of RELEASE_ASSERT with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION.
(WebCore::Node::moveNodeToNewDocument): Renamed from didMoveToNewDocument. Moved the code related to
mutation observers inside hasRareData() check, and moved the the code to move event listeners inside
eventTargetData() check both for clarity, and avoid doing the work for every single node being moved.
Finally, call the old didMoveToNewDocument when "this" is an Element.

  • dom/Node.h:
  • dom/ShadowRoot.cpp:

(WebCore::ShadowRoot::moveShadowRootToNewParentScope): Added. Extracted from didMoveToNewDocument.
(WebCore::ShadowRoot::moveShadowRootToNewDocument): Renamed from didMoveToNewDocument. We now
release-assert that parent tree scope's document matches the new document if any.

  • dom/ShadowRoot.h:
Location:
trunk/Source/WebCore
Files:
9 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r224051 r224053  
     12017-10-26  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        DidMoveToNewDocumentAssertionScope shouldn't be necessary
     4        https://bugs.webkit.org/show_bug.cgi?id=178836
     5        <rdar://problem/35008876>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        DidMoveToNewDocumentAssertionScope was introduced in r217972 to replace an existing assertion to make sure
     10        Node::didMoveToNewDocument is always called by its overrides in Node's subclasses. However, we can ensure
     11        better Node::didMoveToNewDocument is always called if we called it directly in Node::moveTreeToNewScope.
     12
     13        Because only subclasses of Element and ShadowRoot override Node::didMoveToNewDocument and we already have
     14        a specialized code path to adopt a ShadowRoot to a new document, this refactoring eliminates the need for
     15        having a virtual function on Node at all.
     16
     17        Hence this patch names Node::didMoveToNewDocument to Node::moveToNewDocument and makes it non-virtual,
     18        splits ShadowRoot::didMoveToNewDocument into moveShadowRootToNewParentScope and moveShadowRootToNewDocument,
     19        and removes DidMoveToNewDocumentAssertionScope completely.
     20
     21        No new tests since there should be no behavioral change.
     22
     23        * dom/Document.cpp:
     24        (WebCore::Document::moveNodeIteratorsToNewDocumentSlowCase): Renamed from moveNodeIteratorsToNewDocument.
     25        * dom/Document.h:
     26        (WebCore::Document::moveNodeIteratorsToNewDocument): Inlined the check for emptiness of m_nodeIterators to
     27        avoid keep calling moveNodeIteratorsToNewDocumentSlowCase on every single node getting moved.
     28        * dom/Element.cpp:
     29        (WebCore::Element::didMoveToNewDocument): Removed the call to Node::didMoveToNewDocument since this is the
     30        base virtual function now.
     31        * dom/Element.h:
     32        * dom/Node.cpp:
     33        (WebCore::DidMoveToNewDocumentAssertionScope::DidMoveToNewDocumentAssertionScope): Deleted.
     34        (WebCore::DidMoveToNewDocumentAssertionScope::~DidMoveToNewDocumentAssertionScope): Deleted.
     35        (WebCore::DidMoveToNewDocumentAssertionScope::didRecieveCall): Deleted.
     36        (WebCore::moveNodeToNewDocument): Deleted.
     37        (WebCore::Node::moveShadowTreeToNewDocument): Made this a member function of Node since it needs to call
     38        moveNodeToNewDocument, which is private to Node.
     39        (WebCore::Node::moveTreeToNewScope): Removed the release assert for the root node since  the same check
     40        exists inside traverseSubtreeToUpdateTreeScope. Also removed the release assertion for checking that
     41        node's old document matches the old document since document() simply calls treeScope().documentScope()
     42        and we're already release-asserting that the old scope of a node matches the old scope we know of.
     43        We release-assert that the old tree scope's document didn't change after the traversal instead. Finally,
     44        replaced a bunch of RELEASE_ASSERT with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION.
     45        (WebCore::Node::moveNodeToNewDocument): Renamed from didMoveToNewDocument. Moved the code related to
     46        mutation observers inside hasRareData() check, and moved the the code to move event listeners inside
     47        eventTargetData() check both for clarity, and avoid doing the work for every single node being moved.
     48        Finally, call the old didMoveToNewDocument when "this" is an Element.
     49        * dom/Node.h:
     50        * dom/ShadowRoot.cpp:
     51        (WebCore::ShadowRoot::moveShadowRootToNewParentScope): Added. Extracted from didMoveToNewDocument.
     52        (WebCore::ShadowRoot::moveShadowRootToNewDocument): Renamed from didMoveToNewDocument. We now
     53        release-assert that parent tree scope's document matches the new document if any.
     54        * dom/ShadowRoot.h:
     55
    1562017-10-26  Youenn Fablet  <youenn@apple.com>
    257
  • trunk/Source/WebCore/dom/Document.cpp

    r224045 r224053  
    40494049}
    40504050
    4051 void Document::moveNodeIteratorsToNewDocument(Node& node, Document& newDocument)
    4052 {
     4051void Document::moveNodeIteratorsToNewDocumentSlowCase(Node& node, Document& newDocument)
     4052{
     4053    ASSERT(!m_nodeIterators.isEmpty());
    40534054    for (auto* it : copyToVector(m_nodeIterators)) {
    40544055        if (&it->root() == &node) {
  • trunk/Source/WebCore/dom/Document.h

    r224045 r224053  
    752752    void attachNodeIterator(NodeIterator*);
    753753    void detachNodeIterator(NodeIterator*);
    754     void moveNodeIteratorsToNewDocument(Node&, Document&);
     754    void moveNodeIteratorsToNewDocument(Node& node, Document& newDocument)
     755    {
     756        if (!m_nodeIterators.isEmpty())
     757            moveNodeIteratorsToNewDocumentSlowCase(node, newDocument);
     758    }
    755759
    756760    void attachRange(Range*);
     
    14241428    void buildAccessKeyMap(TreeScope* root);
    14251429
     1430    void moveNodeIteratorsToNewDocumentSlowCase(Node&, Document&);
     1431
    14261432    void loadEventDelayTimerFired();
    14271433
  • trunk/Source/WebCore/dom/Element.cpp

    r224011 r224053  
    15721572{
    15731573    ASSERT_WITH_SECURITY_IMPLICATION(&document() == &newDocument);
    1574     Node::didMoveToNewDocument(oldDocument, newDocument);
    15751574
    15761575    if (oldDocument.inQuirksMode() != document().inQuirksMode()) {
  • trunk/Source/WebCore/dom/Element.h

    r223802 r224053  
    261261    void cloneDataFromElement(const Element&);
    262262
     263    virtual void didMoveToNewDocument(Document& oldDocument, Document& newDocument);
     264
    263265    bool hasEquivalentAttributes(const Element* other) const;
    264266
     
    557559    void removeAllEventListeners() final;
    558560    virtual void parserDidSetAttributes();
    559     void didMoveToNewDocument(Document& oldDocument, Document& newDocument) override;
    560561
    561562    void clearTabIndexExplicitlyIfNeeded();
  • trunk/Source/WebCore/dom/Node.cpp

    r223802 r224053  
    19181918}
    19191919
    1920 #if !ASSERT_DISABLED || ENABLE(SECURITY_ASSERTIONS)
    1921 class DidMoveToNewDocumentAssertionScope {
    1922 public:
    1923     DidMoveToNewDocumentAssertionScope(Node& node, Document& oldDocument, Document& newDocument)
    1924         : m_node(node)
    1925         , m_oldDocument(oldDocument)
    1926         , m_newDocument(newDocument)
    1927         , m_previousScope(s_scope)
    1928     {
    1929         s_scope = this;
    1930     }
    1931 
    1932     ~DidMoveToNewDocumentAssertionScope()
    1933     {
    1934         RELEASE_ASSERT(m_called);
    1935         s_scope = m_previousScope;
    1936     }
    1937 
    1938     static void didRecieveCall(Node& node, Document& oldDocument, Document& newDocument)
    1939     {
    1940         RELEASE_ASSERT(s_scope);
    1941         RELEASE_ASSERT(!s_scope->m_called);
    1942         RELEASE_ASSERT(&s_scope->m_node == &node);
    1943         RELEASE_ASSERT(&s_scope->m_oldDocument == &oldDocument);
    1944         RELEASE_ASSERT(&s_scope->m_newDocument == &newDocument);
    1945         s_scope->m_called = true;
    1946     }
    1947 
    1948 private:
    1949     Node& m_node;
    1950     Document& m_oldDocument;
    1951     Document& m_newDocument;
    1952     bool m_called { false };
    1953     DidMoveToNewDocumentAssertionScope* m_previousScope;
    1954 
    1955     static DidMoveToNewDocumentAssertionScope* s_scope;
    1956 };
    1957 
    1958 DidMoveToNewDocumentAssertionScope* DidMoveToNewDocumentAssertionScope::s_scope = nullptr;
    1959 
    1960 #else
    1961 class DidMoveToNewDocumentAssertionScope {
    1962 public:
    1963     DidMoveToNewDocumentAssertionScope(Node&, Document&, Document&) { }
    1964     static void didRecieveCall(Node&, Document&, Document&) { }
    1965 };
    1966 #endif
    1967 
    1968 static ALWAYS_INLINE void moveNodeToNewDocument(Node& node, Document& oldDocument, Document& newDocument)
    1969 {
    1970     ASSERT(!node.isConnected() || &oldDocument != &newDocument);
    1971     DidMoveToNewDocumentAssertionScope scope(node, oldDocument, newDocument);
    1972     node.didMoveToNewDocument(oldDocument, newDocument);
    1973     RELEASE_ASSERT(&node.document() == &newDocument);
    1974 }
    1975 
    19761920template <typename MoveNodeFunction, typename MoveShadowRootFunction>
    19771921static void traverseSubtreeToUpdateTreeScope(Node& root, MoveNodeFunction moveNode, MoveShadowRootFunction moveShadowRoot)
     
    19941938}
    19951939
    1996 static void moveShadowTreeToNewDocument(ShadowRoot& shadowRoot, Document& oldDocument, Document& newDocument)
     1940inline void Node::moveShadowTreeToNewDocument(ShadowRoot& shadowRoot, Document& oldDocument, Document& newDocument)
    19971941{
    19981942    traverseSubtreeToUpdateTreeScope(shadowRoot, [&oldDocument, &newDocument](Node& node) {
    1999         moveNodeToNewDocument(node, oldDocument, newDocument);
     1943        node.moveNodeToNewDocument(oldDocument, newDocument);
    20001944    }, [&oldDocument, &newDocument](ShadowRoot& innerShadowRoot) {
    2001         RELEASE_ASSERT(&innerShadowRoot.document() == &oldDocument);
     1945        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(&innerShadowRoot.document() == &oldDocument);
     1946        innerShadowRoot.moveShadowRootToNewDocument(newDocument);
    20021947        moveShadowTreeToNewDocument(innerShadowRoot, oldDocument, newDocument);
    20031948    });
     
    20071952{
    20081953    ASSERT(&oldScope != &newScope);
    2009     RELEASE_ASSERT(&root.treeScope() == &oldScope);
    20101954
    20111955    Document& oldDocument = oldScope.documentScope();
     
    20151959        traverseSubtreeToUpdateTreeScope(root, [&](Node& node) {
    20161960            ASSERT(!node.isTreeScope());
    2017             RELEASE_ASSERT(&node.treeScope() == &oldScope);
    2018             RELEASE_ASSERT(&node.document() == &oldDocument);
     1961            RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(&node.treeScope() == &oldScope);
    20191962            node.setTreeScope(newScope);
    2020             moveNodeToNewDocument(node, oldDocument, newDocument);
     1963            node.moveNodeToNewDocument(oldDocument, newDocument);
    20211964        }, [&](ShadowRoot& shadowRoot) {
    20221965            ASSERT_WITH_SECURITY_IMPLICATION(&shadowRoot.document() == &oldDocument);
    2023             shadowRoot.setParentTreeScope(newScope);
     1966            shadowRoot.moveShadowRootToNewParentScope(newScope, newDocument);
    20241967            moveShadowTreeToNewDocument(shadowRoot, oldDocument, newDocument);
    20251968        });
    20261969        oldDocument.decrementReferencingNodeCount();
     1970        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(&oldScope.documentScope() == &oldDocument && &newScope.documentScope() == &newDocument);
    20271971    } else {
    20281972        traverseSubtreeToUpdateTreeScope(root, [&](Node& node) {
    20291973            ASSERT(!node.isTreeScope());
    2030             RELEASE_ASSERT(&node.treeScope() == &oldScope);
     1974            RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(&node.treeScope() == &oldScope);
    20311975            node.setTreeScope(newScope);
    2032             if (!node.hasRareData())
     1976            if (UNLIKELY(!node.hasRareData()))
    20331977                return;
    20341978            if (auto* nodeLists = node.rareData()->nodeLists())
     
    20401984}
    20411985
    2042 void Node::didMoveToNewDocument(Document& oldDocument, Document& newDocument)
    2043 {
    2044     RELEASE_ASSERT(&document() == &newDocument);
    2045     DidMoveToNewDocumentAssertionScope::didRecieveCall(*this, oldDocument, newDocument);
    2046 
     1986void Node::moveNodeToNewDocument(Document& oldDocument, Document& newDocument)
     1987{
    20471988    newDocument.incrementReferencingNodeCount();
    20481989    oldDocument.decrementReferencingNodeCount();
     
    20511992        if (auto* nodeLists = rareData()->nodeLists())
    20521993            nodeLists->adoptDocument(oldDocument, newDocument);
     1994        if (auto* registry = mutationObserverRegistry()) {
     1995            for (auto& registration : *registry)
     1996                newDocument.addMutationObserverTypes(registration->mutationTypes());
     1997        }
     1998        if (auto* transientRegistry = transientMutationObserverRegistry()) {
     1999            for (auto& registration : *transientRegistry)
     2000                newDocument.addMutationObserverTypes(registration->mutationTypes());
     2001        }
     2002    } else {
     2003        ASSERT(!mutationObserverRegistry());
     2004        ASSERT(!transientMutationObserverRegistry());
    20532005    }
    20542006
    20552007    oldDocument.moveNodeIteratorsToNewDocument(*this, newDocument);
     2008
     2009    if (AXObjectCache::accessibilityEnabled()) {
     2010        if (auto* cache = oldDocument.existingAXObjectCache())
     2011            cache->remove(this);
     2012    }
    20562013
    20572014    if (auto* eventTargetData = this->eventTargetData()) {
     
    20602017                newDocument.addListenerTypeIfNeeded(type);
    20612018        }
    2062     }
    2063 
    2064     if (AXObjectCache::accessibilityEnabled()) {
    2065         if (auto* cache = oldDocument.existingAXObjectCache())
    2066             cache->remove(this);
    2067     }
    2068 
    2069     unsigned numWheelEventHandlers = eventListeners(eventNames().mousewheelEvent).size() + eventListeners(eventNames().wheelEvent).size();
    2070     for (unsigned i = 0; i < numWheelEventHandlers; ++i) {
    2071         oldDocument.didRemoveWheelEventHandler(*this);
    2072         newDocument.didAddWheelEventHandler(*this);
    2073     }
    2074 
    2075     unsigned numTouchEventListeners = 0;
    2076     for (auto& name : eventNames().touchEventNames())
    2077         numTouchEventListeners += eventListeners(name).size();
    2078 
    2079     for (unsigned i = 0; i < numTouchEventListeners; ++i) {
    2080         oldDocument.didRemoveTouchEventHandler(*this);
    2081         newDocument.didAddTouchEventHandler(*this);
    2082 
     2019
     2020        unsigned numWheelEventHandlers = eventListeners(eventNames().mousewheelEvent).size() + eventListeners(eventNames().wheelEvent).size();
     2021        for (unsigned i = 0; i < numWheelEventHandlers; ++i) {
     2022            oldDocument.didRemoveWheelEventHandler(*this);
     2023            newDocument.didAddWheelEventHandler(*this);
     2024        }
     2025
     2026        unsigned numTouchEventListeners = 0;
     2027        for (auto& name : eventNames().touchEventNames())
     2028            numTouchEventListeners += eventListeners(name).size();
     2029
     2030        for (unsigned i = 0; i < numTouchEventListeners; ++i) {
     2031            oldDocument.didRemoveTouchEventHandler(*this);
     2032            newDocument.didAddTouchEventHandler(*this);
    20832033#if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS)
    2084         oldDocument.removeTouchEventListener(*this);
    2085         newDocument.addTouchEventListener(*this);
    2086 #endif
    2087     }
     2034            oldDocument.removeTouchEventListener(*this);
     2035            newDocument.addTouchEventListener(*this);
     2036#endif
     2037        }
    20882038
    20892039#if ENABLE(TOUCH_EVENTS) && ENABLE(IOS_GESTURE_EVENTS)
    2090     unsigned numGestureEventListeners = 0;
    2091     for (auto& name : eventNames().gestureEventNames())
    2092         numGestureEventListeners += eventListeners(name).size();
    2093 
    2094     for (unsigned i = 0; i < numGestureEventListeners; ++i) {
    2095         oldDocument.removeTouchEventHandler(*this);
    2096         newDocument.addTouchEventHandler(*this);
    2097     }
    2098 #endif
    2099 
    2100     if (auto* registry = mutationObserverRegistry()) {
    2101         for (auto& registration : *registry)
    2102             newDocument.addMutationObserverTypes(registration->mutationTypes());
    2103     }
    2104 
    2105     if (auto* transientRegistry = transientMutationObserverRegistry()) {
    2106         for (auto& registration : *transientRegistry)
    2107             newDocument.addMutationObserverTypes(registration->mutationTypes());
     2040        unsigned numGestureEventListeners = 0;
     2041        for (auto& name : eventNames().gestureEventNames())
     2042            numGestureEventListeners += eventListeners(name).size();
     2043
     2044        for (unsigned i = 0; i < numGestureEventListeners; ++i) {
     2045            oldDocument.removeTouchEventHandler(*this);
     2046            newDocument.addTouchEventHandler(*this);
     2047        }
     2048#endif
    21082049    }
    21092050
     
    21172058#endif
    21182059#endif
     2060
     2061    if (is<Element>(*this))
     2062        downcast<Element>(*this).didMoveToNewDocument(oldDocument, newDocument);
    21192063}
    21202064
  • trunk/Source/WebCore/dom/Node.h

    r223802 r224053  
    492492    ScriptExecutionContext* scriptExecutionContext() const final; // Implemented in Document.h
    493493
    494     virtual void didMoveToNewDocument(Document& oldDocument, Document& newDocument);
    495 
    496494    bool addEventListener(const AtomicString& eventType, Ref<EventListener>&&, const AddEventListenerOptions&) override;
    497495    bool removeEventListener(const AtomicString& eventType, EventListener&, const ListenerOptions&) override;
     
    680678    void* opaqueRootSlow() const;
    681679
     680    static void moveShadowTreeToNewDocument(ShadowRoot&, Document& oldDocument, Document& newDocument);
    682681    static void moveTreeToNewScope(Node&, TreeScope& oldScope, TreeScope& newScope);
     682    void moveNodeToNewDocument(Document& oldDocument, Document& newDocument);
    683683
    684684    int m_refCount;
  • trunk/Source/WebCore/dom/ShadowRoot.cpp

    r223802 r224053  
    101101}
    102102
    103 void ShadowRoot::didMoveToNewDocument(Document& oldDocument, Document& newDocument)
    104 {
    105     ASSERT_WITH_SECURITY_IMPLICATION(&document() == &oldDocument || &document() == &newDocument);
     103void ShadowRoot::moveShadowRootToNewParentScope(TreeScope& newScope, Document& newDocument)
     104{
     105    setParentTreeScope(newScope);
     106    moveShadowRootToNewDocument(newDocument);
     107}
     108
     109void ShadowRoot::moveShadowRootToNewDocument(Document& newDocument)
     110{
    106111    setDocumentScope(newDocument);
    107     ASSERT_WITH_SECURITY_IMPLICATION(&document() == &newDocument);
    108     ASSERT_WITH_SECURITY_IMPLICATION(&m_styleScope->document() == &oldDocument);
     112    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!parentTreeScope() || &parentTreeScope()->documentScope() == &newDocument);
    109113
    110114    // Style scopes are document specific.
    111115    m_styleScope = std::make_unique<Style::Scope>(*this);
    112 
    113     DocumentFragment::didMoveToNewDocument(oldDocument, newDocument);
     116    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(&m_styleScope->document() == &newDocument);
    114117}
    115118
  • trunk/Source/WebCore/dom/ShadowRoot.h

    r223802 r224053  
    8282    const Vector<Node*>* assignedNodesForSlot(const HTMLSlotElement&);
    8383
     84    void moveShadowRootToNewParentScope(TreeScope&, Document&);
     85    void moveShadowRootToNewDocument(Document&);
     86
    8487protected:
    8588    ShadowRoot(Document&, ShadowRootMode);
     
    97100    Node::InsertedIntoAncestorResult insertedIntoAncestor(InsertionType, ContainerNode&) override;
    98101    void removedFromAncestor(RemovalType, ContainerNode& insertionPoint) override;
    99     void didMoveToNewDocument(Document& oldDocument, Document& newDocument) override;
    100102
    101103    bool m_resetStyleInheritance { false };
Note: See TracChangeset for help on using the changeset viewer.