Closed Bug 1454157 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: XBL, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

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.
Flags: needinfo?(bugs)
Blerg, meant to a self-ni?, sorry.
Flags: needinfo?(bugs) → needinfo?(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}
Component: CSS Parsing and Computation → XBL
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.
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.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Comment on attachment 8967954 [details]
Bug 1454157: Fix default content removal to actually work.

https://reviewboard.mozilla.org/r/236648/#review242422
Attachment #8967954 - Flags: review+
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
Attachment #8967955 - Flags: review?(bugs) → review+
Pushed by ecoal95@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/d4ea74f174c0
https://hg.mozilla.org/mozilla-central/rev/8fdb38ff02b6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8967954 [details]
Bug 1454157: Fix default content removal to actually work.

https://reviewboard.mozilla.org/r/236648/#review242716
Attachment #8967954 - Flags: review?(mrbkap) → review+
You need to log in before you can comment on or make changes to this bug.