Bug 210016 - Nullptr crash in WebCore::lastPositionInNode when indenting text node that has user-select:all parent.
Summary: Nullptr crash in WebCore::lastPositionInNode when indenting text node that ha...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Jack
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-04 21:52 PDT by Jack
Modified: 2020-04-06 18:46 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.27 KB, patch)
2020-04-04 22:06 PDT, Jack
no flags Details | Formatted Diff | Diff
Patch (6.26 KB, patch)
2020-04-04 22:08 PDT, Jack
no flags Details | Formatted Diff | Diff
Patch (5.19 KB, patch)
2020-04-06 13:03 PDT, Jack
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jack 2020-04-04 21:52:27 PDT
<rdar://61014577>

    #0 0x3b81dea51 in WebCore::Node::getFlag(WebCore::Node::NodeFlags) const (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x1dca51)
    #1 0x3ba9fa7f4 in WebCore::lastPositionInNode(WebCore::Node*) (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x29f87f4)
    #2 0x3bb4f1edb in WebCore::ApplyBlockElementCommand::rangeForParagraphSplittingTextNodesIfNeeded(WebCore::VisiblePosition const&, WebCore::Position&, WebCore::Position&) (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34efedb)
    #3 0x3bb4f0a3e in WebCore::ApplyBlockElementCommand::formatSelection(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&) (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34eea3e)
    #4 0x3bb4efe83 in WebCore::ApplyBlockElementCommand::doApply() (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34ede83)
    #5 0x3bb4eeba6 in WebCore::CompositeEditCommand::apply() (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34ecba6)
    #6 0x3bb5a4278 in WebCore::executeIndent(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x35a2278)
    #7 0x3bb21f127 in WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)
Comment 1 Jack 2020-04-04 21:52:55 PDT
Root cause: In function rangeForParagraphSplittingTextNodesIfNeeded, the attemt to split text node fails, so there is no sibling node being created. However, the code assumes sibling exists and deref it in lastPositionInNode().

1. In this test, formatSelection is called to indent all the elements and nodes in <body>.
2. To determine which paragraph to indent, fuction RangeForParagraphSplittingTextNodesIfNeeded is called.
3. For text node in pre element, because there is newline in it, fucntion splitTextNode is called to create a new text node for “\n”.
4. However, since <details> and <pre> has use-select set to all, which is considered uneditable, splitTextNode returns without creating a sibling text node.
5. Next we call functions firstPositionInOrBeforeNode and lastPositionInNode with the null sibling node and crashes.

<style>
    #DETAILS { -webkit-user-select: all; }
</style>
<script>
    window.onload = () => {
        document.execCommand("selectAll", false);
        document.execCommand("indent", false);
    }
</script>
<body contentEditable="true"><br></br><details id=DETAILS open="true"><pre>a
</pre></details><span>a</span>

Node tree before  indentation:
BODY	0x60c0000814c0 (renderer 0x612000061cc0) 
	BR	0x60c000081580 (renderer 0x6110000c2f00) 
	BR	0x60c000081640 (renderer 0x6110000c3040) 
	DETAILS	0x60e00005ce00 (renderer 0x612000061e40) 
		#document-fragment	0x61200005a7c0 (renderer 0x0)  (needs style recalc) (child needs style recalc)
			SLOT	0x60d000063e70 (renderer 0x0) 
				SUMMARY	0x60c0000817c0 (renderer 0x612000062140) 
					#document-fragment	0x61200005a940 (renderer 0x0)  (needs style recalc) (child needs style recalc)
						DIV	0x60c000081880 (renderer 0x6120000622c0) 
						SLOT	0x60d000063f40 (renderer 0x0) 
					#text	0x60800004d220 "Details"
			SLOT	0x60d000064280 (renderer 0x0) 
		PRE	0x60c000081b80 (renderer 0x612000062440) 
			#text	0x60800004d2a0 "a\n"
	SPAN	0x60c000081c40 (renderer 0x6110000c3180) 
*		#text	0x60800004d320 "a"
	#text	0x60800004d3a0 "\n"
legacy, offset, offset:1

Comment 2 Jack 2020-04-04 22:06:21 PDT
Created attachment 395480 [details]
Patch
Comment 3 Jack 2020-04-04 22:08:04 PDT
Created attachment 395481 [details]
Patch
Comment 4 Ryosuke Niwa 2020-04-06 12:31:17 PDT
Comment on attachment 395481 [details]
Patch

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

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:247
> +            if (endContainer->previousSibling()) {

We prefer early exit over nested if's so this should check for endContainer not having the previous sibling.
Also, we should probably check that the previous sibling is a text node.

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:258
> +                start = end = { };

Nit: Each statement should have its own line so should be:
start = { };
end = { };
Comment 5 Ryosuke Niwa 2020-04-06 12:31:33 PDT
There is no security implication here.
Comment 6 Jack 2020-04-06 13:03:21 PDT
Created attachment 395604 [details]
Patch
Comment 7 EWS 2020-04-06 18:46:01 PDT
Committed r259619: <https://trac.webkit.org/changeset/259619>

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