Null dereference loading Blink layout test editing/inserting/insert-html-crash-01...
authorjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 9 Nov 2015 20:11:30 +0000 (20:11 +0000)
committerjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 9 Nov 2015 20:11:30 +0000 (20:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=149298
<rdar://problem/22746918>

Reviewed by Ryosuke Niwa.

Source/WebCore:

The test crashes in the method WebCore::CompositeEditCommand::moveParagraphs() because
the other method WebCore::CompositeEditCommand::cleanupAfterDeletion() accidentally
deletes the destination node. In WebCore::CompositeEditCommand::cleanupAfterDeletion(),
it fails to determine that caretAfterDelete equals to destination as Position::operator==,
which is called in VisiblePosition::operator==, only checks the equality of tuple
<Anchor Node, Anchor Type, Offset>. It is insufficient as a single position can be
represented by multiple tuples. Therefore, this change adds Position::equals() to fortify
the equal checking of two positions by considering combinations of different tuple
representations.

Furthermore, it adds VisiblePosition::equals() which considers affinity and call
Position::equals() while comparing two visible positions.

Test: editing/inserting/insert-html-crash-01.html

* dom/Position.cpp:
(WebCore::Position::equals):
* dom/Position.h:
* editing/CompositeEditCommand.cpp:
(WebCore::CompositeEditCommand::cleanupAfterDeletion):
Replace operator== with VisiblePosition::equals() to tackle the test case.
* editing/VisiblePosition.cpp:
(WebCore::VisiblePosition::equals):
* editing/VisiblePosition.h:

LayoutTests:

This test case is from Blink r153982:
https://codereview.chromium.org/16053005

* editing/inserting/insert-html-crash-01-expected.txt: Added.
* editing/inserting/insert-html-crash-01.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/inserting/insert-html-crash-01-expected.txt [new file with mode: 0644]
LayoutTests/editing/inserting/insert-html-crash-01.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Position.cpp
Source/WebCore/dom/Position.h
Source/WebCore/editing/CompositeEditCommand.cpp
Source/WebCore/editing/VisiblePosition.cpp
Source/WebCore/editing/VisiblePosition.h

index a4a937d..5764ed8 100644 (file)
@@ -1,3 +1,17 @@
+2015-11-09  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Null dereference loading Blink layout test editing/inserting/insert-html-crash-01.html
+        https://bugs.webkit.org/show_bug.cgi?id=149298
+        <rdar://problem/22746918>
+
+        Reviewed by Ryosuke Niwa.
+
+        This test case is from Blink r153982:
+        https://codereview.chromium.org/16053005
+
+        * editing/inserting/insert-html-crash-01-expected.txt: Added.
+        * editing/inserting/insert-html-crash-01.html: Added.
+
 2015-11-09  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Some style changes cause tatechuyoko to be drawn off center
diff --git a/LayoutTests/editing/inserting/insert-html-crash-01-expected.txt b/LayoutTests/editing/inserting/insert-html-crash-01-expected.txt
new file mode 100644 (file)
index 0000000..ced5785
--- /dev/null
@@ -0,0 +1,13 @@
+EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of DIV > BODY > HTML > #document to 3 of DIV > BODY > HTML > #document
+EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document toDOMRange:range from 1 of #text > DIV > BODY > HTML > #document to 1 of DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > BODY > HTML > #document to 2 of DIV > DIV > BODY > HTML > #document toDOMRange:range from 9 of #text > DIV > DIV > BODY > HTML > #document to 9 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 9 of #text > DIV > DIV > BODY > HTML > #document to 9 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 2 of DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidEndEditing:WebViewDidEndEditingNotification
+PASS. WebKit didn't crash.
diff --git a/LayoutTests/editing/inserting/insert-html-crash-01.html b/LayoutTests/editing/inserting/insert-html-crash-01.html
new file mode 100644 (file)
index 0000000..682ea8a
--- /dev/null
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../editing.js"></script>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+function editingTest() {
+    var editable = document.getElementById('editable');
+    editable.focus();
+    document.execCommand('SelectAll');
+    document.execCommand('InsertText', false, 'SUCCEEDED');
+    document.execCommand('SelectAll');
+    document.write("PASS. WebKit didn't crash.");
+}
+</script>
+</head>
+<body onload=runEditingTest()>
+<div id="editable" contenteditable="true">
+Foo
+    <div style="overflow:scroll;">
+        <div style="display:table"></div>
+    </div>
+</div>
+</body>
+</html>
index ff48810..6ece6fd 100644 (file)
@@ -1,3 +1,36 @@
+2015-11-09  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Null dereference loading Blink layout test editing/inserting/insert-html-crash-01.html
+        https://bugs.webkit.org/show_bug.cgi?id=149298
+        <rdar://problem/22746918>
+
+        Reviewed by Ryosuke Niwa.
+
+        The test crashes in the method WebCore::CompositeEditCommand::moveParagraphs() because
+        the other method WebCore::CompositeEditCommand::cleanupAfterDeletion() accidentally
+        deletes the destination node. In WebCore::CompositeEditCommand::cleanupAfterDeletion(),
+        it fails to determine that caretAfterDelete equals to destination as Position::operator==,
+        which is called in VisiblePosition::operator==, only checks the equality of tuple
+        <Anchor Node, Anchor Type, Offset>. It is insufficient as a single position can be
+        represented by multiple tuples. Therefore, this change adds Position::equals() to fortify
+        the equal checking of two positions by considering combinations of different tuple
+        representations.
+
+        Furthermore, it adds VisiblePosition::equals() which considers affinity and call
+        Position::equals() while comparing two visible positions.
+
+        Test: editing/inserting/insert-html-crash-01.html
+
+        * dom/Position.cpp:
+        (WebCore::Position::equals):
+        * dom/Position.h:
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::cleanupAfterDeletion):
+        Replace operator== with VisiblePosition::equals() to tackle the test case.
+        * editing/VisiblePosition.cpp:
+        (WebCore::VisiblePosition::equals):
+        * editing/VisiblePosition.h:
+
 2015-11-09  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Some style changes cause tatechuyoko to be drawn off center
