Bug 209529

Summary:

Nullptr crash in WebCore::Node::isDescendantOf when inserting list

Product:

WebKit

Reporter:

Jack <shihchieh_lee>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, darin, ews-feeder, ews-watchlist, mifenton, product-security, rniwa, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	rniwa: review+
Patch	none
Patch for landing	none

Description Jack 2020-03-24 18:27:02 PDT

<rdar://problem/60693542>

Comment 1 Jack 2020-03-24 18:27:18 PDT

    #0 0x794ee89a1 in WebCore::Node::getFlag(WebCore::Node::NodeFlags) const (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x1db9a1)
    #1 0x7980842a3 in WebCore::Node::isDescendantOf(WebCore::Node const&) const (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x33772a3)
    #2 0x79820c6e0 in WebCore::selectionForParagraphIteration(WebCore::VisibleSelection const&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34ff6e0)
    #3 0x79828fcc1 in WebCore::InsertListCommand::doApply() (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x3582cc1)
    #4 0x7981c1d16 in WebCore::CompositeEditCommand::apply() (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34b4d16)
    #5 0x798277b28 in WebCore::executeInsertOrderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x356ab28)
    #6 0x797ef2127 in WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x31e5127)
    #7 0x7957d29fa in WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*, JSC::ThrowScope&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0xac59fa)
    #8 0x7956830d0 in long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x9760d0)
    #9 0x5adc4b601177  
    #10 0x7b101eb13 in llint_entry (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa70b13)
    #11 0x7b10078e8 in vmEntryToJavaScript (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa598e8)
    #12 0x7b265d8f0 in JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x20af8f0)
    #13 0x7b2c540c0 in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26a60c0)
    #14 0x7b2c541c1 in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26a61c1)
    #15 0x7b2c5459f in JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26a659f)
    #16 0x797803254 in WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x2af6254)
    #17 0x79782dc8e in WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x2b20c8e)
    #18 0x798007324 in WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x32fa324)
    #19 0x798002342 in WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x32f5342)
    #20 0x797fef656 in WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x32e2656)
    #21 0x797ff0829 in WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x32e3829)
    #22 0x797ff01d8 in WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x32e31d8)
    #23 0x797fefbde in WebCore::EventDispatcher::dispatchScopedEvent(WebCore::Node&, WebCore::Event&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x32e2bde)
    #24 0x797e7b4cd in WebCore::dispatchChildRemovalEvents(WTF::Ref<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x316e4cd)
    #25 0x797e6d206 in WebCore::ContainerNode::removeChild(WebCore::Node&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x3160206)
    #26 0x79807f135 in WebCore::Node::removeChild(WebCore::Node&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x3372135)
    #27 0x7980d6d45 in WebCore::processNodes(WebCore::Range::ActionType, WTF::Vector<WTF::Ref<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::Node*, WTF::RefPtr<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x33c9d45)
    #28 0x7980d3aa4 in WebCore::Range::processContents(WebCore::Range::ActionType) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x33c6aa4)
    #29 0x7980d291b in WebCore::Range::deleteContents() (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x33c591b)
    #30 0x798e2aee1 in WebCore::DOMSelection::deleteFromDocument() (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x411dee1)
    #31 0x7956fa98d in WebCore::jsDOMSelectionPrototypeFunctionDeleteFromDocumentBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMSelection*, JSC::ThrowScope&) (Safari_ASAN_258763_9b2408707c444da861e139af54b820e789a0ed62.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x9ed98d)
    #32 0x7955716a6 in long long WebCore::IDLOperation<WebCore::JSDOMSelection>::call<&(WebCore::jsDOMSelectionPrototypeFunctionDeleteFromDocumentBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMSelection*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*)

Comment 2 Jack 2020-03-24 18:28:12 PDT

<style>
html { -webkit-user-modify: read-write; }
</style>
<script>
    function runTest() {
        document.execCommand("insertOrderedList", false);
        TD.addEventListener("DOMNodeRemovedFromDocument", runTest);
        document.execCommand("selectAll", false);
        window.getSelection().deleteFromDocument();
    }
</script>
<body onload="runTest()"><table><td id=TD></td></table><li contenteditable="false"></li><span>a</span>

