Bug 224408 – Crash in WebCore::SlotAssignment::assignedNodesForSlot

• Component: DOM
• Status: RESOLVED FIXED

[Michael Catanzaro]

Created attachment 425692 [details]
bt full

WebKitGTK 2.32.0 is unable to load redhat.com, which somehow I didn't notice until now. I will attach a full backtrace, but looks like something bad happening in shadow DOM code:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007fa1b8da7855 in __GI_abort () at abort.c:79
#2  0x00007fa1ba9711bf in CRASH_WITH_INFO(...) ()
    at DerivedSources/ForwardingHeaders/wtf/Assertions.h:713
#3  WebCore::SlotAssignment::assignedNodesForSlot(WebCore::HTMLSlotElement const&, WebCore::ShadowRoot&) (this=0x7f87ceef4360, slotElement=..., shadowRoot=...)
    at ../Source/WebCore/dom/SlotAssignment.cpp:319
#4  0x00007fa1ba971318 in WebCore::ShadowRoot::assignedNodesForSlot(WebCore::HTMLSlotElement const&) (this=this@entry=0x7fa13031ec68, slot=...)
    at ../Source/WebCore/dom/ShadowRoot.cpp:260
#5  0x00007fa1bab469ca in WebCore::HTMLSlotElement::assignedNodes() const
    (this=0x7fa0ec7645a0) at ../Source/WebCore/html/HTMLSlotElement.cpp:104
#6  0x00007fa1ba8836e5 in WebCore::ComposedTreeIterator::traverseNextInShadowTree() (this=0x7fff3e601020) at ../Source/WebCore/dom/ComposedTreeIterator.cpp:164
#7  0x00007fa1bb3cc578 in WebCore::ComposedTreeIterator::traverseNext()
    (this=this@entry=0x7fff3e601020)
    at ../Source/WebCore/dom/ComposedTreeIterator.h:101
#8  0x00007fa1bb3c6473 in WebCore::ComposedTreeIterator::operator++()
    (this=0x7fff3e601020) at ../Source/WebCore/dom/ComposedTreeIterator.h:49
#9  WebCore::RenderTreeUpdater::tearDownRenderers(WebCore::Element&, WebCore::RenderTreeUpdater::TeardownType, WebCore::RenderTreeBuilder&)
    (root=..., teardownType=<optimized out>, builder=...)
    at ../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:601
#10 0x00007fa1bb3c661e in WebCore::RenderTreeUpdater::tearDownRenderers(WebCore::Element&) (root=...)
    at ../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:533
#11 0x00007fa1ba8f31fe in WebCore::ShadowRoot::hostChildElementDidChange(WebCore::Element const&)
    (childElement=..., this=0x7fa13031ee38) at ../Source/WebCore/dom/SlotAssignment.h:137
#12 WebCore::Element::insertedIntoAncestor(WebCore::Node::InsertionType, WebCore::ContainerNode&)
    (this=0x7fa0ec774930, insertionType=..., parentOfInsertedTree=...) at ../Source/WebCore/dom/Element.cpp:2166
#13 0x00007fa1ba88f0cb in WebCore::notifyNodeInsertedIntoDocument(WebCore::ContainerNode&, WebCore::Node&, WebCore::TreeScopeChange, WTF::OptionSet<WebCore::Node::AncestorState>, WebCore::NodeVector&)
    (parentOfInsertedTree=..., node=..., treeScopeChange=WebCore::TreeScopeChange::Changed, ancestorStates=..., postInsertionNotificationTargets=...) at ../Source/WebCore/dom/ContainerNodeAlgorithms.cpp:48
#14 0x00007fa1ba88fa85 in WebCore::notifyChildNodeInserted(WebCore::ContainerNode&, WebCore::Node&)
    (parentOfInsertedTree=..., node=...) at ../Source/WebCore/dom/Node.h:914
#15 0x00007fa1ba87e1a8 in WebCore::executeNodeInsertionWithScriptAssertion<WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck(WebCore::Node&)::<lambda()> >
    (doNodeInsertion=..., replacedAllChildren=WebCore::ReplacedAllChildren::No, source=WebCore::ContainerNode::ChildChange::Source::API, child=..., containerNode=...) at ../Source/WebCore/dom/Document.h:885
#16 WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck(WebCore::Node&)
    (this=this@entry=0x7fa0ec7742a0, newChild=...) at ../Source/WebCore/dom/ContainerNode.cpp:764
#17 0x00007fa1ba87e81e in WebCore::ContainerNode::appendChild(WebCore::Node&) (this=0x7fa0ec7742a0, newChild=...)
    at ../Source/WebCore/dom/ContainerNode.cpp:730
#18 0x00007fa1ba933925 in WebCore::Node::appendChild(WebCore::Node&) (this=this@entry=0x7fa0ec7742a0, newChild=...)
    at ../Source/WebCore/dom/Node.cpp:511
#19 0x00007fa1b9fe20e8 in WebCore::jsNodePrototypeFunction_appendChildBody
    (castedThis=<optimized out>, callFrame=<optimized out>, lexicalGlobalObject=0x7fa130491068)
    at DerivedSources/WebCore/JSNode.cpp:873
#20 WebCore::IDLOperation<WebCore::JSNode>::call<WebCore::jsNodePrototypeFunction_appendChildBody>
    (operationName=0x7fa1bb9a6b9c "appendChild", callFrame=..., lexicalGlobalObject=...)
    at ../Source/WebCore/bindings/js/JSDOMOperation.h:53
