291520 - Crash [@ nsFrameList::DestroyFrames] with evil testcase using float:right and display:table-caption

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Martijn Wargers (dead)]

The upcoming testcase crashes Mozilla when hovering over the text.
Also happens in Mozilla1.7, so no recent regression.

Talkback ID: TB5285309E

nsFrameList::DestroyFrames 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/generic/nsFrameList.cpp,
line 131]
nsTableOuterFrame::RemoveFrame 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/tables/nsTableOuterFrame.cpp,
line 284]
nsCSSFrameConstructor::ContentRemoved 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 9793]
nsCSSFrameConstructor::RecreateFramesForContent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 11599]
nsCSSFrameConstructor::RestyleElement 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 10181]
nsCSSFrameConstructor::ProcessOneRestyle 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 13553]
nsCSSFrameConstructor::ProcessPendingRestyles 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 13601]
nsCSSFrameConstructor::RestyleEvent::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 13654]
SHELL32.dll + 0x520c24 (0x778b0c24)

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Martijn Wargers (dead)]

Oops, almost forgot to CC you Bernd, which I promised I would do.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The reason this crashes, in general, is that floats get pretty confused when
inside a table-caption.  The reason is that the table-caption is a nsBlockFrame,
but doesn't push itself as the float containing block before processing
children.  As a result, thanks to the wonders of BuildFloatList, we end up with
frames on multiple frame lists at once, etc.

So the short-term fix is to push the float containing block here too.

A better fix is to maybe use ConstructBlock here (note that I think passing in
the table creator as we do in ConstructTableCaptionFrame is wrong, so that's not
an obstacle to using ConstructBlock).  That would also have the quite salutary
(imo) effect of making overflow, columns, etc work on table captions.  It
_would_ mean that tables can't assume the thing on the caption list is an
nsTableCaptionFrame, though...  But I don't see any casts in the code to that
concrete class, so that may be ok.

Finally, I still think that we should move all float-containing-block pushing
down into ProcessChildren() itself (conditioned on IsFloatContainingBlock())....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible patch

This fixes the crash for me (and some other asserts too, in the testcase I'm
about to attach).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Test some stuff

I still get:

###!!! ASSERTION: We have to create a continuation, but the block doesn't want
us to reflow it?: 'aStatus & NS_FRAME_REFLOW_NEXTINFLOW', file
/home/bzbarsky/mozilla/xlib/mozilla/layout/generic/nsColumnSetFrame.cpp, line
519

with this testcase, but that may be due to me misusing column styles?  But the
assert about not being able to find a placeholder is gone and the abs pos text
is in the right place...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Note that with that patch we _will_ end up with a column-set frame constructed
by ConstructCaptionFrame as needed, which may confuse our
ContentAppended/ContentInserted logic...  bernd, you know that code best; will it?

Also notice that this still doesn't handle scrolling for captions....

Comment 7

[Bernd]

Attachment: alternative crash fix

I think the root of this bug is rather that the caption is not correctly marked
as a float containing block. I don't see how attachment 181719 [details] [diff] [review] addresses this
problem. <rant>I thought yesterday evening, hey you can code this tomorrow
morning, get out of bed, code it, test it and there is allready a patch
attached to the bug. Life can be brutal</rant>

Comment 8

[Bernd]

s/I don't see how/It seems to me that we do to much with this patch, especially
now that I think I killed nearly all these small caption crashes/

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I think the root of this bug is rather that the caption is not correctly marked
> as a float containing block.

That's true.  Using ConstructBlock() to construct it would solve this problem
(because ConstructBlock() does the right thing), as well as fixing various other
issues with captions... The problem, as I said, is that it may put, say, a
column set frame in the caption list.  But since we need columns in captions
anyway, we need to make that work.

I guess I'd be ok with the limited change to ConstructTableCaptionFrame for now
and a followup bug filed to use ConstructBlock for captions.

Comment 10

[Bernd]

Boris your testcase seems to be a patch rather than html

