280754 - Listbox does not display all its children if more than one is moved

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Erik Fabert]

Testcase is attachment 173089 [details] : The listbox has 6 listitems, three of them are
moved using insertBefore. Only the last one moved is rendered.

This is a fairly recent regression, it works in 1.8b 2005011816 and fails in
20050125.

FWIW, FF 1.0 also fails the testcase: it renders all six elements but in the
wrong order, 1-4-2-5-36. Correct is 4-3-1-5-2-6.

Comment 1

[Erik Fabert]

Fails in 2005012011, too. The patch for bug 244366 seems a likely cause.

Comment 2

[[e-mail]]

bz said so in bug 276698 comment 10, but you beat me to it ;-)

Comment 3

[Boris Zbarsky [:bzbarsky]]

So we have a problem here.

Listbox does this bizarre lazy frame creation.  To handle content insertion and
removal and the like it sets some flags that can only handle _one_ content node
and then does a synchronous reflow to force frame creation.  All this is
happening from inside frame construction.

Now reflow from inside frame construction crashes, in general.  It may not in a
pure-XUL document, but I could pretty easily write up some testcases where it
would happen using mixed-namespace documents.  The patch in bug 244366 added
some safety-checks that prevent reflow flushes inside frame construction.  Hence
this bug, since the listbox can only handle one content at a time.

I've sent mail to hewitt in an attempt to figure out exactly what this code is
trying to accomplish, but if anyone happens to know, please share that knowledge...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Test page to exercise a bunch of listbox DOM manipulation

Current builds fail at "rearrange" and "insert 2".

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This fixes the bug (and I tested it on the various DOM manipulation, and it
seems to do fine).

The basic change is that on ContentRemoved we don't need to flush reflow at
all, and on ContentInserted we replace flushing reflows by a direct call to
CreateRows.  That constructs the new frames, if they are needed.

Comment 6

[[e-mail]]

a) Can we move the declaration of "state" down a bit?
b) What about the other calls to Flush (especially the XXXbz one!)

Comment 7

[Boris Zbarsky [:bzbarsky]]

That one only happens when we're scrolling the list via the scrollbar mediator
stuff, I think...

I'd be perfectly happy to remove those calls, on general principle, but I'm not
sure I'd be happy doing that for 1.8b1 at this point.  Separate bug, perhaps?

Comment 8

[[e-mail]]

Comment on attachment 173382 [details] [diff] [review]
Proposed fix

>   nsBoxLayoutState state(mPresContext);
> 
>   // now create or destroy any rows as needed
>-  CreateRows(state);
>+  CreateRows();
So, could the declaration be moved nearer the remaining use?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> So, could the declaration be moved nearer the remaining use?

Yeah, will do.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fix checked in.  I've filed bug 281147 on removing the remaining flushes.

Comment 11

[Mike Calmus]

*** Bug 279400 has been marked as a duplicate of this bug. ***