Bug 221387 – Nullptr crash in CompositeEditCommand::splitTreeToNode via InsertParagraphSeparatorCommand::doApply

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

[Ryosuke Niwa]

Created attachment 419246 [details]
Test

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000007a917b79f WebCore::Node::ref() const + 0 (Node.h:779) [inlined]
1   com.apple.WebCore             	0x00000007a917b79f WTF::DefaultRefDerefTraits<WebCore::Node>::refIfNotNull(WebCore::Node*) + 0 (RefPtr.h:36) [inlined]
2   com.apple.WebCore             	0x00000007a917b79f WTF::RefPtr<WebCore::Node, WTF::RawPtrTraits<WebCore::Node>, WTF::DefaultRefDerefTraits<WebCore::Node> >::RefPtr(WebCore::Node*) + 0 (RefPtr.h:61) [inlined]
3   com.apple.WebCore             	0x00000007a917b79f WTF::RefPtr<WebCore::Node, WTF::RawPtrTraits<WebCore::Node>, WTF::DefaultRefDerefTraits<WebCore::Node> >::RefPtr(WebCore::Node*) + 0 (RefPtr.h:61) [inlined]
4   com.apple.WebCore             	0x00000007a917b79f WTF::RefPtr<WebCore::Node, WTF::RawPtrTraits<WebCore::Node>, WTF::DefaultRefDerefTraits<WebCore::Node> >::operator=(WebCore::Node*) + 0 (RefPtr.h:151) [inlined]
5   com.apple.WebCore             	0x00000007a917b79f WebCore::CompositeEditCommand::splitTreeToNode(WebCore::Node&, WebCore::Node&, bool) + 79 (CompositeEditCommand.cpp:1705)
6   com.apple.WebCore             	0x00000007a91ca010 WebCore::InsertParagraphSeparatorCommand::doApply() + 6720 (InsertParagraphSeparatorCommand.cpp:389)
7   com.apple.WebCore             	0x00000007a917574b WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::RawPtrTraits<WebCore::EditCommand> >&&) + 43 (CompositeEditCommand.cpp:466)
8   com.apple.WebCore             	0x00000007a9175cf9 WebCore::CompositeEditCommand::insertParagraphSeparator(bool, bool) + 89 (CompositeEditCommand.cpp:507)
9   com.apple.WebCore             	0x00000007a91e0997 WebCore::ReplaceSelectionCommand::doApply() + 13943 (ReplaceSelectionCommand.cpp:1381)
10  com.apple.WebCore             	0x00000007a9165b17 WebCore::CompositeEditCommand::apply() + 327 (CompositeEditCommand.cpp:375)
11  com.apple.WebCore             	0x00000007a9197724 WebCore::Editor::replaceSelectionWithFragment(WebCore::DocumentFragment&, WebCore::Editor::SelectReplacement, WebCore::Editor::SmartReplace, WebCore::Editor::MatchStyle, WebCore::EditAction, WebCore::MailBlockquoteHandling) + 868 (Editor.cpp:684)
12  com.apple.WebCore             	0x00000007a9197ec6 WebCore::Editor::replaceSelectionWithText(WTF::String const&, WebCore::Editor::SelectReplacement, WebCore::Editor::SmartReplace, WebCore::EditAction) + 118 (Editor.cpp:727)
13  com.apple.WebCore             	0x00000007a9197359 WebCore::Editor::handleTextEvent(WebCore::TextEvent&) + 201 (Editor.cpp:335)
14  com.apple.WebCore             	0x00000007a9645f0f WebCore::EventHandler::defaultTextInputEventHandler(WebCore::TextEvent&) + 31 (EventHandler.cpp:4117)
15  com.apple.WebCore             	0x00000007a90d1994 WebCore::callDefaultEventHandlersInBubblingOrder(WebCore::Event&, WebCore::EventPath const&) + 39 (EventDispatcher.cpp:62) [inlined]
16  com.apple.WebCore             	0x00000007a90d1994 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) + 1684 (EventDispatcher.cpp:203)
17  com.apple.WebCore             	0x00000007a91998a2 WebCore::Editor::pasteAsPlainText(WTF::String const&, bool) + 194 (Editor.cpp:607)
18  com.apple.WebCore             	0x00000007a9199c19 WebCore::Editor::pasteAsPlainTextWithPasteboard(WebCore::Pasteboard&) + 361 (Editor.cpp:627)
19  com.apple.WebCore             	0x00000007a91a0650 WebCore::Editor::pasteAsPlainText(WebCore::Editor::FromMenuOrKeyBinding) + 304 (Editor.cpp:1477)
20  com.apple.WebCore             	0x00000007a91c1013 WebCore::executePasteAndMatchStyle(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 51 (EditorCommand.cpp:934)
21  com.apple.WebCore             	0x00000007a909502c WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 76 (Document.cpp:5660)
22  com.apple.WebCore             	0x00000007a839eca0 WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) + 229 (JSDocument.cpp:5889) [inlined]
23  com.apple.WebCore             	0x00000007a839eca0 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]
24  com.apple.WebCore             	0x00000007a839eca0 WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 432 (JSDocument.cpp:5894)
25  ???                           	0x00003cc51b2011d8 0 + 66817261310424
26  com.apple.JavaScriptCore      	0x00000007ae2c9e8d llint_entry + 108286 (LowLevelInterpreter.asm:1093)


