Bug 222718 – Nullptr crash in EventPath::eventTargetRespectingTargetRules

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 422194 [details]
Test

e.g.
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000129605cee WTF::OptionSet<WebCore::Node::NodeFlag>::containsAny(WTF::OptionSet<WebCore::Node::NodeFlag>) const + 190 (OptionSet.h:178)
1   com.apple.WebCore             	0x0000000129605bca WTF::OptionSet<WebCore::Node::NodeFlag>::contains(WebCore::Node::NodeFlag) const + 218 (OptionSet.h:173)
2   com.apple.WebCore             	0x0000000129605aed WebCore::Node::hasNodeFlag(WebCore::Node::NodeFlag) const + 13 (Node.h:575)
3   com.apple.WebCore             	0x00000001296061fe WebCore::Node::isElementNode() const + 14 (Node.h:189)
4   com.apple.WebCore             	0x000000012bccd38e WebCore::Node::pseudoId() const + 14 (Node.h:199)
5   com.apple.WebCore             	0x000000012bccd369 WebCore::Node::isPseudoElement() const + 9 (Node.h:196)
6   com.apple.WebCore             	0x000000012c8d8989 WTF::TypeCastTraits<WebCore::PseudoElement const, WebCore::Node const, false>::isType(WebCore::Node const&) + 9 (PseudoElement.h:62)
7   com.apple.WebCore             	0x000000012c8d8979 WTF::TypeCastTraits<WebCore::PseudoElement const, WebCore::Node const, false>::isOfType(WebCore::Node const&) + 9 (PseudoElement.h:61)
8   com.apple.WebCore             	0x000000012c8d8819 bool WTF::is<WebCore::PseudoElement, WebCore::Node>(WebCore::Node&) + 9 (TypeCasts.h:58)
9   com.apple.WebCore             	0x000000012ce21592 WebCore::EventPath::eventTargetRespectingTargetRules(WebCore::Node&) + 178 (EventPath.h:63)
10  com.apple.WebCore             	0x000000012ce4a18f WebCore::EventPath::buildPath(WebCore::Node&, WebCore::Event&) + 1327 (EventPath.cpp:151)
11  com.apple.WebCore             	0x000000012ce49bd1 WebCore::EventPath::EventPath(WebCore::Node&, WebCore::Event&) + 49 (EventPath.cpp:84)
12  com.apple.WebCore             	0x000000012ce4a549 WebCore::EventPath::EventPath(WebCore::Node&, WebCore::Event&) + 9 (EventPath.cpp:83)
13  com.apple.WebCore             	0x000000012ce219a6 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) + 614 (EventDispatcher.cpp:152)
14  com.apple.WebCore             	0x000000012ced7039 WebCore::Node::dispatchEvent(WebCore::Event&) + 9 (Node.cpp:2374)
15  com.apple.WebCore             	0x000000012d0959cd WebCore::dispatchBeforeInputEvent(WebCore::Element&, WTF::AtomString const&, WTF::String const&, WTF::RefPtr<WebCore::DataTransfer, WTF::RawPtrTraits<WebCore::DataTransfer>, WTF::DefaultRefDerefTraits<WebCore::DataTransfer> >&&, WTF::Vector<WTF::RefPtr<WebCore::StaticRange, WTF::RawPtrTraits<WebCore::StaticRange>, WTF::DefaultRefDerefTraits<WebCore::StaticRange> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::Event::IsCancelable) + 573 (Editor.cpp:141)
16  com.apple.WebCore             	0x000000012d0985c6 WebCore::dispatchBeforeInputEvents(WTF::RefPtr<WebCore::Element, WTF::RawPtrTraits<WebCore::Element>, WTF::DefaultRefDerefTraits<WebCore::Element> >, WTF::RefPtr<WebCore::Element, WTF::RawPtrTraits<WebCore::Element>, WTF::DefaultRefDerefTraits<WebCore::Element> >, WTF::AtomString const&, WTF::String const&, WTF::RefPtr<WebCore::DataTransfer, WTF::RawPtrTraits<WebCore::DataTransfer>, WTF::DefaultRefDerefTraits<WebCore::DataTransfer> >&&, WTF::Vector<WTF::RefPtr<WebCore::StaticRange, WTF::RawPtrTraits<WebCore::StaticRange>, WTF::DefaultRefDerefTraits<WebCore::StaticRange> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::Event::IsCancelable) + 102 (Editor.cpp:1078)
17  com.apple.WebCore             	0x000000012d099beb WebCore::Editor::willUnapplyEditing(WebCore::EditCommandComposition const&) const + 587 (Editor.cpp:1159)
18  com.apple.WebCore             	0x000000012d03a16e WebCore::EditCommandComposition::unapply() + 446 (CompositeEditCommand.cpp:235)
19  com.apple.WebKitLegacy        	0x00000001113e364d -[WebEditorUndoTarget undoEditing:] + 93 (WebEditorClient.mm:189)

