Null dereference loading Blink layout test editing/execCommand/delete-hidden-crash...
authorjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Nov 2015 00:46:34 +0000 (00:46 +0000)
committerjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Nov 2015 00:46:34 +0000 (00:46 +0000)
https://bugs.webkit.org/show_bug.cgi?id=149289
<rdar://problem/22746352>

Reviewed by Enrica Casucci.

Source/WebCore:

This is a merge of Blink r176497:
https://codereview.chromium.org/340713003

It ensures the start & end positions in DeleteSelectionCommand::initializePositionData
are editable.

Test: editing/execCommand/delete-hidden-crash.html

* editing/DeleteSelectionCommand.cpp:
(WebCore::DeleteSelectionCommand::initializePositionData):
* editing/Editor.cpp:
(WebCore::Editor::advanceToNextMisspelling):
* editing/htmlediting.cpp:
(WebCore::firstEditablePositionAfterPositionInRoot):
(WebCore::lastEditablePositionBeforePositionInRoot):
These two functions don't make any sense to return VisiblePosition. Change them
to return Position instead. Since there is a viable conversion from Position to
VisiblePosition. It should not change the behavior of any other components depending
on it.
* editing/htmlediting.h:

LayoutTests:

* editing/execCommand/delete-hidden-crash-expected.txt: Added.
* editing/execCommand/delete-hidden-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/execCommand/delete-hidden-crash-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/delete-hidden-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/editing/DeleteSelectionCommand.cpp
Source/WebCore/editing/Editor.cpp
Source/WebCore/editing/htmlediting.cpp
Source/WebCore/editing/htmlediting.h

index 2e4ddd6..b3dac1c 100644 (file)
@@ -1,3 +1,14 @@
+2015-11-04  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Null dereference loading Blink layout test editing/execCommand/delete-hidden-crash.html
+        https://bugs.webkit.org/show_bug.cgi?id=149289
+        <rdar://problem/22746352>
+
+        Reviewed by Enrica Casucci.
+
+        * editing/execCommand/delete-hidden-crash-expected.txt: Added.
+        * editing/execCommand/delete-hidden-crash.html: Added.
+
 2015-11-03  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Ruby base ending in tatechuyoko forces a line break before the tatechuyoko
diff --git a/LayoutTests/editing/execCommand/delete-hidden-crash-expected.txt b/LayoutTests/editing/execCommand/delete-hidden-crash-expected.txt
new file mode 100644 (file)
index 0000000..2afa0bf
--- /dev/null
@@ -0,0 +1 @@
+PASS. WebKit didn't crash.
diff --git a/LayoutTests/editing/execCommand/delete-hidden-crash.html b/LayoutTests/editing/execCommand/delete-hidden-crash.html
new file mode 100644 (file)
index 0000000..9034b0f
--- /dev/null
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../editing.js"></script>
+
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+function editingTest() {
+    document.execCommand('SelectAll')
+    span = document.getElementsByTagName('span')[0];
+    span.contentEditable = 'true';
+    span.textContent = 'bar';
+    document.execCommand('InsertText', false, '1');
+    document.body.textContent = "PASS. WebKit didn't crash.";
+};
+</script>
+
+<style>
+* { visibility:visible; }
+.inline { display:inline; }
+* :only-child { visibility:hidden; }
+</style>
+</head>
+<body onload=editingTest()>
+<table>
+<tr><td><span><table></table><span></span></span></td></tr>
+<tr><td><td class="inline">foo</td></tr>
+</table>
+</body>
+</html>
index fc491ae..00edd4d 100644 (file)
@@ -1,3 +1,32 @@
+2015-11-04  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Null dereference loading Blink layout test editing/execCommand/delete-hidden-crash.html
+        https://bugs.webkit.org/show_bug.cgi?id=149289
+        <rdar://problem/22746352>
+
+        Reviewed by Enrica Casucci.
+
+        This is a merge of Blink r176497:
+        https://codereview.chromium.org/340713003
+
+        It ensures the start & end positions in DeleteSelectionCommand::initializePositionData
+        are editable.
+
+        Test: editing/execCommand/delete-hidden-crash.html
+
+        * editing/DeleteSelectionCommand.cpp:
+        (WebCore::DeleteSelectionCommand::initializePositionData):
+        * editing/Editor.cpp:
+        (WebCore::Editor::advanceToNextMisspelling):
+        * editing/htmlediting.cpp:
+        (WebCore::firstEditablePositionAfterPositionInRoot):
+        (WebCore::lastEditablePositionBeforePositionInRoot):
+        These two functions don't make any sense to return VisiblePosition. Change them
+        to return Position instead. Since there is a viable conversion from Position to
+        VisiblePosition. It should not change the behavior of any other components depending
+        on it.
+        * editing/htmlediting.h:
+
 2015-11-03  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Ruby base ending in tatechuyoko forces a line break before the tatechuyoko
index 9689e19..118ce36 100644 (file)
@@ -177,6 +177,11 @@ void DeleteSelectionCommand::initializePositionData()
     Position start, end;
     initializeStartEnd(start, end);
     
