<rdar://60915598> 0 com.apple.WebCore 0x0000000113bfdf6d WTF::Optional<WebCore::BoundaryPoint>::operator*() && + 45 1 com.apple.WebCore 0x00000001162fb268 WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool) + 4616 2 com.apple.WebCore 0x00000001163a7fc4 WebCore::InsertListCommand::unlistifyParagraph(WebCore::VisiblePosition const&, WebCore::HTMLElement*, WebCore::Node*) + 1860 3 com.apple.WebCore 0x00000001163a73e1 WebCore::InsertListCommand::doApplyForSingleParagraph(bool, WebCore::HTMLQualifiedName const&, WebCore::Range*) + 2977 4 com.apple.WebCore 0x00000001163a6510 WebCore::InsertListCommand::doApply() + 2624 5 com.apple.WebCore 0x00000001162d7957 WebCore::CompositeEditCommand::apply() + 439 6 com.apple.WebCore 0x000000011638dd19 WebCore::executeInsertOrderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 201 7 com.apple.WebCore 0x0000000116007e88 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 248
In this case we try to insert ol at the position of li. Because li is originally in un-ordered list, we call unlistifyParagraph to move li into a BR element. Since BR needs to be inserted in body, but body is not editable, the insertion fails and leave BR parentless. The next call in moveParagraphs deref the parent of BR and crashed. </style> <script> window.onload = () => { document.getSelection().setPosition(LI); document.execCommand("insertOrderedList", false); } </script> <body><ul><li id=LI contenteditable="true">
Created attachment 394702 [details] Patch
This is a conservative patch. We are checking parentless node after insertion. In debug build, a lot of assertions for editable content trigger before and during insertion. I was wondering if checking parent of container node of the visible position for edibility is okay? Is it too aggressive and may find false positive? I roughly checked all the insertion calls and each of them eventually checks parent or ancestor for editability. Something like this: if (!start.deepEquivalent().containerNode()->hasEditableStyle() || !start.deepEquivalent().containerNode()->parentNode()->hasEditableStyle()) return;
(In reply to Jack from comment #3) > This is a conservative patch. We are checking parentless node after > insertion. In debug build, a lot of assertions for editable content trigger > before and during insertion. Hm... we probably need to fix that because we can't land a test which hits assertions in debug builds. > I was wondering if checking parent of container node of the visible position > for edibility is okay? Is it too aggressive and may find false positive? I > roughly checked all the insertion calls and each of them eventually checks > parent or ancestor for editability. > > Something like this: > > if (!start.deepEquivalent().containerNode()->hasEditableStyle() || > !start.deepEquivalent().containerNode()->parentNode()->hasEditableStyle()) > return; At where? That doesn't seem right if container node itself is editable. Why does the parent of the container node needs to be also editable?
(In reply to Ryosuke Niwa from comment #4) > (In reply to Jack from comment #3) > > This is a conservative patch. We are checking parentless node after > > insertion. In debug build, a lot of assertions for editable content trigger > > before and during insertion. > > Hm... we probably need to fix that because we can't land a test which hits > assertions in debug builds. I see. > > if (!start.deepEquivalent().containerNode()->hasEditableStyle() || > > !start.deepEquivalent().containerNode()->parentNode()->hasEditableStyle()) > > return; > > At where? That doesn't seem right if container node itself is editable. Why > does the parent of the container node needs to be also editable? In the beginning of the function. It's because the function insertNodeAfter or insertNodeBefore eventually append or insert to the parent, for example: void CompositeEditCommand::insertNodeAfter(Ref<Node>&& insertChild, Node& refChild) { ContainerNode* parent = refChild.parentNode(); ... if (parent->lastChild() == &refChild) appendNode(WTFMove(insertChild), *parent); }
And InsertNodeBefore also checks parent for editability. void InsertNodeBeforeCommand::doApply() { ContainerNode* parent = m_refChild->parentNode(); if (!parent || (m_shouldAssumeContentIsAlwaysEditable == DoNotAssumeContentIsAlwaysEditable && !isEditableNode(*parent))) return; ASSERT(isEditableNode(*parent)); parent->insertBefore(m_insertChild, m_refChild.ptr()); }
(In reply to Jack from comment #5) > (In reply to Ryosuke Niwa from comment #4) > > (In reply to Jack from comment #3) > > > This is a conservative patch. We are checking parentless node after > > > insertion. In debug build, a lot of assertions for editable content trigger > > > before and during insertion. > > > > Hm... we probably need to fix that because we can't land a test which hits > > assertions in debug builds. > I see. > > > > if (!start.deepEquivalent().containerNode()->hasEditableStyle() || > > > !start.deepEquivalent().containerNode()->parentNode()->hasEditableStyle()) > > > return; > > > > At where? That doesn't seem right if container node itself is editable. Why > > does the parent of the container node needs to be also editable? > In the beginning of the function. The beginning of which function? InsertListCommand::unlistifyParagraph? > It's because the function insertNodeAfter or insertNodeBefore eventually > append or insert to the parent, for example: In which function? It's unclear what code of which function you're talking about.
> In which function? It's unclear what code of which function you're talking > about. Oh sorry, the changes are in unlistifyParagraph and listifyParagraph.
Created attachment 394706 [details] Patch
Thanks to Ryosuke for discussion. We use more aggressive editability check in unlistifyParagraph because the insertion position is always the listNode. Try this patch on EWS tests. (In reply to Jack from comment #9) > Created attachment 394706 [details] > Patch
Comment on attachment 394706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394706&action=review > Source/WebCore/editing/InsertListCommand.cpp:277 > + Nit: whitespace. > Source/WebCore/editing/InsertListCommand.cpp:280 > + Nit: whitespace. > Source/WebCore/editing/InsertListCommand.cpp:398 > + Nit: whitespace. > LayoutTests/editing/inserting/insert-ol-uneditable-parent.html:1 > +</style> Do we really need this?
There is no security implication here.
Created attachment 394728 [details] Patch for landing
Thanks for the good catches! It's in the original test case, somehow my eyes automatically skip that... :-) (In reply to Ryosuke Niwa from comment #11) > Comment on attachment 394706 [details]
Committed r259153: <https://trac.webkit.org/changeset/259153> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394728 [details].