Bug 211793 - Nullptr crash in DeleteSelectionCommand::doApply() when merge node is disconnected.
Summary: Nullptr crash in DeleteSelectionCommand::doApply() when merge node is disconn...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-12 12:17 PDT by Jack
Modified: 2020-05-13 17:21 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.08 KB, patch)
2020-05-12 15:14 PDT, Jack
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jack 2020-05-12 12:17:27 PDT
<rdar://62993645>

0   com.apple.WebCore             	0x0000000771431195 WebCore::CompositeEditCommand::insertNodeAt(WTF::Ref<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >&&, WebCore::Position const&) + 85
1   com.apple.WebCore             	0x00000007701ca94b WebCore::DeleteSelectionCommand::doApply() + 13531
2   com.apple.WebCore             	0x00000007714454ff WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::DumbPtrTraits<WebCore::EditCommand> >&&) + 79
3   com.apple.WebCore             	0x00000007701b3700 WebCore::InsertTextCommand::doApply() + 2336
4   com.apple.WebCore             	0x0000000771445815 WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::CompositeEditCommand, WTF::DumbPtrTraits<WebCore::CompositeEditCommand> >&&, WebCore::VisibleSelection const&) + 101
5   com.apple.WebCore             	0x00000007701b2d1e WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool) + 222
6   com.apple.WebCore             	0x00000007714f007d WebCore::TypingCommand::insertTextAndNotifyAccessibility(WTF::String const&, bool) + 365
7   com.apple.WebCore             	0x00000007701b28fd WebCore::CompositeEditCommand::apply() + 397
8   com.apple.WebCore             	0x00000007714efd53 WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, WebCore::VisibleSelection const&, unsigned int, WebCore::TypingCommand::TextCompositionType) + 1027
9   com.apple.WebCore             	0x000000077149fc4f WebCore::executeInsertText(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 143
10  com.apple.WebCore             	0x00000007702195f1 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 81
11  com.apple.WebCore             	0x00000007706121ca WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 426
12
Comment 1 Jack 2020-05-12 12:17:45 PDT
<body contentEditable="true"><output></output><data style="writing-mode: vertical-lr;"><menu><table></table></menu><script>
    document.execCommand("selectAll", false);
    document.execCommand("insertText", "text");
</script>

Root cause:
1. In this test case, we are processing InserText command and need to delete the selected range first.     
2. In handleGeneralDelete(), OUTPUT is removed.
3. After the deletion is done, we call mergeParagraphs() and decide to move TABLE (start and end of paragraphToMove) to DATA (mergeDestination).
4. In CompositeEditCommand::moveParagraphs, TABLE is deleted and we call cleanupAfterDeletion and pass DATA to the function.
5. In removeNodeAndPruneAncestors, DATA is also removed when we prune the ancestors, making DATA a dangling node.
6. Later, DATA is used to set the endingSelection() which turns out to be an empty position becuase DATA fails the isCandidate() check, due to having null renderer from being removed.
7. At the end of DeleteSelectionCommand::mergeParagraphs, endingSelection is used to set m_endingPosition, which is used for inserting a BR element as a place holder.
8. Since m_endingPosition is empty, the insertion deref the null anchor node and crashes.

Node tree of the above steps:
1. Node tree and initial selected range before handleGeneralDelete() is called:
BODY	0x4181e3060 (renderer 0x4181e1490) 
S	OUTPUT	0x4181e30e0 (renderer 0x4181e3450) 
	DATA	0x4181e31a0 (renderer 0x4181e3510)  STYLE=writing-mode: vertical-lr;
		MENU	0x4181e3220 (renderer 0x4181e3620) 
E			TABLE	0x4181e32a0 (renderer 0x4181e3730) 
		SCRIPT	0x4181e3330 (renderer 0x0) 
			#text	0x4181e33f0 "\n    document.execCommand("selectAll", false);\n    document.execCommand("insertText", "text");\n"
start: before, offset:0
end: before, offset:0

3. In mregeParagraphs(), both the start and end of ParagraphToMove is TABLE:
BODY	0x4181e3060 (renderer 0x4181e1490) 
	DATA	0x4181e31a0 (renderer 0x4181e3510)  STYLE=writing-mode: vertical-lr;
		MENU	0x4181e3220 (renderer 0x4181e3620) 
*			TABLE	0x4181e32a0 (renderer 0x4181e3730) 
		SCRIPT	0x4181e3330 (renderer 0x0) 
			#text	0x4181e33f0 "\n    document.execCommand("selectAll", false);\n    document.execCommand("insertText", "text");\n"
before, offset:0

and the mergeDestination is found to be Data:
BODY	0x4181e3060 (renderer 0x4181e1490) 
*	DATA	0x4181e31a0 (renderer 0x4181e3510)  STYLE=writing-mode: vertical-lr;
		MENU	0x4181e3220 (renderer 0x4181e3620) 
			TABLE	0x4181e32a0 (renderer 0x4181e3730) 
		SCRIPT	0x4181e3330 (renderer 0x0) 
			#text	0x4181e33f0 "\n    document.execCommand("selectAll", false);\n    document.execCommand("insertText", "text");\n"
offset, offset:0


5. After cleanupAfterDeletion is called in CompositeEditCommand::moveParagraphs, destination becomes dangling.
*DATA	0x4181e31a0 (renderer 0x0)  STYLE=writing-mode: vertical-lr;
	MENU	0x4181e3220 (renderer 0x0) 
	SCRIPT	0x4181e3330 (renderer 0x0) 
		#text	0x4181e33f0 "\n    document.execCommand("selectAll", false);\n    document.execCommand("insertText", "text");\n"
offset, offset:0
Comment 2 Jack 2020-05-12 12:23:03 PDT
Discussions with Geoff and Wenson:

Observations:
First of all, having same start and end positions in moveParagraphs() is weird. And with nothing to be deleted, what is there to be cleaned up?  We also went through history and found that initially cleanupAfterDeletion doesn’t take in any argument. Later, destination was added to handle an infinite loop: https://bugs.webkit.org/attachment.cgi?id=85395&action=prettypatch. There was no mentioning how pruneAncestor would not remove destination.

Questions:
1. Should we skip deleting and cleanup if start=end of paragraph? 
2. Since the pruning of ancestor is done on endingSelection() node, is there a strong assumption that endingSelection() should have different ancestor or some boundary that walking the ancestor tree will not cross destination?
3. If so, does that mean the logic of finding destination is faulty?

Next step, a few options and concerns:
One way to patch this in cleanupAfterDeletion is to call highestNodeToRemoveInPruning before calling removeNodeAndPruneAncestors. However, it is going to impact performance as highestNodeToRemoveInPruning is again called in removeNodeAndPruneAncestors and it walks through all the nodes.

Besides, doing so may change the behavior of another usage when destination is checked after the call and bail out (MoveSelectionCommand::doApply). And there is no effect for the other case in when destination is null (CompositeEditCommand::moveParagraphWithClones).

Lastly, skipping delete and cleanup when start=end may also alter the behavior.
Comment 3 Jack 2020-05-12 12:24:18 PDT
Solution:
Check for disconnection after calling cleanupAfterDeletion(), since cleanupAfterDeletion() removes nodes from the document.

In the long run, we probably want a design where either cleanupAfterDeletion() does not remove destination, or we select a destination after cleanup — so that editing commands are guaranteed to succeed.
Comment 4 Jack 2020-05-12 15:14:22 PDT
Created attachment 399186 [details]
Patch
Comment 5 Geoffrey Garen 2020-05-13 16:46:17 PDT
Comment on attachment 399186 [details]
Patch

r=me
Comment 6 EWS 2020-05-13 17:21:55 PDT
Committed r261664: <https://trac.webkit.org/changeset/261664>

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