| Summary: | Nullptr crash in CompositeEditCommand::moveParagraphs when inserting OL into uneditable parent. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jack <shihchieh_lee> | ||||||||
| Component: | HTML Editing | Assignee: | Jack <shihchieh_lee> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | bfulgham, ews-feeder, ews-watchlist, mifenton, product-security, rniwa, webkit-bug-importer, wenson_hsieh | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Jack
2020-03-26 21:10:11 PDT
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]. |