- Revision
- 192170
- Author
- jiewen_...@apple.com
- Date
- 2015-11-09 12:11:30 -0800 (Mon, 09 Nov 2015)
Log Message
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.
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (192169 => 192170)
--- trunk/LayoutTests/ChangeLog 2015-11-09 19:40:39 UTC (rev 192169)
+++ trunk/LayoutTests/ChangeLog 2015-11-09 20:11:30 UTC (rev 192170)
@@ -1,3 +1,17 @@
+2015-11-09 Jiewen Tan <jiewen_...@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 <mmaxfi...@apple.com>
Some style changes cause tatechuyoko to be drawn off center
Added: trunk/LayoutTests/editing/inserting/insert-html-crash-01-expected.txt (0 => 192170)
--- trunk/LayoutTests/editing/inserting/insert-html-crash-01-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/inserting/insert-html-crash-01-expected.txt 2015-11-09 20:11:30 UTC (rev 192170)
@@ -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.
Added: trunk/LayoutTests/editing/inserting/insert-html-crash-01.html (0 => 192170)
--- trunk/LayoutTests/editing/inserting/insert-html-crash-01.html (rev 0)
+++ trunk/LayoutTests/editing/inserting/insert-html-crash-01.html 2015-11-09 20:11:30 UTC (rev 192170)
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+<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>
Modified: trunk/Source/WebCore/ChangeLog (192169 => 192170)
--- trunk/Source/WebCore/ChangeLog 2015-11-09 19:40:39 UTC (rev 192169)
+++ trunk/Source/WebCore/ChangeLog 2015-11-09 20:11:30 UTC (rev 192170)
@@ -1,3 +1,36 @@
+2015-11-09 Jiewen Tan <jiewen_...@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 <mmaxfi...@apple.com>
Some style changes cause tatechuyoko to be drawn off center
Modified: trunk/Source/WebCore/dom/Position.cpp (192169 => 192170)
--- trunk/Source/WebCore/dom/Position.cpp 2015-11-09 19:40:39 UTC (rev 192169)
+++ trunk/Source/WebCore/dom/Position.cpp 2015-11-09 20:11:30 UTC (rev 192170)
@@ -1453,6 +1453,102 @@
#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)
Modified: trunk/Source/WebCore/dom/Position.h (192169 => 192170)
--- trunk/Source/WebCore/dom/Position.h 2015-11-09 19:40:39 UTC (rev 192169)
+++ trunk/Source/WebCore/dom/Position.h 2015-11-09 20:11:30 UTC (rev 192170)
@@ -207,7 +207,10 @@
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;
Modified: trunk/Source/WebCore/editing/CompositeEditCommand.cpp (192169 => 192170)
--- trunk/Source/WebCore/editing/CompositeEditCommand.cpp 2015-11-09 19:40:39 UTC (rev 192169)
+++ trunk/Source/WebCore/editing/CompositeEditCommand.cpp 2015-11-09 20:11:30 UTC (rev 192170)
@@ -1120,7 +1120,7 @@
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();
Modified: trunk/Source/WebCore/editing/VisiblePosition.cpp (192169 => 192170)
--- trunk/Source/WebCore/editing/VisiblePosition.cpp 2015-11-09 19:40:39 UTC (rev 192169)
+++ trunk/Source/WebCore/editing/VisiblePosition.cpp 2015-11-09 20:11:30 UTC (rev 192170)
@@ -743,6 +743,11 @@
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)
Modified: trunk/Source/WebCore/editing/VisiblePosition.h (192169 => 192170)
--- trunk/Source/WebCore/editing/VisiblePosition.h 2015-11-09 19:40:39 UTC (rev 192169)
+++ trunk/Source/WebCore/editing/VisiblePosition.h 2015-11-09 20:11:30 UTC (rev 192170)
@@ -100,6 +100,10 @@
// 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;
