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.

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

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index e9515557d70a8aeacc079a29ec9faee362dee759..9458406ec277514c899b5cc50293572271acfe4d 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,12 @@
+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>
 
         Reviewed by Adam.

diff --git a/LayoutTests/editing/deleting/5546763-expected.txt b/LayoutTests/editing/deleting/5546763-expected.txt
new file mode 100644 (file)
index 0000000..9be22c4
--- /dev/null
+++ b/LayoutTests/editing/deleting/5546763-expected.txt
@@ -0,0 +1,14 @@
+EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of DIV > BODY > HTML > #document to 5 of DIV > BODY > HTML > #document
+EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 3 of #text > A > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 3 of #text > A > DIV > DIV > BODY > HTML > #document toDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 3 of #text > A > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: shouldDeleteDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 3 of #text > A > DIV > DIV > BODY > HTML > #document
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of DIV > DIV > BODY > HTML > #document to 0 of DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+This tests for a crash when deleting a selection that starts before the first child of a block and ends after the last child (which must be a link) of another block. There shouldn't be any content in the editable region below.
+
+
+

diff --git a/LayoutTests/editing/deleting/5546763.html b/LayoutTests/editing/deleting/5546763.html
new file mode 100644 (file)
index 0000000..b765e00
--- /dev/null
+++ b/LayoutTests/editing/deleting/5546763.html
@@ -0,0 +1,16 @@
+<p>This tests for a crash when deleting a selection that starts before the first child of a block and ends after the last child (which must be a link) of another block. There shouldn't be any content in the editable region below.</p>
+<div id="div" contenteditable="true">
+<div>foo</div>
+<div>bar <a href="http://www.apple.com/">baz</a></div>
+</div>
+
+<script>
+if (window.layoutTestController) {
+    window.layoutTestController.dumpEditingCallbacks();
+    window.layoutTestController.dumpAsText();
+}
+div = document.getElementById("div");
+div.focus();
+document.execCommand("SelectAll");
+document.execCommand("Delete");
+</script>

diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index e23e859bb21208d066391be12e111342afd6612d..1cee7a0309726fa04c9f80f4417e6d03b236e243 100644 (file)
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,3 +1,21 @@
+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>
 
         Reviewed by Adam.

diff --git a/WebCore/editing/DeleteSelectionCommand.cpp b/WebCore/editing/DeleteSelectionCommand.cpp
index e2fe7d6a6d2c0b5175d243c2514c3c4aa6bff182..1a2f4b9704fbb575db9a6f19b416eae8a0e99477 100644 (file)
--- a/WebCore/editing/DeleteSelectionCommand.cpp
+++ b/WebCore/editing/DeleteSelectionCommand.cpp
@@ -446,7 +446,6 @@ void DeleteSelectionCommand::handleGeneralDelete()
         
         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());
             } else {
@@ -519,6 +518,14 @@ void DeleteSelectionCommand::mergeParagraphs()
     VisiblePosition startOfParagraphToMove(m_downstreamEnd);
     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())) {
         insertNodeAt(createBreakElement(document()).get(), m_upstreamStart);
@@ -550,7 +557,11 @@ void DeleteSelectionCommand::mergeParagraphs()
     if (!document()->frame()->editor()->client()->shouldMoveRangeAfterDelete(range.get(), rangeToBeReplaced.get()))
         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();
 }
@@ -718,13 +729,13 @@ void DeleteSelectionCommand::doApply()
     handleGeneralDelete();
     
     fixupWhitespace();
-
-    RefPtr<Node> placeholder = m_needPlaceholder ? createBreakElement(document()) : 0;
     
     mergeParagraphs();
     
     removePreviouslySelectedEmptyTableRows();
     
+    RefPtr<Node> placeholder = m_needPlaceholder ? createBreakElement(document()).get() : 0;
+    
     if (placeholder)
         insertNodeAt(placeholder.get(), m_endingPosition);