#21 WebCore::jsNodePrototypeFunction_appendChild(JSC::JSGlobalObject*, JSC::CallFrame*)
    (lexicalGlobalObject=0x7fa130491068, callFrame=<optimized out>) at DerivedSources/WebCore/JSNode.cpp:879
#22 0x00007fa163fff1d8 in  ()
#23 0x00007fff3e603a90 in  ()
#24 0x00007fa1b750dae7 in llint_op_call ()
    at /usr/lib/debug/source/sdk/webkitgtk.bst/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1093
#25 0x0000000000000000 in  ()

Good news: it's 100% reproducible. Sadly the most useful variables seem to be optimized out. I can try doing a debug build if desired.

[Radar WebKit Bug Importer]

<rdar://problem/76805764>

[Cassondra Roberts]

This bug appears to be specific to this line of code:


https://github.com/patternfly/patternfly-elements/blob/master/elements/pfe-content-set/src/pfe-content-set.js#L481

In our debugging, we found that the preview version (Safari 14.2, WebKit 16612.1.15.1.12) has resolved this issue.  This is the line of code in question:

`else this.shadowRoot.querySelector(`#container`).innerHTML = view.outerHTML;`

[Cassondra Roberts]

We were able to work around the bug using:

      let newEl = document.createElement("div");
      newEl.appendChild(view);
      this.shadowRoot.querySelector(`#container`).innerHTML = newEl.outerHTML;

[Cassondra Roberts]

I wanted to update the URL I provided to: https://github.com/patternfly/patternfly-elements/blob/1f38b8d08a6d73678ff98e5fa1d23daf0ac8cb9e/elements/pfe-content-set/src/pfe-content-set.js#L481

I linked to master and we have already created a workaround for this issue in our project. This seems to be fixed in the 14.2 release of Safari; is that fix likely to make it into 14.1.2?

[Michael Catanzaro]

FWIW I believe the stable branch for 14.1 is https://trac.webkit.org/log/webkit/branches/safari-611-branch, so you can look and see what's there. Though I'm not familiar with Apple ecosystem, and can't be sure I haven't pointed you at the wrong place, I think that's right.

Here is what I can say for sure:

(1) The crash no longer occurs on https://redhat.com/ with WebKitGTK 2.32.1, and it's pretty unlikely that it was fixed in WebKit between 2.32.0 and 2.32.1, so most likely the web content changed. So I no longer have a reproducer. Constructing a reproducer that uses Patternfly would be the first step here.

(2) We haven't identified the commit that fixed the regression yet. I don't want to spend time bisecting it myself since that can take an entire afternoon, and redhat.com is no longer crashing. Apple has some strict rules that make it hard for them to comment on future product releases, but if we can say "here is the commit that fixed the issue, this would be a good backport candidate," then that makes it rather more likely for the fix to be backported. ;)

[Simon Voges]

We are making heavily use of webcomponents and ShadowDom. We noticed this problem with our customers as soon as the iOS update was publicly available. We created a reproducer using vanillaJS.

Hopefully it will help you pinpointing the problem:
https://github.com/HerrDietz/webkit224408/blob/master/poc.html

This example fails reproducible in Safari 14.1.1

[Michael Catanzaro]

Created attachment 430345 [details]
poc.html

Thanks, I'm attaching your poc.html here

[Michael Catanzaro]

(In reply to Cassondra Roberts from comment #4)
> I linked to master and we have already created a workaround for this issue
> in our project. This seems to be fixed in the 14.2 release of Safari; is
> that fix likely to make it into 14.1.2?

Hm, I tried your reproducer with a build of WebKit trunk from sometime last week. It is *not* fixed.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #8)
> Hm, I tried your reproducer with a build of WebKit trunk from sometime last
> week. It is *not* fixed.

Confirmed, r278350 from this morning is still affected so this is definitely not resolved.

I don't know why you were unable to reproduce in Safari 14.2. I guess that is Safari Tech Preview? Maybe you're using an older Tech Preview that actually doesn't yet have whatever commit introduced this regression, and it got backported to 14.1? Dunno.

[Michael Catanzaro]

Looks very similar to bug #225684.

[Ryosuke Niwa]

(In reply to Michael Catanzaro from comment #9)
> (In reply to Michael Catanzaro from comment #8)
> > Hm, I tried your reproducer with a build of WebKit trunk from sometime last
> > week. It is *not* fixed.
> 
> Confirmed, r278350 from this morning is still affected so this is definitely
> not resolved.
> 
> I don't know why you were unable to reproduce in Safari 14.2. I guess that
> is Safari Tech Preview? Maybe you're using an older Tech Preview that
> actually doesn't yet have whatever commit introduced this regression, and it
> got backported to 14.1? Dunno.

I can reproduce it in STP124.

[Ryosuke Niwa]

Created attachment 431645 [details]
Reduction

[Ryosuke Niwa]

Created attachment 431651 [details]
Fixes the crash

[Michael Catanzaro]

Comment on attachment 431651 [details]
Fixes the crash

Several EWS are red. You need #include <wtf/SetForScope.h>

P.S. Nice test.

[Ryosuke Niwa]

Created attachment 431714 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 431714 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Comment on attachment 431714 [details]
Patch for landing

Clearing flags on attachment: 431714

Committed r279010 (238935@main): <https://commits.webkit.org/238935@main>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.