2010-08-25 Ryosuke Niwa <rniwa@webkit.org>
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 25 Aug 2010 22:15:47 +0000 (22:15 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 25 Aug 2010 22:15:47 +0000 (22:15 +0000)
        Reviewed by Darin Adler.

        WebCore::InsertListCommand::modifyRange infinite loop (100% CPU usage)
        https://bugs.webkit.org/show_bug.cgi?id=33668

        The bug was caused by enclosingListChild returning a list child whose enclosing list is
        a sibling of the current list child. Fixed enclosingListChild to traverse upwards
        in the DOM to find the list child which is a sibling of the current list child.
        Also fixed adjacentEnclosingList to only returns the list that belongs to the same outer list.

        In doApplyForSingleParagraph, if the start or the end of currentSelection existed inside a list content
        moved by moveParagraphWithClones, either end could point to a wrong position after the move.
        Fixed this problem by checking this condition upfront and restoring later.

        In doApply, if moveParagraph or moveParagraphWithClones, endOfSelection or startOfLastParagraph
        could be null or orphaned, fixed this problem by indexForVisiblePosition.

        Test: editing/execCommand/insert-list-orphaned-item-with-nested-lists.html

        * editing/InsertListCommand.cpp:
        (WebCore::InsertListCommand::doApply):
        (WebCore::enclosingListChild):
        (WebCore::InsertListCommand::doApplyForSingleParagraph):
        (WebCore::adjacentEnclosingList):
        (WebCore::InsertListCommand::listifyParagraph):
2010-08-25  Ryosuke Niwa  <rniwa@webkit.org>

        Reviewed by Darin Adler.

        WebCore::InsertListCommand::modifyRange infinite loop (100% CPU usage)
        https://bugs.webkit.org/show_bug.cgi?id=33668

        Added a test to convert nested lists with an orphaned list child to an ordered nested list.
        Selection in switch-list-type-with-inner-list.html is restored correctly after inserting list.

        * editing/execCommand/insert-list-orphaned-item-with-nested-lists-expected.txt: Added.
        * editing/execCommand/insert-list-orphaned-item-with-nested-lists.html: Added.
        * editing/execCommand/switch-list-type-with-inner-list-expected.txt: Selection is restored correctly.

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

LayoutTests/ChangeLog
LayoutTests/editing/execCommand/insert-list-nested-with-orphaned-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/insert-list-nested-with-orphaned.html [new file with mode: 0644]
LayoutTests/editing/execCommand/switch-list-type-with-inner-list-expected.txt
WebCore/ChangeLog
WebCore/editing/InsertListCommand.cpp

index 9a11455..81c7daf 100644 (file)
@@ -1,3 +1,17 @@
+2010-08-25  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Darin Adler.
+
+        WebCore::InsertListCommand::modifyRange infinite loop (100% CPU usage)
+        https://bugs.webkit.org/show_bug.cgi?id=33668
+
+        Added a test to convert nested lists with an orphaned list child to an ordered nested list.
+        Selection in switch-list-type-with-inner-list.html is restored correctly after inserting list.
+
+        * editing/execCommand/insert-list-orphaned-item-with-nested-lists-expected.txt: Added.
+        * editing/execCommand/insert-list-orphaned-item-with-nested-lists.html: Added.
+        * editing/execCommand/switch-list-type-with-inner-list-expected.txt: Selection is restored correctly.
+
 2010-08-24  Ryosuke Niwa  <rniwa@webkit.org>
 
         Reviewed by Tony Chang.
diff --git a/LayoutTests/editing/execCommand/insert-list-nested-with-orphaned-expected.txt b/LayoutTests/editing/execCommand/insert-list-nested-with-orphaned-expected.txt
new file mode 100644 (file)
index 0000000..403fe52
--- /dev/null
@@ -0,0 +1,24 @@
+This tests hang when listifying nested lists with an orphaned list child in between (see bug 33668).
+| "
+"
+| <ol>
+|   <ol>
+|     <li>
+|       "hello"
+|   "
+    world
+    "
+|   <ol>
+|     <li>
+|       "WebKit"
+|     <li>
+|       "rocks"
+|     "
+        "
+|     <ol>
+|       <li>
+|         "<#selection-caret>because of you"
+|   "
+    "
+| "
+"
diff --git a/LayoutTests/editing/execCommand/insert-list-nested-with-orphaned.html b/LayoutTests/editing/execCommand/insert-list-nested-with-orphaned.html
new file mode 100644 (file)
index 0000000..d01721f
--- /dev/null
@@ -0,0 +1,23 @@
+<script src="../../resources/dump-as-markup.js"></script>
+<div id="test" contenteditable>
+<ul>
+    <ul><li>hello</li></ul>
+    world
+    <ol><li>WebKit</li></ol>
+    <ul>
+        <li>rocks</li>
+        <ul><li>because of you</li></ul>
+    </ul>
+</ul>
+</div>
+<script type="text/javascript"> 
+
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+
+window.getSelection().selectAllChildren(document.getElementById('test'))
+document.execCommand('InsertOrderedList', false, null);
+
+Markup.description("This tests hang when listifying nested lists with an orphaned list child in between (see bug 33668).")
+Markup.dump('test')
+</script>
index 1242fca..9c25045 100644 (file)
@@ -3,7 +3,7 @@ This tests switching an unordered list with a nested list to an ordered list.
 "
 | <ol>
 |   <li>
-|     "<#selection-caret>hello"
+|     "<#selection-anchor>hello"
 |   "
     "
 |   <ol>
@@ -16,6 +16,6 @@ This tests switching an unordered list with a nested list to an ordered list.
 |   "
     "
 |   <li>
-|     "webkit"
+|     "webkit<#selection-focus>"
 | "
 "
index 06dc40e..252fe4c 100644 (file)
@@ -1,3 +1,31 @@
+2010-08-25  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Darin Adler.
+
+        WebCore::InsertListCommand::modifyRange infinite loop (100% CPU usage)
+        https://bugs.webkit.org/show_bug.cgi?id=33668
+
+        The bug was caused by enclosingListChild returning a list child whose enclosing list is
+        a sibling of the current list child. Fixed enclosingListChild to traverse upwards
+        in the DOM to find the list child which is a sibling of the current list child.
+        Also fixed adjacentEnclosingList to only returns the list that belongs to the same outer list.
+
+        In doApplyForSingleParagraph, if the start or the end of currentSelection existed inside a list content
+        moved by moveParagraphWithClones, either end could point to a wrong position after the move.
+        Fixed this problem by checking this condition upfront and restoring later.
+
+        In doApply, if moveParagraph or moveParagraphWithClones, endOfSelection or startOfLastParagraph
+        could be null or orphaned, fixed this problem by indexForVisiblePosition.
+
+        Test: editing/execCommand/insert-list-orphaned-item-with-nested-lists.html
+
+        * editing/InsertListCommand.cpp:
+        (WebCore::InsertListCommand::doApply):
+        (WebCore::enclosingListChild):
+        (WebCore::InsertListCommand::doApplyForSingleParagraph):
+        (WebCore::adjacentEnclosingList):
+        (WebCore::InsertListCommand::listifyParagraph):
+
 2010-08-25  Brent Fulgham  <bfulgham@webkit.org>
 
         Build corrections, no review.
index 79deb65..f90d5d3 100644 (file)
@@ -37,6 +37,14 @@ namespace WebCore {
 
 using namespace HTMLNames;
 
+static Node* enclosingListChild(Node* node, Node* listNode)
+{
+    Node* listChild = enclosingListChild(node);
+    while (listChild && enclosingList(listChild) != listNode)
+        listChild = enclosingListChild(listChild->parentNode());
+    return listChild;
+}
+
 PassRefPtr<HTMLElement> InsertListCommand::insertList(Document* document, Type type)
 {
     RefPtr<InsertListCommand> insertCommand = create(document, type);
@@ -138,7 +146,19 @@ void InsertListCommand::doApply()
                 if (!startOfLastParagraph.deepEquivalent().node()->inDocument())
                     return;
                 setEndingSelection(startOfCurrentParagraph);
+
+                // Save and restore endOfSelection and startOfLastParagraph when necessary
+                // since moveParagraph and movePragraphWithClones can remove nodes.
+                // FIXME: This is an inefficient way to keep selection alive because indexForVisiblePosition walks from
+                // the beginning of the document to the endOfSelection everytime this code is executed.
+                // But not using index is hard because there are so many ways we can lose selection inside doApplyForSingleParagraph.
+                int indexForEndOfSelection = indexForVisiblePosition(endOfSelection);
                 doApplyForSingleParagraph(forceCreateList, listTag, currentSelection.get());
+                if (endOfSelection.isNull() || endOfSelection.isOrphan() || startOfLastParagraph.isNull() || startOfLastParagraph.isOrphan()) {
+                    RefPtr<Range> lastSelectionRange = TextIterator::rangeFromLocationAndLength(document()->documentElement(), indexForEndOfSelection, 0, true);
+                    endOfSelection = lastSelectionRange->startPosition();
+                    startOfLastParagraph = startOfParagraph(endOfSelection);
+                }
 
                 // Fetch the start of the selection after moving the first paragraph,
                 // because moving the paragraph will invalidate the original start.  
@@ -186,12 +206,35 @@ void InsertListCommand::doApplyForSingleParagraph(bool forceCreateList, const Qu
 
         // If the entire list is selected, then convert the whole list.
         if (switchListType && isNodeVisiblyContainedWithin(listNode.get(), currentSelection)) {
+            bool rangeStartIsInList = visiblePositionBeforeNode(listNode.get()) == currentSelection->startPosition();
+            bool rangeEndIsInList = visiblePositionAfterNode(listNode.get()) == currentSelection->endPosition();
+
             RefPtr<HTMLElement> newList = createHTMLElement(document(), listTag);
             insertNodeBefore(newList, listNode);
-            Node* outerBlock = listChildNode->isBlockFlow() ? listChildNode : listNode.get();
+
+            Node* firstChildInList = enclosingListChild(VisiblePosition(Position(listNode, 0)).deepEquivalent().node(), listNode.get());
+            Node* outerBlock = firstChildInList->isBlockFlow() ? firstChildInList : listNode.get();
+            
             moveParagraphWithClones(firstPositionInNode(listNode.get()), lastPositionInNode(listNode.get()), newList.get(), outerBlock);
+
+            // Manually remove listNode because moveParagraphWithClones sometimes leaves it behind in the document.
+            // See the bug 33668 and editing/execCommand/insert-list-orphaned-item-with-nested-lists.html.
+            // FIXME: This might be a bug in moveParagraphWithClones or deleteSelection.
+            if (listNode && listNode->inDocument())
+                removeNode(listNode);
+
             newList = mergeWithNeighboringLists(newList);
+
+            // Restore the start and the end of current selection if they started inside listNode
+            // because moveParagraphWithClones could have removed them.
+            ExceptionCode ec;
+            if (rangeStartIsInList && newList)
+                currentSelection->setStart(newList, 0, ec);
+            if (rangeEndIsInList && newList)
+                currentSelection->setEnd(newList, lastOffsetInNode(newList.get()), ec);
+
             setEndingSelection(VisiblePosition(firstPositionInNode(newList.get())));
+
             return;
         }
         
@@ -202,14 +245,6 @@ void InsertListCommand::doApplyForSingleParagraph(bool forceCreateList, const Qu
         m_listElement = listifyParagraph(endingSelection().visibleStart(), listTag);
 }
 
-static Node* enclosingListChild(Node* node, Node* listNode)
-{
-    Node* listChild = enclosingListChild(node);
-    if (enclosingList(listChild) != listNode)
-        return 0;
-    return listChild;
-}
-
 void InsertListCommand::unlistifyParagraph(const VisiblePosition& originalStart, HTMLElement* listNode, Node* listChildNode)
 {
     Node* nextListChild;
@@ -278,7 +313,8 @@ static Element* adjacentEnclosingList(const VisiblePosition& pos, const VisibleP
 
     if (!listNode->hasTagName(listTag)
         || listNode->contains(pos.deepEquivalent().node())
-        || previousCell != currentCell)
+        || previousCell != currentCell
+        || enclosingList(listNode) != enclosingList(pos.deepEquivalent().node()))
         return 0;
 
     return listNode;
@@ -288,6 +324,9 @@ PassRefPtr<HTMLElement> InsertListCommand::listifyParagraph(const VisiblePositio
 {
     VisiblePosition start = startOfParagraph(originalStart);
     VisiblePosition end = endOfParagraph(start);
+    
+    if (start.isNull() || end.isNull())
+        return 0;
 
     // Check for adjoining lists.
     RefPtr<HTMLElement> listItemElement = createListItemElement(document());