Crash found by fuzzing. Reproduces on WebKit revision 265375. Stack Trace =========== 0 com.apple.WebCore 0x00000004de1ab37d WTF::Optional<WebCore::SimpleRange>::operator*() && + 45 1 com.apple.WebCore 0x00000004e0c6d4c3 WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool) + 2035 2 com.apple.WebCore 0x00000004e0d1c837 WebCore::InsertListCommand::unlistifyParagraph(WebCore::VisiblePosition const&, WebCore::HTMLElement*, WebCore::Node*) + 1927 3 com.apple.WebCore 0x00000004e0d1bc61 WebCore::InsertListCommand::doApplyForSingleParagraph(bool, WebCore::HTMLQualifiedName const&, WebCore::SimpleRange&) + 2961 4 com.apple.WebCore 0x00000004e0d1aa86 WebCore::InsertListCommand::doApply() + 1750 5 com.apple.WebCore 0x00000004e0c4a6e7 WebCore::CompositeEditCommand::apply() + 535 6 com.apple.WebCore 0x00000004e0d06949 WebCore::executeInsertOrderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 201 7 com.apple.WebCore 0x00000004e0958a84 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 244 8 com.apple.WebCore 0x00000004ddfe71ac WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) + 1164 9 com.apple.WebCore 0x00000004ddf18a5c long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) + 252
<rdar://problem/70987390>
Created attachment 413028 [details] Crashing Input
Created attachment 413298 [details] Reduced test case
The crash happens in CompositeEditCommand::moveParagraphs() because it's receiving a null endOfParagraphToMove. That causes that *makeSimpleRange(start, end) ends up trying to get a value that is actually nullopt. The problem comes from InsertListCommand::unlistifyParagraph(), that tries to get the start and end points of the list item children. start = firstPositionInNode(listChildNode); end = lastPositionInNode(listChildNode); end is null here. There's no </li> so all nodes after the <li> are considered children of the list item: LI 0x7f2102132380 (renderer 0x7f2100000240) CLASS=class1 STYLE=border-image: url(#htmlvar00007) 47% 1 -1 round; -webkit-ltr-ordering: visual; filter: brightness(1) brightness(0.361593078481); background-origin: content-box, padding-box; mso-width-alt: 1 #text 0x7f2102138b58 "\n" VIDEO 0x7f2102132400 (renderer 0x7f2100000360) #text 0x7f2102138bb0 "\n" BASE 0x7f2102132af0 (renderer (nil)) CLASS=class6 #text 0x7f2102138c08 "\n" SOURCE 0x7f2102132b70 (renderer (nil)) STYLE=stroke-linecap: round; scroll-snap-destination: 44% 1px; box-flex-group: 0; -webkit-ruby-position: after; max-width: none #text 0x7f2102138c60 "EFgrVTq+XimoYYd+9$\n" * #text 0x7f2102138cb8 "\n" MARQUEE 0x7f2102132c50 (renderer 0x7f2102128f00) #text 0x7f2102138d10 "\n" TIME 0x7f2102132ce0 (renderer 0x7f2100008f18) #text 0x7f2102138d68 "\n" BODY 0x7f2100000650 (renderer 0x7f2102129000) The video one is the reason null is returned, but I don't know why yet. If I remove the video element then it doesn't crash, but the end point is still wrong, I assume it should be the text child with the offset = len.
Created attachment 413925 [details] Patch In the end I decided to stop the insertion when end position is null to match what firefox and chrome do.
It seems the test failure can just be rebaselined, for some reason there's one empty line less in the outpout now: https://ews-build.s3-us-west-2.amazonaws.com/macOS-Mojave-Release-WK2-Tests-EWS/r413925-22814/editing/inserting/insert-list-in-iframe-in-list-pretty-diff.html
Can we attach the reduced test case since there doesn't appear to be a security bug here?
Comment on attachment 413925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413925&action=review > Source/WebCore/editing/InsertListCommand.cpp:301 > + if (end.isNull()) > + return false; Why is end null here?
(In reply to Ryosuke Niwa from comment #7) > Can we attach the reduced test case since there doesn't appear to be a > security bug here? I already attached the reduced test case, I'll clean it up a bit and include it as part of the patch then.
(In reply to Ryosuke Niwa from comment #8) > Comment on attachment 413925 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413925&action=review > > > Source/WebCore/editing/InsertListCommand.cpp:301 > > + if (end.isNull()) > > + return false; > > Why is end null here? See comment #4. There's an orphan <li> without the </li> causing the elements after that one to be considered children. The next element is a video one, that causes lastPositionInNode(listChildNode) to return null.
(In reply to Carlos Garcia Campos from comment #10) > (In reply to Ryosuke Niwa from comment #8) > > Comment on attachment 413925 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=413925&action=review > > > > > Source/WebCore/editing/InsertListCommand.cpp:301 > > > + if (end.isNull()) > > > + return false; > > > > Why is end null here? > > See comment #4. There's an orphan <li> without the </li> causing the > elements after that one to be considered children. The next element is a > video one, that causes lastPositionInNode(listChildNode) to return null. But why? We should be able to deal with the fact video is the last child of li. There is nothing wrong with that.
(In reply to Ryosuke Niwa from comment #11) > (In reply to Carlos Garcia Campos from comment #10) > > (In reply to Ryosuke Niwa from comment #8) > > > Comment on attachment 413925 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=413925&action=review > > > > > > > Source/WebCore/editing/InsertListCommand.cpp:301 > > > > + if (end.isNull()) > > > > + return false; > > > > > > Why is end null here? > > > > See comment #4. There's an orphan <li> without the </li> causing the > > elements after that one to be considered children. The next element is a > > video one, that causes lastPositionInNode(listChildNode) to return null. > > But why? We should be able to deal with the fact video is the last child of > li. There is nothing wrong with that. It's not the last child, there's also a marquee, leaving the video but removing the marquee doesn't crash either. Let me investigate more.
Created attachment 414011 [details] Reduced test case I've reduced more the test case and cleaned it up
Maybe VisiblePosition is choking up? It would be good to get to the bottom of why it's getting null. That kind of bug can lead to crashes elsewhere in the editing code. We don't want to be keep chasing new crash elsewhere.
This is the new tree with the reduced test case: BODY 0x7f3f118f52d0 (renderer 0x7f3f118f5460) #text 0x7f3f118f6430 "\n" FORM 0x7f3f118f6490 (renderer 0x7f3f118f7070) STYLE=-webkit-user-modify: read-write; #text 0x7f3f118f6590 "\n" DIV 0x7f3f118f65f0 (renderer 0x7f3f118f7180) #text 0x7f3f118f6670 "\n" UL 0x7f3f1192c200 (renderer 0x7f3f11934200) LI 0x7f3f118f66d0 (renderer 0x7f3f118f7290) #text 0x7f3f118f6750 "\n" VIDEO 0x7f3f118f67b0 (renderer 0x7f3f118f73b0) * #text 0x7f3f118f6ea0 "\n" MARQUEE 0x7f3f118f6f00 (renderer 0x7f3f11934300) #text 0x7f3f118f6f90 "\n" TIME 0x7f3f118f6ff0 (renderer 0x7f3f118f7670) #text 0x7f3f1193c058 "\n" BODY 0x7f3f118f78b0 (renderer 0x7f3f11934400) The last BODY, which is what the script adds, is what confuses everything. The tree is iterated looking for the end position and the last BODY returns true for isCandidate() but null is returned because !nextIsInSameEditableElement && !prevIsInSameEditableElement (the editing root for BODY is the body itself). And that seems to be the actual bug, because that body is not editable. Node::rootEditableElement() breaks ig the node is an HTMLBodyElment, but it shoudl actually check it's the document body element.
Created attachment 414037 [details] Patch
(In reply to Carlos Garcia Campos from comment #15) > > The last BODY, which is what the script adds, is what confuses everything. > The tree is iterated looking for the end position and the last BODY returns > true for isCandidate() but null is returned because > !nextIsInSameEditableElement && !prevIsInSameEditableElement (the editing > root for BODY is the body itself). And that seems to be the actual bug, > because that body is not editable. Node::rootEditableElement() breaks ig the > node is an HTMLBodyElment, but it shoudl actually check it's the document > body element. Ah, I see. That makes sense. Thanks for the good analysis.
Comment on attachment 414037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414037&action=review > LayoutTests/ChangeLog:7 > + Please say that we're adding a regression test.
Looks like there is some assertion failure on Mac WK1: CRASHING TEST: editing/inserting/insert-list-in-iframe-in-list.html Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000010d643af0 WTFCrash + 16 (Assertions.cpp:295) 1 com.apple.WebCore 0x0000000126d4115b WTFCrashWithInfo(int, char const*, char const*, int) + 27 2 com.apple.WebCore 0x0000000129f06f75 WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool) + 149 3 com.apple.WebCore 0x0000000129f8ea56 WebCore::InsertListCommand::unlistifyParagraph(WebCore::VisiblePosition const&, WebCore::HTMLElement*, WebCore::Node*) + 2022 4 com.apple.WebCore 0x0000000129f8e0ae WebCore::InsertListCommand::doApplyForSingleParagraph(bool, WebCore::HTMLQualifiedName const&, WebCore::SimpleRange&) + 2014 5 com.apple.WebCore 0x0000000129f8d881 WebCore::InsertListCommand::doApply() + 2385 6 com.apple.WebCore 0x0000000129eef665 WebCore::CompositeEditCommand::apply() + 421 7 com.apple.WebCore 0x0000000129f7a4ad WebCore::executeInsertOrderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 157 8 com.apple.WebCore 0x0000000129f515b7 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const + 231 9 com.apple.WebCore 0x0000000129c0fc46 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 86 10 com.apple.WebCore 0x0000000127772e1e WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) + 990 11 com.apple.WebCore 0x00000001277729f9 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*) + 793 12 com.apple.WebCore 0x00000001276d47c4 WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 36 13 ??? 0x000029dbf7401178 0 + 46024722747768
Comment on attachment 414037 [details] Patch r- because of the aforementioned assertion failure. Please fix that.
Created attachment 414211 [details] Patch I've fixed the assert, end can be null, but only when start is null too.
Comment on attachment 414211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414211&action=review > Source/WebCore/editing/CompositeEditCommand.cpp:1403 > + ASSERT(startOfParagraphToMove == endOfParagraphToMove || !endOfParagraphToMove.isNull()); This is harder to parse than (startOfParagraphToMove.isNull() && endOfParagraphToMove.isNull()) || endOfParagraphToMove.isNull()
Created attachment 414336 [details] Patch for landing
Are we waiting for something to land patch this patch??
Committed r269946: <https://trac.webkit.org/changeset/269946> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414336 [details].