Comment 11

[Boris Zbarsky [:bzbarsky]]

<sigh>... so it is.  And I no longer have the original file I was trying to
attach...   Briefly, the following _should_ work on captions but do not:

overflow:scroll
position:relative (becoming an abs pos containing block)
columns
being a float containing block

Your patch fixes #4.  My patch fixes #2, part of #3, and #4.  But yes, it's a
lot more invasive.  Perhaps better as 1.9 material.

Comment 12

[Bernd]

Comment on attachment 181732 [details] [diff] [review]
alternative crash fix

lets take the small ride now and do the heavy lifting in  1.9

Comment 13

[Bernd]

Comment on attachment 181719 [details] [diff] [review]
Possible patch

this will go into the followup bug

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181732 [details] [diff] [review]
alternative crash fix

>Index: nsCSSFrameConstructor.cpp

>+  HaveSpecialBlockStyle(aContent, aStyleContext,
>+                          &haveFirstLetterStyle, &haveFirstLineStyle);

Fix the indent.

Comment 15

[Bernd]

Comment on attachment 181732 [details] [diff] [review]
alternative crash fix

The patch is medium risk, we postponed the heavy lifting towards 1.9. I
rtest'ed the patch.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I filed bug 291854 on the "Finally" part of comment 3.

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 181732 [details] [diff] [review]
alternative crash fix

a=asa

Comment 18

[Bernd]

fix checked in, the followup for the bigger solution is bug 282173

Comment 19

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050429
Firefox/1.0+ 0251PDT

Testcase 1 behaviour has changed , it does not crash , but doesn't work good either.

this "regressed" beween the 20050428 16:58PDT build and 20050429 0251PDT build
which fits the checkin time

(I haven't got any other builds to get a more accurate time)

Comment 20

[Boris Zbarsky [:bzbarsky]]

> but doesn't work good either.

Meaning what, exactly?

> this "regressed" beween the 20050428 16:58PDT build and 20050429 0251PDT build

Meaning what?  Before that, you couldn't even tell what the testcase looked
like, could you?  Since it crashed?

Bernd, bug 282173 isn't really a followup to this bug.  That bug covers float
issues, which are already fixed.  We need a followup on having captions be
blocks like any other block, with support for columns, absolute positioning,
etc...  I can file one if you don't have time; just let me know.

Comment 21

[Peter van der Woude [:Peter6]]

(In reply to comment #20)
> > but doesn't work good either.
> 
> Meaning what, exactly?
I don't know if it is supposed to wrap every word

Comment 22

[Bernd]

Boris: for you I have always time, just ask :-) (There will be always a tomorrow
when I need help on irc). I will file one.
The wrapping of the caption is incr. reflow bug what happens is that the caption
thinks that it has a table with width 0 below it (so it tries to go to the
minimum width. We had  this in the other caption crashers too. I am not certain
that we dont allready have a bug on it. Peter if you dont find one, file one it
should be something

<table><caption> foo bar baz</caption> document.body.offsetHeight</table>

mabee it needs another empty row group after the layout breakpoint. The code for
it is in nsTableOuterFrame.cpp if you like to fix it.

Comment 23

[Peter van der Woude [:Peter6]]

(In reply to comment #22)
> The wrapping of the caption is incr. reflow bug what happens is that the caption
> thinks that it has a table with width 0 below it (so it tries to go to the
> minimum width. We had  this in the other caption crashers too. I am not certain
> that we dont allready have a bug on it. Peter if you dont find one, file one it
> should be something
> 
> <table><caption> foo bar baz</caption> document.body.offsetHeight</table>
> 
> mabee it needs another empty row group after the layout breakpoint. The code for
> it is in nsTableOuterFrame.cpp if you like to fix it.
The CSS goes well beyond what I know about what it supposed to do and why Bernd.
If a dupe bug was filed in technical terms i wouldn't even recognise it if I'd
be staring at it.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Note to self: bug 292756 is the followup bernd filed.