Changeset 226095 in webkit

Timestamp:

Dec 18, 2017 8:32:43 PM (6 years ago)

Author:

rniwa@webkit.org

Message:

Assertion hit in DocumentOrderedMap::get while removing a form element
​https://bugs.webkit.org/show_bug.cgi?id=137959
<rdar://problem/27702012>

Reviewed by Brent Fulgham.

Source/WebCore:

The assertion failure was caused by FormAssociatedElement::findAssociatedForm calling TreeScope::getElementById
for a form associated element inside FormAttributeTargetObserver::idTargetChanged during the removal of
the owner form element, or the first non-form element with the matching ID. If there are other elements with
the same ID in the removed tree at that moment, MapEntry's count for the ID can be higher than it needs to be
since Element::removedFromAncestor has not been called on those elements yet.

Fixed the bug by checking this condition explicitly. This patch introduces ContainerChildRemovalScope which
keeps track of the container node from which a subtree was removed as well as the root of the removed subtree.
DocumentOrderedMap::get then checks whether the matching element can be found in this removed subtree, and its
isConnected() still returns true (the evidence that Element::removedFromAncestor has not been called) when
count > 0 and there was no matching element in the tree scope.

In the long term, we should refactor the way FormAssociatedElement and HTMLFormElement refers to each other
and avoid calling DocumentOrderedMap::get before finish calling removedFromAncestor on the removed subtree.

Tests: fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5.html

fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6.html

• dom/ContainerNodeAlgorithms.cpp:

(WebCore::notifyChildNodeRemoved):

• dom/ContainerNodeAlgorithms.h:

(WebCore::ContainerChildRemovalScope): Added.
(WebCore::ContainerChildRemovalScope::ContainerChildRemovalScope):
(WebCore::ContainerChildRemovalScope::~ContainerChildRemovalScope):
(WebCore::ContainerChildRemovalScope::parentOfRemovedTree):
(WebCore::ContainerChildRemovalScope::removedChild):
(WebCore::ContainerChildRemovalScope::currentScope):

• dom/DocumentOrderedMap.cpp:

(WebCore::DocumentOrderedMap::get const): Added a special early exit when this function is called during
a node removal.

LayoutTests:

Added regression tests for removing a subtree with a form associated element, its owner form element
and another element with the same ID.

• fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5-expected.txt: Added.
• fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5.html: Added.
• fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6-expected.txt: Added.
• fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5-expected.txt (added)
• LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5.html (added)
• LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6-expected.txt (added)
• LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/ContainerNodeAlgorithms.cpp (modified) (2 diffs)
• Source/WebCore/dom/ContainerNodeAlgorithms.h (modified) (1 diff)
• Source/WebCore/dom/DocumentOrderedMap.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2017-12-18  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Assertion hit in DocumentOrderedMap::get while removing a form element
+        https://bugs.webkit.org/show_bug.cgi?id=137959
+        <rdar://problem/27702012>
+
+        Reviewed by Brent Fulgham.
+
+        Added regression tests for removing a subtree with a form associated element, its owner form element
+        and another element with the same ID.
+
+        * fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5-expected.txt: Added.
+        * fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5.html: Added.
+        * fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6-expected.txt: Added.
+        * fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6.html: Added.
+
 2017-12-18  Youenn Fablet  <youenn@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-12-18  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Assertion hit in DocumentOrderedMap::get while removing a form element
+        https://bugs.webkit.org/show_bug.cgi?id=137959
+        <rdar://problem/27702012>
+
+        Reviewed by Brent Fulgham.
+
+        The assertion failure was caused by FormAssociatedElement::findAssociatedForm calling TreeScope::getElementById
+        for a form associated element inside FormAttributeTargetObserver::idTargetChanged during the removal of
+        the owner form element, or the first non-form element with the matching ID. If there are other elements with
+        the same ID in the removed tree at that moment, MapEntry's count for the ID can be higher than it needs to be
+        since Element::removedFromAncestor has not been called on those elements yet.
+
+        Fixed the bug by checking this condition explicitly. This patch introduces ContainerChildRemovalScope which
+        keeps track of the container node from which a subtree was removed as well as the root of the removed subtree.
+        DocumentOrderedMap::get then checks whether the matching element can be found in this removed subtree, and its
+        isConnected() still returns true (the evidence that Element::removedFromAncestor has not been called) when
+        count > 0 and there was no matching element in the tree scope.
+
+        In the long term, we should refactor the way FormAssociatedElement and HTMLFormElement refers to each other
+        and avoid calling DocumentOrderedMap::get before finish calling removedFromAncestor on the removed subtree.
+
+        Tests: fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5.html
+               fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6.html
+
+        * dom/ContainerNodeAlgorithms.cpp:
+        (WebCore::notifyChildNodeRemoved):
+        * dom/ContainerNodeAlgorithms.h:
+        (WebCore::ContainerChildRemovalScope): Added.
+        (WebCore::ContainerChildRemovalScope::ContainerChildRemovalScope):
+        (WebCore::ContainerChildRemovalScope::~ContainerChildRemovalScope):
+        (WebCore::ContainerChildRemovalScope::parentOfRemovedTree):
+        (WebCore::ContainerChildRemovalScope::removedChild):
+        (WebCore::ContainerChildRemovalScope::currentScope):
+        * dom/DocumentOrderedMap.cpp:
+        (WebCore::DocumentOrderedMap::get const): Added a special early exit when this function is called during
+        a node removal.
+
 2017-12-18  Timothy Hatcher  <timothy@hatcher.name>
 

