Bug 211864 – Nullptr crash in InsertParagraphSeparatorCommand::doApply when the canonical position is uneditable

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

[Jack]

<rdar://62982161>

0   com.apple.WebCore             	0x00000001056e2e19 WebCore::InsertParagraphSeparatorCommand::doApply() + 9977
1   com.apple.WebCore             	0x00000001056764ff WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::DumbPtrTraits<WebCore::EditCommand> >&&) + 79
2   com.apple.WebCore             	0x00000001056f36ec WebCore::ReplaceSelectionCommand::doApply() + 8076
3   com.apple.WebCore             	0x00000001043e38fd WebCore::CompositeEditCommand::apply() + 397
4   com.apple.WebCore             	0x00000001056d4f6a WebCore::executeInsertFragment(WebCore::Frame&, WTF::Ref<WebCore::DocumentFragment, WTF::DumbPtrTraits<WebCore::DocumentFragment> >&&) + 74
5   com.apple.WebCore             	0x00000001056d50a4 WebCore::executeInsertNode(WebCore::Frame&, WTF::Ref<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >&&) + 180
6   com.apple.WebCore             	0x00000001056d05ba WebCore::executeInsertImage(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 74
7   com.apple.WebCore             	0x000000010444a5f1 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 81
8   com.apple.WebCore             	0x00000001048431ca WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 426

[Jack]

Below is the test case. Thanks to Alan for minimizing it!

<video><span id=span_copy><input id=input></input></span></video><button></button><audio><hr id=hr1 contentEditable="true"><hr id=hr2></audio><textarea></textarea>
<script>
    if (window.testRunner)
        testRunner.dumpAsText();
    hr1.appendChild(span_copy);
    input.setSelectionRange(-1,67);
    hr2.appendChild(span_copy);
    document.execCommand("insertImage", "#foo");
    document.body.innerText = "Tests inserting list at the end of a table. The test passes if WebKit doesn't crash or hit an ssertion.";
</script>

[Jack]

Root cause:
1. In this test case, we are inserting a image and needs a paragraph separator at the selected position: hr1.
2. In function InsertParagraphSeparatorCommand::doApply, hr1 fails the check for being candidate at both its upstream and downstream positions.
3. However, we continute to assume hr1is a valid candidate and use it to as the insertaion positiopn for the separator.
4. Since the visible position of hr1 is start of paragraph, we need to insert a br at container of hr1.
5. The container node of hr1 is audio, and it is uneditable, so the insertion fails, leaving the br dangling.
6. Next we find the new insertion position based on the dangling br, so the position becomes empty.
7. Later the null anchor node in empty position is dereferenced and the code crashes.

EndSelection() is hr1:
BODY	0x28b06b920 (renderer 0x28b0697f0) 
	VIDEO	0x28b0b8010 (renderer 0x28b0b9d00) 
	BUTTON	0x28b0b8db0 (renderer 0x28b0b9e60) 
	AUDIO	0x28b0b8e80 (renderer 0x0) 
SE		HR	0x28b0b97b0 (renderer 0x0) 
		HR	0x28b0b9840 (renderer 0x0) 
			SPAN	0x28b0b8a70 (renderer 0x0) 
				INPUT	0x28b0b8b00 (renderer 0x0) 
					#document-fragment	0x28b0b8c10 (renderer 0x0)  (needs style recalc) (child needs style recalc)
						DIV	0x28b0b8d20 (renderer 0x0)  (needs style recalc)
	TEXTAREA	0x28b0b98d0 (renderer 0x28b0ba000) 
		#document-fragment	0x28b0b99d0 (renderer 0x0)  (needs style recalc) (child needs style recalc)
			DIV	0x28b0b9ae0 (renderer 0x28b0ba130) 
	#text	0x28b0b9b70 "\n"
	SCRIPT	0x28b0b9bd0 (renderer 0x0) 
		#text	0x28b0b9ca0 "\n    if (window.testRunner)\n        testRunner.dumpAsText();\n    hr1.appendChild(span_copy);\n    input.setSelectionRange(-1,67);\n    hr2.appendChild(span_copy);\n    document.execCommand("insertImage", "#foo");\n    document.body.innerText = "Tests inserting list at the end of a table. The test passes if WebKit doesn't crash or hit an ssertion.";\n"
start: offset, offset:0
end: offset, offset:0

The container node is the parent of hr1, audio.
BODY	0x28b06b920 (renderer 0x28b0697f0) 
	VIDEO	0x28b0b8010 (renderer 0x28b0b9d00) 
	BUTTON	0x28b0b8db0 (renderer 0x28b0b9e60) 
	AUDIO	0x28b0b8e80 (renderer 0x0) 
*		HR	0x28b0b97b0 (renderer 0x0) 
		HR	0x28b0b9840 (renderer 0x0) 
			SPAN	0x28b0b8a70 (renderer 0x0) 
				INPUT	0x28b0b8b00 (renderer 0x0) 
					#document-fragment	0x28b0b8c10 (renderer 0x0)  (needs style recalc) (child needs style recalc)
						DIV	0x28b0b8d20 (renderer 0x0)  (needs style recalc)
	TEXTAREA	0x28b0b98d0 (renderer 0x28b0ba000) 
		#document-fragment	0x28b0b99d0 (renderer 0x0)  (needs style recalc) (child needs style recalc)
			DIV	0x28b0b9ae0 (renderer 0x28b0ba130) 
	#text	0x28b0b9b70 "\n"
	SCRIPT	0x28b0b9bd0 (renderer 0x0) 
		#text	0x28b0b9ca0 "\n    if (window.testRunner)\n        testRunner.dumpAsText();\n    hr1.appendChild(span_copy);\n    input.setSelectionRange(-1,67);\n    hr2.appendChild(span_copy);\n    document.execCommand("insertImage", "#foo");\n    document.body.innerText = "Tests inserting list at the end of a table. The test passes if WebKit doesn't crash or hit an ssertion.";\n"
before, offset:0

[Jack]

Created attachment 399309 [details]
Patch

[Jack]

Created attachment 399310 [details]
Patch

[Geoffrey Garen]

Comment on attachment 399310 [details]
Patch

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

r=me

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:303
> +        // FIXME: <http://webkit.org/b/211864> insertionPosition is not a candidate. Needs to find an editable canonical candidate.

The meaning here is a little jumbled. I think we should say something like "FIXME: <http://webkit.org/b/211864>: If insertionPosition is not editable, we should compute a position that is."

[zalan]

Comment on attachment 399310 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Check for uneditable insertion position and return.

This part of the changelog should answer the question of "why" not "what".

[Jack]

Thanks! I will add a short explanation.

(In reply to zalan from comment #6)
> Comment on attachment 399310 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399310&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Check for uneditable insertion position and return.
> 
> This part of the changelog should answer the question of "why" not "what".

[Jack]

Created attachment 399318 [details]
Patch for landing

[EWS]

Committed r261666: <https://trac.webkit.org/changeset/261666>

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