Merge r165138 - Fix crash in CompositeEditCommand::cloneParagraphUnde…

…rNewElement() <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.
WebKit · Apr 30, 2014 · 0f12da5 · 0f12da5
1 parent b384743
commit 0f12da5
Show file tree

Hide file tree

Showing 7 changed files with 124 additions and 3 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -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-01  David Kilzer  <ddkilzer@apple.com>
 
         Ensure keySplines is valid in SMIL animations

diff --git a/LayoutTests/editing/execCommand/format-block-crash-expected.txt b/LayoutTests/editing/execCommand/format-block-crash-expected.txt
@@ -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
@@ -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
@@ -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>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -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-01  David Kilzer  <ddkilzer@apple.com>
 
         Ensure keySplines is valid in SMIL animations

diff --git a/Source/WebCore/editing/CompositeEditCommand.cpp b/Source/WebCore/editing/CompositeEditCommand.cpp
@@ -991,8 +991,10 @@ void CompositeEditCommand::pushAnchorElementDown(Node* anchorNode)
 // 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;
@@ -1110,7 +1112,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);
 

diff --git a/Source/WebCore/editing/CompositeEditCommand.h b/Source/WebCore/editing/CompositeEditCommand.h
@@ -157,7 +157,7 @@ class CompositeEditCommand : public EditCommand {
     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();