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

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

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.
Assignee

Updated

a year ago
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}
Assignee

Updated

a year ago
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

Updated

a year ago
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a year ago
mozreview-review
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 9

a year ago
mozreview-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+

Comment 10

a year ago
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

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d4ea74f174c0
https://hg.mozilla.org/mozilla-central/rev/8fdb38ff02b6
Status: NEW → RESOLVED
Last Resolved: a year 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.