Bug 224408 - Crash in WebCore::SlotAssignment::assignedNodesForSlot
Summary: Crash in WebCore::SlotAssignment::assignedNodesForSlot
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2021-04-10 17:17 PDT by Michael Catanzaro
Modified: 2021-06-17 14:55 PDT (History)
14 users (show)

See Also:


Attachments
bt full (45.97 KB, text/plain)
2021-04-10 17:17 PDT, Michael Catanzaro
no flags Details
poc.html (2.65 KB, text/html)
2021-06-02 07:14 PDT, Michael Catanzaro
no flags Details
Reduction (519 bytes, text/html)
2021-06-17 02:36 PDT, Ryosuke Niwa
no flags Details
Fixes the crash (6.77 KB, patch)
2021-06-17 03:43 PDT, Ryosuke Niwa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (6.93 KB, patch)
2021-06-17 13:29 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2021-04-10 17:17:07 PDT
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.
Comment 1 Radar WebKit Bug Importer 2021-04-17 17:18:13 PDT
<rdar://problem/76805764>
Comment 2 Cassondra Roberts 2021-05-27 10:00:53 PDT
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;`
Comment 3 Cassondra Roberts 2021-05-27 10:13:16 PDT
We were able to work around the bug using:

      let newEl = document.createElement("div");
      newEl.appendChild(view);
      this.shadowRoot.querySelector(`#container`).innerHTML = newEl.outerHTML;
Comment 4 Cassondra Roberts 2021-06-01 09:37:34 PDT
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?
Comment 5 Michael Catanzaro 2021-06-01 10:39:11 PDT
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. ;)
Comment 6 Simon Voges 2021-06-02 06:47:33 PDT
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
Comment 7 Michael Catanzaro 2021-06-02 07:14:37 PDT
Created attachment 430345 [details]
poc.html

Thanks, I'm attaching your poc.html here
Comment 8 Michael Catanzaro 2021-06-02 07:34:05 PDT
(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.
Comment 9 Michael Catanzaro 2021-06-02 12:17:40 PDT
(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.
Comment 10 Michael Catanzaro 2021-06-02 12:22:18 PDT
Looks very similar to bug #225684.
Comment 11 Ryosuke Niwa 2021-06-02 12:36:13 PDT
(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.
Comment 12 Ryosuke Niwa 2021-06-17 02:36:05 PDT
Created attachment 431645 [details]
Reduction
Comment 13 Ryosuke Niwa 2021-06-17 03:43:27 PDT
Created attachment 431651 [details]
Fixes the crash
Comment 14 Michael Catanzaro 2021-06-17 05:25:29 PDT
Comment on attachment 431651 [details]
Fixes the crash

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

P.S. Nice test.
Comment 15 Ryosuke Niwa 2021-06-17 13:29:55 PDT
Created attachment 431714 [details]
Patch for landing
Comment 16 Ryosuke Niwa 2021-06-17 13:30:29 PDT
Comment on attachment 431714 [details]
Patch for landing

Wait for EWS.
Comment 17 Ryosuke Niwa 2021-06-17 14:55:26 PDT
Comment on attachment 431714 [details]
Patch for landing

Clearing flags on attachment: 431714

Committed r279010 (238935@main): <https://commits.webkit.org/238935@main>
Comment 18 Ryosuke Niwa 2021-06-17 14:55:29 PDT
All reviewed patches have been landed.  Closing bug.