| Summary: | Nullptr deref in WebCore::ApplyStyleCommand::applyRelativeFontStyleChange | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||
| Component: | HTML Editing | Assignee: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, product-security, rbuis, svillar, webkit-bug-importer, wenson_hsieh | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=223974 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Ryosuke Niwa
2021-03-17 01:09:43 PDT
I can reproduce this crash with DumpRenderTree and WebKitTestRunner at r274459. (In reply to Ryosuke Niwa from comment #0) > Created attachment 423448 [details] > Test > I think this is the wrong test case, I get the division by zero of bug 223366 (and it's the same as attachment 423452 [details] indeed). Created attachment 423616 [details]
Test
Oops, uploaded a new test.
This also hits the following assert in debug mode: ASSERTION FAILED: m_parent->hasEditableStyle() || !m_parent->renderer() https://webkit-search.igalia.com/webkit/rev/fcdc6d66bafc7fbf1eea9276c93f9305331a63c0/Source/WebCore/editing/AppendNodeCommand.cpp#43 for the span added here: https://webkit-search.igalia.com/webkit/rev/fcdc6d66bafc7fbf1eea9276c93f9305331a63c0/Source/WebCore/editing/ApplyStyleCommand.cpp#393 This is because elements with user-select: all are not editable: https://webkit-search.igalia.com/webkit/rev/fcdc6d66bafc7fbf1eea9276c93f9305331a63c0/Source/WebCore/dom/Node.cpp#763 Will upload a patch. Created attachment 423715 [details]
Patch
Created attachment 423868 [details]
Patch
Looks like we can simplify the test like this:
<!DOCTYPE html>
<html>
<head><meta></head>
<body>
<x>aaa</x><div></div>
<style>
body, head { display: inline; }
meta { -webkit-user-modify: readonly; }
span, div { -webkit-user-select: all; }
</style>
<script>
onload = () => {
document.execCommand('SelectAll');
document.designMode = 'on';
document.execCommand('FontSizeDelta', false, '1');
};
</script>
</body>
</html>
Comment on attachment 423868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423868&action=review > Source/WebCore/editing/ApplyStyleCommand.cpp:1317 > + document().updateLayoutIgnorePendingStylesheets(); > + if (element->renderer() && !element->hasEditableStyle()) { Why is renderer() check needed here. Also, just call isContentRichlyEditable() instead of explicitly updating the layout. Comment on attachment 423868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423868&action=review >> Source/WebCore/editing/ApplyStyleCommand.cpp:1317 >> + if (element->renderer() && !element->hasEditableStyle()) { > > Why is renderer() check needed here. Also, just call isContentRichlyEditable() instead of explicitly updating the layout. That's not necessary to address the crash/assert here (and so probably updateLayoutIgnorePendingStylesheets() isn't either). As I say in the changelog, this is for consistency with the assert here: https://webkit-search.igalia.com/webkit/rev/9afaca6c32615cd672fdbd973a62ab68bd9bf00d/Source/WebCore/editing/AppendNodeCommand.cpp#43 which happens in the appendNode call below. (In reply to Frédéric Wang (:fredw) from comment #9) > Comment on attachment 423868 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423868&action=review > > >> Source/WebCore/editing/ApplyStyleCommand.cpp:1317 > >> + if (element->renderer() && !element->hasEditableStyle()) { > > > > Why is renderer() check needed here. Also, just call isContentRichlyEditable() instead of explicitly updating the layout. > > That's not necessary to address the crash/assert here (and so probably > updateLayoutIgnorePendingStylesheets() isn't either). As I say in the > changelog, this is for consistency with the assert here: It's not really a matter of whether it's necessary or not. In general, hasEditableStyle is used when the style has already been updated. If we're unsure ,we should be calling isContentEditable or isContentRichlyEditable instead. Comment on attachment 423868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423868&action=review >>>> Source/WebCore/editing/ApplyStyleCommand.cpp:1317 >>>> + if (element->renderer() && !element->hasEditableStyle()) { >>> >>> Why is renderer() check needed here. Also, just call isContentRichlyEditable() instead of explicitly updating the layout. >> >> That's not necessary to address the crash/assert here (and so probably updateLayoutIgnorePendingStylesheets() isn't either). As I say in the changelog, this is for consistency with the assert here: >> >> https://webkit-search.igalia.com/webkit/rev/9afaca6c32615cd672fdbd973a62ab68bd9bf00d/Source/WebCore/editing/AppendNodeCommand.cpp#43 >> >> which happens in the appendNode call below. > > It's not really a matter of whether it's necessary or not. In general, hasEditableStyle is used when the style has already been updated. If we're unsure ,we should be calling isContentEditable or isContentRichlyEditable instead. I see thanks. Element::isContentEditable seems to be what corresponds to the debug ASSERT. Created attachment 424007 [details]
Patch for landing
Committed r274865: <https://commits.webkit.org/r274865> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424007 [details]. |