Fix crash in CompositeEditCommand::cloneParagraphUnderNewElement()
authorddkilzer@apple.com <ddkilzer@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Mar 2014 00:05:55 +0000 (00:05 +0000)
committerddkilzer@apple.com <ddkilzer@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Mar 2014 00:05:55 +0000 (00:05 +0000)
<http://webkit.org/b/129751>
<rdar://problem/16237965>

Reviewed by Jon Honeycutt.

Merged from Blink (patch by Yuta Kitamura):
https://src.chromium.org/viewvc/blink?revision=168160&view=revision
http://crbug.com/345005

    The root cause is CompositeEditCommand::moveParagraphWithClones() passing
    two positions |start| and |end| which do not follow the document order,
    i.e. in some situations |start| is located after |end| because of
    the difference in affinity.

    This patch fixes this crash by normalizing |end| to |start| in such situations.
    It also adds an ASSERT that checks the relationship between |start| and |end|.

Source/WebCore:

Test: editing/execCommand/format-block-crash.html

* editing/CompositeEditCommand.cpp:
(WebCore::CompositeEditCommand::cloneParagraphUnderNewElement):
(WebCore::CompositeEditCommand::moveParagraphWithClones):
* editing/CompositeEditCommand.h:

LayoutTests:

* editing/execCommand/format-block-crash-expected.txt: Added.
* editing/execCommand/format-block-crash.html: Added.
* editing/execCommand/resources/format-block-crash-iframe.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/execCommand/format-block-crash-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/format-block-crash.html [new file with mode: 0644]
LayoutTests/editing/execCommand/resources/format-block-crash-iframe.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/editing/CompositeEditCommand.cpp
Source/WebCore/editing/CompositeEditCommand.h

index c3e036c2afa44fb5139a788473229bc4342362e8..936d42cb81eb5d308d69a1a922fb690bf32a0b44 100644 (file)
@@ -1,3 +1,27 @@
+2014-03-05  David Kilzer  <ddkilzer@apple.com>
+
+        Fix crash in CompositeEditCommand::cloneParagraphUnderNewElement()
+        <http://webkit.org/b/129751>
+        <rdar://problem/16237965>
+
+        Reviewed by Jon Honeycutt.
+
+        Merged from Blink (patch by Yuta Kitamura):
+        https://src.chromium.org/viewvc/blink?revision=168160&view=revision
+        http://crbug.com/345005
+
+            The root cause is CompositeEditCommand::moveParagraphWithClones() passing
+            two positions |start| and |end| which do not follow the document order,
+            i.e. in some situations |start| is located after |end| because of
+            the difference in affinity.
+
+            This patch fixes this crash by normalizing |end| to |start| in such situations.
+            It also adds an ASSERT that checks the relationship between |start| and |end|.
+
+        * editing/execCommand/format-block-crash-expected.txt: Added.
+        * editing/execCommand/format-block-crash.html: Added.
+        * editing/execCommand/resources/format-block-crash-iframe.html: Added.
+
 2014-03-05  Radu Stavila  <stavila@adobe.com>
 
         [CSS Regions] Scrollable regions
