Created attachment 419813 [details] Test e.g. Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x00007fff3b1a22d9 WebCore::ModifySelectionListLevelCommand::appendSiblingNodeRange(WebCore::Node*, WebCore::Node*, WebCore::Element*) + 57 1 com.apple.WebCore 0x00007fff3b1a24e1 WebCore::IncreaseSelectionListLevelCommand::doApply() + 337 2 com.apple.WebCore 0x00007fff3b12e10f WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::DumbPtrTraits<WebCore::EditCommand> >&&) + 79 3 com.apple.WebCore 0x00007fff3b196fbf WebCore::InsertNestedListCommand::doApply() + 1647 4 com.apple.WebCore 0x00007fff39dcb684 WebCore::CompositeEditCommand::apply() + 500 5 com.apple.WebCore 0x00007fff3b188462 WebCore::executeInsertNestedOrderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 98 6 com.apple.WebCore 0x00007fff39e30f7d WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 77 7 com.apple.WebCore 0x00007fff3a2324cc WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 428 <rdar://problem/74148852>
Created attachment 420807 [details] Patch So the issue here is that ModifySelectionListLevelCommand::appendSiblingNodeRange browses the list of children from the startNode to the endNode, assuming they are siblings in that order. But will do a null pointer dereference if that's assumption is false: https://webkit-search.igalia.com/webkit/rev/96373eab5c9a5d968cdfac1e0c0069773d717c23/Source/WebCore/editing/ModifySelectionListLevel.cpp#123 Debugging backwards (see output below), the mistake is happening in getStartEndListChildren where the end node is moved to the next sibling in the render tree, which does not correspond to a sibling in the DOM tree (here, because the SLOT element does not generate any renderer). It seems the crash in ModifySelectionListLevelCommand::appendSiblingNodeRange is not a security bug (just a nullptr dereference) so I'm adding the tests to the patch for now and can change if you disagree. I haven't checked whether the mistake in getStartEndListChildren can cause similar issues, but that function is used for both list level increment and decrement. (rr) p showTree(startListChild) BODY 0x7f76f41f7d30 (renderer 0x7f76f41f57c0) SLOT 0x7f76f41f7dc0 (renderer (nil)) #text 0x7f76f41f7e50 "\n " LI 0x7f76f41f7ec0 (renderer 0x7f76f41e8190) * LI 0x7f76f41e8800 (renderer 0x7f76f41e8890) #text 0x7f76f41f7f50 "\n" #text 0x7f76f41e8010 "\n" OL 0x7f76f41e8080 (renderer 0x7f76f41e83f0) #text 0x7f76f41e8120 "\n" $1 = void (rr) p showTree(endListChild) BODY 0x7f76f41f7d30 (renderer 0x7f76f41f57c0) SLOT 0x7f76f41f7dc0 (renderer (nil)) #text 0x7f76f41f7e50 "\n " LI 0x7f76f41f7ec0 (renderer 0x7f76f41e8190) * LI 0x7f76f41e8800 (renderer 0x7f76f41e8890) #text 0x7f76f41f7f50 "\n" #text 0x7f76f41e8010 "\n" OL 0x7f76f41e8080 (renderer 0x7f76f41e83f0) #text 0x7f76f41e8120 "\n" $2 = void (rr) p showRenderTree(r) (B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, hasLayer(S)crollableArea, (C)omposited, (+)Dirty style, (+)Dirty layout B---YGLS- -- RenderView at (0,0) size 800x600 renderer->(0x7f76f41f5240) B-----LS- -- HTML RenderBlock at (0,0) size 800x600 renderer->(0x7f76f41f56a0) node->(0x7f76f41f7a60) B-------- -- BODY RenderBody at (8,8) size 784x576 renderer->(0x7f76f41f57c0) node->(0x7f76f41f7d30) (layout overflow -1,0 785x576) (visual overflow -1,0 785x576) B-------- -- LI RenderListItem at (0,0) size 784x18 renderer->(0x7f76f41e8190) node->(0x7f76f41f7ec0) (layout overflow -1,0 785x18) (visual overflow -1,0 785x18) -------- -- Line: (top: 0 bottom: 17) with leading (top: 0 bottom: 18) -------- -- RootInlineBox at (0,0) size 14x17 (0x7f76f41e8560) renderer->(0x7f76f41e8190) -------- -- InlineBox at (-1,0) size 7x17 (0x7f76f41e8510) renderer->(0x7f76f41e82d0) I---YG--- -- RenderListMarker at (-1,0) size 7x17 renderer->(0x7f76f41e82d0) B-------- -- LI RenderListItem at (0,18) size 784x18 renderer->(0x7f76f41e8890) node->(0x7f76f41e8800) (layout overflow -1,0 785x18) (visual overflow -1,0 785x18) -------- -- Line: (top: 0 bottom: 17) with leading (top: 0 bottom: 18) -------- -- RootInlineBox at (0,0) size 14x17 (0x7f76f41e8b40) renderer->(0x7f76f41e8890) -------- -- InlineBox at (-1,0) size 7x17 (0x7f76f41e8af0) renderer->(0x7f76f41e89d0) I---YG--- -- RenderListMarker at (-1,0) size 7x17 renderer->(0x7f76f41e89d0) B-------- --* OL RenderBlock at (0,52) size 784x0 renderer->(0x7f76f41e83f0) node->(0x7f76f41e8080) $3 = void (rr) n 84 endListChild = r->node(); (rr) n 87 start = startListChild; (rr) p showTree(endListChild) BODY 0x7f76f41f7d30 (renderer 0x7f76f41f57c0) SLOT 0x7f76f41f7dc0 (renderer (nil)) #text 0x7f76f41f7e50 "\n " LI 0x7f76f41f7ec0 (renderer 0x7f76f41e8190) LI 0x7f76f41e8800 (renderer 0x7f76f41e8890) #text 0x7f76f41f7f50 "\n" #text 0x7f76f41e8010 "\n" * OL 0x7f76f41e8080 (renderer 0x7f76f41e83f0) #text 0x7f76f41e8120 "\n" $4 = void
(In reply to Frédéric Wang (:fredw) from comment #1) > Debugging backwards (see output below), the mistake is happening in > getStartEndListChildren where the end node is moved to the next sibling in > the render tree, which does not correspond to a sibling in the DOM tree > (here, because the SLOT element does not generate any renderer). Additional remark here. The slot element has "display: contents" https://webkit-search.igalia.com/webkit/rev/6c8fdfee755dcaf35bd338726b21a7527d3917de/Source/WebCore/editing/markup.cpp#575 so this is really another issue with editing & display: contents.
Comment on attachment 420807 [details] Patch Ok, seems reasonable.
Committed r273302: <https://commits.webkit.org/r273302> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420807 [details].