Created attachment 413015 [details] Crashing input Description: Crash found by fuzzing. Reproduces on WebKit revision 268263 Stack Trace ========= frame #0: WebCore.framework/Versions/A/WebCore`WebCore::RenderObject::parent() const+ frame #1: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeBuilder::Block::attachIgnoringContinuation(WebCore::RenderBlock&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*)+ frame #2: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeBuilder::Block::attach(WebCore::RenderBlock&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*)+ frame #3: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeBuilder::BlockFlow::attach(WebCore::RenderBlockFlow&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*)+ frame #4: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeBuilder::attachInternal(WebCore::RenderElement&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*)+ frame #5: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeBuilder::attach(WebCore::RenderElement&, std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*)+ frame #6: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeUpdater::createTextRenderer(WebCore::Text&, WebCore::Style::TextUpdate const*)+ frame #7: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeUpdater::updateTextRenderer(WebCore::Text&, WebCore::Style::TextUpdate const*)+ frame #8: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&)+ frame #9: WebCore.framework/Versions/A/WebCore`WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >)+ frame #10: WebCore.framework/Versions/A/WebCore`WebCore::Document::updateRenderTree(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >)+
<rdar://problem/70985057>
<rdar://problem/70583715>
Created attachment 414025 [details] Super-reduced test case
Created attachment 414052 [details] Patch
Comment on attachment 414052 [details] Patch r=me
Comment on attachment 414052 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414052&action=review > Source/WebCore/editing/InsertListCommand.cpp:59 > + auto* parentNode = node.parentNode(); Please use RefPtr. > Source/WebCore/editing/InsertListCommand.cpp:60 > + if (parentNode && !parentNode->hasEditableStyle()) It seems like we should be checking hasRichlyEditableStyle() instead then? > Source/WebCore/editing/InsertListCommand.cpp:214 > Node* listChildNode = enclosingListChild(selectionNode); Can we make these pointers RefPtr?
Comment on attachment 414052 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414052&action=review Thanks for the review. >> Source/WebCore/editing/InsertListCommand.cpp:59 >> + auto* parentNode = node.parentNode(); > > Please use RefPtr. OK >> Source/WebCore/editing/InsertListCommand.cpp:60 >> + if (parentNode && !parentNode->hasEditableStyle()) > > It seems like we should be checking hasRichlyEditableStyle() instead then? Maybe the ChangeLog text is a bit misleading but this is the actual condition used by InsertNodeBeforeCommand ASSERT. >> Source/WebCore/editing/InsertListCommand.cpp:214 >> Node* listChildNode = enclosingListChild(selectionNode); > > Can we make these pointers RefPtr? Should we do it in a follow-up? Seems not directly related to the bug.
Comment on attachment 414052 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414052&action=review >>> Source/WebCore/editing/InsertListCommand.cpp:59 >>> + auto* parentNode = node.parentNode(); >> >> Please use RefPtr. > > OK Side comment: The idiom I prefer for RefPtr in case case like this one is this: auto parentNode = makeRefPtr(node.parentNode());
Comment on attachment 414052 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414052&action=review >>> Source/WebCore/editing/InsertListCommand.cpp:60 >>> + if (parentNode && !parentNode->hasEditableStyle()) >> >> It seems like we should be checking hasRichlyEditableStyle() instead then? > > Maybe the ChangeLog text is a bit misleading but this is the actual condition used by InsertNodeBeforeCommand ASSERT. Well, we shouldn't be listifying in plain text mode so we need to be checking hasRichlyEditableStyle. The existence of other code which checks for a wrong condition isn't a good argument for introducing new wrong code.
(In reply to Sergio Villar Senin from comment #7) > Comment on attachment 414052 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=414052&action=review > > >> Source/WebCore/editing/InsertListCommand.cpp:214 > >> Node* listChildNode = enclosingListChild(selectionNode); > > > > Can we make these pointers RefPtr? > > Should we do it in a follow-up? Seems not directly related to the bug. Sure, that sounds good.
Okay, there doesn't seem to any security bug here so can we include the test case in the patch?
(In reply to Ryosuke Niwa from comment #9) > Comment on attachment 414052 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=414052&action=review > > >>> Source/WebCore/editing/InsertListCommand.cpp:60 > >>> + if (parentNode && !parentNode->hasEditableStyle()) > >> > >> It seems like we should be checking hasRichlyEditableStyle() instead then? > > > > Maybe the ChangeLog text is a bit misleading but this is the actual condition used by InsertNodeBeforeCommand ASSERT. > > Well, we shouldn't be listifying in plain text mode so we need to be > checking hasRichlyEditableStyle. > The existence of other code which checks for a wrong condition isn't a good > argument for introducing new wrong code. Hmm then I guess this file should need a review pass later on because it has several appearances of the same check (that's why I thought it was correct) see for example: * InsertListCommand::unlistifyParagraph() * InsertListCommand::listifyParagraph()
Created attachment 414330 [details] Patch for landing
Are we waiting for something to land this patch??
Comment on attachment 414330 [details] Patch for landing Actually marking for review
Comment on attachment 414330 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=414330&action=review > LayoutTests/fast/editing/insert-list-in-orphaned-list-item-crash.html:4 > + testRunner.dumpAsText(); We use 4 space indentation?
Committed r270018: <https://trac.webkit.org/changeset/270018>