1454157 - Listboxes suck, and layout/base/crashtests/414175-1.xul breaks invariants.

Status: RESOLVED FIXED

(Core :: XBL, enhancement)

Description

[Emilio Cobos Álvarez [:emilio]]

With https://hg.mozilla.org/integration/autoland/rev/b12b7c5b8178, it asserts on teardown, meaning that something is broken.

I tried to land this patch in bug 1453206, but failed because of this single crashtest.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Blerg, meant to a self-ni?, sorry.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Ugh, this is a weird XBL bug, we're losing track of the inserted kid somewhere:

(rr) p node
$28 = (const nsXULElement *) 0x7f9e87c4b1f0
(rr) p $28->mSlots->mExtendedSlots->mXBLInsertionPoint.mRawPtr
$29 = (mozilla::dom::XBLChildrenElement *) 0x7f9e87c43520
(rr) p $29->mInsertedChildren
$30 = nsTArray<nsIContent*> = {0x7f9e87cbd940}

Comment 3

[Emilio Cobos Álvarez [:emilio]]

I cannot make any sense of the MaybeRemoveDefaultContent() bits in:

  https://searchfox.org/mozilla-central/rev/fca4426325624fecbd493c31389721513fc49fef/dom/xbl/XBLChildrenElement.h#58

Like... We first insert, _then_ check and only if there's no inserted children we remove the insertion point. Which is clearly bogus.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

That of course fixes the inconsistency issue. Yet that still doesn't fix the fact that we don't tell layout about it, so with that fixed the assertion will still happen.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1454157: Fix default content removal to actually work.

If you add to the mInsertedChildren array first, it's never going to be empty in
MaybeRemoveDefaultContent.

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

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1454157: Tell layout about default content going away.

Pretty much the same way we do for Shadow DOM.

There are a couple more places broken, but provided our fronted folks don't do
weird stuff nothing bad should happen, and the assertion that this allows me to
add should catch those if they do, so I'm punting on it for now.

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

Comment 7

[Emilio Cobos Álvarez [:emilio]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22e521879343fce3bf76e9adeadc22d896e2c481

Comment 8

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

Comment on attachment 8967954 [details]
Bug 1454157: Fix default content removal to actually work.

https://reviewboard.mozilla.org/r/236648/#review242422

Comment 9

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

Comment on attachment 8967955 [details]
Bug 1454157: Tell layout about default content going away.

https://reviewboard.mozilla.org/r/236650/#review242424

::: dom/xbl/nsXBLBinding.cpp:350
(Diff revision 1)
>  
>      // Insert explicit children into insertion points
>      if (mDefaultInsertionPoint && mInsertionPoints.IsEmpty()) {
>        ExplicitChildIterator iter(mBoundElement);
>        for (nsIContent* child = iter.GetNextChild(); child; child = iter.GetNextChild()) {
> -        mDefaultInsertionPoint->AppendInsertedChild(child);
> +        mDefaultInsertionPoint->AppendInsertedChild(child, false);

Please add a comment why false is passed here (since we're just creating the whole anon content)

::: dom/xbl/nsXBLBinding.cpp:360
(Diff revision 1)
>        // node has any non <xul:template> or <xul:observes> children.
>        ExplicitChildIterator iter(mBoundElement);
>        for (nsIContent* child = iter.GetNextChild(); child; child = iter.GetNextChild()) {
>          XBLChildrenElement* point = FindInsertionPointForInternal(child);
>          if (point) {
> -          point->AppendInsertedChild(child);
> +          point->AppendInsertedChild(child, false);

and here too

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ea74f174c0
Fix default content removal to actually work. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fdb38ff02b6
Tell layout about default content going away. r=smaug

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d4ea74f174c0
https://hg.mozilla.org/mozilla-central/rev/8fdb38ff02b6

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8967954 [details]
Bug 1454157: Fix default content removal to actually work.

https://reviewboard.mozilla.org/r/236648/#review242716