Changeset 83322 in webkit

Timestamp:

Apr 8, 2011 12:29:13 PM (13 years ago)

Author:

rniwa@webkit.org

Message:

2011-04-08 Ryosuke Niwa <​rniwa@webkit.org>

Reviewed by Tony Chang, Darin Adler, and Enrica Casucci.

REGRESSION(r81887): Crash in SplitElement
​https://bugs.webkit.org/show_bug.cgi?id=57743

The crash was caused by ReplaceSelectionCommand::doApply's calling splitElement with computeNodeAfterPosition
even when the position was after the last node in it container. Since all we are doing here is to splitting tree
up until the highest ancestor with isInlineNodeWithStyle, replaced the while loop by calls to splitTreeToNode
and highestEnclosingNodeOfType.

Also fixed a bug in splitTreeToNode not to check the difference in visible position when splitting the ancestor,
which would have introduced unnecessary nodes when splitting tree and a bug in highestEnclosingNodeOfType that
it incorrectly called deprecatedNode instead of containerNode.

Test: editing/inserting/insert-images-in-pre-x-crash.html

• editing/CompositeEditCommand.cpp: (WebCore::CompositeEditCommand::splitTreeToNode):
• editing/ReplaceSelectionCommand.cpp: (WebCore::ReplaceSelectionCommand::doApply):
• editing/htmlediting.cpp: (WebCore::highestEnclosingNodeOfType):

2011-04-08 Ryosuke Niwa <​rniwa@webkit.org>

Reviewed by Tony Chang, Darin Adler, and Enrica Casucci.

REGRESSION(r81887): Crash in SplitElement
​https://bugs.webkit.org/show_bug.cgi?id=57743

Added a regression test for a crash in ReplaceSelectionCommand.

• editing/inserting/insert-images-in-pre-x-crash-expected.txt: Added.
• editing/inserting/insert-images-in-pre-x-crash.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/editing/inserting/insert-images-in-pre-x-crash-expected.txt (added)
• LayoutTests/editing/inserting/insert-images-in-pre-x-crash.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/editing/CompositeEditCommand.cpp (modified) (1 diff)
• Source/WebCore/editing/ReplaceSelectionCommand.cpp (modified) (1 diff)
• Source/WebCore/editing/htmlediting.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2011-04-08  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Tony Chang, Darin Adler, and Enrica Casucci.
+
+        REGRESSION(r81887): Crash in SplitElement
+        https://bugs.webkit.org/show_bug.cgi?id=57743
+
+        Added a regression test for a crash in ReplaceSelectionCommand.
+
+        * editing/inserting/insert-images-in-pre-x-crash-expected.txt: Added.
+        * editing/inserting/insert-images-in-pre-x-crash.html: Added.
+
 2011-04-08  Antti Koivisto  <antti@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-04-08  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Tony Chang, Darin Adler, and Enrica Casucci.
+
+        REGRESSION(r81887): Crash in SplitElement
+        https://bugs.webkit.org/show_bug.cgi?id=57743
+
+        The crash was caused by ReplaceSelectionCommand::doApply's calling splitElement with computeNodeAfterPosition
+        even when the position was after the last node in it container. Since all we are doing here is to splitting tree
+        up until the highest ancestor with isInlineNodeWithStyle, replaced the while loop by calls to splitTreeToNode
+        and highestEnclosingNodeOfType.
+
+        Also fixed a bug in splitTreeToNode not to check the difference in visible position when splitting the ancestor,
+        which would have introduced unnecessary nodes when splitting tree and a bug in highestEnclosingNodeOfType that
+        it incorrectly called deprecatedNode instead of containerNode.
+
+        Test: editing/inserting/insert-images-in-pre-x-crash.html
+
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::splitTreeToNode):
+        * editing/ReplaceSelectionCommand.cpp:
+        (WebCore::ReplaceSelectionCommand::doApply):
+        * editing/htmlediting.cpp:
+        (WebCore::highestEnclosingNodeOfType):
+
 2011-04-08  Antti Koivisto  <antti@apple.com>
 

trunk/Source/WebCore/editing/CompositeEditCommand.cpp

@@ -1218 +1218 @@
 // Splits the tree parent by parent until we reach the specified ancestor. We use VisiblePositions
 // to determine if the split is necessary. Returns the last split node.
