Bug 221509 – Crash in makeBoundaryPoint via ReplaceSelectionCommand::insertedContentRange

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

e.g.

0   com.apple.WebCore             	0x00000005711127ff WebCore::Node::ref() const + 0 (Node.h:779) [inlined]
1   com.apple.WebCore             	0x00000005711127ff WTF::Ref<WebCore::Node, WTF::RawPtrTraits<WebCore::Node> >::Ref(WebCore::Node&) + 0 (Ref.h:67) [inlined]
2   com.apple.WebCore             	0x00000005711127ff WTF::Ref<WebCore::Node, WTF::RawPtrTraits<WebCore::Node> >::Ref(WebCore::Node&) + 0 (Ref.h:66) [inlined]
3   com.apple.WebCore             	0x00000005711127ff WebCore::makeBoundaryPoint(WebCore::Position const&) + 63 (Position.cpp:1599)
4   com.apple.WebCore             	0x000000057116f57a decltype(makeBoundaryPoint(std::forward<WebCore::Position const&>(fp))) WebCore::makeBoundaryPointHelper<WebCore::Position const&>(WebCore::Position const&) + 5 (SimpleRange.h:56) [inlined]
5   com.apple.WebCore             	0x000000057116f57a decltype(makeSimpleRangeHelper(makeBoundaryPointHelper(std::forward<WebCore::Position const&>(fp)), makeBoundaryPointHelper(std::forward<WebCore::Position const&>(fp)))) WebCore::makeSimpleRange<WebCore::Position const&, WebCore::Position const&>(WebCore::Position const&, WebCore::Position const&) + 26 (SimpleRange.h:58)
6   com.apple.WebCore             	0x00000005711e2d9f WebCore::ReplaceSelectionCommand::insertedContentRange() const + 31 (ReplaceSelectionCommand.cpp:1778)
7   com.apple.WebCore             	0x000000057119779e WebCore::Editor::replaceSelectionWithFragment(WebCore::DocumentFragment&, WebCore::Editor::SelectReplacement, WebCore::Editor::SmartReplace, WebCore::Editor::MatchStyle, WebCore::EditAction, WebCore::MailBlockquoteHandling) + 990 (Editor.cpp:687)
8   com.apple.WebCore             	0x0000000571197ec6 WebCore::Editor::replaceSelectionWithText(WTF::String const&, WebCore::Editor::SelectReplacement, WebCore::Editor::SmartReplace, WebCore::EditAction) + 118 (Editor.cpp:727)
9   com.apple.WebCore             	0x0000000571197359 WebCore::Editor::handleTextEvent(WebCore::TextEvent&) + 201 (Editor.cpp:335)
10  com.apple.WebCore             	0x0000000571645f0f WebCore::EventHandler::defaultTextInputEventHandler(WebCore::TextEvent&) + 31 (EventHandler.cpp:4117)
11  com.apple.WebCore             	0x00000005710d1994 WebCore::callDefaultEventHandlersInBubblingOrder(WebCore::Event&, WebCore::EventPath const&) + 39 (EventDispatcher.cpp:62) [inlined]
12  com.apple.WebCore             	0x00000005710d1994 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) + 1684 (EventDispatcher.cpp:203)
13  com.apple.WebCore             	0x00000005711998a2 WebCore::Editor::pasteAsPlainText(WTF::String const&, bool) + 194 (Editor.cpp:607)
14  com.apple.WebCore             	0x0000000571199c19 WebCore::Editor::pasteAsPlainTextWithPasteboard(WebCore::Pasteboard&) + 361 (Editor.cpp:627)
15  com.apple.WebCore             	0x00000005711a0650 WebCore::Editor::pasteAsPlainText(WebCore::Editor::FromMenuOrKeyBinding) + 304 (Editor.cpp:1477)
16  com.apple.WebCore             	0x00000005711c1093 WebCore::executePasteAsPlainText(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 51 (EditorCommand.cpp:949)
17  com.apple.WebCore             	0x000000057109502c WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 76 (Document.cpp:5660)
18  com.apple.WebCore             	0x000000057039eca0 WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) + 229 (JSDocument.cpp:5889) [inlined]
19  com.apple.WebCore             	0x000000057039eca0 long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) + 402 (JSDOMOperation.h:53) [inlined]
20  com.apple.WebCore             	0x000000057039eca0 WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 432 (JSDocument.cpp:5894)
21  ???                           	0x000044b2f78011d8 0 + 75535447232984

<rdar://problem/74021456>

[Ryosuke Niwa]

Created attachment 419485 [details]
Test

