Bug 223753 – Crash in InsertTextCommand::positionInsideTextNode

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ali Juma]

Created attachment 424262 [details]
Minimal test case

Filing this as a security bug since it was found using a fuzzer; there's no disclosure deadline for this bug.

This reproduces in an ASan build of WebKitTestRunner, and also in STP 122.

Stack:
=================================================================
==79602==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000001c (pc 0x00062fcb967e bp 0x7ffee96cabd0 sp 0x7ffee96cab20 T0)
==79602==The signal is caused by a READ memory access.
==79602==Hint: address points to the zero page.
    #0 0x62fcb967d in WTF::OptionSet<WebCore::Node::NodeFlag>::containsAny(WTF::OptionSet<WebCore::Node::NodeFlag>) const (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x18d67d)
    #1 0x62fcb9559 in WTF::OptionSet<WebCore::Node::NodeFlag>::contains(WebCore::Node::NodeFlag) const (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x18d559)
    #2 0x6333ef9e2 in WebCore::InsertTextCommand::positionInsideTextNode(WebCore::Position const&) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x38c39e2)
    #3 0x6333f0b1c in WebCore::InsertTextCommand::doApply() (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x38c4b1c)
    #4 0x633340d42 in WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::CompositeEditCommand, WTF::RawPtrTraits<WebCore::CompositeEditCommand> >&&, WebCore::VisibleSelection const&) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x3814d42)
    #5 0x63344f1c1 in WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x39231c1)
    #6 0x633473f5d in WebCore::TypingCommandLineOperation::operator()(unsigned long, unsigned long, bool) const (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x3947f5d)
    #7 0x63344eef0 in WebCore::TypingCommand::insertText(WTF::String const&, bool) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x3922ef0)
    #8 0x63344c7fc in WebCore::TypingCommand::insertTextAndNotifyAccessibility(WTF::String const&, bool) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x39207fc)
    #9 0x633325af4 in WebCore::CompositeEditCommand::apply() (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x37f9af4)
    #10 0x63342f7f9 in WebCore::TextInsertionBaseCommand::applyTextInsertionCommand(WebCore::Frame*, WebCore::TextInsertionBaseCommand&, WebCore::VisibleSelection const&, WebCore::VisibleSelection const&) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x39037f9)
    #11 0x63344c5e2 in WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, WebCore::VisibleSelection const&, unsigned int, WebCore::TypingCommand::TextCompositionType) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x39205e2)
    #12 0x6333d5a8c in WebCore::executeInsertText(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x38a9a8c)
    #13 0x63303e5f3 in WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x35125f3)
    #14 0x6305005d1 in WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x9d45d1)
    #15 0x6305000cb in 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*) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x9d40cb)
    #16 0x27518fa011d7  (<unknown module>)
    #17 0x64c0e1c90 in llint_entry (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xb4cc90)
    #18 0x64c0c6ca8 in vmEntryToJavaScript (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xb31ca8)
    #19 0x64d8585b0 in JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x22c35b0)
    #20 0x64def44cf in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x295f4cf)
    #21 0x64def488b in JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x295f88b)
    #22 0x63289cf08 in WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x2d70f08)
    #23 0x63289c531 in WebCore::JSCallbackData::invokeCallback(WebCore::JSDOMGlobalObject&, JSC::JSObject*, JSC::JSValue, JSC::MarkedArgumentBuffer&, WebCore::JSCallbackData::CallbackType, JSC::PropertyName, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x2d70531)
    #24 0x630011eef in WebCore::JSCallbackDataStrong::invokeCallback(JSC::JSValue, JSC::MarkedArgumentBuffer&, WebCore::JSCallbackData::CallbackType, JSC::PropertyName, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x4e5eef)
    #25 0x630ec9d40 in WebCore::JSRequestAnimationFrameCallback::handleEvent(double) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x139dd40)
    #26 0x63325e3f3 in WebCore::ScriptedAnimationController::serviceRequestAnimationFrameCallbacks(WTF::Seconds) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x37323f3)
    #27 0x634196a02 in WebCore::Page::forEachDocument(WTF::Function<void (WebCore::Document&)> const&) const (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x466aa02)
    #28 0x6341a25a1 in WebCore::Page::updateRendering()::$_17::operator()(WebCore::RenderingUpdateStep, WTF::Function<void (WebCore::Document&)> const&) const (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x46765a1)
    #29 0x6341a1f84 in WebCore::Page::updateRendering() (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebCore.framework/Versions/A/WebCore:x86_64+0x4675f84)
    #30 0x621caa511 in WebKit::TiledCoreAnimationDrawingArea::updateRendering(WebKit::TiledCoreAnimationDrawingArea::UpdateRenderingType) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebKit.framework/Versions/A/WebKit:x86_64+0x1caa511)
    #31 0x7fff36e99e5b in __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x83e5b)
    #32 0x7fff36e99d8b in __CFRunLoopDoObservers (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x83d8b)
    #33 0x7fff36e9898d in CFRunLoopRunSpecific (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x8298d)
    #34 0x7fff395561c7 in -[NSRunLoop(NSRunLoop) runMode:beforeDate:] (/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation:x86_64+0x601c7)
    #35 0x7fff39608c6e in -[NSRunLoop(NSRunLoop) run] (/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation:x86_64+0x112c6e)
    #36 0x7fff710754e9 in _xpc_objc_main.cold.4 (/usr/lib/system/libxpc.dylib:x86_64+0x164e9)
    #37 0x7fff7107542f in _xpc_objc_main (/usr/lib/system/libxpc.dylib:x86_64+0x1642f)
    #38 0x7fff71074f62 in xpc_main (/usr/lib/system/libxpc.dylib:x86_64+0x15f62)
    #39 0x620f0f0a3 in WebKit::XPCServiceMain(int, char const**) (/Users/chrome-bot/clusterfuzz/bot/builds/chrome-ios-webkit-to-fuzz_ios-webkit-to-fuzz_cb292771138f3c7c4bb12f2df778e2b1c42b4cd7/revisions/WebKitMacOS/WebKit.framework/Versions/A/WebKit:x86_64+0xf0f0a3)
    #40 0x7fff70e23cc8 in start (/usr/lib/system/libdyld.dylib:x86_64+0x1acc8)
