Nullptr crash in CompositeEditCommand::moveParagraphs when root editable element...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Apr 2019 02:12:56 +0000 (02:12 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Apr 2019 02:12:56 +0000 (02:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193027

Reviewed by Wenson Hsieh.

Source/WebCore:

Added an early exit when the root editable element (editing host in HTML5 spec terminology) is null
during CompositeEditCommand::moveParagraphs. This could happen when the website does something crazy
like removing contenteditable content attribute during DOM mutations or when the destination becomes
disconnected (orphaned) from the document due to bugs elsewhere in the codebase.

Test: editing/deleting/merge-paragraphs-null-root-editable-element-crash.html

* editing/CompositeEditCommand.cpp:
(WebCore::CompositeEditCommand::moveParagraphs): Added an early exit.

LayoutTests:

Added a regression test. Note that the test works around debug assertions in moveParagraphs.
These assertions are generally correct & useful unless the website does something crazy like
removing the contenteditable content attribute during editing operations.

* editing/deleting/merge-paragraphs-null-root-editable-element-crash-expected.txt: Added.
* editing/deleting/merge-paragraphs-null-root-editable-element-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/deleting/merge-paragraphs-null-root-editable-element-crash-expected.txt [new file with mode: 0644]
LayoutTests/editing/deleting/merge-paragraphs-null-root-editable-element-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/editing/CompositeEditCommand.cpp

index 74eb041..f6e270d 100644 (file)
@@ -1,3 +1,17 @@
+2019-04-10  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Nullptr crash in CompositeEditCommand::moveParagraphs when root editable element goes away
+        https://bugs.webkit.org/show_bug.cgi?id=193027
+
+        Reviewed by Wenson Hsieh.
+
+        Added a regression test. Note that the test works around debug assertions in moveParagraphs.
+        These assertions are generally correct & useful unless the website does something crazy like
+        removing the contenteditable content attribute during editing operations.
+
+        * editing/deleting/merge-paragraphs-null-root-editable-element-crash-expected.txt: Added.
+        * editing/deleting/merge-paragraphs-null-root-editable-element-crash.html: Added.
+
 2019-04-10  Alicia Boya GarcĂ­a  <aboya@igalia.com>
 
         [GTK] Unreviewed test gardening
diff --git a/LayoutTests/editing/deleting/merge-paragraphs-null-root-editable-element-crash-expected.txt b/LayoutTests/editing/deleting/merge-paragraphs-null-root-editable-element-crash-expected.txt
new file mode 100644 (file)
index 0000000..861cac5
--- /dev/null
@@ -0,0 +1,5 @@
+This test is for the root editable element being nullptr in CompositeEditCommand::moveParagraph. WebKit should not crash.
+
+
+
+PASS. WebKit did not crash
diff --git a/LayoutTests/editing/deleting/merge-paragraphs-null-root-editable-element-crash.html b/LayoutTests/editing/deleting/merge-paragraphs-null-root-editable-element-crash.html
new file mode 100644 (file)
index 0000000..4589848
--- /dev/null
@@ -0,0 +1,28 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This test is for the root editable element being nullptr in CompositeEditCommand::moveParagraph. WebKit should not crash.</p>
+<div id="editor" contenteditable><span id="start">hello</span><span id="end"> world</span><span id="text"> WebKit</span></div>
+<script>
+
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+const frame = document.createElement('iframe');
+start.appendChild(frame);
+frame.contentWindow.addEventListener('unload', () => {
+    editor.removeAttribute('contenteditable');
+    end.innerHTML = '<div><br></div>';
+
+    // Avoid hitting debug assertions in CompositeEditCommand::moveParagraph
+    text.firstChild.data = ' ';
+    text.prepend(document.createElement('br'));
+});
+getSelection().setBaseAndExtent(start, 0, end, 1);
+document.execCommand('delete', false, null);
+
+document.write('PASS. WebKit did not crash');
+
+</script>
+</body>
+</html>
index fdd8ef5..3cefecd 100644 (file)
@@ -1,3 +1,20 @@
+2019-04-10  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Nullptr crash in CompositeEditCommand::moveParagraphs when root editable element goes away
+        https://bugs.webkit.org/show_bug.cgi?id=193027
+
+        Reviewed by Wenson Hsieh.
+
+        Added an early exit when the root editable element (editing host in HTML5 spec terminology) is null
+        during CompositeEditCommand::moveParagraphs. This could happen when the website does something crazy
+        like removing contenteditable content attribute during DOM mutations or when the destination becomes
+        disconnected (orphaned) from the document due to bugs elsewhere in the codebase.
+
+        Test: editing/deleting/merge-paragraphs-null-root-editable-element-crash.html
+
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::moveParagraphs): Added an early exit.
+
 2019-04-10  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: save sheet should be anchored underneath the tab bar when detached
index ebee074..33bd30e 100644 (file)
@@ -1504,9 +1504,12 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
         document().updateLayoutIgnorePendingStylesheets();
     }
 
-    auto editableRoot = destination.rootEditableElement();
-    RefPtr<Range> startToDestinationRange(Range::create(document(), firstPositionInNode(editableRoot), destination.deepEquivalent().parentAnchoredEquivalent()));
-    destinationIndex = TextIterator::rangeLength(startToDestinationRange.get(), true);
+    RefPtr<ContainerNode> editableRoot = destination.rootEditableElement();
+    if (!editableRoot)
+        editableRoot = &document();
+
+    auto startToDestinationRange = Range::create(document(), firstPositionInNode(editableRoot.get()), destination.deepEquivalent().parentAnchoredEquivalent());
+    destinationIndex = TextIterator::rangeLength(startToDestinationRange.ptr(), true);
 
     setEndingSelection(VisibleSelection(destination, originalIsDirectional));
     ASSERT(endingSelection().isCaretOrRange());
@@ -1528,8 +1531,8 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
         // causes spaces to be collapsed during the move operation.  This results
         // in a call to rangeFromLocationAndLength with a location past the end
         // of the document (which will return null).
-        RefPtr<Range> start = TextIterator::rangeFromLocationAndLength(editableRoot, destinationIndex + startIndex, 0, true);
-        RefPtr<Range> end = TextIterator::rangeFromLocationAndLength(editableRoot, destinationIndex + endIndex, 0, true);
+        RefPtr<Range> start = TextIterator::rangeFromLocationAndLength(editableRoot.get(), destinationIndex + startIndex, 0, true);
+        RefPtr<Range> end = TextIterator::rangeFromLocationAndLength(editableRoot.get(), destinationIndex + endIndex, 0, true);
         if (start && end)
             setEndingSelection(VisibleSelection(start->startPosition(), end->startPosition(), DOWNSTREAM, originalIsDirectional));
     }