[Frédéric Wang (:fredw)]

The testcase hits the following assert in debug mode:

Thread 1 received signal SIGSEGV, Segmentation fault.
0x00007f8fd1fe0763 in WebCore::Node::ref (this=0x0)
    at DerivedSources/ForwardingHeaders/WebCore/Node.h:780
780	    ASSERT(!m_deletionHasBegun);
(rr) bt
#0  0x00007f8fd1fe0763 in WebCore::Node::ref() const (this=0x0)
    at DerivedSources/ForwardingHeaders/WebCore/Node.h:780
#1  0x00007f8fd1fe15f1 in WTF::Ref<WebCore::Node, WTF::RawPtrTraits<WebCore::Node> >::Ref(WebCore::Node&) (this=0x7fffd6a188e8, object=...)
    at DerivedSources/ForwardingHeaders/wtf/Ref.h:67
#2  0x00007f8fd474ff85 in WebCore::makeBoundaryPoint(WebCore::Position const&)
    (position=...) at ../../Source/WebCore/dom/Position.cpp:1599

[Frédéric Wang (:fredw)]

The code involved seems very similar to bug 221387 and a similar patch works:

--- a/Source/WebCore/editing/markup.cpp
+++ b/Source/WebCore/editing/markup.cpp
@@ -1199,6 +1199,7 @@ Ref<DocumentFragment> createFragmentFromText(const SimpleRange& context, const S
     bool useClonesOfEnclosingBlock = block
         && !block->hasTagName(bodyTag)
         && !block->hasTagName(htmlTag)
+        && !block->hasTagName(inputTag)
         && block != editableRootForPosition(start);
     bool useLineBreak = enclosingTextFormControl(start);

Not sure yet whether this is the right thing to do and probably this can happens with more tags.

The debugging session below shows that in Editor::replaceSelectionWithText(), we have a #document-fragment attached to an <input>, which looks a bit suspicious to me... This weird structure propagates all the way.

Thread 1 received signal SIGSEGV, Segmentation fault.
(rr) b Editor::replaceSelectionWithText
Breakpoint 1 at 0x7f6058157e70 (2 locations)
(rr) rc
(rr) 
https://webkit-search.igalia.com/webkit/rev/30faa6e9cea015b4d5e21fe19163ec12e80ca700/Source/WebCore/editing/Editor.cpp#724
(rr) p text.show()
\u000Aonload = () => { document.styleSheets[0].insertRule(`head, script, meta, input { display: block; }`); document.styleSheets[0].insertRule(`input { text-indent: 1px; }`); document.querySelector('meta').appendChild(document.createElement('input')); document.execCommand('SelectAll'); document.designMode = 'on'; document.execCommand('Copy'); document.execCommand('PasteAsPlainText'); };\u000A
$1 = void
(rr) n
(rr) n
(rr) p showTree(createLiveRange(range).get())
#document	0x7f603fd6ac70 (renderer 0x7f603fd69250) 
	HTML	0x7f603fd6ba80 (renderer 0x7f603fd696c0) 
		HEAD	0x7f603fd6bb10 (renderer 0x7f603fd5c350) 
S			META	0x7f603fd6bba0 (renderer 0x7f603fd5c470) 
				INPUT	0x7f603fd5c0a0 (renderer 0x7f603fd5c590) 
					#document-fragment	0x7f603fd5c1b0 (renderer (nil))  (needs style recalc) (child needs style recalc)
						DIV	0x7f603fd5c2c0 (renderer 0x7f603fd5c6b0) 
			#text	0x7f603fd6bc30 "\n"
			STYLE	0x7f603fd6bca0 (renderer (nil)) 
				#text	0x7f603fd6bd60 "\n"
			#text	0x7f603fd6bdd0 "\n"
			SCRIPT	0x7f603fd6be40 (renderer 0x7f603fd5c7d0) 
				#text	0x7f603fd6bf10 "\n  onload = () => {\n    document.styleSheets[0].insertRule(`head, script, meta, input { display: block; }`);\n    document.styleSheets[0].insertRule(`input { text-indent: 1px; }`);\n    document.querySelector('meta').appendChild(document.createElement('input'));\n    document.execCommand('SelectAll');\n    document.designMode = 'on';\n    document.execCommand('Copy');\n    document.execCommand('PasteAsPlainText');\n  };\n"
			#text	0x7f603fd6bf80 "\n"
E		BODY	0x7f603fd5c010 (renderer 0x7f603fd697e0) 
start offset: 0, end offset: 0
$2 = void
(rr) b createFragmentFromText
(rr) c
(rr) 
https://webkit-search.igalia.com/webkit/rev/30faa6e9cea015b4d5e21fe19163ec12e80ca700/Source/WebCore/editing/markup.cpp#1159
(rr) b 1227
(rr) c
https://webkit-search.igalia.com/webkit/rev/30faa6e9cea015b4d5e21fe19163ec12e80ca700/Source/WebCore/editing/markup.cpp#1227
(rr) p showTree(&fragment.get())
*#document-fragment	0x7f603fd5d1f0 (renderer (nil)) 
	INPUT	0x7f603fd5d270 (renderer (nil)) 
		#document-fragment	0x7f603fd5d380 (renderer (nil)) 
			DIV	0x7f603fd5d490 (renderer (nil)) 
		BR	0x7f603fd5d520 (renderer (nil)) 
	INPUT	0x7f603fd5d5b0 (renderer (nil)) 
		#document-fragment	0x7f603fd5d6c0 (renderer (nil)) 
			DIV	0x7f603fd5d7d0 (renderer (nil)) 
		#text	0x7f603fd5d860 "onload = () => { document.styleSheets[0].insertRule(`head, script, meta, input { display: block; }`); document.styleSheets[0].insertRule(`input { text-indent: 1px; }`); document.querySelector('meta').appendChild(document.createElement('input')); document.execCommand('SelectAll'); document.designMode = 'on'; document.execCommand('Copy'); document.execCommand('PasteAsPlainText'); };"
	BR	0x7f603fd5d8d0 (renderer (nil))  CLASS=Apple-interchange-newline
$3 = void

[Ryosuke Niwa]

That document fragment is probably a shadow root. It's expected that an input element has a shadow root and an editable div inside of it.

What's rather unexpected is that the fragment in createFragmentFromText has an input element in it based on your second output. How are those input elements end up being placed there?

[Frédéric Wang (:fredw)]

(In reply to Ryosuke Niwa from comment #4)
> That document fragment is probably a shadow root. It's expected that an
> input element has a shadow root and an editable div inside of it.
> 

OK, that makes sense.

> What's rather unexpected is that the fragment in createFragmentFromText has
> an input element in it based on your second output. How are those input
> elements end up being placed there?

So as I mentioned in comment 3, this is generally very similar to bug 221387, where a "bad" enclosing block is selected, cloned for each line break by createFragmentFromText and causes issues later in the code. A patch to force useClonesOfEnclosingBlock to false allows to work around the issue here too, but we probably want a more general approach (I haven't investigated yet why the <input> tag is causing issues later in the code).

In this particular case, the start context is the <meta> tag, to which the test appends an <input> child. Additionally, the test forces `display: block;` on that input tag, so it is selected as the enclosing block. See more debug output below.

https://webkit-search.igalia.com/webkit/rev/5040394ad2b060de17f12aa8f84018147f22b4f4/Source/WebCore/editing/markup.cpp#1203
(rr) p showTree(start.firstNode().get())
#document	0x7fa43f156c70 (renderer 0x7fa43f155250) 
	HTML	0x7fa43f157a80 (renderer 0x7fa43f1556c0) 
		HEAD	0x7fa43f157b10 (renderer 0x7fa43f148350) 
			META	0x7fa43f157ba0 (renderer 0x7fa43f148470) 
*				INPUT	0x7fa43f1480a0 (renderer 0x7fa43f148590) 
					#document-fragment	0x7fa43f1481b0 (renderer (nil))  (needs style recalc) (child needs style recalc)
						DIV	0x7fa43f1482c0 (renderer 0x7fa43f1486b0) 
			#text	0x7fa43f157c30 "\n"
			STYLE	0x7fa43f157ca0 (renderer (nil)) 
				#text	0x7fa43f157d60 "\n"
			#text	0x7fa43f157dd0 "\n"
			SCRIPT	0x7fa43f157e40 (renderer 0x7fa43f1487d0) 
				#text	0x7fa43f157f10 "\n  onload = () => {\n    document.styleSheets[0].insertRule(`head, script, meta, input { display: block; }`);\n    document.styleSheets[0].insertRule(`input { text-indent: 1px; }`);\n    document.querySelector('meta').appendChild(document.createElement('input'));\n    document.execCommand('SelectAll');\n    document.designMode = 'on';\n    document.execCommand('Copy');\n    document.execCommand('PasteAsPlainText');\n  };\n"
			#text	0x7fa43f157f80 "\n"
		BODY	0x7fa43f148010 (renderer 0x7fa43f1557e0) 
$1 = void
(rr) p start.firstNode().get()==block
$2 = true

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #5)
> (I haven't investigated yet why the <input> tag is causing issues later in the code).

So here is what's happening. The <input> tag is initially a child of <body> when WebCore::ReplaceSelectionCommand::doApply() calculates m_startOfInsertedContent. But at the end of that method, the call to ReplaceSelectionCommand::completeHTMLReplacement() ends up executing moveParagraphContentsToNewBlockIfNecessary() which wraps copies of the <input> elements into a new <div> tag. The original <input> elements are detached from the tree. When we later try to access the container node of m_startOfInsertedContent, we obtain a null pointer, causing a nullptr dereference.

Thread 1 received signal SIGSEGV, Segmentation fault.
(rr) reverse-f
(rr) 
(rr) 
https://webkit-search.igalia.com/webkit/rev/5040394ad2b060de17f12aa8f84018147f22b4f4/Source/WebCore/dom/Position.cpp#1599
(rr)  p position.containerNode()
$1 = (WebCore::Node *) 0x0
(rr) reverse-f
(rr) 
(rr) 
https://webkit-search.igalia.com/webkit/rev/5040394ad2b060de17f12aa8f84018147f22b4f4/Source/WebCore/editing/ReplaceSelectionCommand.cpp#1778
(rr) p showTree(m_startOfInsertedContent->m_anchorNode.get())
*INPUT	0x7f0596461270 (renderer (nil)) 
	#document-fragment	0x7f0596461380 (renderer (nil))  (needs style recalc) (child needs style recalc)
		DIV	0x7f0596461490 (renderer (nil)) 
	BR	0x7f0596461520 (renderer (nil))  (needs style recalc)
$2 = void
(rr) watch -l m_startOfInsertedContent
(rr) rc
(rr) finish
(rr) 
https://webkit-search.igalia.com/webkit/rev/5040394ad2b060de17f12aa8f84018147f22b4f4/Source/WebCore/editing/ReplaceSelectionCommand.cpp#1325
(rr) p showTree(m_startOfInsertedContent->m_anchorNode.get())
BODY	0x7f0596460010 (renderer 0x7f059646d7e0) 
*	INPUT	0x7f0596461270 (renderer 0x7f0596460590) 
		#document-fragment	0x7f0596461380 (renderer (nil))  (needs style recalc) (child needs style recalc)
			DIV	0x7f0596461490 (renderer 0x7f05964606b0) 
		BR	0x7f0596461520 (renderer (nil))  (needs style recalc)
	INPUT	0x7f05964615b0 (renderer 0x7f0596461a30) 
		#document-fragment	0x7f05964616c0 (renderer (nil))  (needs style recalc) (child needs style recalc)
			DIV	0x7f05964617d0 (renderer 0x7f0596461b50) 
			STYLE	0x7f059646fca0 (renderer (nil)) 
				#text	0x7f059646fd60 "\n"
$4 = void
(rr) watch -l ((Node*) 0x7f0596461270)->m_parentNode
(rr) c
(rr) delete
(rr) reverse-f
(rr) 
(rr) 
(rr) 
(rr) 
(rr) 
(rr) 
(rr) 
(rr) 
(rr) 
(rr) 
(rr) 
(rr) 
https://webkit-search.igalia.com/webkit/rev/5040394ad2b060de17f12aa8f84018147f22b4f4/Source/WebCore/editing/ApplyStyleCommand.cpp#263
(rr) p showTree(block.get())
*BODY	0x7f0596460010 (renderer 0x7f059646d7e0) 
	INPUT	0x7f0596461270 (renderer 0x7f0596460590) 
		#document-fragment	0x7f0596461380 (renderer (nil))  (needs style recalc) (child needs style recalc)
			DIV	0x7f0596461490 (renderer 0x7f05964606b0) 
		BR	0x7f0596461520 (renderer (nil))  (needs style recalc)
	INPUT	0x7f05964615b0 (renderer 0x7f0596461a30) 
		#document-fragment	0x7f05964616c0 (renderer (nil))  (needs style recalc) (child needs style recalc)
			DIV	0x7f05964617d0 (renderer 0x7f0596461b50) 
	DIV	0x7f0596461d10 (renderer 0x7f0596460350) 
		BR	0x7f0596462710 (renderer 0x7f05964627a0) 
			STYLE	0x7f059646fca0 (renderer (nil)) 
				#text	0x7f059646fd60 "\n"
$5 = void
(rr) n
(rr) p showTree(newBlock.get())
BODY	0x7f0596460010 (renderer 0x7f059646d7e0) 
*	DIV	0x7f0596463050 (renderer 0x7f05964607d0) 
		INPUT	0x7f0596463b30 (renderer 0x7f0596460590) 
			#document-fragment	0x7f0596463c40 (renderer (nil))  (needs style recalc) (child needs style recalc)
				DIV	0x7f0596463d50 (renderer 0x7f05964606b0) 
		INPUT	0x7f0596463de0 (renderer 0x7f0596461a30) 
			#document-fragment	0x7f0596463ef0 (renderer (nil))  (needs style recalc) (child needs style recalc)
				DIV	0x7f05963ec010 (renderer 0x7f0596461b50) 
	DIV	0x7f0596461d10 (renderer 0x7f0596460350) 
		BR	0x7f0596462710 (renderer 0x7f05964627a0) 
			STYLE	0x7f059646fca0 (renderer (nil)) 
				#text	0x7f059646fd60 "\n"
$6 = void
(rr) bt
#0  WebCore::ApplyStyleCommand::applyBlockStyle(WebCore::EditingStyle&)
    (this=0x7f0596719b00, style=...) at ../../Source/WebCore/editing/ApplyStyleCommand.cpp:264
#1  0x00007f05b416bada in WebCore::ApplyStyleCommand::doApply() (this=0x7f0596719b00)
    at ../../Source/WebCore/editing/ApplyStyleCommand.cpp:209
#2  0x00007f05b4177292 in WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::RawPtrTraits<WebCore::EditCommand> >&&) (this=0x7f05967a3ae0, command=...)
    at ../../Source/WebCore/editing/CompositeEditCommand.cpp:466
#3  0x00007f05b4177531 in WebCore::CompositeEditCommand::applyStyle(WebCore::EditingStyle const*, WebCore::Position const&, WebCore::Position const&, WebCore::EditAction)
    (this=0x7f05967a3ae0, style=0x7f0544297450, start=..., end=..., editingAction=WebCore::EditAction::ChangeAttributes) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:492
#4  0x00007f05b24ebcb2 in WebCore::ReplaceSelectionCommand::completeHTMLReplacement(WebCore::Position const&) (this=0x7f05967a3ae0, lastPositionToSelect=...)
    at ../../Source/WebCore/editing/ReplaceSelectionCommand.cpp:1586
#5  0x00007f05b24ea845 in WebCore::ReplaceSelectionCommand::doApply() (this=0x7f05967a3ae0)
    at https://webkit-search.igalia.com/webkit/rev/5040394ad2b060de17f12aa8f84018147f22b4f4/Source/WebCore/editing/ReplaceSelectionCommand.cpp#1411

[Frédéric Wang (:fredw)]

Created attachment 421512 [details]
Patch

Just for completeness, the patch from comment 3.

[Frédéric Wang (:fredw)]

Created attachment 421526 [details]
Patch

PTAL.

I don't know how much effort we want to put in order to fix that crash.

I tried to save the start/end position before calling applyStyle(), which is closer to the desired semantics, but I don't think that can be done in a simple & concise way.

Another alternative would be use some placeholders to save start/end positions https://webkit-search.igalia.com/webkit/rev/488c0b1edd37ad487a55f25b92a624a5d3564858/Source/WebCore/editing/ReplaceSelectionCommand.cpp#966

Finally, we could probably just modify ReplaceSelectionCommand::insertedContentRange() to return an empty range or something when the m_startOfInsertedContent or m_endOfInsertedContent are orphan.

[Ryosuke Niwa]

Comment on attachment 421526 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1590
> +            // applyStyle may clone content to new block wrappers and make anchor nodes orphan.
> +            // FIXME: Ideally, start/end should be set on the cloned content instead.
> +            if (start.isOrphan() || end.isOrphan()) {
> +                start = end = { };

Can we use endingSelection().start() / endingSelection().end() instead?

[Frédéric Wang (:fredw)]

Created attachment 422928 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 421526 [details]
Patch

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

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1590
>> +                start = end = { };
> 
> Can we use endingSelection().start() / endingSelection().end() instead?

Yes we can. Done in the latest patch.

[Ryosuke Niwa]

Comment on attachment 422928 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1588
> +            // FIXME: Ideally, start/end should be set on the cloned content instead.

This FIXME isn't accurate. Please remove.

[Ryosuke Niwa]

Comment on attachment 422928 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        * fast/editing/replace-selection-and-apply-style-crash.html: Added.
> +

Looks like this is missing the entry for -expected.txt?
Also please mention that we're adding a regression test.

[Frédéric Wang (:fredw)]

Created attachment 423019 [details]
Patch for landing

[EWS]

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

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