Summary: | Nullptr crash in Node::isTextNode() via ApplyBlockElementCommand::endOfNextParagraphSplittingTextNodesIfNeeded | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Venky Dass <yaranamavenkataramana> | ||||||||||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bfulgham, cdumez, ews-feeder, ews-watchlist, mifenton, product-security, rniwa, webkit-bug-importer, wenson_hsieh | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | Safari 12 | ||||||||||||||||||
Hardware: | Mac (Intel) | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Venky Dass
2021-03-02 14:58:43 PST
Created attachment 422000 [details]
Patch
Created attachment 422010 [details]
test case
Comment on attachment 422000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422000&action=review r- due to the lack of a test. > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). Please add a test. > Source/WebCore/editing/ApplyBlockElementCommand.cpp:294 > + if (previousSiblingOfText && is<Text>(*text->previousSibling()) is<Text>(*text->previousSibling()) -> is<Text>(is<Text>(*text->previousSibling())) Comment on attachment 422000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422000&action=review >> Source/WebCore/editing/ApplyBlockElementCommand.cpp:294 >> + if (previousSiblingOfText && is<Text>(*text->previousSibling()) > > is<Text>(*text->previousSibling()) -> is<Text>(is<Text>(*text->previousSibling())) Did you mean: `if (is<Text>(previousSiblingOfText))` ? (In reply to Chris Dumez from comment #6) > Comment on attachment 422000 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=422000&action=review > > >> Source/WebCore/editing/ApplyBlockElementCommand.cpp:294 > >> + if (previousSiblingOfText && is<Text>(*text->previousSibling()) > > > > is<Text>(*text->previousSibling()) -> is<Text>(is<Text>(*text->previousSibling())) > > Did you mean: > `if (is<Text>(previousSiblingOfText))` > ? Ugh... yes, copy & paste error. Created attachment 422449 [details]
Patch
Comment on attachment 422449 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422449&action=review > Source/WebCore/ChangeLog:7 > + In function ApplyBlockElementCommand::endOfNextParagraphSplittingTextNodes(..) there is a > + null check missing when using the previous silbing call which is causing the crash. > + the return from previousSiblingOfText. > + > + https://bugs.webkit.org/show_bug.cgi?id=222620 This is not the right format. See other entries in the change log. The line above bugzilla URL should match bug's title, and this description should appear below "reviewed by ~" line below. > LayoutTests/ChangeLog:6 > + In function ApplyBlockElementCommand::endOfNextParagraphSplittingTextNodes(..) there is a > + null check missing when using the previous silbing call which is causing the crash. > + the return from previousSiblingOfText. > + > + https://bugs.webkit.org/show_bug.cgi?id=222620 Ditto. We should be saying something like "Adding a regression test" instead. > LayoutTests/ChangeLog:11 > + * editing/inserting/scrollingelement-indent-crash-expected.txt: Added. > + * editing/inserting/scrollingelement-indent-crash.html: Added. This is nothing to do with scrolling element. It's about the text node not having a previous sibling. Probably something like: indent-split-text-not-having-previous-sibling-crash.html or something like that. > LayoutTests/editing/inserting/scrollingelement-indent-crash.html:3 > +.class8,input:required,#x12:read-only { white-space: pre-line;-webkit-text-stroke: 100%;-webkit-user-modify: read-only;contain: } We should reduce this test case further. Created attachment 422474 [details]
Patch
Comment on attachment 422474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422474&action=review > LayoutTests/editing/inserting/indent-split-text-not-having-previous-sibling-crash.html:6 > + testRunner.dumpAsText(); Nit: Wrong indentation. You need to use spaces, not tab characters. Created attachment 422489 [details]
Patch
removed the extra spaces in the html file. Not sure why the following script could not catch it. Tools/Scripts/check-webkit-style Total errors found: 0 in 5 files Comment on attachment 422489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422489&action=review > Source/WebCore/ChangeLog:1 > +2021-03-05 venky dass <yaranamavenkataramana@apple.com> Please capitalize your name. > LayoutTests/ChangeLog:2 > +2021-03-05 venky dass <yaranamavenkataramana@apple.com> > + Nullptr crash in Node::isTextNode() via ApplyBlockElementCommand::endOfNextParagraphSplittingTextNodesIfNeeded You need a blank line between these. two lines. > LayoutTests/ChangeLog:10 > + > + Adding a regression test. This description must appear before the list of files but below "Reviewed by" line Created attachment 422543 [details]
Patch
Comment on attachment 422543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422543&action=review > LayoutTests/editing/inserting/indent-split-text-not-having-previous-sibling-crash.html:7 > + if (window.testRunner) { > + testRunner.dumpAsText(); > + } Nit: no curly braces around a single line statement: https://webkit.org/code-style-guidelines/#braces-one-line Changing to non-security component since there is no security implication here. Created attachment 422712 [details]
Patch
Committed r274200: <https://commits.webkit.org/r274200> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422712 [details]. |