==79602==Register values:
rax = 0x0000100000000000  rbx = 0x00007ffee96cab60  rcx = 0x0000100000000000  rdx = 0x0000000000000000
rdi = 0x000000000000001c  rsi = 0x0000000000000002  rbp = 0x00007ffee96cabd0  rsp = 0x00007ffee96cab20
 r8 = 0x0000100000000000   r9 = 0x0000000000000000  r10 = 0xffffffffffffffff  r11 = 0xfffffffffffffe60
r12 = 0x00001fffdd2d9564  r13 = 0x00007ffee96cab40  r14 = 0x00007ffee96cab20  r15 = 0x0000000000000000
=========
Clusterfuzz-id: 5721820586901504

[Radar WebKit Bug Importer]

<rdar://problem/75844794>

[Ryosuke Niwa]

More concise test case:
<style>
  :empty {
    -webkit-appearance: checkbox;
  }
</style>
<script>
  onload = () => {
    document.body.appendChild(document.createElement('iframe'));
    let n0 = document.createElement('tr');
    document.body.appendChild(n0);
    n0.appendChild(document.createElement('div'));
    document.body.appendChild(document.createElement('tr'));
    document.execCommand('SelectAll');
    document.designMode = 'on';
    document.execCommand('InsertText', false, '');
  };
</script>

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

Created attachment 425272 [details]
Reduced testcase

Just copying it from comment 2.

This is also reproducible on Linux GTK.

On debug, we hit the following assert :

ASSERTION FAILED: ancestor
https://webkit-search.igalia.com/webkit/rev/8c31d75d471dfa8cffb95e0ec9cae9b4703503df/Source/WebCore/dom/Position.cpp#1570
#0  WTFCrash() () at ../../Source/WTF/wtf/Assertions.cpp:295
#1  0x00007fd07b2911f1 in CRASH_WITH_INFO(...) ()
    at DerivedSources/ForwardingHeaders/wtf/Assertions.h:713
#2  0x00007fd07e2addca in WebCore::positionInParentBeforeNode(WebCore::Node*)
    (node=0x7fd070e06900) at ../../Source/WebCore/dom/Position.cpp:1570
#3  0x00007fd07e3d8989 in WebCore::InsertTextCommand::doApply() (this=
    0x7fd070d435c8) at ../../Source/WebCore/editing/InsertTextCommand.cpp:177
#4  0x00007fd0800eb740 in WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::CompositeEditCommand, WTF::RawPtrTraits<WebCore::CompositeEditCommand> >&&, WebCore::VisibleSelection const&)
    (this=0x7fd016a94a00, command=..., selection=...)
    at ../../Source/WebCore/editing/CompositeEditCommand.cpp:485
