Changeset 27796 in webkit

Timestamp:

Nov 14, 2007 2:33:07 PM (16 years ago)

Author:

justin.garcia@apple.com

Message:

WebCore:

Reviewed by Alexey Proskuryakov.

<rdar://problem/5546763> CrashTracer: [USER] 362 crashes at WebCore::DeleteSelectionCommand::mergeParagraphs

• editing/DeleteSelectionCommand.cpp: (WebCore::DeleteSelectionCommand::handleGeneralDelete): Removed an irrelevant FIXME. (WebCore::DeleteSelectionCommand::mergeParagraphs): If the block that contained the end of the selection hasn't been removed but has been emptied by deletion, we would to try and fail to create a VisiblePosition inside that block, which could lead to a crash. If that happens, there's no content in the block to move, so just remove the block and return. Preserve m_needPlaceholder during the call to moveParagraphs, since it may change it and since it does its own placeholder insertion when necessary. (WebCore::DeleteSelectionCommand::doApply): No need to check m_needPlaceholder before calling mergeParagraphs, because it handles preserving m_needPlaceholder when it calls moveParagraphs.

LayoutTests:

Reviewed by Alexey Proskuryakov.

<rdar://problem/5546763> CrashTracer: [USER] 362 crashes at WebCore::DeleteSelectionCommand::mergeParagraphs

• editing/deleting/5546763-expected.txt: Added.
• editing/deleting/5546763.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/editing/deleting/5546763-expected.txt (added)
• LayoutTests/editing/deleting/5546763.html (added)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/editing/DeleteSelectionCommand.cpp (modified) (4 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2007-11-14  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by Alexey Proskuryakov.
+        
+        <rdar://problem/5546763> CrashTracer: [USER] 362 crashes at WebCore::DeleteSelectionCommand::mergeParagraphs
+
+        * editing/deleting/5546763-expected.txt: Added.
+        * editing/deleting/5546763.html: Added.
+
 2007-11-14  Anders Carlsson  <andersca@apple.com>
 

trunk/WebCore/ChangeLog

@@ +1 @@
+2007-11-14  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by Alexey Proskuryakov.
+
+        <rdar://problem/5546763> CrashTracer: [USER] 362 crashes at WebCore::DeleteSelectionCommand::mergeParagraphs
+
+        * editing/DeleteSelectionCommand.cpp:
+        (WebCore::DeleteSelectionCommand::handleGeneralDelete): 
+        Removed an irrelevant FIXME.
+        (WebCore::DeleteSelectionCommand::mergeParagraphs): If the block that contained the end of the selection
+        hasn't been removed but has been emptied by deletion, we would to try and fail to create a VisiblePosition
+        inside that block, which could lead to a crash.  If that happens, there's no content in the block to move,
+        so just remove the block and return.
+        Preserve m_needPlaceholder during the call to moveParagraphs, since it may change it and since it does
+        its own placeholder insertion when necessary.
+        (WebCore::DeleteSelectionCommand::doApply): No need to check m_needPlaceholder before calling mergeParagraphs,
+        because it handles preserving m_needPlaceholder when it calls moveParagraphs.
+        
 2007-11-14  Timothy Hatcher  <timothy@apple.com>
 

trunk/WebCore/editing/DeleteSelectionCommand.cpp

@@ -447 +447 @@
         if (m_downstreamEnd.node() != startNode && !m_upstreamStart.node()->isDescendantOf(m_downstreamEnd.node()) && m_downstreamEnd.node()->inDocument() && m_downstreamEnd.offset() >= caretMinOffset(m_downstreamEnd.node())) {
             if (m_downstreamEnd.offset() >= maxDeepOffset(m_downstreamEnd.node()) && !canHaveChildrenForEditing(m_downstreamEnd.node())) {
-                // FIXME: Shouldn't remove m_downstreamEnd.node() if its offsets refer to children. 
                 // The node itself is fully selected, not just its contents.  Delete it.
                 removeNode(m_downstreamEnd.node());
@@ -520 +519 @@
     VisiblePosition mergeDestination(m_upstreamStart);
     
+    // m_downstreamEnd's block has been emptied out by deletion.  There is no content inside of it to
+    // move, so just remove it.
+    Element* endBlock = static_cast<Element*>(enclosingBlock(m_downstreamEnd.node()));
+    if (!startOfParagraphToMove.deepEquivalent().node() || !endBlock->contains(startOfParagraphToMove.deepEquivalent().node())) {
+        removeNode(enclosingBlock(m_downstreamEnd.node()));
+        return;
+    }
+    
     // We need to merge into m_upstreamStart's block, but it's been emptied out and collapsed by deletion.
     if (!mergeDestination.deepEquivalent().node() || !mergeDestination.deepEquivalent().node()->isDescendantOf(m_upstreamStart.node()->enclosingBlockFlowElement())) {
@@ -551 +558 @@
         return;
     
+    // moveParagraphs will insert placeholders if it removes blocks that would require their use, don't let block
+    // removals that it does cause the insertion of *another* placeholder.
+    bool needPlaceholder = m_needPlaceholder;
     moveParagraph(startOfParagraphToMove, endOfParagraphToMove, mergeDestination);
+    m_needPlaceholder = needPlaceholder;
     // The endingPosition was likely clobbered by the move, so recompute it (moveParagraph selects the moved paragraph).
     m_endingPosition = endingSelection().start();
@@ -719 +730 @@
     
     fixupWhitespace();
-
-    RefPtr<Node> placeholder = m_needPlaceholder ? createBreakElement(document()) : 0;
     
     mergeParagraphs();
     
     removePreviouslySelectedEmptyTableRows();
+    
+    RefPtr<Node> placeholder = m_needPlaceholder ? createBreakElement(document()).get() : 0;
     
     if (placeholder)