Bug 223364 – Nullptr deref in WebCore::ApplyStyleCommand::applyRelativeFontStyleChange

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Created attachment 423448 [details]
Test

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000012b2e678e WTF::OptionSet<WebCore::Node::NodeFlag>::containsAny(WTF::OptionSet<WebCore::Node::NodeFlag>) const + 190 (OptionSet.h:178)
1   com.apple.WebCore             	0x000000012b2e666a WTF::OptionSet<WebCore::Node::NodeFlag>::contains(WebCore::Node::NodeFlag) const + 218 (OptionSet.h:173)
2   com.apple.WebCore             	0x000000012b2e658d WebCore::Node::hasNodeFlag(WebCore::Node::NodeFlag) const + 13 (Node.h:578)
3   com.apple.WebCore             	0x000000012b2e657e WebCore::Node::isHTMLElement() const + 14 (Node.h:192)
4   com.apple.WebCore             	0x000000012b2e6569 WTF::TypeCastTraits<WebCore::HTMLElement const, WebCore::Node const, false>::isType(WebCore::Node const&) + 9 (HTMLElement.h:192)
5   com.apple.WebCore             	0x000000012b2e6559 WTF::TypeCastTraits<WebCore::HTMLElement const, WebCore::Node const, false>::isOfType(WebCore::Node const&) + 9 (HTMLElement.h:191)
6   com.apple.WebCore             	0x000000012c230099 bool WTF::is<WebCore::HTMLElement, WebCore::Node>(WebCore::Node&) + 9 (TypeCasts.h:58)
7   com.apple.WebCore             	0x000000012ecf31e1 WebCore::ApplyStyleCommand::applyRelativeFontStyleChange(WebCore::EditingStyle*) + 2353 (ApplyStyleCommand.cpp:385)
8   com.apple.WebCore             	0x000000012ecf1847 WebCore::ApplyStyleCommand::doApply() + 423 (ApplyStyleCommand.cpp:212)
9   com.apple.WebCore             	0x000000012eceba77 WebCore::CompositeEditCommand::apply() + 535 (CompositeEditCommand.cpp:375)
10  com.apple.WebCore             	0x000000012ed629d1 WebCore::Editor::applyStyle(WTF::RefPtr<WebCore::EditingStyle, WTF::RawPtrTraits<WebCore::EditingStyle>, WTF::DefaultRefDerefTraits<WebCore::EditingStyle> >&&, WebCore::EditAction, WebCore::Editor::ColorFilterMode) + 1057 (Editor.cpp:963)
11  com.apple.WebCore             	0x000000012edae75e WebCore::applyCommandToFrame(WebCore::Frame&, WebCore::EditorCommandSource, WebCore::EditAction, WTF::Ref<WebCore::EditingStyle, WTF::RawPtrTraits<WebCore::EditingStyle> >&&) + 270 (EditorCommand.cpp:111)
12  com.apple.WebCore             	0x000000012edae5e7 WebCore::executeApplyStyle(WebCore::Frame&, WebCore::EditorCommandSource, WebCore::EditAction, WebCore::CSSPropertyID, WTF::String const&) + 215 (EditorCommand.cpp:130)
13  com.apple.WebCore             	0x000000012eda8b58 WebCore::executeFontSizeDelta(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 24 (EditorCommand.cpp:394)
14  com.apple.WebCore             	0x000000012ed6e97c WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const + 220 (EditorCommand.cpp:1860)
15  com.apple.WebCore             	0x000000012e9eeda4 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 244 (Document.cpp:5687)
16  com.apple.WebCore             	0x000000012bbd4e6a WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) + 1130 (JSDocument.cpp:5890)
17  com.apple.WebCore             	0x000000012bbd495c 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*) + 252 (JSDOMOperation.h:53)
18  com.apple.WebCore             	0x000000012bbbf239 WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 9 (JSDocument.cpp:5895)
19  ???                           	0x000042dc850011d8 0 + 73514891612632
20  com.apple.JavaScriptCore      	0x000000010dc7ddb1 llint_entry + 110057 (LowLevelInterpreter.asm:1093)
21  com.apple.JavaScriptCore      	0x000000010dc62dc9 vmEntryToJavaScript + 216 (LowLevelInterpreter64.asm:316)
22  com.apple.JavaScriptCore      	0x000000010f4ab7d2 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 258 (JITCodeInlines.h:42) [inlined]
23  com.apple.JavaScriptCore      	0x000000010f4ab7d2 JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1554 (Interpreter.cpp:907)
24  com.apple.JavaScriptCore      	0x000000010fb94d15 JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 101 (CallData.cpp:57)
25  com.apple.JavaScriptCore      	0x000000010fb94e10 JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 224 (CallData.cpp:64)
26  com.apple.JavaScriptCore      	0x000000010fb951cc JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 268 (CallData.cpp:85)
27  com.apple.WebCore             	0x000000012e1e28a9 WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 233 (JSExecState.h:73)
28  com.apple.WebCore             	0x000000012e20ebab WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) + 2731 (JSEventListener.cpp:186)
29  com.apple.WebCore             	0x000000012eb1c263 WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::RawPtrTraits<WebCore::RegisteredEventListener>, WTF::DefaultRefDerefTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase) + 1315 (EventTarget.cpp:344)
30  com.apple.WebCore             	0x000000012eb1bb03 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) + 435 (EventTarget.cpp:276)
31  com.apple.WebCore             	0x000000012faf6295 WebCore::DOMWindow::dispatchEvent(WebCore::Event&, WebCore::EventTarget*) + 773 (DOMWindow.cpp:2312)
32  com.apple.WebCore             	0x000000012fb09a08 WebCore::DOMWindow::dispatchLoadEvent() + 552 (DOMWindow.cpp:2252)
33  com.apple.WebCore             	0x000000012e9d49d6 WebCore::Document::dispatchWindowLoadEvent() + 86 (Document.cpp:4943)
34  com.apple.WebCore             	0x000000012e9d43f4 WebCore::Document::implicitClose() + 756 (Document.cpp:3142)
35  com.apple.WebCore             	0x000000012f902a09 WebCore::FrameLoader::checkCallImplicitClose() + 217 (FrameLoader.cpp:940)
36  com.apple.WebCore             	0x000000012f901e93 WebCore::FrameLoader::checkCompleted() + 691 (FrameLoader.cpp:881)
37  com.apple.WebCore             	0x000000012f8fde95 WebCore::FrameLoader::finishedParsing() + 453 (FrameLoader.cpp:786)
38  com.apple.WebCore             	0x000000012e9f4c84 WebCore::Document::finishedParsing() + 612 (Document.cpp:6001)
39  com.apple.WebCore             	0x000000012f35d8c5 WebCore::HTMLConstructionSite::finishedParsing() + 37 (HTMLConstructionSite.cpp:419)
40  com.apple.WebCore             	0x000000012f3c1d3e WebCore::HTMLTreeBuilder::finished() + 30 (HTMLTreeBuilder.cpp:2843)
41  com.apple.WebCore             	0x000000012f366568 WebCore::HTMLDocumentParser::end() + 24 (HTMLDocumentParser.cpp:448)
42  com.apple.WebCore             	0x000000012f363e99 WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd() + 57 (HTMLDocumentParser.cpp:457)
43  com.apple.WebCore             	0x000000012f363dab WebCore::HTMLDocumentParser::prepareToStopParsing() + 267 (HTMLDocumentParser.cpp:152)
44  com.apple.WebCore             	0x000000012f3665b0 WebCore::HTMLDocumentParser::attemptToEnd() + 64 (HTMLDocumentParser.cpp:469)
45  com.apple.WebCore             	0x000000012f36664a WebCore::HTMLDocumentParser::finish() + 42 (HTMLDocumentParser.cpp:497)
46  com.apple.WebCore             	0x000000012f8cb281 WebCore::DocumentWriter::end() + 417 (DocumentWriter.cpp:292)
47  com.apple.WebCore             	0x000000012f87a74d WebCore::DocumentLoader::finishedLoading() + 733 (DocumentLoader.cpp:480)
48  com.apple.WebCore             	0x000000012f87a0ca WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource&, WebCore::NetworkLoadMetrics const&) + 714 (DocumentLoader.cpp:424)
49  com.apple.WebCore             	0x000000012fa52c00 WebCore::CachedResource::checkNotify(WebCore::NetworkLoadMetrics const&) + 384 (CachedResource.cpp:379)
50  com.apple.WebCore             	0x000000012fa4d08f WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&) + 79 (CachedResource.cpp:395)
51  com.apple.WebCore             	0x000000012fa4eb09 WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&) + 601 (CachedRawResource.cpp:123)
52  com.apple.WebCore             	0x000000012f9c2ec3 WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&) + 1843 (SubresourceLoader.cpp:736)
53  com.apple.WebCore             	0x000000012f9a1c51 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*) + 321 (ResourceLoader.cpp:718)

<rdar://75424192>

[Ryosuke Niwa]

I can reproduce this crash with DumpRenderTree and WebKitTestRunner at r274459.

[Frédéric Wang (:fredw)]

(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).

[Ryosuke Niwa]

Created attachment 423616 [details]
Test

Oops, uploaded a new test.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 423715 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 423868 [details]
Patch

[Ryosuke Niwa]

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>

[Ryosuke Niwa]

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.

[Frédéric Wang (:fredw)]

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.

[Ryosuke Niwa]

(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.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 424007 [details]
Patch for landing

[EWS]

Committed r274865: <https://commits.webkit.org/r274865>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424007 [details].