<rdar://problem/74463714>

[Ryosuke Niwa]

Reproduced with DumpRenderTree at r273811.

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

Created attachment 423182 [details]
Test for ASSERTION FAILED: m_parent->hasEditableStyle() || !m_parent->renderer()

At r274413, the crash does not seem to reproduce on Linux release/debug. Here is a reduced testcase that demonstrates a debug ASSERT (on linux and macos). That seems to be the only issue on Linux (will investigate more the macos case...).

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

Created attachment 423215 [details]
Test for ASSERTION FAILED: foundAncestor

This is a reduced testcase for the assert obtained on macos debug with testrunner (also reproducible on Linux).

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

Created attachment 423312 [details]
Patch

I forgot to say yesterday that I was able to reproduce the test with DumpRenderTree on macos. I've tried to simplify the testcase, but it seems non-deterministic and hard to reduce.

I have not tried to analyze more but the node is set to a nullptr here

https://webkit-search.igalia.com/webkit/rev/9e4f1037bbe4b7e829e4b4af99aeaadddbb8d817/Source/WebCore/dom/EventPath.cpp#147

and is then dereferenced. Attached is a very basic patch to prevent the crash.

[Ryosuke Niwa]

Comment on attachment 423312 [details]
Patch

That shouldn't be happening. We need to figure out why.

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

