Handle appending multiple elements to a listbox correctly.

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)

See the panic in bug 1446368.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
Comment on attachment 8966509 [details]
Bug 1452889: Handle appending multiple items to a listbox correctly.

https://reviewboard.mozilla.org/r/235230/#review243818

::: commit-message-1ccce:7
(Diff revision 1)
> +
> +What happened in bug 1446368 is the following: We append two items to an empty
> +listbox.
> +
> +We can't construct lazily because this is XUL, so that goes through
> +IssueSingleInsertNotifications for each of both. When we insert the first one we

"for each of the items"
Attachment #8966509 - Flags: review?(bzbarsky) → review+

Comment 5

a year ago
mozreview-review
Comment on attachment 8966510 [details]
Bug 1452889: Assert more tightly.

https://reviewboard.mozilla.org/r/235232/#review243822

::: layout/base/nsCSSFrameConstructor.cpp:5561
(Diff revision 1)
>    // XXX the GetContent() != aContent check is needed due to bug 135040.
>    // Remove it once that's fixed.
>    if (aContent->GetPrimaryFrame() &&
>        aContent->GetPrimaryFrame()->GetContent() == aContent &&
>        !aState.mCreatingExtraFrames) {
> +    MOZ_ASSERT(MaybeGetListBoxBodyFrame(aContent),

I don't follow this.  We already have an unconditional NS_ERROR here.  What is this looser assert adding?

Of is that NS_ERROR triggering at the moment?
Attachment #8966510 - Flags: review?(bzbarsky) → review-
Assignee

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8966510 [details]
Bug 1452889: Assert more tightly.

https://reviewboard.mozilla.org/r/235232/#review243822

> I don't follow this.  We already have an unconditional NS_ERROR here.  What is this looser assert adding?
> 
> Of is that NS_ERROR triggering at the moment?

That NS_ERROR triggers (with the listbox) in bug 1446368. I want to know if something that isn't a listbox hits this.

Comment 7

a year ago
mozreview-review
Comment on attachment 8966510 [details]
Bug 1452889: Assert more tightly.

https://reviewboard.mozilla.org/r/235232/#review243830

r=me if you add a comment explaining why both assertions are there.

::: commit-message-6b3f9:1
(Diff revision 1)
> +Bug 1452889: Assert more tightly. r?bz

This is actually a looser assert, just more fatal, right?
Attachment #8966510 - Flags: review- → review+

Comment 8

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02c0f1ec90d0
Handle appending multiple items to a listbox correctly. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/38b392d4b4c1
Assert fatally if we're constructing frames for something that has frames and isn't a listbox item. r=bz

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/02c0f1ec90d0
https://hg.mozilla.org/mozilla-central/rev/38b392d4b4c1
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.