Changeset 274626 in webkit

Timestamp:

Mar 18, 2021, 2:18:40 AM (5 years ago)

Author:

commit-queue@webkit.org

Message:

ASSERTION FAILED: node.isConnected() in matchSlottedPseudoElementRules
​https://bugs.webkit.org/show_bug.cgi?id=221440

Patch by Frédéric Wang <​fwang@igalia.com> on 2021-03-18
Reviewed by Ryosuke Niwa.

ReplaceSelectionCommand::doApply() removes a <br> from an element and immediately calls
highestNodeToRemoveInPruning() on that element. The former operation may destroy the
element's renderer and confuses the latter operation. This happens in particular for a
<summary> element which ends up being removed from the tree. This in turn causes unexpected
issues such as a debug assertion failure in matchSlottedPseudoElementRules. To address that
problem, ensure the document is laid out before calling highestNodeToRemoveInPruning().
This patch also increases and improves use of RefPtr<Node>.

• editing/CompositeEditCommand.cpp:

(WebCore::CompositeEditCommand::removeNodeAndPruneAncestors): Use auto & makeRefPtr.
(WebCore::CompositeEditCommand::prune): Store local highestNodeToRemove variable in a RefPtr.
(WebCore::CompositeEditCommand::cleanupAfterDeletion): Store local node variable in a RefPtr.
(WebCore::CompositeEditCommand::breakOutOfEmptyMailBlockquotedParagraph): Store local parentNode variable in a RefPtr.

• editing/Editing.cpp:

(WebCore::highestNodeToRemoveInPruning): Store local currentNode variable in a a RefPtr.

• editing/ReplaceSelectionCommand.cpp:

(WebCore::ReplaceSelectionCommand::doApply): Use auto & makeRefPtr. Store local odeToRemove variable in a RefPtr.
Ensure the document is laid out before calling highestNodeToRemoveInPruning.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• editing/CompositeEditCommand.cpp (modified) (5 diffs)
• editing/Editing.cpp (modified) (1 diff)
• editing/ReplaceSelectionCommand.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-03-18  Frédéric Wang  <fwang@igalia.com>
+
+        ASSERTION FAILED: node.isConnected() in matchSlottedPseudoElementRules
+        https://bugs.webkit.org/show_bug.cgi?id=221440
+
+        Reviewed by Ryosuke Niwa.
+
+        ReplaceSelectionCommand::doApply() removes a <br> from an element and immediately calls
+        highestNodeToRemoveInPruning() on that element. The former operation may destroy the
+        element's renderer and confuses the latter operation. This happens in particular for a
+        <summary> element which ends up being removed from the tree. This in turn causes unexpected
+        issues such as a debug assertion failure in matchSlottedPseudoElementRules. To address that
+        problem, ensure the document is laid out before calling highestNodeToRemoveInPruning().
+        This patch also increases and improves use of RefPtr<Node>.
+
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::removeNodeAndPruneAncestors): Use auto & makeRefPtr.
+        (WebCore::CompositeEditCommand::prune): Store local highestNodeToRemove variable in a RefPtr.
+        (WebCore::CompositeEditCommand::cleanupAfterDeletion): Store local node variable in a RefPtr.
+        (WebCore::CompositeEditCommand::breakOutOfEmptyMailBlockquotedParagraph): Store local parentNode variable in a RefPtr.
+        * editing/Editing.cpp:
+        (WebCore::highestNodeToRemoveInPruning): Store local currentNode variable in a  a RefPtr.
+        * editing/ReplaceSelectionCommand.cpp:
+        (WebCore::ReplaceSelectionCommand::doApply): Use auto & makeRefPtr. Store local odeToRemove variable in a  RefPtr.
+        Ensure the document is laid out before calling highestNodeToRemoveInPruning.
+
 2021-03-18  Devin Rousso  <drousso@apple.com>
 

trunk/Source/WebCore/editing/CompositeEditCommand.cpp

@@ -613 +613 @@
 void CompositeEditCommand::removeNodeAndPruneAncestors(Node& node)
 {
-    RefPtr<ContainerNode> parent = node.parentNode();
+    auto parent = makeRefPtr(node.parentNode());
     removeNode(node);
     prune(parent.get());
@@ -657 +657 @@
 void CompositeEditCommand::prune(Node* node)
 {
-    if (auto* highestNodeToRemove = highestNodeToRemoveInPruning(node))
+    if (auto highestNodeToRemove = makeRefPtr(highestNodeToRemoveInPruning(node)))
         removeNode(*highestNodeToRemove);
 }
@@ -1298 +1298 @@
         // Note: We want the rightmost candidate.
         Position position = caretAfterDelete.deepEquivalent().downstream();
-        Node* node = position.deprecatedNode();
+        auto node = makeRefPtr(position.deprecatedNode());
         ASSERT(node);
         // Normally deletion will leave a br as a placeholder.
@@ -1307 +1307 @@
         // div or an li), remove it during the move (the list removal code
         // expects this behavior).
-        else if (isBlock(node)) {
+        else if (isBlock(node.get())) {
             // If caret position after deletion and destination position coincides,
             // node should not be removed.
             if (!position.rendersInDifferentPosition(destination.deepEquivalent())) {
-                prune(node);
+                prune(node.get());
                 return;
             }
@@ -1628 +1628 @@
         ASSERT(caretPos.deprecatedEditingOffset() == 0);
         Text& textNode = downcast<Text>(*caretPos.deprecatedNode());
-        ContainerNode* parentNode = textNode.parentNode();
+        auto parentNode = makeRefPtr(textNode.parentNode());
         // The preserved newline must be the first thing in the node, since otherwise the previous
         // paragraph would be quoted, and we verified that it wasn't above.
         deleteTextFromNode(textNode, 0, 1);
-        prune(parentNode);
+        prune(parentNode.get());
     }
 

trunk/Source/WebCore/editing/Editing.cpp

@@ -629 +629 @@
     Node* previousNode = nullptr;
     auto* rootEditableElement = node ? node->rootEditableElement() : nullptr;
-    for (; node; node = node->parentNode()) {
-        if (auto* renderer = node->renderer()) {
-            if (!renderer->canHaveChildren() || hasARenderedDescendant(node, previousNode) || rootEditableElement == node)
+    for (auto currentNode = makeRefPtr(node); currentNode; currentNode = currentNode->parentNode()) {
+        if (auto* renderer = currentNode->renderer()) {
+            if (!renderer->canHaveChildren() || hasARenderedDescendant(currentNode.get(), previousNode) || rootEditableElement == currentNode.get())
                 return previousNode;
         }
-        previousNode = node;
+        previousNode = currentNode.get();
     }
     return nullptr;

trunk/Source/WebCore/editing/ReplaceSelectionCommand.cpp

@@ -1298 +1298 @@
 
     if (endBR && (plainTextFragment || shouldRemoveEndBR(endBR.get(), originalVisPosBeforeEndBR))) {
-        RefPtr<Node> parent = endBR->parentNode();
+        auto parent = makeRefPtr(endBR->parentNode());
         insertedNodes.willRemoveNode(endBR.get());
         removeNode(*endBR);
-        if (Node* nodeToRemove = highestNodeToRemoveInPruning(parent.get())) {
-            insertedNodes.willRemoveNode(nodeToRemove);
+        document().updateLayoutIgnorePendingStylesheets();
+        if (auto nodeToRemove = makeRefPtr(highestNodeToRemoveInPruning(parent.get()))) {
+            insertedNodes.willRemoveNode(nodeToRemove.get());
             removeNode(*nodeToRemove);
         }