<rdar://problem/73884361>

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

The code involved was added in bug 61594. I have not read the details on that bug or of InsertParagraphSeparatorCommand::doApply() yet but the code added there assumes that the result of splitTo->traverseNextNode(startBlock) is non-null. But this is clearly wrong here where splitTo is the unique child of startBlock. Cf preliminary debugging below.

391	            ASSERT(splitTo);
(rr) p splitTo
$1 = (WebCore::Node *) 0x0
(rr) reverse-next
390	                splitTo = NodeTraversal::next(*splitTo, startBlock.get());
(rr) p showTree(splitTo)
warning: RTTI symbol not found for class 'WebCore::Text'
BODY	0x7fd6adc07ed0 (renderer 0x7fd6ad85c1b0) 
	HR	0x7fd6ad85c650 (renderer 0x7fd6adc057c0) 
	TABLE	0x7fd6ad85ce50 (renderer 0x7fd6ad85cff0) 
*		#text	0x7fd6ad85cef0 "head, style { display: block; }"
	DIV	0x7fd6ad85d550 (renderer 0x7fd6ad85c010) 
		BR	0x7fd6ad85cf60 (renderer 0x7fd6ad85d5e0) 
	TABLE	0x7fd6ad85caf0 (renderer 0x7fd6ad85cb90) 
$2 = void
(rr) p showTree((Node*) startBlock.get())
BODY	0x7fd6adc07ed0 (renderer 0x7fd6ad85c1b0) 
	HR	0x7fd6ad85c650 (renderer 0x7fd6adc057c0) 
*	TABLE	0x7fd6ad85ce50 (renderer 0x7fd6ad85cff0) 
		#text	0x7fd6ad85cef0 "head, style { display: block; }"
	DIV	0x7fd6ad85d550 (renderer 0x7fd6ad85c010) 
		BR	0x7fd6ad85cf60 (renderer 0x7fd6ad85d5e0) 
	TABLE	0x7fd6ad85caf0 (renderer 0x7fd6ad85cb90) 
$3 = void

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

Note that tweaking attachment 419246 [details] to use a <div> instead of a <table> avoids the crash.

Below is the relevant backtrace demonstrating where the <table> tag is involved. In WebCore::InsertParagraphSeparatorCommand::doApply(), isFirstInBlack is false and, because endOfBlock(visiblePos) returns the table parent of the #text node, isLastInBlock is false too. Then the "more complicated" path is executed, hitting the assertion failure from comment 1 which expects there is always another node after splitTo. If one uses a <div> instead, then endOfBlock(visiblePos) returns the #text node and isLastInBlock becomes true. This then goes to the case when "position is in the last visible position in its block" which runs without problem.

