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