Bug 211793

Summary: Nullptr crash in DeleteSelectionCommand::doApply() when merge node is disconnected.
Product: WebKit Reporter: Jack <shihchieh_lee>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, ews-feeder, ggaren, product-security, rniwa, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=223753
Attachments:
Description Flags
Patch none

Jack
Reported 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
Attachments
Patch (5.08 KB, patch)
2020-05-12 15:14 PDT, Jack 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Jack
Comment 1 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
Jack
Comment 2 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.
Jack
Comment 3 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.
Jack
Comment 4 2020-05-12 15:14:22 PDT
Created attachment 399186 [details] Patch
Geoffrey Garen
Comment 5 2020-05-13 16:46:17 PDT
Comment on attachment 399186 [details] Patch r=me
EWS
Comment 6 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].
Note You need to log in before you can comment on or make changes to this bug.