#0  WebCore::Position::upstream(WebCore::EditingBoundaryCrossingRule) const
    at https://webkit-search.igalia.com/webkit/rev/30faa6e9cea015b4d5e21fe19163ec12e80ca700/Source/WebCore/dom/Position.cpp#723
#1  0x00007efc27a0e35d in WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (passedPosition=...)
    at ../../Source/WebCore/editing/VisiblePosition.cpp:549
#2  0x00007efc27a0b963 in WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity)
    (this=0x7ffd201049c0, position=..., affinity=WebCore::Affinity::Downstream)
    at ../../Source/WebCore/editing/VisiblePosition.cpp:58
#3  0x00007efc27a1c1ae in WebCore::endOfBlock(WebCore::VisiblePosition const&, WebCore::EditingBoundaryCrossingRule)
    (visiblePosition=..., rule=WebCore::CanCrossEditingBoundary) at ../../Source/WebCore/editing/VisibleUnits.cpp:1354
#4  0x00007efc27a1c3ce in WebCore::isEndOfBlock(WebCore::VisiblePosition const&) (pos=...)
    at https://webkit-search.igalia.com/webkit/source/Source/WebCore/editing/VisibleUnits.cpp#1369
#5  0x00007efc279b35e2 in WebCore::InsertParagraphSeparatorCommand::doApply() (this=0x7efc0bc4a6c0)
    at https://webkit-search.igalia.com/webkit/rev/30faa6e9cea015b4d5e21fe19163ec12e80ca700/Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp#200
(rr) p showTree(&visiblePos)
BODY	0x7efc0b957ef0 (renderer 0x7efc0b9441b0) 
	HR	0x7efc0b944650 (renderer 0x7efc0b9557e0) 
	TABLE	0x7efc0b944e50 (renderer 0x7efc0b944ff0) 
*		#text	0x7efc0b944ef0 "head, style { display: block; }"
	TABLE	0x7efc0b944af0 (renderer 0x7efc0b944b90) 
legacy, offset, offset:31
$32 = void

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

Created attachment 420973 [details]
Tentative patch

Here is a tentative patch (see comment 2 for an explanation about why this works). BTW, I noticed that added a special return for table cell was added in bug 84995. I don't have access to the details and can't find an associated test, but another way to avoid the crash here is a similar thing like:

--- a/Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp
+++ b/Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp
@@ -167,6 +167,7 @@ void InsertParagraphSeparatorCommand::doApply()
     if (!startBlock
         || !startBlock->nonShadowBoundaryParentNode()
         || isTableCell(startBlock.get())
+        || isRenderedTable(startBlock.get())

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

Created attachment 421174 [details]
Tentative patch

Tweak test log and expectation.

[Ryosuke Niwa]

Comment on attachment 421174 [details]
Tentative patch

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

> Source/WebCore/ChangeLog:13
> +        Reviewed by NOBODY (OOPS!).
> +

This should appear above the long description but below URL.

> Source/WebCore/editing/markup.cpp:1203
> +        && !block->hasTagName(tableTag)
>          && block != editableRootForPosition(start);

Maybe what we want is unsplittableElementForPosition instead of editableRootForPosition?

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

Comment on attachment 421174 [details]
Tentative patch

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

>> Source/WebCore/ChangeLog:13
>> +
> 
> This should appear above the long description but below URL.

Done.

>> Source/WebCore/editing/markup.cpp:1203
>>          && block != editableRootForPosition(start);
> 
> Maybe what we want is unsplittableElementForPosition instead of editableRootForPosition?

Maybe, although that does not really help to fix the crash since there is no table cell in the DOM (incidentally, I tried the test with tr and td instead and that does not cause any crash) and that will probably be a separate behavior change. It seems using isRenderedTable works too, but I'm not really sure we can always assume the block node has a renderer here.

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

Created attachment 421290 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 421290 [details]
Patch

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

> Source/WebCore/editing/markup.cpp:1202
> +        && !block->hasTagName(tableTag)

We may want to add a comment saying that we avoid using table as the line break due to its special treatment in Position.

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

Created attachment 421342 [details]
Patch for landing

[EWS]

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

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