#5  0x00007fd07e4109b2 in WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool)
    (this=0x7fd016a94a00, text=..., selectInsertedText=false)
    at ../../Source/WebCore/editing/TypingCommand.cpp:540
...

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

So positionInParentBeforeNode receives an orphan TR. The endingSelection's start is set to that TR by mergeParagraphs() and then detached from the tree by removePreviouslySelectedEmptyTableRows():

https://webkit-search.igalia.com/webkit/rev/8c31d75d471dfa8cffb95e0ec9cae9b4703503df/Source/WebCore/editing/DeleteSelectionCommand.cpp#943

Below is a basic debug session:

Thread 1 received signal SIGSEGV, Segmentation fault.
WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:295
(rr) reverse-f
(rr) 
(rr) 
https://webkit-search.igalia.com/webkit/rev/8c31d75d471dfa8cffb95e0ec9cae9b4703503df/Source/WebCore/dom/Position.cpp#1570
1570	    ASSERT(ancestor);
(rr)  p node->parentNode()
$1 = (WebCore::ContainerNode *) 0x0
(rr) p showTree(node)
*TR	0x7fd070e06900 (renderer (nil)) 
$2 = void
(rr) reverse-f
https://webkit-search.igalia.com/webkit/rev/8c31d75d471dfa8cffb95e0ec9cae9b4703503df/Source/WebCore/editing/InsertTextCommand.cpp#177
(rr) p showTree(endingSelection().start())
*TR	0x7fd070e06900 (renderer (nil)) 
legacy, offset, offset:0
$3 = void
(rr) watch -l endingSelection().m_start
(rr) rc
(rr) delete
(rr) reverse-f
(rr) 
(rr) 
(rr) 
(rr) 
(rr) 
(rr)
https://webkit-search.igalia.com/webkit/rev/8c31d75d471dfa8cffb95e0ec9cae9b4703503df/Source/WebCore/editing/CompositeEditCommand.cpp#1504
(rr) p showTree(destination)
BODY	0x7fd070e041c0 (renderer 0x7fd070e11800) 
*	TR	0x7fd070e06900 (renderer 0x7fd070e05a40) 
	TR	0x7fd070e06a20 (renderer 0x7fd070e07380) 
legacy, offset, offset:0
$4 = void
(rr) bt
#0  0x00007fd0800f1d60 in WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool)
    (this=0x7fd016a9e6c0, startOfParagraphToMove=..., endOfParagraphToMove=..., destination=..., preserveSelection=false, preserveStyle=false)
    at ../../Source/WebCore/editing/CompositeEditCommand.cpp:1487
#1  0x00007fd0800f1073 in WebCore::CompositeEditCommand::moveParagraph(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool)
    (this=0x7fd016a9e6c0, startOfParagraphToMove=..., endOfParagraphToMove=..., destination=..., preserveSelection=false, preserveStyle=false)
    at ../../Source/WebCore/editing/CompositeEditCommand.cpp:1391
#2  0x00007fd07e3774f8 in WebCore::DeleteSelectionCommand::mergeParagraphs() (this=0x7fd016a9e6c0)
    at ../../Source/WebCore/editing/DeleteSelectionCommand.cpp:759
#3  0x00007fd07e37850f in WebCore::DeleteSelectionCommand::doApply() (this=0x7fd016a9e6c0)
    at ../../Source/WebCore/editing/DeleteSelectionCommand.cpp:943
#4  0x00007fd0800eb5fc in WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::RawPtrTraits<WebCore::EditCommand> >&&) (this=0x7fd070d435c8, command=...) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:470
#5  0x00007fd0800eddc5 in WebCore::CompositeEditCommand::deleteSelection(bool, bool, bool, bool, bool)
    (this=0x7fd070d435c8, smartDelete=false, mergeBlocksAfterDelete=true, replace=true, expandForSpecialElements=false, sanitizeMarkup=false)
    at ../../Source/WebCore/editing/CompositeEditCommand.cpp:839
#6  0x00007fd07e3d87ba in WebCore::InsertTextCommand::doApply() (this=0x7fd070d435c8) at ../../Source/WebCore/editing/InsertTextCommand.cpp:143
#7  0x00007fd0800eb740 in WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::CompositeEditCommand, WTF::RawPtrTraits<WebCore::CompositeEditCommand> >&&, WebCore::VisibleSelection const&) (this=0x7fd016a94a00, command=..., selection=...)
    at ../../Source/WebCore/editing/CompositeEditCommand.cpp:485
