Bug 226870

Summary: Nullptr crash in positionInParentBeforeNode via InsertTextCommand::doApply
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cgarcia, ews-feeder, ews-watchlist, fred.wang, gpoo, mifenton, product-security, rbuis, svillar, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test
none
Patch
ews-feeder: commit-queue-
Patch
rniwa: review+
Patch none

Description Ryosuke Niwa 2021-06-10 03:20:32 PDT 
Created attachment 431058 [details]
Test

e.g.

ASSERTION FAILED: ancestor
./dom/Position.cpp(1579) : WebCore::Position WebCore::positionInParentBeforeNode(WebCore::Node *)
1   0x1a999ee59 WTFCrash
2   0x18a8370db WTFCrashWithInfo(int, char const*, char const*, int)
3   0x18db1f9d0 WebCore::positionInParentBeforeNode(WebCore::Node*)
4   0x18dcbd546 WebCore::InsertTextCommand::doApply()
5   0x18dc32d05 WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::CompositeEditCommand, WTF::RawPtrTraits<WebCore::CompositeEditCommand> >&&, WebCore::VisibleSelection const&)
6   0x18dcfe7e4 WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool)
7   0x18dd209da WebCore::TypingCommandLineOperation::operator()(unsigned long, unsigned long, bool) const
8   0x18dcfe6aa void WebCore::forEachLineInString<WebCore::TypingCommandLineOperation>(WTF::String const&, WebCore::TypingCommandLineOperation const&)
9   0x18dcfe5bc WebCore::TypingCommand::insertText(WTF::String const&, bool)
10  0x18dcfd084 WebCore::TypingCommand::insertTextAndNotifyAccessibility(WTF::String const&, bool)
11  0x18dcfdc32 WebCore::TypingCommand::doApply()
12  0x18dc1e12c WebCore::CompositeEditCommand::apply()
13  0x18dceac43 WebCore::TextInsertionBaseCommand::applyTextInsertionCommand(WebCore::Frame*, WebCore::TextInsertionBaseCommand&, WebCore::VisibleSelection const&, WebCore::VisibleSelection const&)
14  0x18dcfcf77 WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, WebCore::VisibleSelection const&, unsigned int, WebCore::TypingCommand::TextCompositionType)
15  0x18dcfccf3 WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, unsigned int, WebCore::TypingCommand::TextCompositionType)
16  0x18dca4e44 WebCore::executeInsertText(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)
17  0x18dc7cfbb WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
18  0x18d985e45 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)
19  0x18b242004 WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)
20  0x18b24196c 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*)
21  0x18b22a524 WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*)
22  0x26d010e011d8
23  0x1a9fa671a llint_entry
24  0x1a9f84440 vmEntryToJavaScript
25  0x1aaea8d4b JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
26  0x1aaea94ac JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
27  0x1ab2774ad JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
28  0x1ab27758f JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
29  0x1ab277872 JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
30  0x18d306fee WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
31  0x18d326fdb WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)

