[webkit-changes] [226095] trunk

Mon, 18 Dec 2017 20:32:58 -0800

Title: [226095] trunk
Revision
226095
Author
rn...@webkit.org
Date
2017-12-18 20:32:43 -0800 (Mon, 18 Dec 2017)

Log 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.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (226094 => 226095)


--- trunk/LayoutTests/ChangeLog	2017-12-19 04:09:11 UTC (rev 226094)
+++ trunk/LayoutTests/ChangeLog	2017-12-19 04:32:43 UTC (rev 226095)
@@ -1,3 +1,19 @@
+2017-12-18  Ryosuke Niwa  <rn...@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  <you...@apple.com>
 
         Service worker served response tainting should keep its tainting

Added: trunk/LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5-expected.txt (0 => 226095)


--- trunk/LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5-expected.txt	2017-12-19 04:32:43 UTC (rev 226095)
@@ -0,0 +1,4 @@
+This tests removing a subtree containing a form associated element with two non-form elements with matching ID.
+The test passses if WebKit doesn't hit a debug assertion.
+
+PASS.

Added: trunk/LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5.html (0 => 226095)


--- trunk/LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5.html	                        (rev 0)
+++ trunk/LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-5.html	2017-12-19 04:32:43 UTC (rev 226095)
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<body>
+<a>
+    <p>
+        <b>
+            <u id="test"/>
+            <keygen form="test"/>
+        </b>
+</a>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+document.body.innerText = `This tests removing a subtree containing a form associated element with two non-form elements with matching ID.
+The test passses if WebKit doesn't hit a debug assertion.
+
+PASS.`;
+</script>

Added: trunk/LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6-expected.txt (0 => 226095)


--- trunk/LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6-expected.txt	2017-12-19 04:32:43 UTC (rev 226095)
@@ -0,0 +1,3 @@
+This tests removing a subtree containing a form associated element with two form elements with matching ID.
+The test passses if WebKit doesn't hit a debug assertion.
+PASS.

Added: trunk/LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6.html (0 => 226095)


--- trunk/LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6.html	                        (rev 0)
+++ trunk/LayoutTests/fast/forms/update-form-owner-in-moved-subtree-assertion-failure-6.html	2017-12-19 04:32:43 UTC (rev 226095)
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html>
+<body>
+<div id="container">
+    <form id="test"></form>
+    <form id="test"></form>
+    <input form="test">
+</div>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+container.remove();
+document.write(`This tests removing a subtree containing a form associated element with two form elements with matching ID.<br>
+The test passses if WebKit doesn't hit a debug assertion.<br>
+PASS.`);
+</script>
+</body>
+</html>

Modified: trunk/Source/WebCore/ChangeLog (226094 => 226095)


--- trunk/Source/WebCore/ChangeLog	2017-12-19 04:09:11 UTC (rev 226094)
+++ trunk/Source/WebCore/ChangeLog	2017-12-19 04:32:43 UTC (rev 226095)
@@ -1,3 +1,42 @@
+2017-12-18  Ryosuke Niwa  <rn...@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  <timo...@hatcher.name>
 
         [GTK][WPE] Conditionalize libTASN1 use behind ENABLE_SUBTLE_CRYPTO in the CMake files

Modified: trunk/Source/WebCore/dom/ContainerNodeAlgorithms.cpp (226094 => 226095)


--- trunk/Source/WebCore/dom/ContainerNodeAlgorithms.cpp	2017-12-19 04:09:11 UTC (rev 226094)
+++ trunk/Source/WebCore/dom/ContainerNodeAlgorithms.cpp	2017-12-19 04:32:43 UTC (rev 226095)
@@ -35,6 +35,10 @@
 
 namespace WebCore {
 
+#if !ASSERT_DISABLED
+ContainerChildRemovalScope* ContainerChildRemovalScope::s_scope = nullptr;
+#endif
+
 enum class TreeScopeChange { Changed, DidNotChange };
 
 static void notifyNodeInsertedIntoDocument(ContainerNode& parentOfInsertedTree, Node& node, TreeScopeChange treeScopeChange, NodeVector& postInsertionNotificationTargets)
@@ -149,6 +153,7 @@
 {
     // 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.
     auto treeScopeChange = oldParentOfRemovedTree.isInTreeScope() ? TreeScopeChange::Changed : TreeScopeChange::DidNotChange;

Modified: trunk/Source/WebCore/dom/ContainerNodeAlgorithms.h (226094 => 226095)


--- trunk/Source/WebCore/dom/ContainerNodeAlgorithms.h	2017-12-19 04:09:11 UTC (rev 226094)
+++ trunk/Source/WebCore/dom/ContainerNodeAlgorithms.h	2017-12-19 04:32:43 UTC (rev 226095)
@@ -26,6 +26,41 @@
 
 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&);
 void removeDetachedChildrenInContainer(ContainerNode&);

Modified: trunk/Source/WebCore/dom/DocumentOrderedMap.cpp (226094 => 226095)


--- trunk/Source/WebCore/dom/DocumentOrderedMap.cpp	2017-12-19 04:09:11 UTC (rev 226094)
+++ trunk/Source/WebCore/dom/DocumentOrderedMap.cpp	2017-12-19 04:32:43 UTC (rev 226095)
@@ -31,6 +31,7 @@
 #include "config.h"
 #include "DocumentOrderedMap.h"
 
+#include "ContainerNodeAlgorithms.h"
 #include "ElementIterator.h"
 #include "HTMLImageElement.h"
 #include "HTMLLabelElement.h"
@@ -120,7 +121,24 @@
         ASSERT_WITH_SECURITY_IMPLICATION(entry.registeredElements.contains(entry.element));
         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;
 }
 

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to