trunk/Source/WebCore/dom/ContainerNodeAlgorithms.cpp

@@ -36 +36 @@
 namespace WebCore {
 
+#if !ASSERT_DISABLED
+ContainerChildRemovalScope* ContainerChildRemovalScope::s_scope = nullptr;
+#endif
+
 enum class TreeScopeChange { Changed, DidNotChange };
 
@@ -150 +154 @@
     // Assert that the caller of this function has an instance of NoEventDispatchAssertion.
     ASSERT(!isMainThread() || !NoEventDispatchAssertion::InMainThread::isEventAllowed());
+    ContainerChildRemovalScope removalScope(oldParentOfRemovedTree, child);
 
     // Tree scope has changed if the container node from which "node" is removed is in a document or a shadow root.

trunk/Source/WebCore/dom/ContainerNodeAlgorithms.h

@@ -27 +27 @@
 namespace WebCore {
 
+// FIXME: Delete this class after fixing FormAssociatedElement to avoid calling getElementById during a tree removal.
+#if !ASSERT_DISABLED
+class ContainerChildRemovalScope {
+public:
+    ContainerChildRemovalScope(ContainerNode& parentOfRemovedTree, Node& child)
+        : m_parentOfRemovedTree(parentOfRemovedTree)
+        , m_removedChild(child)
+        , m_previousScope(s_scope)
+    {
+        s_scope = this;
+    }
+
+    ~ContainerChildRemovalScope()
+    {
+        s_scope = m_previousScope;
+    }
+
+    ContainerNode& parentOfRemovedTree() { return m_parentOfRemovedTree; }
+    Node& removedChild() { return m_removedChild; }
+
+    static ContainerChildRemovalScope* currentScope() { return s_scope; }
+
+private:
+    ContainerNode& m_parentOfRemovedTree;
+    Node& m_removedChild;
+    ContainerChildRemovalScope* m_previousScope;
+    static ContainerChildRemovalScope* s_scope;
+};
+#else
+class ContainerChildRemovalScope {
+public:
+    ContainerChildRemovalScope(ContainerNode&, Node&) { }
+};
+#endif
+
 NodeVector notifyChildNodeInserted(ContainerNode& parentOfInsertedTree, Node&);
 void notifyChildNodeRemoved(ContainerNode& oldParentOfRemovedTree, Node&);

trunk/Source/WebCore/dom/DocumentOrderedMap.cpp

@@ -32 +32 @@
 #include "DocumentOrderedMap.h"
 
+#include "ContainerNodeAlgorithms.h"
 #include "ElementIterator.h"
 #include "HTMLImageElement.h"
@@ -121 +122 @@
         return &element;
     }
+
+#if !ASSERT_DISABLED
+    // FormAssociatedElement may call getElementById to find its owner form in the middle of a tree removal.
+    if (auto* currentScope = ContainerChildRemovalScope::currentScope()) {
+        ASSERT(&scope.rootNode() == &currentScope->parentOfRemovedTree().rootNode());
+        Node& removedTree = currentScope->removedChild();
+        ASSERT(is<ContainerNode>(removedTree));
+        for (auto& element : descendantsOfType<Element>(downcast<ContainerNode>(removedTree))) {
+            if (!keyMatches(key, element))
+                continue;
+            bool removedFromAncestorHasNotBeenCalledYet = element.isConnected();
+            ASSERT(removedFromAncestorHasNotBeenCalledYet);
+            return nullptr;
+        }
+    }
     ASSERT_NOT_REACHED();
+#endif
+
     return nullptr;
 }