370703 - "ASSERTION: How did our kid's height change if nothing was dirty?"

Status: RESOLVED WORKSFORME

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Loading this testcase triggers:

###!!! ASSERTION: How did our kid's height change if nothing was dirty?: 'oldVisibleHeight == GetScrolledFrame()->GetSize().height', file /Users/jruderman/trunk/mozilla/layout/forms/nsListControlFrame.cpp, line 667

Last month, bz fixed another bug that triggered the same assertion (bug 366537).

Comment 1

[Boris Zbarsky [:bzbarsky]]

This seems like a table or scrollframe bug to me... at the end of the first reflow (or rather at the beginning of the second one), the frame tree inside that dropdown has:

Area(select)(3)@0xb1843dc [view=0xb185690] {120,60,59820,0} [state=00946008] [overflow=0,0,59820,1020] sc=0xb184ae8(i=1,b=1) pst=:-moz-scrolled-content<
  line 0xb18b030: count=1 state=block,clean,prevmarginclean,not impacted,not
  wrapped,before:nobr,after:nobr[0x2088] {0,0,59820,0} ca={0,0,540,1020} <
    TableOuter(select)(3)@0xb185028 {0,0,59820,0} [state=00054008]
    [content=0xb186960] [overflow=0,0,540,1020] [sc=0xb184f7c]
    pst=:-moz-table-outer<
      Table(select)(3)@0xb185070 {0,0,540,0} [state=00054008] [content=0xb186960]
      [overflow=0,0,540,1020] [sc=0xb184e80] pst=:-moz-table<
        HTMLScroll(option)(1)@0xb18514c [view=0xb185748] {0,0,540,1020}
        [state=00046000] [content=0xb076cf8] [sc=0xb1894e0]<

Note that heights -- the inner table has a height of 0, as does the outer table, etc.  But the scrollframe inside there has a height of 1020!  That's the height everything in sight ends up with after the reflow, and the height it should be to start with, no?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Actually, looks like a tables bug and a reflow branch regression.  The first time through nsTableFrame::Reflow collapsing the rowgroup decreases our height, but the second time it does not.  Probably because nsTableRowFrame::CollapseRowIfNecessary munges the row height _and_ the cell heights, so the next time we come through we get a 0 for the height to collapse out.

Not sure where the 1020 height on the second reflow comes from, but the second time around we definitely do NOT call nsTableRowFrame::Reflow.  In fact, we don't even reflow the rowgroup.  In nsTableFrame::Reflow, we aren't dirty and don't have dirty kids, so we skip reflowing all that stuff.  Then we call nsTableFrame::CalcDesiredHeight and recompute our height.  But this doesn't look at the _real_ rowgroups.  It looks at the scrollframes containing those rowgroups (which makes sense).  But the scrollframe has its original rect, because the collapsing happens from inside nsTableRowGroupFrame!  Which probably actually means we collapse out too much if the rowgroup actually has to scroll, but anyway...

Anyway, that inconsistency is what we end up asserting about in the end.

Comment 3

[Jesse Ruderman]

WFM, Mac trunk.

bz, does anything from your analysis (comment 1 and comment 2) need to become a separate bug?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Uh... could be.  The behavior here changed because bug 367706 changed CalcDesiredHeight look at the actual rowgroups, not at the scrollframes.  I'm not sure that's a correct change, as I said in that bug.  Bernd, do you know what that function is trying to do?  Can we somehow get it to screw up?

Comment 5

[Bernd]

I am not sure that the mechanism for collapsing both for rows, row groups and col and col groups is compatible with the reflow branch change at all. And more sadly I have no idea how to implement this correctly.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Uh... That doesn't actually answer my question.  My question is what the mechanism is trying to accomplish, in particular if a rowgroup is scrollable what it should be doing.  Once we have that figured out, we can worry about incremental reflow.  ;)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Bernd and I talked about this.  Basically, bug 367706 caused a regression.  That's bug 387344.  Once that's fixed, this bug will reappear.

I filed bug 387343 on the underlying problem described in comment 2.

Comment 8

[Jesse Ruderman]

Crashtest checked in.