576890 - Crash [@ nsFrameList::InsertFrames] on print preview

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Martijn Wargers (dead)]

See testcase, which crashes current trunk build on print preview.

http://crash-stats.mozilla.com/report/index/e3b5958e-450b-424f-8134-3a0c82100704
0  	xul.dll  	nsFrameList::InsertFrames  	 layout/generic/nsFrameList.cpp:236
1 	xul.dll 	nsTableFrame::PushChildren 	layout/tables/nsTableFrame.cpp:1959
2 	xul.dll 	nsTableFrame::ReflowChildren 	
3 	xul.dll 	nsTableFrame::ReflowTable 	layout/tables/nsTableFrame.cpp:1883
4 	xul.dll 	nsTableFrame::Reflow 	layout/tables/nsTableFrame.cpp:1787
5 	xul.dll 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:738
6 	xul.dll 	nsTableOuterFrame::Reflow 	layout/tables/nsTableOuterFrame.cpp:1090
7 		@0x8d9aeff 

I guess this could be fixed by bug 563584.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Otoh, I seem to have a case where no floats are present.

Comment 3

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

Works for me in a build with the bug 563584 patches.

Comment 4

[Martijn Wargers (dead)]

The testcase in this bug doesn't crash anymore, but I still have an unminzimed testcase, that crashes with this stacktrace. I'll try to minimize it and then upload that testcase here.

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase

This testcase still crashes on print preview in current trunk build.

http://crash-stats.mozilla.com/report/index/fd3bd5ac-65a0-4326-aedd-7bc602100809
0  	xul.dll  	nsFrameList::InsertFrames  	 layout/generic/nsFrameList.cpp:242
1 	xul.dll 	nsTableFrame::PushChildren 	layout/tables/nsTableFrame.cpp:1959
2 	xul.dll 	nsTableFrame::ReflowChildren 	
3 	xul.dll 	nsTableFrame::ReflowTable 	layout/tables/nsTableFrame.cpp:1883
4 	xul.dll 	nsTableFrame::Reflow 	layout/tables/nsTableFrame.cpp:1787
5 	xul.dll 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:738
6 	xul.dll 	nsContainerFrame::StealOverflowFrames 	layout/generic/nsContainerFrame.h:623
7 		@0x2 	
8 		@0xe7a2dff

Comment 6

[Martijn Wargers (dead)]

Still crashes, using:
Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100822 Minefield/4.0b5pre

Comment 7

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

Still crashing?

Comment 8

[Mats Palmgren (inactive)]

Yes, I can reproduce the crash on x86-64 Linux.  I'll take a look.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase

Same test, just changing <span> to <b> for the table-footer so it's easier
to tell apart from the table-header in the frame dump.

Comment 10

[Mats Palmgren (inactive)]

Attachment: frame dump for Testcase 1

There are multiple bugs here, but let's focus on the crash cause first.
In the 1st reflow, we find the page break after the header and we call
PushChildren on line 2719:
http://hg.mozilla.org/mozilla-central/file/070567151424/layout/tables/nsTableFrame.cpp#l2718
since the child we're looking at is the tfoot (which IsRepeatable) there
are no frames to push so PushChildren is a NOP and we break out of
ReflowChildren with NS_FRAME_NOT_COMPLETE.
2nd reflow we reflow the table next-in-flow, nothing interesting here.
3rd reflow we reflow the first table again, we hit the same PushChildren
but this time we have a next-in-flow and we try to insert the empty
frame list (which is illegal):
http://hg.mozilla.org/mozilla-central/file/070567151424/layout/tables/nsTableFrame.cpp#l1936

Comment 11

[Mats Palmgren (inactive)]

Attachment: Crash fix

This fixes the crash but the rendering is incorrect...

Comment 12

[Mats Palmgren (inactive)]

Attachment: Testcase 2

Same as Testcase 1, but with some text in the frames.

Comment 13

[Mats Palmgren (inactive)]

Attachment: Screenshot, print preview of Testcase #2

This is what Testcase 2 looks like with the patch.
Apart from the overlapping content, I still think this is incorrect.
I think the correct rendering is a table-header on the first page and
a table-footer on the second page, nothing else.  Although they are
repeatable, there's no table-body so they should not repeat.

Is this correct?

We should probably spawn off the rendering problem to a separate bug though.
The crash fix is pretty trivial so we should get it into 2.0.

Comment 14

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

(In reply to comment #13)
> Is this correct?

I think so.

Comment 15

[Mats Palmgren (inactive)]

Attachment: Crash fix with tests

Ok, I'll spawn off the rendering bug.

Comment 16

[Mats Palmgren (inactive)]

Filed bug 610245 for the rendering bug.

http://hg.mozilla.org/mozilla-central/rev/933bfe553fb0