#8  0x00007fd07e4109b2 in WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool)
    (this=0x7fd016a94a00, text=..., selectInsertedText=false) at ../../Source/WebCore/editing/TypingCommand.cpp:540
--Type <RET> for more, q to quit, c to continue without paging--c
#9  0x00007fd07e419284 in WebCore::TypingCommandLineOperation::operator()(unsigned long, unsigned long, bool) const (this=0x7ffceccb8c20, lineOffset=0, lineLength=0, isLastLine=true) at ../../Source/WebCore/editing/TypingCommand.cpp:69
#10 0x00007fd07e41c7fe in WebCore::forEachLineInString<WebCore::TypingCommandLineOperation>(WTF::String const&, WebCore::TypingCommandLineOperation const&) (string=..., operation=...) at ../../Source/WebCore/editing/TextInsertionBaseCommand.h:60
#11 0x00007fd07e410745 in WebCore::TypingCommand::insertText(WTF::String const&, bool) (this=0x7fd016a94a00, text=..., selectInsertedText=false) at ../../Source/WebCore/editing/TypingCommand.cpp:519
(rr) watch -l ((Node)*0x7fd070e06900)->m_parentNode
(rr) c
(rr) delete
(rr) reverse-f
(rr) reverse-f
(rr) 
(rr) 
(rr) 
(rr) 
(rr) 
(rr) 
https://webkit-search.igalia.com/webkit/rev/8c31d75d471dfa8cffb95e0ec9cae9b4703503df/Source/WebCore/editing/DeleteSelectionCommand.cpp#777
(rr) p showTree(row)
BODY	0x7fd070e041c0 (renderer 0x7fd070e11800) 
*	TR	0x7fd070e06900 (renderer 0x7fd070e05a40) 
	TR	0x7fd070e06a20 (renderer 0x7fd070e07380) 
$5 = void
(rr) p m_startTableRow
$6 = {... , m_ptr = 0x0}
(rr) p showTree((Node*)m_endTableRow)
BODY	0x7fd070e041c0 (renderer 0x7fd070e11800) 
	TR	0x7fd070e06900 (renderer 0x7fd070e05a40) 
*	TR	0x7fd070e06a20 (renderer 0x7fd070e07380) 
$7 = void
(rr) bt
#0  0x00007fd07e3777ae in WebCore::DeleteSelectionCommand::removePreviouslySelectedEmptyTableRows() (this=0x7fd016a9e6c0)
    at ../../Source/WebCore/editing/DeleteSelectionCommand.cpp:777
#1  0x00007fd07e37851e in WebCore::DeleteSelectionCommand::doApply() (this=0x7fd016a9e6c0)
    at ../../Source/WebCore/editing/DeleteSelectionCommand.cpp:945
#2  0x00007fd0800eb5fc in WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::RawPtrTraits<WebCore::EditCommand> >&&) (this=0x7fd070d435c8, command=...) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:470
#3  0x00007fd0800eddc5 in WebCore::CompositeEditCommand::deleteSelection(bool, bool, bool, bool, bool)
    (this=0x7fd070d435c8, smartDelete=false, mergeBlocksAfterDelete=true, replace=true, expandForSpecialElements=false, sanitizeMarkup=false)
    at ../../Source/WebCore/editing/CompositeEditCommand.cpp:839
#4  0x00007fd07e3d87ba in WebCore::InsertTextCommand::doApply() (this=0x7fd070d435c8) at ../../Source/WebCore/editing/InsertTextCommand.cpp:143
#5  0x00007fd0800eb740 in WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::CompositeEditCommand, WTF::RawPtrTraits<WebCore::CompositeEditCommand> >&&, WebCore::VisibleSelection const&) (this=0x7fd016a94a00, command=..., selection=...)
    at ../../Source/WebCore/editing/CompositeEditCommand.cpp:485
#6  0x00007fd07e4109b2 in WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool)
    (this=0x7fd016a94a00, text=..., selectInsertedText=false) at ../../Source/WebCore/editing/TypingCommand.cpp:540
#7  0x00007fd07e419284 in WebCore::TypingCommandLineOperation::operator()(unsigned long, unsigned long, bool) const
    (this=0x7ffceccb8c20, lineOffset=0, lineLength=0, isLastLine=true) at ../../Source/WebCore/editing/TypingCommand.cpp:69
