Bug 225267 – Crash in CompositeEditCommand::splitTreeToNode via InsertParagraphSeparatorCommand::doApply

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Created attachment 427490 [details]
Test

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010d479fef WebCore::Node::ref() const + 0 (Node.h:780) [inlined]
1   com.apple.WebCore             	0x000000010d479fef WTF::DefaultRefDerefTraits<WebCore::Node>::refIfNotNull(WebCore::Node*) + 0 (RefPtr.h:36) [inlined]
2   com.apple.WebCore             	0x000000010d479fef WTF::RefPtr<WebCore::Node, WTF::RawPtrTraits<WebCore::Node>, WTF::DefaultRefDerefTraits<WebCore::Node> >::RefPtr(WebCore::Node*) + 0 (RefPtr.h:63) [inlined]
3   com.apple.WebCore             	0x000000010d479fef WTF::RefPtr<WebCore::Node, WTF::RawPtrTraits<WebCore::Node>, WTF::DefaultRefDerefTraits<WebCore::Node> >::RefPtr(WebCore::Node*) + 0 (RefPtr.h:63) [inlined]
4   com.apple.WebCore             	0x000000010d479fef WTF::RefPtr<WebCore::Node, WTF::RawPtrTraits<WebCore::Node>, WTF::DefaultRefDerefTraits<WebCore::Node> >::operator=(WebCore::Node*) + 0 (RefPtr.h:153) [inlined]
5   com.apple.WebCore             	0x000000010d479fef WebCore::CompositeEditCommand::splitTreeToNode(WebCore::Node&, WebCore::Node&, bool) + 79 (CompositeEditCommand.cpp:1751)
6   com.apple.WebCore             	0x000000010d4cbafd WebCore::InsertParagraphSeparatorCommand::doApply() + 6637 (InsertParagraphSeparatorCommand.cpp:396)
7   com.apple.WebCore             	0x000000010d473ea8 WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::RawPtrTraits<WebCore::EditCommand> >&&) + 40 (CompositeEditCommand.cpp:488)
8   com.apple.WebCore             	0x000000010d4743d9 WebCore::CompositeEditCommand::insertParagraphSeparator(bool, bool) + 89 (CompositeEditCommand.cpp:529)
9   com.apple.WebCore             	0x000000010d4e2c96 WebCore::ReplaceSelectionCommand::doApply() + 14134 (ReplaceSelectionCommand.cpp:1426)
10  com.apple.WebCore             	0x000000010d4631e7 WebCore::CompositeEditCommand::apply() + 167 (CompositeEditCommand.cpp:397)
11  com.apple.WebCore             	0x000000010d4972c4 WebCore::Editor::replaceSelectionWithFragment(WebCore::DocumentFragment&, WebCore::Editor::SelectReplacement, WebCore::Editor::SmartReplace, WebCore::Editor::MatchStyle, WebCore::EditAction, WebCore::MailBlockquoteHandling) + 868 (Editor.cpp:694)
12  com.apple.WebCore             	0x000000010d497a86 WebCore::Editor::replaceSelectionWithText(WTF::String const&, WebCore::Editor::SelectReplacement, WebCore::Editor::SmartReplace, WebCore::EditAction) + 118 (Editor.cpp:737)
13  com.apple.WebCore             	0x000000010d496ef9 WebCore::Editor::handleTextEvent(WebCore::TextEvent&) + 201 (Editor.cpp:345)
14  com.apple.WebCore             	0x000000010d96633f WebCore::EventHandler::defaultTextInputEventHandler(WebCore::TextEvent&) + 31 (EventHandler.cpp:4153)
15  com.apple.WebCore             	0x000000010d3cd353 WebCore::callDefaultEventHandlersInBubblingOrder(WebCore::Event&, WebCore::EventPath const&) + 39 (EventDispatcher.cpp:63) [inlined]
16  com.apple.WebCore             	0x000000010d3cd353 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) + 1779 (EventDispatcher.cpp:204)
17  com.apple.WebCore             	0x000000010d499699 WebCore::Editor::pasteAsPlainText(WTF::String const&, bool) + 217 (Editor.cpp:617)
18  com.apple.WebCore             	0x000000010d499aa9 WebCore::Editor::pasteAsPlainTextWithPasteboard(WebCore::Pasteboard&) + 361 (Editor.cpp:637)
19  com.apple.WebCore             	0x000000010d4a0bcc WebCore::Editor::pasteAsPlainText(WebCore::Editor::FromMenuOrKeyBinding) + 412 (Editor.cpp:1489)
20  com.apple.WebCore             	0x000000010d4c22b3 WebCore::executePasteAndMatchStyle(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 51 (EditorCommand.cpp:935)
21  com.apple.WebCore             	0x000000010d390ccc WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 76 (Document.cpp:5870)

<rdar://77377890>

[Ryosuke Niwa]

Reproduced with non-ASAN release build at r276747.

[Frédéric Wang (:fredw)]

I can reproduce too. In debug, we hit the following assert. In release, it's a nullptr dereference on the next line. I'll investigate. 

ASSERT(splitTo);
splitTreeToNode(*splitTo, *startBlock);

[Frédéric Wang (:fredw)]

This is the tree when the crashes happen. H

insertionPosition.containerNode() == TEXT
startBlock == 


#document	0x7f2e6e803040 (renderer 0x7f2e6e8012e0) 
	HTML	0x7f2e6e803ec0 (renderer 0x7f2e6e801a20) 
		HEAD	0x7f2e6e7f9770 (renderer 0x7f2e6e7f83f0) 
			#text	0x7f2e6e7f9800 "onload = () => { document.execCommand('SelectAll'); document.execCommand('Copy'); document.execCommand('SelectAll'); document.designMode = 'on'; document.execCommand('PasteAndMatchStyle'); };"
		DIV	0x7f2e6e7f9e30 (renderer 0x7f2e6e7f9ab0) 
			BR	0x7f2e6e7f9ec0 (renderer 0x7f2e6e7f99d0) 
		BODY	0x7f2e6e7f8360 (renderer 0x7f2e6e801b40) 
		STYLE	0x7f2e6e7f8010 (renderer (nil)) 
			#text	0x7f2e6e7f80d0 "\n  head, script {\n    display: table;\n  }\n"

[Frédéric Wang (:fredw)]

This is the tree when the crashes happen. H

insertionPosition.containerNode() == TEXT
startBlock == HEAD

so NodeTraversal::next returns nullptr.

HEAD is actually a display: table.

#document	0x7f2e6e803040 (renderer 0x7f2e6e8012e0) 
	HTML	0x7f2e6e803ec0 (renderer 0x7f2e6e801a20) 
		HEAD	0x7f2e6e7f9770 (renderer 0x7f2e6e7f83f0) 
			  # text	0x7f2e6e7f9800 "onload = () => { document.execCommand('SelectAll'); document.execCommand('Copy'); document.execCommand('SelectAll'); document.designMode = 'on'; document.execCommand('PasteAndMatchStyle'); };"
		DIV	0x7f2e6e7f9e30 (renderer 0x7f2e6e7f9ab0) 
			BR	0x7f2e6e7f9ec0 (renderer 0x7f2e6e7f99d0) 
		BODY	0x7f2e6e7f8360 (renderer 0x7f2e6e801b40) 
		STYLE	0x7f2e6e7f8010 (renderer (nil)) 
			#text	0x7f2e6e7f80d0 "\n  head, script {\n    display: table;\n  }\n"

[Frédéric Wang (:fredw)]

Created attachment 428919 [details]
Tentative patch

[Ryosuke Niwa]

Comment on attachment 428919 [details]
Tentative patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428919&action=review

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:169
> +        || isRenderedTable(startBlock.get())

What is the rational behind this change?

[Frédéric Wang (:fredw)]

Comment on attachment 428919 [details]
Tentative patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428919&action=review

>> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:169
>> +        || isRenderedTable(startBlock.get())
> 
> What is the rational behind this change?

To be honest it was kind of a guess, based on comment 2 and comment 4 where the startBlock has display: table. This is similar to isTableCell at the next line or other similar tweaks I did in other bugs. However, I probably need more analysis to decide what would be the "right" insertion. 

BTW, this patch also works for bug 224977.

[Ryosuke Niwa]

Comment on attachment 428919 [details]
Tentative patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428919&action=review

>>> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:169
>>> +        || isRenderedTable(startBlock.get())
>> 
>> What is the rational behind this change?
> 
> To be honest it was kind of a guess, based on comment 2 and comment 4 where the startBlock has display: table. This is similar to isTableCell at the next line or other similar tweaks I did in other bugs. However, I probably need more analysis to decide what would be the "right" insertion. 
> 
> BTW, this patch also works for bug 224977.

I guess we can land this for now since it does fix the crash.
Could you post a real patch with a test & change logs?

[Frédéric Wang (:fredw)]

Created attachment 429517 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 429517 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429517&action=review

> LayoutTests/ChangeLog:8
> +        Add regression test.
> +
> +        Reviewed by NOBODY (OOPS!).

The order of these need to be swapped.

> LayoutTests/ChangeLog:12
> +        * fast/editing/paste-and-match-style-with-table-2-crash.html: Added.
>  2021-05-21  Diego Pino Garcia  <[e-mail]>

Need a blank line here.

[Frédéric Wang (:fredw)]

Created attachment 429636 [details]
Patch for landing

[EWS]

Invalid ChangeLog at /Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog

[Frédéric Wang (:fredw)]

Created attachment 429637 [details]
Patch for landing

[EWS]

Committed r278002 (238115@main): <https://commits.webkit.org/238115@main>

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