index 5da319d..f876308 100644 (file)
@@ -1453,6 +1453,102 @@ void Position::showTreeForThis() const
 
 #endif
 
+bool Position::equals(const Position& other) const
+{
+    if (!m_anchorNode)
+        return !m_anchorNode == !other.m_anchorNode;
+    if (!other.m_anchorNode)
+        return false;
+
+    switch (anchorType()) {
+    case PositionIsBeforeChildren:
+        ASSERT(!m_anchorNode->isTextNode());
+        switch (other.anchorType()) {
+        case PositionIsBeforeChildren:
+            ASSERT(!other.m_anchorNode->isTextNode());
+            return m_anchorNode == other.m_anchorNode;
+        case PositionIsAfterChildren:
+            ASSERT(!other.m_anchorNode->isTextNode());
+            return m_anchorNode == other.m_anchorNode && !m_anchorNode->hasChildNodes();
+        case PositionIsOffsetInAnchor:
+            return m_anchorNode == other.m_anchorNode && !other.m_offset;
+        case PositionIsBeforeAnchor:
+            return m_anchorNode->firstChild() == other.m_anchorNode;
+        case PositionIsAfterAnchor:
+            return false;
+        }
+        break;
+    case PositionIsAfterChildren:
+        ASSERT(!m_anchorNode->isTextNode());
+        switch (other.anchorType()) {
+        case PositionIsBeforeChildren:
+            ASSERT(!other.m_anchorNode->isTextNode());
+            return m_anchorNode == other.m_anchorNode && !m_anchorNode->hasChildNodes();
+        case PositionIsAfterChildren:
+            ASSERT(!other.m_anchorNode->isTextNode());
+            return m_anchorNode == other.m_anchorNode;
+        case PositionIsOffsetInAnchor:
+            return m_anchorNode == other.m_anchorNode && m_anchorNode->countChildNodes() == static_cast<unsigned>(m_offset);
+        case PositionIsBeforeAnchor:
+            return false;
+        case PositionIsAfterAnchor:
+            return m_anchorNode->lastChild() == other.m_anchorNode;
+        }
+        break;
+    case PositionIsOffsetInAnchor:
+        switch (other.anchorType()) {
+        case PositionIsBeforeChildren:
+            ASSERT(!other.m_anchorNode->isTextNode());
+            return m_anchorNode == other.m_anchorNode && !m_offset;
+        case PositionIsAfterChildren:
+            ASSERT(!other.m_anchorNode->isTextNode());
+            return m_anchorNode == other.m_anchorNode && m_offset == static_cast<int>(other.m_anchorNode->countChildNodes());
+        case PositionIsOffsetInAnchor:
+            return m_anchorNode == other.m_anchorNode && m_offset == other.m_offset;
+        case PositionIsBeforeAnchor:
+            return m_anchorNode->traverseToChildAt(m_offset) == other.m_anchorNode;
+        case PositionIsAfterAnchor:
+            return m_offset && m_anchorNode->traverseToChildAt(m_offset - 1) == other.m_anchorNode;
+        }
+        break;
+    case PositionIsBeforeAnchor:
+        switch (other.anchorType()) {
+        case PositionIsBeforeChildren:
+            ASSERT(!other.m_anchorNode->isTextNode());
+            return m_anchorNode == other.m_anchorNode->firstChild();
+        case PositionIsAfterChildren:
+            ASSERT(!other.m_anchorNode->isTextNode());
+            return false;
+        case PositionIsOffsetInAnchor:
+            return m_anchorNode == other.m_anchorNode->traverseToChildAt(other.m_offset);
+        case PositionIsBeforeAnchor:
+            return m_anchorNode == other.m_anchorNode;
+        case PositionIsAfterAnchor:
+            return m_anchorNode->previousSibling() == other.m_anchorNode;
+        }
+        break;
+    case PositionIsAfterAnchor:
+        switch (other.anchorType()) {
+        case PositionIsBeforeChildren:
+            ASSERT(!other.m_anchorNode->isTextNode());
+            return false;
+        case PositionIsAfterChildren:
+            ASSERT(!other.m_anchorNode->isTextNode());
+            return m_anchorNode == other.m_anchorNode->lastChild();
+        case PositionIsOffsetInAnchor:
+            return other.m_offset && m_anchorNode == other.m_anchorNode->traverseToChildAt(other.m_offset - 1);
+        case PositionIsBeforeAnchor:
+            return m_anchorNode->nextSibling() == other.m_anchorNode;
+        case PositionIsAfterAnchor:
+            return m_anchorNode == other.m_anchorNode;
+        }
+        break;
+    }
+
+    ASSERT_NOT_REACHED();
+    return false;
+}
+
 } // namespace WebCore
 
 #if ENABLE(TREE_DEBUGGING)