Root cause:
While we process the second “insertOrderedList” command, in function selectionForParagraphIteration the end position of the selected range is null and it is deref ilater in the function.

The following happens that causes end position to become null:
1. First insertOrderedList command is bypassed because there is no selected range for the command.
2. DOMNodeRemovedFromDocument listner is added.
3. We select all the elements in the document and delete them.
4. In deletion, we call processContentsBetweenOffsets which would set the renderer of end position to null.
5. In the middle of deletion, DOMNodeRemovedFromDocument event is triggered so we reenter JSC and start to process the second insertOrderedList command. 
6. Because the selection is still valid, we try to insert ol for all the elements, which has the same selected range as delete command.
7. Before inserting ol, we call selectionForParagraphIteration to re-evaluate the selected range position.
8. When looking at the end of selection, because the renderer is set to null in the original end position, it is skipped when we consider selection candidate. Then we walk up the node tree but find no alternative and return a null position.

Comment 3 Jack 2020-03-24 18:35:43 PDT

Created attachment 394455 [details]
Patch

Comment 4 Ryosuke Niwa 2020-03-24 18:55:58 PDT

Comment on attachment 394455 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        The visible positions may be null if the DOM tree is altered before an edit command is applied. Add null check for visible positions at the beginning of InsertListCommand::doApply.

Please insert a line break after period. Otherwise, this is a really long line.

> LayoutTests/ChangeLog:9
> +        The visible positions may be null if the DOM tree is altered before an edit command is applied. Add null check for visible positions at the beginning of InsertListCommand::doApply.

This is a change log for a test it should mention that you're adding a test.
e.g. Added a regression test for the crash.

Comment 5 Jack 2020-03-24 19:13:46 PDT

Created attachment 394459 [details]
Patch

Comment 6 Jack 2020-03-24 19:14:37 PDT

Thanks! Please see the new patch.
> Comment on attachment 394455 [details]

Comment 7 Ryosuke Niwa 2020-03-24 19:19:37 PDT

There is no security implication here.

Comment 8 Ryosuke Niwa 2020-03-24 19:19:47 PDT

Comment on attachment 394459 [details]
Patch

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

> LayoutTests/fast/lists/insert-list-during-node-removal-crash.html:3
> +<style>
> +html { -webkit-user-modify: read-write; }
> +</style>

Just add contenteditable="true" on body instead.

Comment 9 Jack 2020-03-24 20:24:12 PDT

Created attachment 394466 [details]
Patch

Comment 10 Ryosuke Niwa 2020-03-24 22:37:54 PDT

Comment on attachment 394466 [details]
Patch

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

> LayoutTests/ChangeLog:12
> +        * fast/lists/insert-list-during-node-removal-crash-expected.txt: Added.
> +        * fast/lists/insert-list-during-node-removal-crash.html: Added.

Oops, sorry, I didn't realize this earlier.
This should be inside editing/execCommand or editing/inserting, not fast/lists.

Comment 11 Darin Adler 2020-03-25 08:38:57 PDT

Comment on attachment 394466 [details]
Patch

r=me assuming we move the test to the editing directory

Comment 12 Jack 2020-03-25 10:21:14 PDT

Thanks, I will move the test files.

Somehow the test case shows timeout message in WebKitTestRunner because of "testRunner.waitUntilDone". Am investigating and will upload new patch soon.

Comment 13 Jack 2020-03-25 11:29:32 PDT

Added print and verified that event handler is called once and finished afterwards when --no-timeout option is specified.

Besides, when running WebKitTestRunner with ./Tools/Scripts/run-webkit-tests, this error does not happen, probably because it sets a longer timeout:

"Regular timeout: 30000, slow test timeout: 150000".

It should be safe to upload the test case.

(In reply to Jack from comment #12)
> Somehow the test case shows timeout message in WebKitTestRunner because of
> "testRunner.waitUntilDone". Am investigating and will upload new patch soon.

Comment 14 Jack 2020-03-25 12:48:48 PDT

Created attachment 394529 [details]
Patch for landing

Comment 15 EWS 2020-03-25 18:51:19 PDT

Committed r259027: <https://trac.webkit.org/changeset/259027>

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