1281745 - "Assertion failure: insertion.mContainer == GetInsertionPoint(parent, child).mContainer" with createShadowRoot, <xbl:children>

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: insertion.mContainer == GetInsertionPoint(parent, child).mContainer, at layout/base/nsCSSFrameConstructor.cpp:10671

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

I thought XBL was disabled for web content, so it's disturbing that a testcase with <xbl:children> can make special things happen. Do I need to re-teach my fuzzer to create XBL elements? Which ones?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

ni? bz for comment 2.

Comment 4

[Boris Zbarsky [:bzbarsky]]

XBL binding attachment is disabled for web content, with some exceptions.

ChildIterator.cpp has explicit checks for xbl:children and assumes stuff if it sees them.  That's probably broken.  :(  They should probably be using IsActiveChildrenElement() instead or something.  That was added in bug 890775 but apparently not all consumers were updated to it.

This is fallout from bug 653881, looks like.

In general, xbl:content and xbl:children are the "interesting" things.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #4)
> ChildIterator.cpp has explicit checks for xbl:children and assumes stuff if
> it sees them.  That's probably broken.  :(  They should probably be using
> IsActiveChildrenElement() instead or something.  That was added in bug
> 890775 but apparently not all consumers were updated to it.

ChildIterator.cpp does use IsActiveChildrenElement via nsContentUtils::IsContentInsertionPoint. The problem is that when we append an <xbl:children> to an active shadow root, we set its insertion parent, making it "active". We should probably detect when we're appending an <xbl:children> to a shadow root node and avoid setting its binding parent.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Still reproducible on trunk. With Stylo enabled, the assertion is different, though.
Assertion failure: content->GetFlattenedTreeParentNodeForStyle() == &aContent, at z:/build/build/src/layout/base/ServoRestyleManager.cpp:1220

Comment 7

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1281745: Don't consider <xbl:children> in a shadow root without any binding active.

Review commit: https://reviewboard.mozilla.org/r/236500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236500/

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8967819 [details]
Bug 1281745: Don't consider <xbl:children> in a shadow root without any binding active.

https://reviewboard.mozilla.org/r/236500/#review242298

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6090c4592979
Don't consider <xbl:children> in a shadow root without any binding active. r=smaug

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8967819 [details]
Bug 1281745: Don't consider <xbl:children> in a shadow root without any binding active.

https://reviewboard.mozilla.org/r/236500/#review242372

::: dom/base/nsIContentInlines.h:179
(Diff revision 1)
> +  nsIContent* bindingParent = GetBindingParent();
> +  if (!bindingParent) {
> +    return false;
> +  }
> +
> +  return !bindingParent->GetShadowRoot();

Please add a comment about why this is here. It's a bit surprising to see code talking about shadow roots when everything else is XBL-based.

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6090c4592979

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac542a489af
bug 1453206: Add a comment pointing out why shadow root checks are needed. r=me

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6ac542a489af