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>
Reproduced with non-ASAN release build at r276747.
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);
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"
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"
Created attachment 428919 [details] Tentative patch
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?
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.
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?
Created attachment 429517 [details] Patch
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 <dpino@igalia.com> Need a blank line here.
Created attachment 429636 [details] Patch for landing
Invalid ChangeLog at /Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog
Created attachment 429637 [details] Patch for landing
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].