1452889 - Handle appending multiple elements to a listbox correctly.

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

See the panic in bug 1446368.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1452889: Handle appending multiple items to a listbox correctly.

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
call LazilyStyleNewChildRange _only on the first sibling_, yet the listbox code
tries to construct frames for the next sibling too from CreateRows, yet the next
sibling is unstyled, so we panic.

Instead of handling it in ContentRangeInserted but not ContentAppended, just do
it in the listbox-specific code instead, which looks slightly cleaner (though we
can't assert we're constructing async).

This should fix the case where the listbox is display: none or what not which,
combined with the patch in bug 1303605, supersede the backed out patch in
bug 1429088, which was backed out because listboxes suck.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1452889: Assert more tightly.

Let's see if listboxes are the only ones breaking invariants.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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"

Comment 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/02c0f1ec90d0
https://hg.mozilla.org/mozilla-central/rev/38b392d4b4c1