[Webkit-unassigned] [Bug 38231] crash in WebCore::CompositeEditCommand::splitTreeToNode when indenting pre

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jun 22 10:02:23 PDT 2010


https://bugs.webkit.org/show_bug.cgi?id=38231





--- Comment #13 from Enrica Casucci <enrica at apple.com>  2010-06-22 10:02:23 PST ---
(From update of attachment 55814)
> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 4dc9e8e05791fadb3e720b13f0e8e89db4975aa3..00771643fc6e56ee7c894d53a5a71e543806c352 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,13 @@
> +2010-05-12  Tony Chang  <tony at chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        crash in WebCore::CompositeEditCommand::splitTreeToNode when indenting pre
> +        https://bugs.webkit.org/show_bug.cgi?id=38231
> +
> +        * editing/execCommand/indent-pre-expected.txt: Added.
> +        * editing/execCommand/indent-pre.html: Added.
> +
>  2010-05-11  Tony Chang  <tony at chromium.org>
>  
>          Reviewed by Darin Fisher.
> diff --git a/LayoutTests/editing/execCommand/indent-pre-expected.txt b/LayoutTests/editing/execCommand/indent-pre-expected.txt
> new file mode 100644
> index 0000000000000000000000000000000000000000..883a704e8bd4e99bba9300fe30813c566363a586
> --- /dev/null
> +++ b/LayoutTests/editing/execCommand/indent-pre-expected.txt
> @@ -0,0 +1,179 @@
> +
> +<HTML>
> +<HEAD>
> +</HEAD>
> +<BODY>
> +<DIV contentEditable="">
> +<#text>
> +</#text>
> +<BLOCKQUOTE style="margin: 0 0 0 40px; border: none; padding: 0px;" class="webkit-indent-blockquote">
> +<PRE id="pre-basic">
> +<#text>line one
> +</#text>
> +</PRE>
> +</BLOCKQUOTE>
> +<PRE id="pre-basic">
> +<#text>line two
> +</#text>
> +</PRE>
> +<BLOCKQUOTE style="margin: 0 0 0 40px; border: none; padding: 0px;" class="webkit-indent-blockquote">
> +<PRE id="pre-basic">
> +<#text>line three
> +</#text>
> +</PRE>
> +<PRE id="pre-basic">
> +<#text>line four</#text>
> +</PRE>
> +</BLOCKQUOTE>
> +<#text>
> +
> +</#text>
> +<UL>
> +<LI>
> +<PRE id="pre-list">
> +<#text>list one
> +</#text>
> +<BLOCKQUOTE style="margin: 0 0 0 40px; border: none; padding: 0px;" class="webkit-indent-blockquote">
> +<#text>list two
> +</#text>
> +<#text>list three
> +</#text>
> +</BLOCKQUOTE>
> +<#text>list four
> +</#text>
> +</PRE>
> +</LI>
> +</UL>
> +<#text>
> +
> +</#text>
> +<BLOCKQUOTE style="margin: 0 0 0 40px; border: none; padding: 0px;" class="webkit-indent-blockquote">
> +<TABLE>
> +<#text>
> +</#text>
> +<TBODY>
> +<TR>
> +<TD>
> +<PRE id="pre-table">
> +<#text>table one
> +</#text>
> +<#text><selection-anchor>table two
> +</#text>
> +</PRE>
> +<BLOCKQUOTE style="margin: 0 0 0 40px; border: none; padding: 0px;" class="webkit-indent-blockquote">
> +<PRE id="pre-table">
> +<#text>table three<selection-focus></#text>
> +</PRE>
> +<PRE id="pre-table">
> +<#text>table three</#text>
> +</PRE>
> +</BLOCKQUOTE>
> +</TD>
> +<TD>
> +<BLOCKQUOTE style="margin: 0 0 0 40px; border: none; padding: 0px;" class="webkit-indent-blockquote">
> +<#text>right cell</#text>
> +</BLOCKQUOTE>
> +</TD>
> +</TR>
> +</TBODY>
> +</TABLE>
> +<DIV id="results">
> +<#text>PASSED</#text>
> +</DIV>
> +</BLOCKQUOTE>
> +<#text>
> +
> +</#text>
> +<#text>
> +</#text>
> +</DIV>
> +<#text>
> +
> +</#text>
> +<A href="javascript:document.execCommand('indent')">
> +<#text>indent</#text>
> +</A>
> +<#text>
> +</#text>
> +<A href="javascript:document.execCommand('outdent')">
> +<#text>outdent</#text>
> +</A>
> +<#text>
> +</#text>
> +<SCRIPT src="../../resources/dump-as-markup.js"></SCRIPT>
> +<#text>
> +</#text>
> +<SCRIPT src="../editing.js"></SCRIPT>
> +<#text>
> +</#text>
> +<SCRIPT>
> +function setSelection(node, startOffset, endOffset)
> +{
> +var textNode = node.firstChild;
> +if (textNode.nodeType != Node.TEXT_NODE)
> +throw "Wrong node type: " + textNode;
> +execSetSelectionCommand(textNode, startOffset, endOffset);
> +}
> +
> +function verifyTextSelection(startNode, startOffset, endNode, endOffset)
> +{
> +if (startNode.nodeType != Node.TEXT_NODE)
> +console.log("Wrong start node type: " + startNode);
> +if (endNode.nodeType != Node.TEXT_NODE)
> +console.log("Wrong end node type: " + endNode);
> +var sel = window.getSelection();
> +if (sel.anchorNode != startNode || sel.focusNode != endNode)
> +console.log("Wrong node selected.");
> +if (sel.anchorOffset != startOffset)
> +console.log("Wrong anchor offset: " + sel.anchorOffset + " instead of " + startOffset);
> +if (sel.focusOffset != endOffset)
> +console.log("Wrong focus offset: " + sel.focusOffset + " instead of " + endOffset);
> +}
> +
> +// Indent a single line in a pre and make sure the selection is correctly preserved.
> +var pre = document.getElementById("pre-basic");
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByCharacterCommand();
> +execExtendSelectionForwardByWordCommand();
> +document.execCommand("indent");
> +verifyTextSelection(document.getElementsByTagName("pre")[0].firstChild, 1,
> +document.getElementsByTagName("pre")[0].firstChild, 4);
> +
> +// Indent 2 lines.
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +execExtendSelectionForwardByWordCommand();
> +document.execCommand("indent");
> +if (document.getElementsByTagName("pre").length > 3) {
> +// FIXME: The selection for the anchorNode is wrong. It should be (pre[2], 0).
> +verifyTextSelection(document.getElementsByTagName("pre")[1].firstChild, 8,
> +document.getElementsByTagName("pre")[3].firstChild, 4);
> +} else {
> +console.log("Wrong number of pre nodes.");
> +}
> +// Indent <pre> lines in a list.
> +pre = document.getElementById("pre-list");
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +document.execCommand("indent");
> +verifyTextSelection(document.getElementsByTagName("blockquote")[2].firstChild, 0,
> +document.getElementsByTagName("blockquote")[2].firstChild.nextSibling, 10);
> +
> +// Indenting <pre> lines in a table.
> +pre = document.getElementById("pre-table");
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +// FIXME: This is wrong.
> +document.execCommand("indent");
> +
> +document.getElementById("results").innerText = "PASSED";
> +</SCRIPT>
> +<#text>
> +</#text>
> +</BODY>
> +</HTML>
> diff --git a/LayoutTests/editing/execCommand/indent-pre.html b/LayoutTests/editing/execCommand/indent-pre.html
> new file mode 100644
> index 0000000000000000000000000000000000000000..0641c844de9fdd8538d60461b5c1525798ae1f90
> --- /dev/null
> +++ b/LayoutTests/editing/execCommand/indent-pre.html
> @@ -0,0 +1,91 @@
> +<div contentEditable>
> +<pre id="pre-basic">line one
> +line two
> +line three
> +line four</pre>
> +
> +<ul><li><pre id="pre-list">list one
> +list two
> +list three
> +list four
> +</pre></li></ul>
> +
> +<table>
> +<tr><td><pre id="pre-table">table one
> +table two
> +table three</pre></td><td>right cell</td></tr></table>
> +
> +<div id="results">FAILED</div>
> +</div>
> +
> +<a href="javascript:document.execCommand('indent')">indent</a>
> +<a href="javascript:document.execCommand('outdent')">outdent</a>
> +<script src="../../resources/dump-as-markup.js"></script>
> +<script src="../editing.js"></script>
> +<script>
> +function setSelection(node, startOffset, endOffset)
> +{
> +    var textNode = node.firstChild;
> +    if (textNode.nodeType != Node.TEXT_NODE)
> +        throw "Wrong node type: " + textNode;
> +    execSetSelectionCommand(textNode, startOffset, endOffset);
> +}
> +
> +function verifyTextSelection(startNode, startOffset, endNode, endOffset)
> +{
> +    if (startNode.nodeType != Node.TEXT_NODE)
> +        console.log("Wrong start node type: " + startNode);
> +    if (endNode.nodeType != Node.TEXT_NODE)
> +        console.log("Wrong end node type: " + endNode);
> +    var sel = window.getSelection();
> +    if (sel.anchorNode != startNode || sel.focusNode != endNode)
> +        console.log("Wrong node selected.");
> +    if (sel.anchorOffset != startOffset)
> +        console.log("Wrong anchor offset: " + sel.anchorOffset + " instead of " + startOffset);
> +    if (sel.focusOffset != endOffset)
> +        console.log("Wrong focus offset: " + sel.focusOffset + " instead of " + endOffset);
> +}
> +
> +// Indent a single line in a pre and make sure the selection is correctly preserved.
> +var pre = document.getElementById("pre-basic");
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByCharacterCommand();
> +execExtendSelectionForwardByWordCommand();
> +document.execCommand("indent");
> +verifyTextSelection(document.getElementsByTagName("pre")[0].firstChild, 1,
> +                    document.getElementsByTagName("pre")[0].firstChild, 4);
> +
> +// Indent 2 lines.
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +execExtendSelectionForwardByWordCommand();
> +document.execCommand("indent");
> +if (document.getElementsByTagName("pre").length > 3) {
> +    // FIXME: The selection for the anchorNode is wrong.  It should be (pre[2], 0).
> +    verifyTextSelection(document.getElementsByTagName("pre")[1].firstChild, 8,
> +                        document.getElementsByTagName("pre")[3].firstChild, 4);
> +} else {
> +    console.log("Wrong number of pre nodes.");
> +}
> +// Indent <pre> lines in a list.
> +pre = document.getElementById("pre-list");
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +document.execCommand("indent");
> +verifyTextSelection(document.getElementsByTagName("blockquote")[2].firstChild, 0,
> +                    document.getElementsByTagName("blockquote")[2].firstChild.nextSibling, 10);
> +
> +// Indenting <pre> lines in a table.
> +pre = document.getElementById("pre-table");
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +// FIXME: This is wrong.
> +document.execCommand("indent");
> +
> +document.getElementById("results").innerText = "PASSED";
> +</script>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 482ee438dd8109f3a9b4b4105770931a21c640fc..266ea14b48c21c1f7d97320797651fc5f3f033bf 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,19 @@
> +2010-05-12  Tony Chang  <tony at chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        crash in WebCore::CompositeEditCommand::splitTreeToNode when indenting pre
> +        https://bugs.webkit.org/show_bug.cgi?id=38231
> +
> +        Test: editing/execCommand/indent-pre.html
> +
> +        * editing/IndentOutdentCommand.cpp:
> +        (WebCore::countParagraphs):
> +        (WebCore::IndentOutdentCommand::indentRegion): Split text nodes into one node per paragraph
> +                                                       so moveParagraph doesn't get confused.
> +        (WebCore::IndentOutdentCommand::splitTextNodes):
> +        * editing/IndentOutdentCommand.h:
> +
>  2010-05-11  David Hyatt  <hyatt at apple.com>
>  
>          Reviewed by Maciej Stachowiak.
> diff --git a/WebCore/editing/IndentOutdentCommand.cpp b/WebCore/editing/IndentOutdentCommand.cpp
> index 0f3975b0c13f6e8e93e32e8c659ab3b77b463f39..b68527d044ee3af7d12d7d6f2b605fba4f912290 100644
> --- a/WebCore/editing/IndentOutdentCommand.cpp
> +++ b/WebCore/editing/IndentOutdentCommand.cpp
> @@ -34,6 +34,7 @@
>  #include "InsertListCommand.h"
>  #include "Range.h"
>  #include "SplitElementCommand.h"
> +#include "Text.h"
>  #include "TextIterator.h"
>  #include "htmlediting.h"
>  #include "visible_units.h"
> @@ -62,6 +63,22 @@ static bool isListOrIndentBlockquote(const Node* node)
>      return node && (node->hasTagName(ulTag) || node->hasTagName(olTag) || node->hasTagName(blockquoteTag));
>  }
>  
> +// This function can return -1 if we are unable to count the paragraphs between |start| and |end|.
> +static int countParagraphs(const VisiblePosition& start, const VisiblePosition& end)
> +{
> +    int count = 0;
> +    VisiblePosition cur = start;
> +    while (cur != end) {
> +        ++count;
> +        cur = endOfParagraph(cur.next());
> +        // If start is before a table and end is inside a table, we will never hit end because the
> +        // whole table is considered a single paragraph.
> +        if (cur.isNull())
> +            return -1;
> +    }
> +    return count;
> +}
> +
>  IndentOutdentCommand::IndentOutdentCommand(Document* document, EIndentType typeOfAction, int marginInPixels)
>      : CompositeEditCommand(document), m_typeOfAction(typeOfAction), m_marginInPixels(marginInPixels)
>  {
> @@ -151,6 +168,18 @@ void IndentOutdentCommand::indentRegion(const VisiblePosition& startOfSelection,
>      RefPtr<Element> blockquoteForNextIndent;
>      VisiblePosition endOfCurrentParagraph = endOfParagraph(startOfSelection);
>      VisiblePosition endAfterSelection = endOfParagraph(endOfParagraph(endOfSelection).next());
> +    int endOfCurrentParagraphIndex = indexForVisiblePosition(endOfCurrentParagraph);
> +    int endAfterSelectionIndex = indexForVisiblePosition(endAfterSelection);
> +
> +    // When indenting within a <pre> tag, we need to split each paragraph into a separate node for moveParagraphWithClones to work.
> +    // However, splitting text nodes can cause endOfCurrentParagraph and endAfterSelection to point to an invalid position if we
> +    // changed the text node it was pointing at.  So we have to reset these positions.
> +    int numParagraphs = countParagraphs(endOfCurrentParagraph, endAfterSelection);
> +    if (splitTextNodes(startOfParagraph(startOfSelection), numParagraphs + 1)) {
> +        endOfCurrentParagraph = VisiblePosition(TextIterator::rangeFromLocationAndLength(document()->documentElement(), endOfCurrentParagraphIndex, 0)->startPosition(), UPSTREAM);
> +        endAfterSelection = VisiblePosition(TextIterator::rangeFromLocationAndLength(document()->documentElement(), endAfterSelectionIndex, 0)->startPosition(), UPSTREAM);
> +    }
> +
>      while (endOfCurrentParagraph != endAfterSelection) {
>          // Iterate across the selected paragraphs...
>          VisiblePosition endOfNextParagraph = endOfParagraph(endOfCurrentParagraph.next());
> @@ -174,6 +203,28 @@ void IndentOutdentCommand::indentRegion(const VisiblePosition& startOfSelection,
>      }   
>  }
>  
> +// Returns true if at least one text node was split.
> +bool IndentOutdentCommand::splitTextNodes(const VisiblePosition& start, int numParagraphs)
> +{
> +    VisiblePosition currentParagraphStart = start;
> +    bool hasSplit = false;
> +    int paragraphCount;
> +    for (paragraphCount = 0; paragraphCount < numParagraphs; ++paragraphCount) {
> +        if (currentParagraphStart.deepEquivalent().node()->isTextNode() && currentParagraphStart.deepEquivalent().node() == startOfParagraph(currentParagraphStart.previous()).deepEquivalent().node()) {
> +            Text* textNode = static_cast<Text *>(currentParagraphStart.deepEquivalent().node());
> +            int offset = currentParagraphStart.deepEquivalent().offsetInContainerNode();
> +            splitTextNode(textNode, offset);
> +            currentParagraphStart = VisiblePosition(textNode, 0, VP_DEFAULT_AFFINITY);
> +            hasSplit = true;
> +        }
> +        VisiblePosition nextParagraph = startOfParagraph(endOfParagraph(currentParagraphStart).next());
> +        if (nextParagraph.isNull())
> +            break;
> +        currentParagraphStart = nextParagraph;
> +    }
> +    return hasSplit;
> +}
> +
>  void IndentOutdentCommand::outdentParagraph()
>  {
>      VisiblePosition visibleStartOfParagraph = startOfParagraph(endingSelection().visibleStart());
> diff --git a/WebCore/editing/IndentOutdentCommand.h b/WebCore/editing/IndentOutdentCommand.h
> index 8705bf103c22122298d1e8495dc007e9c2580911..8644cc56aa82083052e52e7a654a460fef3db964 100644
> --- a/WebCore/editing/IndentOutdentCommand.h
> +++ b/WebCore/editing/IndentOutdentCommand.h
> @@ -51,6 +51,7 @@ private:
>      void outdentParagraph();
>      bool tryIndentingAsListItem(const VisiblePosition&);
>      void indentIntoBlockquote(const VisiblePosition&, const VisiblePosition&, RefPtr<Element>&);
> +    bool splitTextNodes(const VisiblePosition& start, int numParagraphs);
>  
>      EIndentType m_typeOfAction;
>      int m_marginInPixels;

The patch looks good to me.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list