imported/w3c/web-platform-tests/shadow-dom/form-control-form-attribute.html hits...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Sep 2018 22:36:51 +0000 (22:36 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Sep 2018 22:36:51 +0000 (22:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189493

Reviewed by Alex Christensen.

Source/WebCore:

The debug assertion was caused by RefPtr in FormAssociatedElement::formOwnerRemovedFromTree introduced
by r224390 and r223644 ref'ing ShadowRoot while calling removeDetachedChildren inside ~ShadowRoot.
When a form (or any other) element has more than one ref inside removeDetachedChildren,
addChildNodesToDeletionQueue calls notifyChildNodeRemoved in the tree oreder.

However, when a form associated element of this form element appears later in the tree order,
FormAssociatedElement::formOwnerRemovedFromTree can traverse up ancestors including the ShadowRoot.

Fixed the bug by using raw pointers instead. Luckily, there is no DOM mutations or other non-trivial
operations happening in this function so this should be safe.

Test: imported/w3c/web-platform-tests/shadow-dom/form-control-form-attribute.html

* html/FormAssociatedElement.cpp:
(WebCore::FormAssociatedElement::formOwnerRemovedFromTree): Fixed the bug.

LayoutTests:

Unskip the test now that it doesn't hit a debug assertion.

* TestExpectations:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@235956 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/html/FormAssociatedElement.cpp

index ab4cc27..60c1daa 100644 (file)
@@ -1,3 +1,14 @@
+2018-09-11  Ryosuke Niwa  <rniwa@webkit.org>
+
+        imported/w3c/web-platform-tests/shadow-dom/form-control-form-attribute.html hits assertion
+        https://bugs.webkit.org/show_bug.cgi?id=189493
+
+        Reviewed by Alex Christensen.
+
+        Unskip the test now that it doesn't hit a debug assertion.
+
+        * TestExpectations:
+
 2018-09-12  Dan Bernstein  <mitz@apple.com>
 
         [Cocoa] Complete support for Paste as Quotation
index 4196cfe..294edd9 100644 (file)
@@ -2237,8 +2237,6 @@ webkit.org/b/187269 [ Debug ] imported/w3c/web-platform-tests/FileAPI/reading-da
 
 webkit.org/b/185308 legacy-animation-engine/animations/combo-transform-translate+scale.html [ Pass Failure ]
 
-webkit.org/b/189493 imported/w3c/web-platform-tests/shadow-dom/form-control-form-attribute.html [ Skip ]
-
 fast/gradients/conic-repeating.html [ Skip ]
 fast/gradients/conic.html [ Skip ]
 fast/gradients/conic-off-center.html [ Skip ]
index 888ca18..f024f6b 100644 (file)
@@ -1,3 +1,26 @@
+2018-09-11  Ryosuke Niwa  <rniwa@webkit.org>
+
+        imported/w3c/web-platform-tests/shadow-dom/form-control-form-attribute.html hits assertion
+        https://bugs.webkit.org/show_bug.cgi?id=189493
+
+        Reviewed by Alex Christensen.
+
+        The debug assertion was caused by RefPtr in FormAssociatedElement::formOwnerRemovedFromTree introduced
+        by r224390 and r223644 ref'ing ShadowRoot while calling removeDetachedChildren inside ~ShadowRoot.
+        When a form (or any other) element has more than one ref inside removeDetachedChildren,
+        addChildNodesToDeletionQueue calls notifyChildNodeRemoved in the tree oreder.
+
+        However, when a form associated element of this form element appears later in the tree order,
+        FormAssociatedElement::formOwnerRemovedFromTree can traverse up ancestors including the ShadowRoot.
+
+        Fixed the bug by using raw pointers instead. Luckily, there is no DOM mutations or other non-trivial
+        operations happening in this function so this should be safe.
+
+        Test: imported/w3c/web-platform-tests/shadow-dom/form-control-form-attribute.html
+
+        * html/FormAssociatedElement.cpp:
+        (WebCore::FormAssociatedElement::formOwnerRemovedFromTree): Fixed the bug.
+
 2018-09-12  Dan Bernstein  <mitz@apple.com>
 
         [Cocoa] Complete support for Paste as Quotation
index 8575293..7aa5e76 100644 (file)
@@ -122,8 +122,9 @@ HTMLFormElement* FormAssociatedElement::findAssociatedForm(const HTMLElement* el
 void FormAssociatedElement::formOwnerRemovedFromTree(const Node& formRoot)
 {
     ASSERT(m_form);
-    RefPtr<Node> rootNode = &asHTMLElement();
-    for (auto ancestor = makeRefPtr(asHTMLElement().parentNode()); ancestor; ancestor = ancestor->parentNode()) {
+    // Can't use RefPtr here beacuse this function might be called inside ~ShadowRoot via addChildNodesToDeletionQueue. See webkit.org/b/189493.
+    Node* rootNode = &asHTMLElement();
+    for (auto* ancestor = asHTMLElement().parentNode(); ancestor; ancestor = ancestor->parentNode()) {
         if (ancestor == m_form) {
             // Form is our ancestor so we don't need to reset our owner, we also no longer
             // need an id observer since we are no longer connected.