Nullptr dereference in DeleteSelectionCommand::mergeParagraphs()

[Ian Gilbert]

Description:
Crash found by fuzzing. Reproduces on WebKit revision 265497.

    #0 WebCore::Node::getFlag(WebCore::Node::NodeFlags) const 
    #1 WebCore::DeleteSelectionCommand::mergeParagraphs() 
    #2 WebCore::DeleteSelectionCommand::doApply() 
    #3 WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::DumbPtrTraits<WebCore::EditCommand> >&&) 
    #4 WebCore::CompositeEditCommand::deleteSelection(WebCore::VisibleSelection const&, bool, bool, bool, bool, bool) 
    #5 WebCore::TypingCommand::deleteKeyPressed(WebCore::TextGranularity, bool) 
    #6 WebCore::CompositeEditCommand::apply() 
    #7 WebCore::TypingCommand::deleteKeyPressed(WebCore::Document&, unsigned int, WebCore::TextGranularity) 
    #8 WebCore::executeDelete(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) 
    #9 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) 
    #10 WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)

[Radar WebKit Bug Importer]

<rdar://problem/70986855>

[Ian Gilbert]

Created attachment 413025 [details]
Crashing input

[Ryosuke Niwa]

<rdar://problem/66842923>

[Ryosuke Niwa]

Something like this is supposed to work but there are some test failures:

Index: Source/WebCore/editing/CompositeEditCommand.cpp
===================================================================
--- Source/WebCore/editing/CompositeEditCommand.cpp	(revision 268963)
+++ Source/WebCore/editing/CompositeEditCommand.cpp	(working copy)
@@ -1456,8 +1456,9 @@
     ASSERT(destination.deepEquivalent().anchorNode()->isConnected());
     cleanupAfterDeletion(destination);
 
+    auto newEndingSelection = VisibleSelection(destination, originalIsDirectional);
     // FIXME (Bug 211793): We should redesign cleanupAfterDeletion or find another destination when it is removed.
-    if (!destination.deepEquivalent().anchorNode()->isConnected())
+    if (!destination.deepEquivalent().anchorNode()->isConnected() || newEndingSelection.isNone())
         return;
 
     // Add a br if pruning an empty block level element caused a collapse. For example:
@@ -1482,7 +1483,7 @@
 
     auto destinationIndex = characterCount({ { *editableRoot, 0 }, *makeBoundaryPoint(destination) }, TextIteratorEmitsCharactersBetweenAllVisiblePositions);
 
-    setEndingSelection(VisibleSelection(destination, originalIsDirectional));
+    setEndingSelection(newEndingSelection);
     ASSERT(endingSelection().isCaretOrRange());
     OptionSet<ReplaceSelectionCommand::CommandOption> options { ReplaceSelectionCommand::SelectReplacement, ReplaceSelectionCommand::MovingParagraph };
     if (!preserveStyle)

[Rob Buis]

Created attachment 414166 [details]
Patch

[Rob Buis]

Created attachment 414212 [details]
Patch

[Rob Buis]

Comment on attachment 414212 [details]
Patch

There is probably some overlap with 218492.

[Ryosuke Niwa]

(In reply to Rob Buis from comment #7)
> Comment on attachment 414212 [details]
> Patch
> 
> There is probably some overlap with 218492.

In that case, can we re-land the fix for the bug 218492 first and check if it also fixes this bug or not?

[Rob Buis]

(In reply to Ryosuke Niwa from comment #8)
> (In reply to Rob Buis from comment #7)
> > Comment on attachment 414212 [details]
> > Patch
> > 
> > There is probably some overlap with 218492.
> 
> In that case, can we re-land the fix for the bug 218492 first and check if
> it also fixes this bug or not?

I *think* this is happening (sorry my wording was a bit imprecise):
- 218492 bt is specific to Mac. GTK instead has the bt from 218491 for the test case in 218492.
- so the 218492 fix for Mac had a test case that fixes 218492 on Mac and thus equals it to GTK status, i.e. still hitting 218491.

Hope that clears things up. If I am right fixing 218491 first and then relanding the 218492 fix should fix both bugs.

[Ryosuke Niwa]

Ah ok.

[EWS]

Committed r269894: <https://trac.webkit.org/changeset/269894>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414212 [details].