<>ASSERTION FAILED: ancestor
./dom/Position.cpp(1579) : WebCore::Position WebCore::positionInParentBeforeNode(WebCore::Node *)
1   0x1a999ee59 WTFCrash
2   0x18a8370db WTFCrashWithInfo(int, char const*, char const*, int)
3   0x18db1f9d0 WebCore::positionInParentBeforeNode(WebCore::Node*)
4   0x18dcbd546 WebCore::InsertTextCommand::doApply()
5   0x18dc32d05 WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::CompositeEditCommand, WTF::RawPtrTraits<WebCore::CompositeEditCommand> >&&, WebCore::VisibleSelection const&)
6   0x18dcfe7e4 WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool)
7   0x18dd209da WebCore::TypingCommandLineOperation::operator()(unsigned long, unsigned long, bool) const
8   0x18dcfe6aa void WebCore::forEachLineInString<WebCore::TypingCommandLineOperation>(WTF::String const&, WebCore::TypingCommandLineOperation const&)
9   0x18dcfe5bc WebCore::TypingCommand::insertText(WTF::String const&, bool)
10  0x18dcfd084 WebCore::TypingCommand::insertTextAndNotifyAccessibility(WTF::String const&, bool)
11  0x18dcfdc32 WebCore::TypingCommand::doApply()
12  0x18dc1e12c WebCore::CompositeEditCommand::apply()
13  0x18dceac43 WebCore::TextInsertionBaseCommand::applyTextInsertionCommand(WebCore::Frame*, WebCore::TextInsertionBaseCommand&, WebCore::VisibleSelection const&, WebCore::VisibleSelection const&)
14  0x18dcfcf77 WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, WebCore::VisibleSelection const&, unsigned int, WebCore::TypingCommand::TextCompositionType)
15  0x18dcfccf3 WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, unsigned int, WebCore::TypingCommand::TextCompositionType)
16  0x18dca4e44 WebCore::executeInsertText(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)
17  0x18dc7cfbb WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
18  0x18d985e45 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)
19  0x18b242004 WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)
20  0x18b24196c 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*)
21  0x18b22a524 WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*)
22  0x26d010e011d8
23  0x1a9fa671a llint_entry
24  0x1a9f84440 vmEntryToJavaScript
25  0x1aaea8d4b JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
26  0x1aaea94ac JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
27  0x1ab2774ad JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
28  0x1ab27758f JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
29  0x1ab277872 JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
30  0x18d306fee WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
31  0x18d326fdb WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)

<rdar://78607686>
Comment 1 Ryosuke Niwa 2021-06-10 03:21:10 PDT 
Reproduced with debug build of WebKitTestRunner at r278627.
Comment 2 Frédéric Wang (:fredw) 2021-06-15 07:23:25 PDT 
Created attachment 431435 [details]
Patch

From a quick debugging, the <summary> element is detached from the tree in the testcase. Here is a tentative patch that fixes the assert.
Comment 3 Ryosuke Niwa 2021-06-15 14:23:10 PDT 
Comment on attachment 431435 [details]
Patch

Please add the test for this.
Also, the actual selection isn't orphaned here, is it?
Comment 4 Frédéric Wang (:fredw) 2021-06-15 21:27:33 PDT 
(In reply to Ryosuke Niwa from comment #3)
> Please add the test for this.

Will do.

> Also, the actual selection isn't orphaned here, is it?

I guess you mean endingSelection().isOrphan()? Yes, it became orphan here too (see reverse debugging below). Do you mean we should rely on that method instead?

(rr) bt
#0  0x00007feb1ace29f7 in CRASH_WITH_INFO(...) ()
    at WTF/Headers/wtf/Assertions.h:744
#1  0x00007feb2162f5b7 in WebCore::positionInParentBeforeNode(WebCore::Node*)
    (node=0x60c0002a7fc0) at ../../Source/WebCore/dom/Position.cpp:1579
#2  0x00007feb218d622b in WebCore::InsertTextCommand::doApply() (this=
    0x61200006f340) at ../../Source/WebCore/editing/InsertTextCommand.cpp:177
...
(rr) reverse-f
(rr) p showTree(node)
*SUMMARY	0x60c0002a7fc0 (renderer (nil)) 
	#document-fragment	0x61200007e940 (renderer (nil))  (needs style recalc) (child needs style recalc)
		DIV	0x60c0002a8080 (renderer (nil)) 
		SLOT	0x60d000065fc0 (renderer (nil)) 
	BODY	0x60c0002a82c0 (renderer (nil)) 
	Q	0x60c0002a8380 (renderer (nil)) 
$1 = void
(rr) watch -l node->m_parentNode
(rr) rc
(rr) delete
(rr) reverse-f
(rr) bt
#0  0x00007feb2128da05 in WebCore::ContainerNode::removeBetween(WebCore::Node*, WebCore::Node*, WebCore::Node&)
    (this=0x60c0002a6a00, previousChild=0x0, nextChild=0x0, oldChild=...)
    at ../../Source/WebCore/dom/ContainerNode.cpp:655