-PassRefPtr<Node> CompositeEditCommand::splitTreeToNode(Node* start, Node* end, bool splitAncestor)
-{
+PassRefPtr<Node> CompositeEditCommand::splitTreeToNode(Node* start, Node* end, bool shouldSplitAncestor)
+{
+    ASSERT(start);
+    ASSERT(end);
     ASSERT(start != end);
 
     RefPtr<Node> node;
-    for (node = start; node && node->parentNode() != end; node = node->parentNode()) {
+    if (shouldSplitAncestor && end->parentNode())
+        end = end->parentNode();
+
+    RefPtr<Node> endNode = end;
+    for (node = start; node && node->parentNode() != endNode; node = node->parentNode()) {
         if (!node->parentNode()->isElementNode())
             break;
-        VisiblePosition positionInParent(firstPositionInNode(node->parentNode()), DOWNSTREAM);
-        VisiblePosition positionInNode(firstPositionInOrBeforeNode(node.get()), DOWNSTREAM);
+        // Do not split a node when doing so introduces an empty node.
+        VisiblePosition positionInParent = firstPositionInNode(node->parentNode());
+        VisiblePosition positionInNode = firstPositionInOrBeforeNode(node.get());
         if (positionInParent != positionInNode)
-            applyCommandToComposite(SplitElementCommand::create(static_cast<Element*>(node->parentNode()), node));
-    }
-    if (splitAncestor) {
-        splitElement(static_cast<Element*>(end), node);
-        return node->parentNode();
-    }
+            splitElement(static_cast<Element*>(node->parentNode()), node);
+    }
+
     return node.release();
 }

trunk/Source/WebCore/editing/ReplaceSelectionCommand.cpp

@@ -953 +953 @@
     // our style spans and for positions inside list items
     // since insertAsListItems already does the right thing.
-    if (!m_matchStyle && !enclosingList(insertionPos.anchorNode()) && isStyleSpan(fragment.firstChild())) {
-        Node* parentNode = insertionPos.anchorNode()->parentNode();
-        while (parentNode && parentNode->renderer() && isInlineNodeWithStyle(parentNode)) {
-            // If we are in the middle of a text node, we need to split it before we can
-            // move the insertion position.
-            if (insertionPos.anchorNode()->isTextNode() && insertionPos.anchorType() == Position::PositionIsOffsetInAnchor && insertionPos.offsetInContainerNode() && !insertionPos.atLastEditingPositionForNode())
-                splitTextNodeContainingElement(static_cast<Text*>(insertionPos.anchorNode()), insertionPos.offsetInContainerNode());
-            
-            // If the style element has more than one child, we need to split it.
-            if (parentNode->firstChild()->nextSibling())
-                splitElement(static_cast<Element*>(parentNode), insertionPos.computeNodeAfterPosition());
-            
-            insertionPos = positionInParentBeforeNode(parentNode);
-            parentNode = parentNode->parentNode();
+    if (!m_matchStyle && !enclosingList(insertionPos.containerNode()) && isStyleSpan(fragment.firstChild())) {
+        if (insertionPos.containerNode()->isTextNode() && insertionPos.offsetInContainerNode() && !insertionPos.atLastEditingPositionForNode()) {
+            splitTextNodeContainingElement(static_cast<Text*>(insertionPos.containerNode()), insertionPos.offsetInContainerNode());
+            insertionPos = firstPositionInNode(insertionPos.containerNode());
+        }
+
+        // FIXME: isInlineNodeWithStyle does not check editability.
+        if (RefPtr<Node> nodeToSplitTo = highestEnclosingNodeOfType(insertionPos, isInlineNodeWithStyle)) {
+            if (insertionPos.containerNode() != nodeToSplitTo)
+                nodeToSplitTo = splitTreeToNode(insertionPos.anchorNode(), nodeToSplitTo.get(), true).get();
+            insertionPos = positionInParentBeforeNode(nodeToSplitTo.get());
         }
     }

trunk/Source/WebCore/editing/htmlediting.cpp

@@ -639 +639 @@
     Node* highest = 0;
     Node* root = rule == CannotCrossEditingBoundary ? highestEditableRoot(p) : 0;
-    for (Node* n = p.deprecatedNode(); n; n = n->parentNode()) {
+    for (Node* n = p.containerNode(); n; n = n->parentNode()) {
         if (root && !n->rendererIsEditable())
             continue;