index 0a0ca78..78e082e 100644 (file)
@@ -207,7 +207,10 @@ public:
     void showAnchorTypeAndOffset() const;
     void showTreeForThis() const;
 #endif
-    
+
+    // This is a tentative enhancement of operator== to account for different position types.
+    // FIXME: Combine this function with operator==
+    bool equals(const Position&) const;
 private:
     WEBCORE_EXPORT int offsetForPositionAfterAnchor() const;
     
index 6c1b56b..5440f08 100644 (file)
@@ -1120,7 +1120,7 @@ void CompositeEditCommand::cloneParagraphUnderNewElement(const Position& start,
 void CompositeEditCommand::cleanupAfterDeletion(VisiblePosition destination)
 {
     VisiblePosition caretAfterDelete = endingSelection().visibleStart();
-    if (caretAfterDelete != destination && isStartOfParagraph(caretAfterDelete) && isEndOfParagraph(caretAfterDelete)) {
+    if (!caretAfterDelete.equals(destination) && isStartOfParagraph(caretAfterDelete) && isEndOfParagraph(caretAfterDelete)) {
         // Note: We want the rightmost candidate.
         Position position = caretAfterDelete.deepEquivalent().downstream();
         Node* node = position.deprecatedNode();
index d9ad664..e3f1e57 100644 (file)
@@ -743,6 +743,11 @@ bool isLastVisiblePositionInNode(const VisiblePosition &visiblePosition, const N
     return next.isNull() || !next.deepEquivalent().deprecatedNode()->isDescendantOf(node);
 }
 
+bool VisiblePosition::equals(const VisiblePosition& other) const
+{
+    return m_affinity == other.m_affinity && m_deepPosition.equals(other.m_deepPosition);
+}
+
 }  // namespace WebCore
 
 #if ENABLE(TREE_DEBUGGING)
index d004251..85ad177 100644 (file)
@@ -100,6 +100,10 @@ public:
     // FIXME: navigation with transforms should be smarter.
     WEBCORE_EXPORT int lineDirectionPointForBlockDirectionNavigation() const;
 
+    // This is a tentative enhancement of operator== to account for affinity.
+    // FIXME: Combine this function with operator==
+    bool equals(const VisiblePosition&) const;
+
 #if ENABLE(TREE_DEBUGGING)
     void debugPosition(const char* msg = "") const;
     void formatForDebugger(char* buffer, unsigned length) const;