+    if (!isEditablePosition(start, ContentIsEditable))
+        start = firstEditablePositionAfterPositionInRoot(start, highestEditableRoot(start));
+    if (!isEditablePosition(end, ContentIsEditable))
+        end = lastEditablePositionBeforePositionInRoot(end, highestEditableRoot(start));
+
     m_upstreamStart = start.upstream();
     m_downstreamStart = start.downstream();
     m_upstreamEnd = end.upstream();
index ed0de37..d4b0fad 100644 (file)
@@ -1902,7 +1902,7 @@ void Editor::advanceToNextMisspelling(bool startBeforeSelection)
         // when spell checking the whole document before sending the message.
         // In that case the document might not be editable, but there are editable pockets that need to be spell checked.
 
-        position = firstEditablePositionAfterPositionInRoot(position, document().documentElement()).deepEquivalent();
+        position = VisiblePosition(firstEditablePositionAfterPositionInRoot(position, document().documentElement())).deepEquivalent();
         if (position.isNull())
             return;
         
index 9229d79..f949092 100644 (file)
@@ -285,7 +285,7 @@ Position previousVisuallyDistinctCandidate(const Position& position)
     return Position();
 }
 
-VisiblePosition firstEditablePositionAfterPositionInRoot(const Position& position, Node* highestRoot)
+Position firstEditablePositionAfterPositionInRoot(const Position& position, Node* highestRoot)
 {
     // position falls before highestRoot.
     if (comparePositions(position, firstPositionInNode(highestRoot)) == -1 && highestRoot->hasEditableStyle())
@@ -296,7 +296,7 @@ VisiblePosition firstEditablePositionAfterPositionInRoot(const Position& positio
     if (&position.deprecatedNode()->treeScope() != &highestRoot->treeScope()) {
         Node* shadowAncestor = highestRoot->treeScope().ancestorInThisScope(p.deprecatedNode());
         if (!shadowAncestor)
-            return VisiblePosition();
+            return Position();
 
         p = positionAfterNode(shadowAncestor);
     }
@@ -305,12 +305,12 @@ VisiblePosition firstEditablePositionAfterPositionInRoot(const Position& positio
         p = isAtomicNode(p.deprecatedNode()) ? positionInParentAfterNode(p.deprecatedNode()) : nextVisuallyDistinctCandidate(p);
     
     if (p.deprecatedNode() && p.deprecatedNode() != highestRoot && !p.deprecatedNode()->isDescendantOf(highestRoot))
-        return VisiblePosition();
+        return Position();
     
-    return VisiblePosition(p);
+    return p;
 }
 
-VisiblePosition lastEditablePositionBeforePositionInRoot(const Position& position, Node* highestRoot)
+Position lastEditablePositionBeforePositionInRoot(const Position& position, Node* highestRoot)
 {
     // When position falls after highestRoot, the result is easy to compute.
     if (comparePositions(position, lastPositionInNode(highestRoot)) == 1)
@@ -321,7 +321,7 @@ VisiblePosition lastEditablePositionBeforePositionInRoot(const Position& positio
     if (&position.deprecatedNode()->treeScope() != &highestRoot->treeScope()) {
         Node* shadowAncestor = highestRoot->treeScope().ancestorInThisScope(p.deprecatedNode());
         if (!shadowAncestor)
-            return VisiblePosition();
+            return Position();
 
         p = firstPositionInOrBeforeNode(shadowAncestor);
     }
@@ -330,9 +330,9 @@ VisiblePosition lastEditablePositionBeforePositionInRoot(const Position& positio
         p = isAtomicNode(p.deprecatedNode()) ? positionInParentBeforeNode(p.deprecatedNode()) : previousVisuallyDistinctCandidate(p);
     
     if (p.deprecatedNode() && p.deprecatedNode() != highestRoot && !p.deprecatedNode()->isDescendantOf(highestRoot))
-        return VisiblePosition();
+        return Position();
     
-    return VisiblePosition(p);
+    return p;
 }
 
 // FIXME: The method name, comment, and code say three different things here!
index 10ced02..9fe5f47 100644 (file)
@@ -161,6 +161,9 @@ inline Position lastPositionInOrAfterNode(Node* node)
     return editingIgnoresContent(node) ? positionAfterNode(node) : lastPositionInNode(node);
 }
 
+Position firstEditablePositionAfterPositionInRoot(const Position&, Node*);
+Position lastEditablePositionBeforePositionInRoot(const Position&, Node*);
+
 // comparision functions on Position
     
 int comparePositions(const Position&, const Position&);
@@ -185,9 +188,7 @@ void updatePositionForNodeRemoval(Position&, Node&);
 // -------------------------------------------------------------------------
     
 // Functions returning VisiblePosition
-    
-VisiblePosition firstEditablePositionAfterPositionInRoot(const Position&, Node*);
-VisiblePosition lastEditablePositionBeforePositionInRoot(const Position&, Node*);
+
 VisiblePosition visiblePositionBeforeNode(Node*);
 VisiblePosition visiblePositionAfterNode(Node*);