#1  0x00007feb2129b9a2 in WebCore::ContainerNode::removeNodeWithScriptAssertion(WebCore::Node&, WebCore::ContainerNode::ChildChange::Source)
    (this=0x60c0002a6a00, childToRemove=..., source=WebCore::ContainerNode::ChildChange::Source::API) at ../../Source/WebCore/dom/ContainerNode.cpp:181
#2  0x00007feb2128d355 in WebCore::ContainerNode::removeChild(WebCore::Node&)
    (this=0x60c0002a6a00, oldChild=...)
    at ../../Source/WebCore/dom/ContainerNode.cpp:614
...
#15 0x00007feb218d5f30 in WebCore::InsertTextCommand::doApply()
    (this=0x61200006f340)
    at ../../Source/WebCore/editing/InsertTextCommand.cpp:143
...
(rr) b Source/WebCore/editing/InsertTextCommand.cpp:143
Breakpoint 2 at 0x7feb218d5f06: file ../../Source/WebCore/editing/InsertTextCommand.cpp, line 143.
(rr) rc
Continuing.
143	        deleteSelection(false, true, true, false, false);
(rr) p endingSelection().isNoneOrOrphaned()
$2 = false
(rr) p endingSelection().isOrphan()
$3 = false
(rr) p endingSelection().start().isOrphan()
$4 = false
(rr) p endingSelection().end().isOrphan()
$5 = false
(rr) n
147	        if (endingSelection().isNone())
(rr) p endingSelection().isNoneOrOrphaned()
$6 = true
(rr) p endingSelection().isOrphan()
$7 = true
(rr) p endingSelection().start().isOrphan()
$8 = true
(rr) p endingSelection().end().isOrphan()
$9 = true
Comment 5 Ryosuke Niwa 2021-06-15 21:31:26 PDT 
no, I mean FrameSelection
Comment 6 Frédéric Wang (:fredw) 2021-06-15 22:04:47 PDT 
(In reply to Ryosuke Niwa from comment #5)
> no, I mean FrameSelection

OK, yes after deleteSelection, endingSelection().isOrphan() is true but document().selection().selection().isOrphan() is false.
Comment 7 Frédéric Wang (:fredw) 2021-06-15 22:16:23 PDT 
Created attachment 431515 [details]
Patch
Comment 8 Ryosuke Niwa 2021-06-15 22:43:11 PDT 
(In reply to Frédéric Wang (:fredw) from comment #6)
> (In reply to Ryosuke Niwa from comment #5)
> > no, I mean FrameSelection
> 
> OK, yes after deleteSelection, endingSelection().isOrphan() is true but
> document().selection().selection().isOrphan() is false.

Cool.
Comment 9 Ryosuke Niwa 2021-06-15 22:44:18 PDT 
Comment on attachment 431515 [details]
Patch

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

> LayoutTests/ChangeLog:6
> +        Add regression test.

This should appear after "reviewed by" line.

> Source/WebCore/ChangeLog:7
> +        deleteSelection call In InsertTextCommand::doApply() can make the endingSelection() orphan.
> +        If that happens, exit early to avoid dereferencing a nullptr pointer later.

This long description should appear below "reviewed by" line while the bug title & URL should continue to appear above it.
Comment 10 Frédéric Wang (:fredw) 2021-06-15 22:58:45 PDT 
Comment on attachment 431515 [details]
Patch

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

>> LayoutTests/ChangeLog:6
>> +        Add regression test.
> 
> This should appear after "reviewed by" line.

Oops, I thought about it but got confused with the 'Test:' thing that is put after... Will fix.
Comment 11 Frédéric Wang (:fredw) 2021-06-15 23:05:44 PDT 
Created attachment 431521 [details]
Patch
Comment 12 EWS 2021-06-16 06:36:28 PDT 
Committed r278930 (238861@main): <https://commits.webkit.org/238861@main>

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