| Summary: | Crash in CompositeEditCommand::splitTreeToNode via InsertParagraphSeparatorCommand::doApply | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||
| Component: | HTML Editing | Assignee: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, product-security, rbuis, svillar, 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=221387 https://bugs.webkit.org/show_bug.cgi?id=224977 |
||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Ryosuke Niwa
2021-05-01 00:29:24 PDT
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]. |