Bug 225219 – SHOULD NEVER BE REACHED in FrameSelection::setSelectionWithoutUpdatingAppearance for editing/selection/selection-in-iframe-removed-crash.html

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

[Fujii Hironori]

[WinCairo] SHOULD NEVER BE REACHED in FrameSelection::setSelectionWithoutUpdatingAppearance for editing/selection/selection-in-iframe-removed-crash.html

WinCairo WK2 Debug

> SHOULD NEVER BE REACHED
> C:\home\webkit\gb\Source\WebCore\editing/FrameSelection.cpp(361) : WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance

python.exe ./Tools/Scripts/run-webkit-tests --wincairo --debug --no-retry-failures editing/selection/selection-in-iframe-removed-crash.html --iterations=4 -v

[1/4] editing/selection/selection-in-iframe-removed-crash.html passed
[2/4] editing/selection/selection-in-iframe-removed-crash.html passed
[3/4] editing/selection/selection-in-iframe-removed-crash.html failed unexpectedly (WebProcess crashed [pid=15016])
[4/4] editing/selection/selection-in-iframe-removed-crash.html passed


Callstack:

 # Child-SP          RetAddr           Call Site
00 000000d5`6ab6b320 00007ffc`2a723d41 WTF!WTFCrash(void)+0x1f [C:\home\webkit\gb\Source\WTF\wtf\Assertions.cpp @ 305]
01 000000d5`6ab6b350 00007ffc`2e851e05 WebKit2!WTFCrashWithInfo(int __formal = 0n361, char * __formal = 0x00007ffc`3e482368 "C:\home\webkit\gb\Source\WebCore\editing/FrameSelection.cpp", char * __formal = 0x00007ffc`3e481b28 "WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance", int __formal = 0n2248)+0x31 [C:\home\webkit\gb\WebKitBuild\Debug\WTF\Headers\wtf\Assertions.h @ 693]
02 000000d5`6ab6b380 00007ffc`2e84abd6 WebKit2!WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance(class WebCore::VisibleSelection * newSelectionPossiblyWithoutDirection = 0x000000d5`6ab6b960, class WTF::OptionSet<enum WebCore::FrameSelection::SetSelectionOption> options = class WTF::OptionSet<enum WebCore::FrameSelection::SetSelectionOption>, WebCore::FrameSelection::CursorAlignOnScroll align = AlignCursorOnScrollIfNeeded (0n0), WebCore::TextGranularity granularity = CharacterGranularity (0n0))+0x435 [C:\home\webkit\gb\Source\WebCore\editing\FrameSelection.cpp @ 361]
03 000000d5`6ab6b730 00007ffc`2e85688a WebKit2!WebCore::FrameSelection::setSelection(class WebCore::VisibleSelection * selection = 0x000000d5`6ab6b960, class WTF::OptionSet<enum WebCore::FrameSelection::SetSelectionOption> options = class WTF::OptionSet<enum WebCore::FrameSelection::SetSelectionOption>, struct WebCore::AXTextStateChangeIntent * intent = 0x000000d5`6ab6baf0, WebCore::FrameSelection::CursorAlignOnScroll align = AlignCursorOnScrollIfNeeded (0n0), WebCore::TextGranularity granularity = CharacterGranularity (0n0))+0x186 [C:\home\webkit\gb\Source\WebCore\editing\FrameSelection.cpp @ 426]
04 000000d5`6ab6b870 00007ffc`2e8523a3 WebKit2!WebCore::FrameSelection::selectFrameElementInParentIfFullySelected(void)+0x41a [C:\home\webkit\gb\Source\WebCore\editing\FrameSelection.cpp @ 1961]
05 000000d5`6ab6bb60 00007ffc`2e84abd6 WebKit2!WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance(class WebCore::VisibleSelection * newSelectionPossiblyWithoutDirection = 0x000000d5`6ab6c0a0, class WTF::OptionSet<enum WebCore::FrameSelection::SetSelectionOption> options = class WTF::OptionSet<enum WebCore::FrameSelection::SetSelectionOption>, WebCore::FrameSelection::CursorAlignOnScroll align = AlignCursorOnScrollIfNeeded (0n0), WebCore::TextGranularity granularity = CharacterGranularity (0n0))+0x9d3 [C:\home\webkit\gb\Source\WebCore\editing\FrameSelection.cpp @ 413]
06 000000d5`6ab6bf10 00007ffc`2e851c9e WebKit2!WebCore::FrameSelection::setSelection(class WebCore::VisibleSelection * selection = 0x000000d5`6ab6c0a0, class WTF::OptionSet<enum WebCore::FrameSelection::SetSelectionOption> options = class WTF::OptionSet<enum WebCore::FrameSelection::SetSelectionOption>, struct WebCore::AXTextStateChangeIntent * intent = 0x000000d5`6ab6c380, WebCore::FrameSelection::CursorAlignOnScroll align = AlignCursorOnScrollIfNeeded (0n0), WebCore::TextGranularity granularity = CharacterGranularity (0n0))+0x186 [C:\home\webkit\gb\Source\WebCore\editing\FrameSelection.cpp @ 426]
07 000000d5`6ab6c050 00007ffc`2e84abd6 WebKit2!WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance(class WebCore::VisibleSelection * newSelectionPossiblyWithoutDirection = 0x000000d5`6ab6c748, class WTF::OptionSet<enum WebCore::FrameSelection::SetSelectionOption> options = class WTF::OptionSet<enum WebCore::FrameSelection::SetSelectionOption>, WebCore::FrameSelection::CursorAlignOnScroll align = AlignCursorOnScrollIfNeeded (0n0), WebCore::TextGranularity granularity = CharacterGranularity (0n0))+0x2ce [C:\home\webkit\gb\Source\WebCore\editing\FrameSelection.cpp @ 346]
08 000000d5`6ab6c400 00007ffc`2f24b0f5 WebKit2!WebCore::FrameSelection::setSelection(class WebCore::VisibleSelection * selection = 0x000000d5`6ab6c748, class WTF::OptionSet<enum WebCore::FrameSelection::SetSelectionOption> options = class WTF::OptionSet<enum WebCore::FrameSelection::SetSelectionOption>, struct WebCore::AXTextStateChangeIntent * intent = 0x000000d5`6ab6c840, WebCore::FrameSelection::CursorAlignOnScroll align = AlignCursorOnScrollIfNeeded (0n0), WebCore::TextGranularity granularity = CharacterGranularity (0n0))+0x186 [C:\home\webkit\gb\Source\WebCore\editing\FrameSelection.cpp @ 426]
09 000000d5`6ab6c540 00007ffc`2c6d4141 WebKit2!WebCore::DOMSelection::addRange(class WebCore::Range * liveRange = 0x000001f9`e6c9b390)+0x405 [C:\home\webkit\gb\Source\WebCore\page\DOMSelection.cpp @ 398]
0a 000000d5`6ab6c880 00007ffc`2c6cc787 WebKit2!<lambda_ede694bd6c7f0e3386b51a17f396b85a>::operator()(void)+0x41 [C:\home\webkit\gb\WebKitBuild\Debug\WebCore\DerivedSources\JSDOMSelection.cpp @ 427]
0b 000000d5`6ab6c8c0 00007ffc`2c6b4688 WebKit2!WebCore::toJS<WebCore::IDLUndefined,<lambda_ede694bd6c7f0e3386b51a17f396b85a> >(class JSC::JSGlobalObject * lexicalGlobalObject = 0x000001f9`e2e32048, class JSC::ThrowScope * throwScope = 0x000000d5`6ab6c940, class WebCore::jsDOMSelectionPrototypeFunction_addRangeBody::__l20::<lambda_ede694bd6c7f0e3386b51a17f396b85a> * valueOrFunctor = 0x000000d5`6ab6ca38)+0x37 [C:\home\webkit\gb\Source\WebCore\bindings\js\JSDOMConvertBase.h @ 166]
0c 000000d5`6ab6c8f0 00007ffc`2c6bc889 WebKit2!WebCore::jsDOMSelectionPrototypeFunction_addRangeBody(class JSC::JSGlobalObject * lexicalGlobalObject = 0x000001f9`e2e32048, class JSC::CallFrame * callFrame = 0x000000d5`6ab6cc60, class WebCore::JSDOMSelection * castedThis = 0x000001f9`e6dcd318)+0x328 [C:\home\webkit\gb\WebKitBuild\Debug\WebCore\DerivedSources\JSDOMSelection.cpp @ 427]
0d 000000d5`6ab6cab0 00007ffc`2c6b2269 WebKit2!WebCore::IDLOperation<WebCore::JSDOMSelection>::call<&WebCore::jsDOMSelectionPrototypeFunction_addRangeBody,0>(class JSC::JSGlobalObject * lexicalGlobalObject = 0x000001f9`e2e32048, class JSC::CallFrame * callFrame = 0x000000d5`6ab6cc60, char * operationName = 0x00007ffc`3749e088 "addRange")+0x309 [C:\home\webkit\gb\Source\WebCore\bindings\js\JSDOMOperation.h @ 55]
0e 000000d5`6ab6cc10 000001f9`800011be WebKit2!WebCore::jsDOMSelectionPrototypeFunction_addRange(class JSC::JSGlobalObject * lexicalGlobalObject = 0x000001f9`e2e32048, class JSC::CallFrame * callFrame = 0x000000d5`6ab6cc60)+0x39 [C:\home\webkit\gb\WebKitBuild\Debug\WebCore\DerivedSources\JSDOMSelection.cpp @ 433]
0f 000000d5`6ab6cc40 000001f9`e2e32048 0x000001f9`800011be
10 000000d5`6ab6cc48 000000d5`6ab6cc60 0x000001f9`e2e32048
11 000000d5`6ab6cc50 000000d5`6ab6cce0 0x000000d5`6ab6cc60
12 000000d5`6ab6cc58 00007ffc`467c942e 0x000000d5`6ab6cce0
13 000000d5`6ab6cc60 000000d5`6ab6cce0 JavaScriptCore!llint_entry+0x21aee
14 000000d5`6ab6cc68 00007ffc`467c942e 0x000000d5`6ab6cce0
15 000000d5`6ab6cc70 00000000`00000000 JavaScriptCore!llint_entry+0x21aee

[Fujii Hironori]

Created attachment 427388 [details]
selection-in-iframe-removed-crash-crash-log.txt (WinCairo WK2 Debug)

[Fujii Hironori]

Created attachment 427389 [details]
selection-in-iframe-removed-crash-crash-log.txt (GTK Debug)

GTK-Linux-64-bit-Debug-Tests is also failing the assertion randomly.

This seems that oldest testing job. r274685 
https://build.webkit.org/#/builders/63/builds/441

https://results.webkit.org/?suite=layout-tests&test=editing%2Fselection%2Fselection-in-iframe-removed-crash.html&platform=GTK

[Fujii Hironori]

(In reply to Fujii Hironori from comment #2)
> This seems that oldest testing job. r274685 

No. This is older.

r274380 (235247@main)
https://build.webkit.org/#/builders/63/builds/365

[Fujii Hironori]

WinCairo WK1 is also failing the assertion on my PC. However, WinCairo WK1 testing Buildbot has reported no such failurs so far.

python.exe ./Tools/Scripts/run-webkit-tests --wincairo --debug --no-retry-failures editing/selection/selection-in-iframe-removed-crash.html --iterations=4 -v -1

[Fujii Hironori]

newSelection.isOrphan() was true because m_base.m_anchorNode is the iframe element which is not isConnected.

[Fujii Hironori]

Created attachment 427847 [details]
Patch to change setTimeout

This test case is reproducing the assertion failure randomly.
However, changing the argument of setTimeout from 0ms to 50ms makes it constantly failing.
And, it also makes Mac port failing as the same assertion failure.

[Radar WebKit Bug Importer]

<rdar://problem/77632554>

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

Preliminary debugging: the selection is set in

https://webkit-search.igalia.com/webkit/rev/45eee1160003c6d3022e8d0b88fe15770b05ffac/Source/WebCore/editing/FrameSelection.cpp#1960

but the frame gets disconnected when the focus is set just the line before, due to the DOMFocusOut callback being executed

iframe1.addEventListener("DOMFocusOut", function () { document1.adoptNode(iframe1); }, false);

Taking bug, will investigate more later...

(BTW, FrameSelection::selectFrameElementInParentIfFullySelected should probably use more RefPtr per https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html )

---------------------------- rr session ------------------------------------------

Thread 1 received signal SIGSEGV, Segmentation fault.
(rr) reverse-f
(rr) 
(rr)
    at ../../Source/WebCore/editing/FrameSelection.cpp:361
361	            ASSERT_NOT_REACHED();
(rr) p newSelection.m_start.m_anchorNode->isConnected()
$1 = false
(rr) watch -l newSelection.m_start
(rr) rc
(rr) delete
(rr) bt
#0  0x00007f71aa598e16 in WebCore::Position::Position(WebCore::Position const&) (this=0x7ffd737a03e0)
    at WebCore/PrivateHeaders/WebCore/Position.h:54
#1  0x00007f71aa5990ff in WebCore::VisibleSelection::VisibleSelection(WebCore::VisibleSelection const&)
    (this=0x7ffd737a03a0) at WebCore/PrivateHeaders/WebCore/VisibleSelection.h:36
#2  0x00007f71ac64645f in WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance(WebCore::VisibleSelection const&, WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)
    (this=0x7f719e9bc6b8, newSelectionPossiblyWithoutDirection=..., options=..., align=WebCore::FrameSelection::AlignCursorOnScrollIfNeeded, granularity=WebCore::TextGranularity::CharacterGranularity)
    at ../../Source/WebCore/editing/FrameSelection.cpp:333
#3  0x00007f71ac646e65 in WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>, WebCore::AXTextStateChangeIntent, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)
    (this=0x7f719e9bc6b8, selection=..., options=..., intent=..., align=WebCore::FrameSelection::AlignCursorOnScrollIfNeeded, granularity=WebCore::TextGranularity::CharacterGranularity)
    at ../../Source/WebCore/editing/FrameSelection.cpp:426
#4  0x00007f71ac64ec04 in WebCore::FrameSelection::selectFrameElementInParentIfFullySelected()
    (this=0x7f719e9bcac0) at ../../Source/WebCore/editing/FrameSelection.cpp:1959
(rr) reverse-f
(rr) 
(rr) 
(rr) 
    at ../../Source/WebCore/editing/FrameSelection.cpp:1959
1959	        parent->selection().setSelection(newSelection);
(rr) p newSelection.m_start.m_anchorNode->isConnected()
$2 = false
(rr) p newSelection.m_start.m_anchorNode.get() == ownerElement
$3 = true
(rr) watch -l ((Node*)ownerElement)->m_nodeFlags
(rr) rc
(rr) bt
#0  0x00007f71ac3e73c4 in WTF::OptionSet<WebCore::Node::NodeFlag>::remove(WTF::OptionSet<WebCore::Node::NodeFlag>) (this=0x7f719ea18378, optionSet=...) at WTF/Headers/wtf/OptionSet.h:193
#1  0x00007f71ac3df9d5 in WebCore::Node::clearNodeFlag(WebCore::Node::NodeFlag) const
    (this=0x7f719ea18350, flag=WebCore::Node::NodeFlag::IsConnected) at ../../Source/WebCore/dom/Node.h:586
#2  0x00007f71ac4f25d9 in WebCore::Node::removedFromAncestor(WebCore::Node::RemovalType, WebCore::ContainerNode&) (this=0x7f719ea18350, removalType=..., oldParentOfRemovedTree=...)
    at ../../Source/WebCore/dom/Node.cpp:1312
#3  0x00007f71ac45fb88 in WebCore::Element::removedFromAncestor(WebCore::Node::RemovalType, WebCore::ContainerNode&) (this=0x7f719ea18350, removalType=..., oldParentOfRemovedTree=...)
    at ../../Source/WebCore/dom/Element.cpp:2316
#4  0x00007f71ac37c8a2 in WebCore::notifyNodeRemovedFromDocument(WebCore::ContainerNode&, WebCore::TreeScopeChange, WebCore::Node&) (oldParentOfRemovedTree=..., treeScopeChange=WebCore::TreeScopeChange::Changed, node=...)
    at ../../Source/WebCore/dom/ContainerNodeAlgorithms.cpp:126
#5  0x00007f71ac37cd6d in WebCore::notifyChildNodeRemoved(WebCore::ContainerNode&, WebCore::Node&)
    (oldParentOfRemovedTree=..., child=...) at ../../Source/WebCore/dom/ContainerNodeAlgorithms.cpp:178
#6  0x00007f71ac35dd16 in WebCore::ContainerNode::removeNodeWithScriptAssertion(WebCore::Node&, WebCore::ContainerNode::ChildChange::Source)
    (this=0x7f719ea23f40, childToRemove=..., source=WebCore::ContainerNode::ChildChange::Source::API)
    at ../../Source/WebCore/dom/ContainerNode.cpp:182
#7  0x00007f71ac35741a in WebCore::ContainerNode::removeChild(WebCore::Node&)
    (this=0x7f719ea23f40, oldChild=...) at ../../Source/WebCore/dom/ContainerNode.cpp:614
#8  0x00007f71ac4f05aa in WebCore::Node::remove() (this=0x7f719ea18350)
    at ../../Source/WebCore/dom/Node.cpp:639
#9  0x00007f71ac3a75bb in WebCore::Document::adoptNode(WebCore::Node&) (this=0x7f719ea19c10, source=...)
    at ../../Source/WebCore/dom/Document.cpp:1191
#10 0x00007f71aabae600 in WebCore::jsDocumentPrototypeFunction_adoptNodeBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::IDLOperation<WebCore::JSDocument>::ClassParameter) (lexicalGlobalObject=
    0x7f714ebdf668, callFrame=0x7ffd7379f030, castedThis=0x7f714c5c0f08)
    at WebCore/DerivedSources/JSDocument.cpp:5417
#11 0x00007f71aabd6b7f in WebCore::IDLOperation<WebCore::JSDocument>::call<WebCore::jsDocumentPrototypeFunction_adoptNodeBody>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*)
    (lexicalGlobalObject=..., callFrame=..., operationName=0x7f71b008d91b "adoptNode")
    at ../../Source/WebCore/bindings/js/JSDOMOperation.h:55
#12 0x00007f71aabae6b2 in WebCore::jsDocumentPrototypeFunction_adoptNode(JSC::JSGlobalObject*, JSC::CallFrame*) (lexicalGlobalObject=0x7f714ebdf668, callFrame=0x7ffd7379f030) at WebCore/DerivedSources/JSDocument.cpp:5422
#13 0x00007f715e6262b8 in  ()
#14 0x00007ffd7379f0b0 in  ()
#15 0x00007f71a4158afe in llint_op_call ()
    at /app/webkit/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1097
#16 0x0000000000000000 in  ()

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

Created attachment 428826 [details]
experimental patch ; based on comment 8

This fixes the issue for me on GTK.

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

Created attachment 428856 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 428856 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        editing/selection/selection-in-iframe-removed-crash.html, with the timeout changed to 50
> +        instead of 0.

Why 50ms? That sounds rather arbitrary. Can we requestAnimationFrame instead for example?
Also, you're missing the expected result.

> LayoutTests/editing/selection/selection-in-iframe-removed-2-crash.html:3
> +Test passes if it does not crash.

You mean hit any assertions?

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

Created attachment 428861 [details]
Patch

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

Comment on attachment 428856 [details]
Patch

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

>> LayoutTests/ChangeLog:10
>> +        instead of 0.
> 
> Why 50ms? That sounds rather arbitrary. Can we requestAnimationFrame instead for example?
> Also, you're missing the expected result.

Oops... This was the value provided by Fujii. requestAnimationFrame does not seem to make the test crash. In general, it seems crash does not happen with small timeout values while it happens reliably with long values.

>> LayoutTests/editing/selection/selection-in-iframe-removed-2-crash.html:3
>> +Test passes if it does not crash.
> 
> You mean hit any assertions?

Done.

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

(In reply to Frédéric Wang (:fredw) from comment #8)
> (BTW, FrameSelection::selectFrameElementInParentIfFullySelected should
> probably use more RefPtr per
> https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html )

Oops, I forgot about this... Do you want me to do that in the same patch?

[Ryosuke Niwa]

(In reply to Frédéric Wang (:fredw) from comment #14)
> (In reply to Frédéric Wang (:fredw) from comment #8)
> > (BTW, FrameSelection::selectFrameElementInParentIfFullySelected should
> > probably use more RefPtr per
> > https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html )
> 
> Oops, I forgot about this... Do you want me to do that in the same patch?

let's do that in a separate patch.

[Ryosuke Niwa]

Comment on attachment 428861 [details]
Patch

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

> LayoutTests/editing/selection/selection-in-iframe-removed-assert.html:18
> +    setTimeout("finish();", 50);

I think waiting for requestAnimationFrame and then setTimeout(~, 0) should do the trick.

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

Created attachment 428912 [details]
Patch for landing

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

Comment on attachment 428861 [details]
Patch

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

>> LayoutTests/editing/selection/selection-in-iframe-removed-assert.html:18
>> +    setTimeout("finish();", 50);
> 
> I think waiting for requestAnimationFrame and then setTimeout(~, 0) should do the trick.

Indeed, it does. Thanks for the hint!

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

(In reply to Ryosuke Niwa from comment #15)
> (In reply to Frédéric Wang (:fredw) from comment #14)
> > (In reply to Frédéric Wang (:fredw) from comment #8)
> > > (BTW, FrameSelection::selectFrameElementInParentIfFullySelected should
> > > probably use more RefPtr per
> > > https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html )
> > 
> > Oops, I forgot about this... Do you want me to do that in the same patch?
> 
> let's do that in a separate patch.

Done in https://bugs.webkit.org/show_bug.cgi?id=225908

[EWS]

Committed r277644 (237850@main): <https://commits.webkit.org/237850@main>

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