#8  0x00007fd07e41c7fe in WebCore::forEachLineInString<WebCore::TypingCommandLineOperation>(WTF::String const&, WebCore::TypingCommandLineOperation const&) (string=..., operation=...) at ../../Source/WebCore/editing/TextInsertionBaseCommand.h:60
#9  0x00007fd07e410745 in WebCore::TypingCommand::insertText(WTF::String const&, bool) (this=0x7fd016a94a00, text=..., selectInsertedText=false)
    at ../../Source/WebCore/editing/TypingCommand.cpp:519

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

Created attachment 425282 [details]
Patch (test)

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

Created attachment 425283 [details]
Patch (tentative)

[Ryosuke Niwa]

Hm... I think these test failures are real.

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

Created attachment 425366 [details]
Patch (tentative)

(In reply to Ryosuke Niwa from comment #7)
> Hm... I think these test failures are real.

Yeah... let's try a less heavy change.

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

Created attachment 425367 [details]
Patch (tentative)

[Ryosuke Niwa]

Comment on attachment 425283 [details]
Patch (tentative)

View in context: https://bugs.webkit.org/attachment.cgi?id=425283&action=review

> Source/WebCore/editing/DeleteSelectionCommand.cpp:945
>      removePreviouslySelectedEmptyTableRows();

Can we fix this function so that the ending selection doesn't get orphaned in the first place?

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

Comment on attachment 425283 [details]
Patch (tentative)

View in context: https://bugs.webkit.org/attachment.cgi?id=425283&action=review

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:945
>>      removePreviouslySelectedEmptyTableRows();
> 
> Can we fix this function so that the ending selection doesn't get orphaned in the first place?

I was thinking about that too, but it looks it can have the same impact as the original patch or will require more care about checking exactly when it becomes orphan. Will give a try later.

[Ryosuke Niwa]

(In reply to Frédéric Wang (:fredw) from comment #11)
> Comment on attachment 425283 [details]
> Patch (tentative)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425283&action=review
> 
> >> Source/WebCore/editing/DeleteSelectionCommand.cpp:945
> >>      removePreviouslySelectedEmptyTableRows();
> > 
> > Can we fix this function so that the ending selection doesn't get orphaned in the first place?
> 
> I was thinking about that too, but it looks it can have the same impact as
> the original patch or will require more care about checking exactly when it
> becomes orphan. Will give a try later.

Are you sure about that? I think the issue was the original patch was that we were exiting early.

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

Comment on attachment 425283 [details]
Patch (tentative)

View in context: https://bugs.webkit.org/attachment.cgi?id=425283&action=review

>>>> Source/WebCore/editing/DeleteSelectionCommand.cpp:945
>>>>      removePreviouslySelectedEmptyTableRows();
>>> 
>>> Can we fix this function so that the ending selection doesn't get orphaned in the first place?
>> 
>> I was thinking about that too, but it looks it can have the same impact as the original patch or will require more care about checking exactly when it becomes orphan. Will give a try later.
> 
> Are you sure about that? I think the issue was the original patch was that we were exiting early.

The original patch (which is the one we are commenting on) was just clearing the selection and not exiting, see below. So just moving the code into removePreviouslySelectedEmptyTableRows() won't help but I can probably try to do something better about when/how we adjust the selection.

[Ryosuke Niwa]

(In reply to Frédéric Wang (:fredw) from comment #13)
> Comment on attachment 425283 [details]
> Patch (tentative)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425283&action=review
> 
> >>>> Source/WebCore/editing/DeleteSelectionCommand.cpp:945
> >>>>      removePreviouslySelectedEmptyTableRows();
> >>> 
> >>> Can we fix this function so that the ending selection doesn't get orphaned in the first place?
> >> 
> >> I was thinking about that too, but it looks it can have the same impact as the original patch or will require more care about checking exactly when it becomes orphan. Will give a try later.
> > 
> > Are you sure about that? I think the issue was the original patch was that we were exiting early.
> 
> The original patch (which is the one we are commenting on) was just clearing
> the selection and not exiting, see below. So just moving the code into
> removePreviouslySelectedEmptyTableRows() won't help but I can probably try
> to do something better about when/how we adjust the selection.

I think the issue is that we're getting ending selection orphaned in the first place. That shouldn't be happening. We should be adjusting ending selection to stay in the document as we make more DOM mutations. There is a bunch of code to do that. We need to figure out how & why ending selection end up being orphaned here.

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

(In reply to Ryosuke Niwa from comment #14)
> I think the issue is that we're getting ending selection orphaned in the
> first place. That shouldn't be happening. We should be adjusting ending
> selection to stay in the document as we make more DOM mutations. There is a
> bunch of code to do that. We need to figure out how & why ending selection
> end up being orphaned here.

I think I explained this in comment 4, basically "The endingSelection's start is set to that TR by mergeParagraphs() and then detached from the tree by removePreviouslySelectedEmptyTableRows()". So maybe we can add something in removePreviouslySelectedEmptyTableRows() to check that when we are removing the TR we adjust the selection accordingly e.g. by selecting the whole table or something else. The thing is that my original patch was just clearing the whole selection as long as it is orphan after that function call, which I agree is probably a bit too much.

[Ryosuke Niwa]

(In reply to Frédéric Wang (:fredw) from comment #15)
> (In reply to Ryosuke Niwa from comment #14)
> > I think the issue is that we're getting ending selection orphaned in the
> > first place. That shouldn't be happening. We should be adjusting ending
> > selection to stay in the document as we make more DOM mutations. There is a
> > bunch of code to do that. We need to figure out how & why ending selection
> > end up being orphaned here.
> 
> I think I explained this in comment 4, basically "The endingSelection's
> start is set to that TR by mergeParagraphs() and then detached from the tree
> by removePreviouslySelectedEmptyTableRows()". So maybe we can add something
> in removePreviouslySelectedEmptyTableRows() to check that when we are
> removing the TR we adjust the selection accordingly e.g. by selecting the
> whole table or something else.

Sorry, I missed that. Yeah, that sounds like a good approach to me.

[Ryosuke Niwa]

Comment on attachment 425367 [details]
Patch (tentative)

Based on the above this discussion, I'd say r- on this particular patch.

Also this patch is missing the test.

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

Created attachment 427417 [details]
Alternative WIP patch

Some relevant changes for this issue are:

https://trac.webkit.org/changeset/28670/webkit
https://trac.webkit.org/changeset/272779/webkit

Attached is an alternative patch doing the same as in bug 213514.

I put this as WIP, mostly because we should probably do similar replacement for the remaining CompositeEditCommand::removeNode() call in DeleteSelectionCommand::removePreviouslySelectedEmptyTableRows() too, and maybe updates all the comments of this method, add the changelog & tests, etc

One thing I wonder is this:

                // Use a raw removeNode, instead of DeleteSelectionCommand's, because
                // that won't remove rows, it only empties them in preparation for this function.

I believe this means we actually don't want to call DeleteSelectionCommand::removeNode but it's still safe to call DeleteSelectionCommand::removeNodeUpdatingStates, which calls CompositeEditCommand::removeNode directly?

I also wonder whether we should update this comment now that we actually adjust the selection:

                // FIXME: We probably shouldn't remove m_endTableRow unless it's fully selected, even if it is empty.
                // We'll need to start adjusting the selection endpoints during deletion to know whether or not m_endTableRow
                // was fully selected here.

[Ryosuke Niwa]

Comment on attachment 427417 [details]
Alternative WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427417&action=review

> Source/WebCore/editing/DeleteSelectionCommand.cpp:856
> -                CompositeEditCommand::removeNode(*row);
> +                removeNodeUpdatingStates(*row, DoNotAssumeContentIsAlwaysEditable);

Ok, this makes more sense to me.
There is anther call to CompositeEditCommand::removeNode below.
We should probably call removeNodeUpdatingStates there instead as well.

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

Comment on attachment 427417 [details]
Alternative WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427417&action=review

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:856
>> +                removeNodeUpdatingStates(*row, DoNotAssumeContentIsAlwaysEditable);
> 
> Ok, this makes more sense to me.
> There is anther call to CompositeEditCommand::removeNode below.
> We should probably call removeNodeUpdatingStates there instead as well.

Yes, please see my comments/questions at https://bugs.webkit.org/show_bug.cgi?id=223753#c18

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

Created attachment 427644 [details]
Patch

OK, I updated the comment about raw call to removeNode. About the TODO from https://trac.webkit.org/changeset/28670/webkit, I'm still not sure what is meant here, so I'll keep it as it.

I'm integrating the test too, since this is similar to https://trac.webkit.org/changeset/272779/webkit which landed with a test and was removed from security component.

[EWS]

Committed r277163 (237449@main): <https://commits.webkit.org/237449@main>

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