(In reply to Frédéric Wang (:fredw) from comment #2)
> Created attachment 423182 [details]
> Test for ASSERTION FAILED: m_parent->hasEditableStyle() ||
> !m_parent->renderer()
> 
> At r274413, the crash does not seem to reproduce on Linux release/debug.
> Here is a reduced testcase that demonstrates a debug ASSERT (on linux and
> macos). That seems to be the only issue on Linux (will investigate more the
> macos case...).

This one was fixed in bug 223364.

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

(In reply to Frédéric Wang (:fredw) from comment #3)
> Created attachment 423215 [details]
> Test for ASSERTION FAILED: foundAncestor
> 
> This is a reduced testcase for the assert obtained on macos debug with
> testrunner (also reproducible on Linux).

I reopened bug 135648 for this one (I don't have access to bug 116980 but maybe that's related too).

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

Created attachment 424696 [details]
Testcase reduced a bit

(In reply to Frédéric Wang (:fredw) from comment #4)
> I forgot to say yesterday that I was able to reproduce the test with
> DumpRenderTree on macos. I've tried to simplify the testcase, but it seems
> non-deterministic and hard to reduce.

This one reduces thing a bit, but I suspect garbage collection is involved here, and makes things non-deterministic. Perhaps one can reduce that more.

[Ryosuke Niwa]

Can we call GCController.collect() in a bunch of places and see if that makes the test case more reliably reproduce?

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

Created attachment 424740 [details]
Reduced testcase (html)

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

Created attachment 424741 [details]
Reduced testcase (js)

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

(In reply to Ryosuke Niwa from comment #9)
> Can we call GCController.collect() in a bunch of places and see if that
> makes the test case more reliably reproduce?

I did that this morning. I attached an even more reduced testcase and I'm able to use it to reproduce the issue with DumpRenderTree. The only drawback is that the JS code is moved to a separate file, there is probably some async stuff involved. The JS part is still a bit complex.

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

(In reply to Ryosuke Niwa from comment #5)
> Comment on attachment 423312 [details]
> Patch
> 
> That shouldn't be happening. We need to figure out why.

So debugging a bit, this null shadowRoot.host() happens when the "beforeinput" event is triggered by the "undo" command (see function f1 in the testcase):

* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
    frame #0: 0x0000000124a97d88 WebCore`WebCore::EventPath::buildPath(this=0x000000016fdf86c8, originalTarget=0x00000001447778e0, event=0x0000000144777cb0) at EventPath.cpp:148:13
    frame #1: 0x0000000124a978a0 WebCore`WebCore::EventPath::EventPath(this=0x000000016fdf86c8, originalTarget=0x00000001447778e0, event=0x0000000144777cb0) at EventPath.cpp:84:5
    frame #2: 0x0000000124a980e8 WebCore`WebCore::EventPath::EventPath(this=0x000000016fdf86c8, originalTarget=0x00000001447778e0, event=0x0000000144777cb0) at EventPath.cpp:83:1
    frame #3: 0x0000000124a6be0c WebCore`WebCore::EventDispatcher::dispatchEvent(node=0x00000001447778e0, event=0x0000000144777cb0) at EventDispatcher.cpp:153:15
    frame #4: 0x0000000124b26c54 WebCore`WebCore::Node::dispatchEvent(this=0x00000001447778e0, event=0x0000000144777cb0) at Node.cpp:2374:5
    frame #5: 0x0000000124cd4f20 WebCore`WebCore::dispatchBeforeInputEvent(element=0x00000001447778e0, inputType=0x000000016fdf8ba8, data=0x000000016fdf8ba0, dataTransfer=0x000000016fdf8b98, targetRanges=0x000000016fdf8b88, cancelable=Yes) at Editor.cpp:146:13
    frame #6: 0x0000000124cd6358 WebCore`WebCore::dispatchBeforeInputEvents(startRoot=RefPtr<WebCore::Element, WTF::RawPtrTraits<WebCore::Element>, WTF::DefaultRefDerefTraits<WebCore::Element> > @ 0x000000016fdf8bb8, endRoot=RefPtr<WebCore::Element, WTF::RawPtrTraits<WebCore::Element>, WTF::DefaultRefDerefTraits<WebCore::Element> > @ 0x000000016fdf8bb0, inputTypeName=0x000000016fdf8ba8, data=0x000000016fdf8ba0, dataTransfer=0x000000016fdf8b98, targetRanges=0x000000016fdf8b88, cancelable=Yes) at Editor.cpp:1083:40
    frame #7: 0x0000000124cd6f0c WebCore`WebCore::Editor::willUnapplyEditing(this=0x0000000145de0240, composition=0x00000001683c8000) const at Editor.cpp:1164:12
    frame #8: 0x0000000124c8cafc WebCore`WebCore::EditCommandComposition::unapply(this=0x00000001683c8000) at CompositeEditCommand.cpp:235:31
    frame #9: 0x000000013bba0ba4 WebKitLegacy`-[WebEditorUndoTarget undoEditing:](self=0x000000013cd58750, _cmd="undoEditing:", arg=0x000000013cfcec40) at WebEditorClient.mm:189:16
    frame #10: 0x000000018f3a51d8 Foundation`-[_NSUndoStack popAndInvoke] + 188
    frame #11: 0x000000018f3a4f08 Foundation`-[NSUndoManager undoNestedGroup] + 292
    frame #12: 0x000000013bba30b8 WebKitLegacy`WebEditorClient::undo(this=0x0000000145d94000) at WebEditorClient.mm:670:9
    frame #13: 0x0000000124cdaac4 WebCore`WebCore::Editor::undo(this=0x0000000145de0240) at Editor.cpp:1817:19
  * frame #14: 0x0000000124d06084 WebCore`WebCore::executeUndo(frame=0x0000000145d14240, (null)=0x0000000000000000, (null)=CommandFromDOM, (null)=0x000000016fdf8f80) at EditorCommand.cpp:1151:20
...


==================

The shadow host was set to null when the input element was destroyed earlier in one GC call of the onwebkitanimationiteration handler (probably the one just after inputElement.outerHTML is set):

* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 3.1
  * frame #0: 0x0000000124a5c580 WebCore`WebCore::ShadowRoot::setHost(this=0x0000000144777300, host=0x000000016fdfa5d0) at ShadowRoot.h:76:54
    frame #1: 0x0000000124a4f76c WebCore`WebCore::Element::removeShadowRoot(this=0x00000001447771f0) at Element.cpp:2386:14
    frame #2: 0x0000000124a4f2bc WebCore`WebCore::Element::~Element(this=0x00000001447771f0) at Element.cpp:217:5
    frame #3: 0x0000000124bf2d2c WebCore`WebCore::StyledElement::~StyledElement(this=0x00000001447771f0) at StyledElement.cpp:72:1
    frame #4: 0x00000001242c95b0 WebCore`WebCore::HTMLElement::~HTMLElement(this=0x00000001447771f0) at HTMLElement.h:46:7
    frame #5: 0x0000000125043a50 WebCore`WebCore::LabelableElement::~LabelableElement(this=0x00000001447771f0) at LabelableElement.cpp:42:37
    frame #6: 0x0000000124ec2278 WebCore`WebCore::HTMLFormControlElement::~HTMLFormControlElement(this=0x00000001447771f0) at HTMLFormControlElement.cpp:82:1
    frame #7: 0x0000000124ec7450 WebCore`WebCore::HTMLFormControlElementWithState::~HTMLFormControlElementWithState(this=0x00000001447771f0) at HTMLFormControlElementWithState.cpp:42:67
    frame #8: 0x0000000124ff0848 WebCore`WebCore::HTMLTextFormControlElement::~HTMLTextFormControlElement(this=0x00000001447771f0) at HTMLTextFormControlElement.cpp:75:57
    frame #9: 0x0000000124f0453c WebCore`WebCore::HTMLInputElement::~HTMLInputElement(this=0x00000001447771f0) at HTMLInputElement.cpp:183:1
    frame #10: 0x0000000124f04724 WebCore`WebCore::HTMLInputElement::~HTMLInputElement(this=0x00000001447771f0) at HTMLInputElement.cpp:164:1
    frame #11: 0x0000000124f04774 WebCore`WebCore::HTMLInputElement::~HTMLInputElement(this=0x00000001447771f0) at HTMLInputElement.cpp:164:1
    frame #12: 0x0000000124b27a5c WebCore`WebCore::Node::removedLastRef(this=0x00000001447771f0) at Node.cpp:2551:5
    frame #13: 0x00000001221021d4 WebCore`WebCore::Node::deref(this=0x00000001447771f0) const at Node.h:799:34
    frame #14: 0x0000000124b1f3d8 WebCore`WebCore::Node::derefEventTarget(this=0x00000001447771f0) at Node.cpp:838:5
    frame #15: 0x0000000122069bf4 WebCore`WebCore::EventTarget::deref(this=0x00000001447771f0) at EventTarget.h:63:20
    frame #16: 0x0000000122069bc0 WebCore`WTF::Ref<WebCore::EventTarget, WTF::RawPtrTraits<WebCore::EventTarget> >::~Ref(this=0x0000000145d29e80) at Ref.h:61:39
    frame #17: 0x00000001220dcb30 WebCore`WTF::Ref<WebCore::EventTarget, WTF::RawPtrTraits<WebCore::EventTarget> >::~Ref(this=0x0000000145d29e80) at Ref.h:55:5
    frame #18: 0x00000001223dad4c WebCore`WebCore::JSDOMWrapper<WebCore::EventTarget>::~JSDOMWrapper(this=0x0000000145d29e68) at JSDOMWrapper.h:72:46
    frame #19: 0x00000001223dad14 WebCore`WebCore::JSEventTarget::~JSEventTarget(this=0x0000000145d29e68) at JSEventTarget.h:29:7
    frame #20: 0x000000012234c200 WebCore`WebCore::JSEventTarget::~JSEventTarget(this=0x0000000145d29e68) at JSEventTarget.h:29:7
    frame #21: 0x00000001222e1710 WebCore`WebCore::JSEventTarget::destroy(cell=0x0000000145d29e68) at JSEventTarget.cpp:194:32
    frame #22: 0x0000000102fa9b24 JavaScriptCore`JSC::JSDestructibleObjectDestroyFunc::operator(this=0x000000016fdfa927, (null)=0x0000000146109000, cell=0x0000000145d29e68)(JSC::VM&, JSC::JSCell*) const at JSDestructibleObjectHeapCellType.cpp:38:9
    frame #23: 0x0000000102fa9aec JavaScriptCore`JSC::JSDestructibleObjectHeapCellType::destroy(this=0x0000000145dcdc20, vm=0x0000000146109000, cell=0x0000000145d29e68) at JSDestructibleObjectHeapCellType.cpp:58:5
    frame #24: 0x00000001028d9a50 JavaScriptCore`JSC::Subspace::destroy(this=0x0000000145d2d318, vm=0x0000000146109000, cell=0x0000000145d29e68) at Subspace.cpp:65:21
    frame #25: 0x00000001028ccd08 JavaScriptCore`JSC::PreciseAllocation::sweep(this=0x0000000145d29e00) at PreciseAllocation.cpp:234:25
    frame #26: 0x00000001028b4208 JavaScriptCore`JSC::MarkedSpace::sweepPreciseAllocations(this=0x0000000146109130) at MarkedSpace.cpp:234:21
    frame #27: 0x000000010282eb84 JavaScriptCore`JSC::Heap::sweepInFinalize(this=0x0000000146109048) at Heap.cpp:2150:19
    frame #28: 0x000000010282e824 JavaScriptCore`JSC::Heap::finalize(this=0x0000000146109048) at Heap.cpp:2095:9
    frame #29: 0x000000010282df64 JavaScriptCore`JSC::Heap::handleNeedFinalize(this=0x0000000146109048, oldState=21) at Heap.cpp:2016:9
    frame #30: 0x000000010282d018 JavaScriptCore`JSC::Heap::handleNeedFinalize(this=0x0000000146109048) at Heap.cpp:2032:12
    frame #31: 0x0000000102829bb8 JavaScriptCore`JSC::Heap::finishChangingPhase(this=0x0000000146109048, conn=Mutator) at Heap.cpp:1603:17
    frame #32: 0x000000010282aee8 JavaScriptCore`JSC::Heap::changePhase(this=0x0000000146109048, conn=Mutator, nextPhase=NotRunning) at Heap.cpp:1577:12
    frame #33: 0x000000010282ae80 JavaScriptCore`JSC::Heap::runEndPhase(this=0x0000000146109048, conn=Mutator) at Heap.cpp:1567:12
    frame #34: 0x000000010282947c JavaScriptCore`JSC::Heap::runCurrentPhase(this=0x0000000146109048, conn=Mutator, currentThreadState=0x000000016fdfae50) at Heap.cpp:1220:18
    frame #35: 0x000000010285dba0 JavaScriptCore`JSC::Heap::collectInMutatorThread(this=0x000000016fdfaeb0, state=0x000000016fdfae50)::$_0::operator()(JSC::CurrentThreadState&) const at Heap.cpp:1842:52
    frame #36: 0x000000010285db1c JavaScriptCore`WTF::ScopedLambdaFunctor<void (JSC::CurrentThreadState&), JSC::Heap::collectInMutatorThread()::$_0>::implFunction(argument=0x000000016fdfaea0, arguments=0x000000016fdfae50) at ScopedLambda.h:106:16
    frame #37: 0x000000010289b490 JavaScriptCore`void WTF::ScopedLambda<void (JSC::CurrentThreadState&)>::operator(this=0x000000016fdfaea0, arguments=0x000000016fdfae50)<JSC::CurrentThreadState&>(JSC::CurrentThreadState&) const at ScopedLambda.h:58:16
    frame #38: 0x000000010289b400 JavaScriptCore`JSC::callWithCurrentThreadState(lambda=0x000000016fdfaea0)> const&) at MachineStackMarker.cpp:221:5
    frame #39: 0x000000010282e054 JavaScriptCore`JSC::Heap::collectInMutatorThread(this=0x0000000146109048) at Heap.cpp:1854:13
    frame #40: 0x000000010282ddf0 JavaScriptCore`JSC::Heap::stopIfNecessarySlow(this=0x0000000146109048, oldState=37) at Heap.cpp:1823:9
    frame #41: 0x000000010282ee20 JavaScriptCore`void JSC::Heap::waitForCollector<JSC::Heap::waitForCollection(unsigned long long)::$_27>(this=0x0000000146109048, func=0x000000016fdfaf90)::$_27 const&) at Heap.cpp:1880:13
    frame #42: 0x0000000102829138 JavaScriptCore`JSC::Heap::waitForCollection(this=0x0000000146109048, ticket=31) at Heap.cpp:2142:5
    frame #43: 0x0000000102828afc JavaScriptCore`JSC::Heap::collectSync(this=0x0000000146109048, request=GCRequest @ 0x000000016fdfb060) at Heap.cpp:1127:5
    frame #44: 0x0000000102828bb8 JavaScriptCore`JSC::Heap::collectNow(this=0x0000000146109048, synchronousness=Sync, request=GCRequest @ 0x000000016fdfb0c0) at Heap.cpp:1075:9
    frame #45: 0x0000000124293864 WebCore`WebCore::GCController::garbageCollectNow(this=0x00000001290e6c90) at GCController.cpp:96:25
    frame #46: 0x000000013bb7dc7c WebKitLegacy`+[WebCoreStatistics garbageCollectJavaScriptObjects](self=WebCoreStatistics, _cmd="garbageCollectJavaScriptObjects") at WebCoreStatistics.mm:108:31
    frame #47: 0x0000000100070b54 DumpRenderTree`GCController::collect(this=0x0000000145df4178) const at GCControllerMac.mm:38:5
    frame #48: 0x0000000100070a28 DumpRenderTree`collectCallback(context=0x0000000145ef9268, function=0x0000000145d9d718, thisObject=0x0000000168363818, argumentCount=0, arguments=0x000000016fdfb278, exception=0x000000016fdfb248) at GCController.cpp:39:16
    frame #49: 0x0000000101cf2160 JavaScriptCore`long long JSC::APICallbackFunction::callImpl<JSC::JSCallbackFunction>(globalObject=0x0000000145ef9268, callFrame=0x000000016fdfb3e0) at APICallbackFunction.h:61:18
    frame #50: 0x0000000101ce5f78 JavaScriptCore`JSC::callJSCallbackFunction(globalObject=0x0000000145ef9268, callFrame=0x000000016fdfb3e0) at JSCallbackFunction.cpp:42:12
    frame #51: 0x0000000280004030
    frame #52: 0x0000000286003534
    frame #53: 0x0000000101bd4a5c JavaScriptCore`llint_entry at LowLevelInterpreter.asm:1093
    frame #54: 0x0000000101bb0d68 JavaScriptCore`vmEntryToJavaScript at LowLevelInterpreter64.asm:316
    frame #55: 0x0000000102a63998 JavaScriptCore`JSC::JITCode::execute(this=0x0000000168342a58, vm=0x0000000146109000, protoCallFrame=0x000000016fdfb798) at JITCodeInlines.h:42:38
    frame #56: 0x0000000102a64050 JavaScriptCore`JSC::Interpreter::executeCall(this=0x0000000145df44f0, lexicalGlobalObject=0x0000000145ef9268, function=0x0000000168337600, callData=0x000000016fdfbe60, thisValue=JSValue @ 0x000000016fdfb8e0, args=0x000000016fdfbd20) at Interpreter.cpp:907:27
    frame #57: 0x0000000102de1ca4 JavaScriptCore`JSC::call(globalObject=0x0000000145ef9268, functionObject=JSValue @ 0x000000016fdfb960, callData=0x000000016fdfbe60, thisValue=JSValue @ 0x000000016fdfb958, args=0x000000016fdfbd20) at CallData.cpp:57:28
    frame #58: 0x0000000102de1d84 JavaScriptCore`JSC::call(globalObject=0x0000000145ef9268, functionObject=JSValue @ 0x000000016fdfba60, callData=0x000000016fdfbe60, thisValue=JSValue @ 0x000000016fdfba58, args=0x000000016fdfbd20, returnedException=0x000000016fdfbd48) at CallData.cpp:64:22
    frame #59: 0x0000000102de2078 JavaScriptCore`JSC::profiledCall(globalObject=0x0000000145ef9268, reason=Other, functionObject=JSValue @ 0x000000016fdfbb10, callData=0x000000016fdfbe60, thisValue=JSValue @ 0x000000016fdfbb08, args=0x000000016fdfbd20, returnedException=0x000000016fdfbd48) at CallData.cpp:85:12
    frame #60: 0x000000012429a154 WebCore`WebCore::JSExecState::profiledCall(lexicalGlobalObject=0x0000000145ef9268, reason=Other, functionObject=JSValue @ 0x000000016fdfbb90, callData=0x000000016fdfbe60, thisValue=JSValue @ 0x000000016fdfbb88, args=0x000000016fdfbd20, returnedException=0x000000016fdfbd48) at JSExecState.h:73:16
    frame #61: 0x00000001242b66f8 WebCore`WebCore::JSEventListener::handleEvent(this=0x0000000168344510, scriptExecutionContext=0x0000000144775bf0, event=0x0000000144777010) at JSEventListener.cpp:186:22
    frame #62: 0x0000000124a9b908 WebCore`WebCore::EventTarget::innerInvokeEventListeners(this=0x0000000144775100, event=0x0000000144777010, listeners=WebCore::EventListenerVector @ 0x000000016fdfc120, phase=Bubbling) at EventTarget.cpp:344:40
    frame #63: 0x0000000124a9b494 WebCore`WebCore::EventTarget::fireEventListeners(this=0x0000000144775100, event=0x0000000144777010, phase=Bubbling) at EventTarget.cpp:289:13
    frame #64: 0x0000000124a6b828 WebCore`WebCore::EventContext::handleLocalEvents(this=0x000000016fdfc468, event=0x0000000144777010, phase=Bubbling) const at EventContext.cpp:75:26
    frame #65: 0x0000000124a6c7e8 WebCore`WebCore::dispatchEventInDOM(event=0x0000000144777010, path=0x000000016fdfc408) at EventDispatcher.cpp:107:22
    frame #66: 0x0000000124a6c04c WebCore`WebCore::EventDispatcher::dispatchEvent(node=0x00000001447768c0, event=0x0000000144777010) at EventDispatcher.cpp:188:9
    frame #67: 0x0000000124b26c54 WebCore`WebCore::Node::dispatchEvent(this=0x00000001447768c0, event=0x0000000144777010) at Node.cpp:2374:5
    frame #68: 0x000000012424237c WebCore`WebCore::DocumentTimelinesController::updateAnimationsAndSendEvents(this=0x0000000145db7890, timestamp=(m_value = 12.009)) at DocumentTimelinesController.cpp:156:26
    frame #69: 0x000000012498d8d4 WebCore`WebCore::Document::updateAnimationsAndSendEvents(this=0x0000000144775bf0) at Document.cpp:8297:30
...

[Ryosuke Niwa]

(In reply to Frédéric Wang (:fredw) from comment #13)
> (In reply to Ryosuke Niwa from comment #5)
> > Comment on attachment 423312 [details]
> > Patch
> > 
> > That shouldn't be happening. We need to figure out why.
> 
> So debugging a bit, this null shadowRoot.host() happens when the
> "beforeinput" event is triggered by the "undo" command (see function f1 in
> the testcase):

Yeah, I figured that much from the reduced test case you had posted and from the backtrace.

> The shadow host was set to null when the input element was destroyed earlier
> in one GC call of the onwebkitanimationiteration handler (probably the one
> just after inputElement.outerHTML is set):

Right. That makes sense. I think this is a problem for the Shadow DOM in general. It doesn't necessarily keep the host alive so it can end up losing the host if only C++ reference was to a node inside a shadow root.

Editing is specical in this case because it can remove & hold onto a node without ever creating JS wrappers and it can add it back later. There isn't really any other Web API that does this.

Based on that, I think adding some code specifically to deal with this issue in editing might be okay. Specifically, for undo, we should check whether m_endingRootEditableElement is still connected in EditCommandComposition::unapply. If not, then just exit early. For redo, we should check that m_startingRootEditableElement is still connected in EditCommandComposition::reapply.

That should do it.

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

(In reply to Ryosuke Niwa from comment #14)
> Right. That makes sense. I think this is a problem for the Shadow DOM in
> general. It doesn't necessarily keep the host alive so it can end up losing
> the host if only C++ reference was to a node inside a shadow root.
> 
> Editing is specical in this case because it can remove & hold onto a node
> without ever creating JS wrappers and it can add it back later. There isn't
> really any other Web API that does this.
> 

OK, I see.

> Based on that, I think adding some code specifically to deal with this issue
> in editing might be okay. Specifically, for undo, we should check whether
> m_endingRootEditableElement is still connected in
> EditCommandComposition::unapply. If not, then just exit early. For redo, we
> should check that m_startingRootEditableElement is still connected in
> EditCommandComposition::reapply.
> 
> That should do it.

I'm not sure I understand this statement though. As I see both unapply and reapply end up calling the two functions dispatchBeforeInputEvents and dispatchInputEvents, which involve the both start and end root editable elements. So it looks we should always check whether both of them are connected ?

In any case, for this specific repro case, the debugger says it's for unapply and m_startingRootEditableElement not connected (and I just checked exiting early in that case avoids the crash).

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

Created attachment 424891 [details]
Patch

Here is a new patch based on previous discussion. The testcase causes a nullptr crash, but I understand this involves a DOM mutation earlier. Not sure whether we should consider this a security issue (and so not include a test).

[Ryosuke Niwa]

Comment on attachment 424891 [details]
Patch

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

> Source/WebCore/editing/CompositeEditCommand.cpp:218
> +    if (m_startingRootEditableElement && !m_startingRootEditableElement->isConnected())
> +        return false;
> +
> +    if (m_endingRootEditableElement && !m_endingRootEditableElement->isConnected())
> +        return false;

Hm... I don't think it would be correct to check both of these.
For unapply, we want to check m_endingRootEditableElement, and for reapply, we want to check m_startingRootEditableElement.
The reason these two variables are different is because they could end up being different.

> Source/WebCore/editing/CompositeEditCommand.cpp:246
> +    if (!areRootEditabledElementsConnected())
> +        return;

We should do check right after !frame check before even trying to cancel composition or update the layout.

> Source/WebCore/editing/CompositeEditCommand.cpp:279
> +    if (!areRootEditabledElementsConnected())
> +        return;

Ditto.

[Ryosuke Niwa]

I don't think there is any security implication here. Just a nullptr crash.

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

(In reply to Ryosuke Niwa from comment #17)
> Hm... I don't think it would be correct to check both of these.
> For unapply, we want to check m_endingRootEditableElement, and for reapply,
> we want to check m_startingRootEditableElement.
> The reason these two variables are different is because they could end up
> being different.
> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:246
> > +    if (!areRootEditabledElementsConnected())
> > +        return;
> 
> We should do check right after !frame check before even trying to cancel
> composition or update the layout.

I tried the patch below but that does not help. As I said in the specific repro, the crash happens for unapply and m_startingRootEditableElement not connected. 

diff --git a/Source/WebCore/editing/CompositeEditCommand.cpp b/Source/WebCore/editing/CompositeEditCommand.cpp
index d148ecf34e..6b3c7b561c 100644
--- a/Source/WebCore/editing/CompositeEditCommand.cpp
+++ b/Source/WebCore/editing/CompositeEditCommand.cpp
@@ -215,6 +215,8 @@ void EditCommandComposition::unapply()
     RefPtr<Frame> frame = m_document->frame();
     if (!frame)
         return;
+    if (m_endingRootEditableElement && !m_endingRootEditableElement->isConnected())
+        return;
 
     m_replacedText.captureTextForUnapply();
 
@@ -253,6 +255,8 @@ void EditCommandComposition::reapply()
     RefPtr<Frame> frame = m_document->frame();
     if (!frame)
         return;
+    if (m_startingRootEditableElement && !m_startingRootEditableElement->isConnected())
+        return;
 
     m_replacedText.captureTextForReapply();

[Ryosuke Niwa]

(In reply to Frédéric Wang (:fredw) from comment #19)
> (In reply to Ryosuke Niwa from comment #17)
> > Hm... I don't think it would be correct to check both of these.
> > For unapply, we want to check m_endingRootEditableElement, and for reapply,
> > we want to check m_startingRootEditableElement.
> > The reason these two variables are different is because they could end up
> > being different.
> > 
> > > Source/WebCore/editing/CompositeEditCommand.cpp:246
> > > +    if (!areRootEditabledElementsConnected())
> > > +        return;
> > 
> > We should do check right after !frame check before even trying to cancel
> > composition or update the layout.
> 
> I tried the patch below but that does not help. As I said in the specific
> repro, the crash happens for unapply and m_startingRootEditableElement not
> connected.

You mean m_endingRootEditableElement is connected but m_startingRootEditableElement is not? That's interesting. Maybe m_endingRootEditableElement is nullptr instead? I think the condition should be either m_endingRootEditableElement is nullptr or it's no longer connected.

[Ryosuke Niwa]

Comment on attachment 424891 [details]
Patch

r=me if we really need to check that m_startingRootEditableElement is connected but please move up the check (right below !frame check) as I had suggested.

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

(In reply to Ryosuke Niwa from comment #21)
> Comment on attachment 424891 [details]
> Patch
> 
> r=me if we really need to check that m_startingRootEditableElement is
> connected but please move up the check (right below !frame check) as I had
> suggested.

will do and add the test too

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

Created attachment 425142 [details]
Patch

Here is a patch but I writing the regression test is a bit difficult :

- If I run with --repeat=XXX, only the first run has the "B" in the -expected.txt
- If I add waitUntilDone/notifyDone, expectation is no longer flaky ("B" is always absent from the output) but the test does not allow to reproduce the crash.

Is it possible to run a crash test without an -expected.txt ?

[Ryosuke Niwa]

Comment on attachment 425142 [details]
Patch

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

> LayoutTests/editing/undo/undo-with-disconnected-editable-element-crash.js:2
> +function runTests() {
> +    if (window.testRunner) {

Maybe this whole thing has to happen after requestAnimationFrame?
Also, try increasing the animation length from 1ms to something like 1s?

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

Created attachment 425146 [details]
Patch for landing

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

(In reply to Ryosuke Niwa from comment #24)
> Comment on attachment 425142 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425142&action=review
> 
> > LayoutTests/editing/undo/undo-with-disconnected-editable-element-crash.js:2
> > +function runTests() {
> > +    if (window.testRunner) {
> 
> Maybe this whole thing has to happen after requestAnimationFrame?
> Also, try increasing the animation length from 1ms to something like 1s?

OK, I tweaked a bit and I get both the crash (without c++ change) and consistent -expected.txt (with the c++ change). 1s makes the test a bit slow, but I guess that's ok.

[Ryosuke Niwa]

Comment on attachment 425146 [details]
Patch for landing

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

> LayoutTests/ChangeLog:1248
> [phone]  Frédéric Wang  <[e-mail]>

Please move the change log entry to the top.

> LayoutTests/editing/undo/undo-with-disconnected-editable-element-crash.js:20
> +    if (window.GCController) GCController.collect();

Can we reformat this function so that it's more readable?
Also, is this the simplest form we can reduce to? It seems like we ought to be able to simplify it more?

> Source/WebCore/ChangeLog:2601
> [phone]  Frédéric Wang  <[e-mail]>

Ditto.

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

Created attachment 425253 [details]
Patch for landing

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

Comment on attachment 425146 [details]
Patch for landing

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

>> LayoutTests/editing/undo/undo-with-disconnected-editable-element-crash.js:20
>> +    if (window.GCController) GCController.collect();
> 
> Can we reformat this function so that it's more readable?
> Also, is this the simplest form we can reduce to? It seems like we ought to be able to simplify it more?

I tried simplifying it more, but removing a single instruction prevents the crash to happen. Even keeping some try/catch instructions is essential to make it happen (others I added them so they don't show up in the text output if inputElement is destroyed). I also tried to trigger the DOMSubtreeModified callback by other means, but was not able to reproduce the crash either.

[EWS]

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

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