391148 - Hang UI with nested fieldset with display: inline

Status: VERIFIED WORKSFORME

(Core :: Layout: Block and Inline, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which hangs current trunk build or at least takes very long to render.
This regressed between 2006-12-07 and 2006-12-08, so I think a regression from bug 300030, somehow.

Comment 1

[dolphinling]

On my computer,
<= 16 fieldsets renders instantly
17 fieldsets takes 3 seconds
18 fieldsets takes 22 seconds
19 fieldsets takes 43 seconds
20 fieldsets takes 85 seconds
21 fieldsets takes 169 seconds
(Anyone else see a doubling pattern? :)

Also of note: at 15 fieldsets, the -es are all at the bottom of their boxes; at 16, some are at the top, at 17, most are at the top, and at 18 fieldsets, all of the -es are at the top of their boxes.

Comment 2

[Wayne Mery (:wsmwk)]

hangs UI - goes to "not responding" within a couple seconds on 2.4Ghz core duo vista, 080108 and 080217 nightly

3 minutes and stopped counting.
rendered and UI freed when I returned about 5 minutes later

Comment 3

[Boris Zbarsky [:bzbarsky]]

Fun times.

So the key is that the available width is too small.  If I set that width to 0px, then the number of calls to nsFieldSetFrame::Reflow grows like so:

  # of nested fieldsets     # of reflow calls
          1                          3
          2                          9
          3                         21

Exponential growth == bad, of course.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, so the factor of 3 is really just due to us doing multiple reflows (due to the scrollframe?) during pageload.  The real scaling comes from the fact that we reflow the outermost fieldset.  We place the '-' on the first line, then reflow the inner fieldset.  It doesn't fit.  So we go ahead and push it to the next line.  Then we go on to that next line and reflow it again.  In the testcase I'm using here (inserting the fieldsets via innerHTML to skip the scrollframe stuff) we have the NS_FRAME_IS_DIRTY flag set for both reflows (the one before we push to the next line and the one after), so the fieldset can't even optimize them away sanely.

The same exact issue should arise with inline-blocks instead of fieldsets, I bet.

David, roc, is there any way we could avoid doing the full dirty reflow for pushed frames?  Doesn't seem like they'd need it after we've done the one dirty reflow on them....  Even then the testcase could use percentage-height inline-blocks, in which case the mVResize flag being true would still screw us over.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing the problem with inline-block

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't see why we'd need to do a dirty reflow unless that's needed to make inlines pull continuations back together (which I don't think it should be).  But roc knows that code better than I do.

A vertical resize to the same size should be really cheap and get optimized away without reflowing contents.  If it's not, we should fix it.

Comment 7

[Boris Zbarsky [:bzbarsky]]

We're doing a dirty reflow because the parent block is doing a dirty reflow, so we smack all the child reflow states with the dirty bit.

As for the vertical resize, the mVResize flag is set, and if we have percentage-height kids we'd end up reflowing them...

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds fixable, but not 1.9 fodder.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I mean, I agree with comment #6.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ah, so we'd need a special way to invoke the reflow state constructor (or undo its work) for the case when we've already reflowed the frame once.  I'm not sure if we currently keep track of that state...

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Or we could mark all the kids dirty up front and then always tell the reflow state not to propagate the bit... sort of like the old code did.

Comment 12

[Guilherme Lima]

The testcase made my Nightly hang forever and didn't trigger anything that could save me =/

Comment 13

[Wayne Mery (:wsmwk)]

This does work for me now. Win7, nightly build