Bug 222620 – Nullptr crash in Node::isTextNode() via ApplyBlockElementCommand::endOfNextParagraphSplittingTextNodesIfNeeded

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

[Venky Dass]

Crash when running a page.
<rdar://74574192>

[Venky Dass]

<rdar://74574192>

[Radar WebKit Bug Importer]

<rdar://problem/74950677>

[Venky Dass]

Created attachment 422000 [details]
Patch

[Venky Dass]

Created attachment 422010 [details]
test case

[Ryosuke Niwa]

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()))

[Chris Dumez]

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))`
?

[Ryosuke Niwa]

(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.

[Venky Dass]

Created attachment 422449 [details]
Patch

[Ryosuke Niwa]

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.

[Venky Dass]

Created attachment 422474 [details]
Patch

[Ryosuke Niwa]

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.

[Venky Dass]

Created attachment 422489 [details]
Patch

[Venky Dass]

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

[Ryosuke Niwa]

Comment on attachment 422489 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> [phone]  venky dass  <[e-mail]>

Please capitalize your name.

> LayoutTests/ChangeLog:2
> [phone]  venky dass  <[e-mail]>
> +        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

[Venky Dass]

Created attachment 422543 [details]
Patch

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Changing to non-security component since there is no security implication here.

[Venky Dass]

Created attachment 422712 [details]
Patch

[EWS]

Committed r274200: <https://commits.webkit.org/r274200>

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