diff --git a/LayoutTests/editing/execCommand/format-block-crash-expected.txt b/LayoutTests/editing/execCommand/format-block-crash-expected.txt
new file mode 100644 (file)
index 0000000..56a9ff9
--- /dev/null
@@ -0,0 +1,11 @@
+Should not crash if we load a test case from crbug.com/345005.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS event.data is "FINISH"
+PASS Did not crash.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/editing/execCommand/format-block-crash.html b/LayoutTests/editing/execCommand/format-block-crash.html
new file mode 100644 (file)
index 0000000..881a6d8
--- /dev/null
@@ -0,0 +1,28 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>FormatBlock crash</title>
+<script src="../../resources/js-test.js"></script>
+</head>
+<body>
+<script>
+description('Should not crash if we load a test case from crbug.com/345005.');
+
+window.jsTestIsAsync = true;
+
+window.addEventListener('message', didReceiveMessage, false);
+
+var iframe = document.createElement('iframe');
+iframe.src = 'resources/format-block-crash-iframe.html';
+document.body.appendChild(iframe);
+
+function didReceiveMessage(event)
+{
+    shouldBeEqualToString('event.data', 'FINISH');
+    document.body.removeChild(iframe);
+    testPassed('Did not crash.');
+    window.finishJSTest();
+}
+</script>
+</body>
+</html>
diff --git a/LayoutTests/editing/execCommand/resources/format-block-crash-iframe.html b/LayoutTests/editing/execCommand/resources/format-block-crash-iframe.html
new file mode 100644 (file)
index 0000000..350ef56
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>FormatBlock crash test case</title>
+</head>
+<!-- This is a minified version of the clusterfuzz test case at https://code.google.com/p/chromium/issues/detail?id=345005 -->
+<body style="display: table-row;">
+<script>
+function run()
+{
+    document.designMode = 'on';
+    document.execCommand('SelectAll');
+    document.execCommand('FormatBlock', false, '<' + 'div>');
+    window.setTimeout(notifyFinish, 0);
+}
+
+function notifyFinish()
+{
+    window.parent.postMessage('FINISH', '*');
+}
+
+window.setTimeout(run, 0);
+</script>
+<span contenteditable="true" style="display: table-caption;"></span>
+<span></span>
+<div style="display: -webkit-inline-box;"><span><span>B</span></span></div>
+<div></div>
+</body>
+</html>
index d42196123970be9deeb48ac5cd1f338b71a090b8..d3816f25e63e3f585d21961a30efef227af3c98c 100644 (file)
@@ -1,3 +1,30 @@
+2014-03-05  David Kilzer  <ddkilzer@apple.com>
+
+        Fix crash in CompositeEditCommand::cloneParagraphUnderNewElement()
+        <http://webkit.org/b/129751>
+        <rdar://problem/16237965>
+
+        Reviewed by Jon Honeycutt.
+
+        Merged from Blink (patch by Yuta Kitamura):
+        https://src.chromium.org/viewvc/blink?revision=168160&view=revision
+        http://crbug.com/345005
+
+            The root cause is CompositeEditCommand::moveParagraphWithClones() passing
+            two positions |start| and |end| which do not follow the document order,
+            i.e. in some situations |start| is located after |end| because of
+            the difference in affinity.
+
+            This patch fixes this crash by normalizing |end| to |start| in such situations.
+            It also adds an ASSERT that checks the relationship between |start| and |end|.
+
+        Test: editing/execCommand/format-block-crash.html
+
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::cloneParagraphUnderNewElement):
+        (WebCore::CompositeEditCommand::moveParagraphWithClones):
+        * editing/CompositeEditCommand.h:
+
 2014-03-05  Radu Stavila  <stavila@adobe.com>
 
         [CSS Regions] Scrollable regions
index 0021ec39e92d1f71973b2546514f2b206bddd100..1c8957f2f8e5759911b1c289c1cfd3daad9e28ff 100644 (file)
@@ -1048,8 +1048,10 @@ void CompositeEditCommand::pushAnchorElementDown(Element& anchorElement)
 // Clone the paragraph between start and end under blockElement,
 // preserving the hierarchy up to outerNode. 
 
-void CompositeEditCommand::cloneParagraphUnderNewElement(Position& start, Position& end, Node* passedOuterNode, Element* blockElement)
+void CompositeEditCommand::cloneParagraphUnderNewElement(const Position& start, const Position& end, Node* passedOuterNode, Element* blockElement)
 {
+    ASSERT(comparePositions(start, end) <= 0);
+
     // First we clone the outerNode
     RefPtr<Node> lastNode;
     RefPtr<Node> outerNode = passedOuterNode;
@@ -1167,7 +1169,7 @@ void CompositeEditCommand::moveParagraphWithClones(const VisiblePosition& startO
     // We upstream() the end and downstream() the start so that we don't include collapsed whitespace in the move.
     // When we paste a fragment, spaces after the end and before the start are treated as though they were rendered.
     Position start = startOfParagraphToMove.deepEquivalent().downstream();
-    Position end = endOfParagraphToMove.deepEquivalent().upstream();
+    Position end = startOfParagraphToMove == endOfParagraphToMove ? start : endOfParagraphToMove.deepEquivalent().upstream();
 
     cloneParagraphUnderNewElement(start, end, outerNode, blockElement);
       
index 50879149441c705d45db7f5580143fcbc4c471b2..1cd46e2ee02728a430d1e49e5da005c6457464c5 100644 (file)
@@ -160,7 +160,7 @@ protected:
     void moveParagraph(const VisiblePosition&, const VisiblePosition&, const VisiblePosition&, bool preserveSelection = false, bool preserveStyle = true);
     void moveParagraphs(const VisiblePosition&, const VisiblePosition&, const VisiblePosition&, bool preserveSelection = false, bool preserveStyle = true);
     void moveParagraphWithClones(const VisiblePosition& startOfParagraphToMove, const VisiblePosition& endOfParagraphToMove, Element* blockElement, Node* outerNode);
-    void cloneParagraphUnderNewElement(Position& start, Position& end, Node* outerNode, Element* blockElement);
+    void cloneParagraphUnderNewElement(const Position& start, const Position& end, Node* outerNode, Element* blockElement);
     void cleanupAfterDeletion(VisiblePosition destination = VisiblePosition());
     
     bool breakOutOfEmptyListItem();