686421 - Printing email crashes Thunderbird [@ nsIFrame::SetNextSibling(nsIFrame*) ]

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Nick Greaves]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0.2) Gecko/20100101 Firefox/6.0.2
Build ID: 20110902133214

Steps to reproduce:

I received an email (an invoice from our broadband supplier).  It did not have any attachments.

I selected "Print" or "Print Preview".




Actual results:

Thunderbird crashed.  Here are the details:

AdapterDeviceID: 2a02
AdapterVendorID: 8086
Add-ons: sendvia@doublesix:1.0.2,{41e52bd9-defc-4a0f-a542-cf643a0776d1}:1.9.1,{58D4392A-842E-11DE-B51A-C7B855D89593}:1.4.0,{C8534C26-F59A-11DA-9804-B622A1EF5492}:5.0,[e-mail]:0.9,[e-mail]:1.19.1,extra-cols@jminta_gmail.com:1.1.3,[e-mail]:1.0.0,[e-mail]:1.0.1,{e2fda1a4-762b-4020-b5ad-a41df1933103}:1.0b5,[e-mail]:1.0,[e-mail]:1.0.0.1
AvailableVirtualMemory: 1710157824
BuildID: 20110902221921
CrashTime: 1315898453
EMCheckCompatibility: false
FramePoisonBase: 00000000f0de0000
FramePoisonSize: 65536
InstallTime: 1315862266
Notes: AdapterVendorID: 8086, AdapterDeviceID: 2a02, AdapterDriverVersion: 8.15.10.1930
D3D10 Layers? D3D10 Layers-
D3D9 Layers? D3D9 Layers+

ProductName: Thunderbird
ReleaseChannel: release
StartupTime: 1315862266
SystemMemoryUsePercentage: 69
Theme: ignoreaero
Throttleable: 1
TotalVirtualMemory: 2147352576
URL: 
Vendor: 
Version: 6.0.2
Winsock_LSP: MSAFD Tcpip [TCP/IP] : 2 : 1 :  
 MSAFD Tcpip [UDP/IP] : 2 : 2 : %SystemRoot%\system32\mswsock.dll 
 MSAFD Tcpip [RAW/IP] : 2 : 3 :  
 MSAFD Tcpip [TCP/IPv6] : 2 : 1 : %SystemRoot%\system32\mswsock.dll 
 MSAFD Tcpip [UDP/IPv6] : 2 : 2 :  
 MSAFD Tcpip [RAW/IPv6] : 2 : 3 : %SystemRoot%\system32\mswsock.dll 
 RSVP TCPv6 Service Provider : 2 : 1 :  
 RSVP TCP Service Provider : 2 : 1 : %SystemRoot%\system32\mswsock.dll 
 RSVP UDPv6 Service Provider : 2 : 2 :  
 RSVP UDP Service Provider : 2 : 2 : %SystemRoot%\system32\mswsock.dll 
 MSAFD RfComm [Bluetooth] : 2 : 1 : 

This report also contains technical information about the state of the application when it crashed.



Expected results:

The email should have printed (or the print preview should have appeared).

The email itself does not contain any particularly sensitive information so I can save it as a file and upload it if necessary for testing.

Comment 1

[Wayne Mery (:wsmwk)]

please post your crash ID
see http://support.mozillamessaging.com/en-US/kb/Mozilla-Crash-Reporter#w_viewing-crash-reports

Comment 2

[Nick Greaves]

Have just reproduced the crash again.  ID is:

bp-6ddb4620-d856-4636-9d6c-030f32110913

Comment 3

[Wayne Mery (:wsmwk)]

Ludo, can you check if this is leftovers from Bug 563009? Print preview crashes Firefox [@ nsIFrame::SetNextSibling(nsIFrame*) ]

Nick, can you attach a testcase message to the bug?  

(In reply to Nick Greaves from comment #2)
> Have just reproduced the crash again.  ID is:
> 
> bp-6ddb4620-d856-4636-9d6c-030f32110913

EXCEPTION_ACCESS_VIOLATION_READ
0x20
0	xul.dll	nsIFrame::SetNextSibling	layout/generic/nsIFrame.h:1057
1	xul.dll	nsFrameList::RemoveFrame	layout/generic/nsFrameList.cpp:133
2	xul.dll	nsTableFrame::PushChildren	layout/tables/nsTableFrame.cpp:1958
3	xul.dll	nsTableFrame::ReflowChildren	layout/tables/nsTableFrame.cpp:2883
4	xul.dll	nsTableFrame::ReflowTable	layout/tables/nsTableFrame.cpp:1900
5	xul.dll	nsTableFrame::Reflow	layout/tables/nsTableFrame.cpp:1804
6	xul.dll	nsContainerFrame::ReflowChild	layout/generic/nsContainerFrame.cpp:959
7	xul.dll	nsTableOuterFrame::OuterDoReflowChild	layout/tables/nsTableOuterFrame.cpp:946
8	xul.dll	nsTableOuterFrame::Reflow	layout/tables/nsTableOuterFrame.cpp:1088
9	xul.dll	nsBlockReflowContext::ReflowBlock	layout/generic/nsBlockReflowContext.cpp:296
10	xul.dll	nsBlockFrame::ReflowBlockFrame	layout/generic/nsBlockFrame.cpp:3211
11	xul.dll	nsBlockFrame::ReflowLine	layout/generic/nsBlockFrame.cpp:2518
12	xul.dll	nsBlockFrame::ReflowDirtyLines	layout/generic/nsBlockFrame.cpp:2000
13	xul.dll	nsBlockFrame::Reflow	layout/generic/nsBlockFrame.cpp:1081
14	xul.dll	nsBlockReflowContext::ReflowBlock	layout/generic/nsBlockReflowContext.cpp:296
15	xul.dll	nsBlockFrame::ReflowBlockFrame	layout/generic/nsBlockFrame.cpp:3211
16	xul.dll	nsBlockFrame::ReflowLine	layout/generic/nsBlockFrame.cpp:2518

Comment 4

[Nick Greaves]

(In reply to Wayne Mery (:wsmwk) from comment #3)
> Ludo, can you check if this is leftovers from Bug 563009? Print preview
> crashes Firefox [@ nsIFrame::SetNextSibling(nsIFrame*) ]
> 
> Nick, can you attach a testcase message to the bug?  

Forgive my ignorance, but what is it you want me to do?

If you want me to save the email message that causes the crash and attach the file then yes, I can do that.  I presume *.eml format is OK?

Comment 5

[Nick Greaves]

Attachment: Email which causes print/print preview crash

Comment 6

[Mats Palmgren (inactive)]

The crashes with nsTableFrame::PushChildren on the stack has crash address near zero,
so it's clear 'prevSibling' is null here:
http://hg.mozilla.org/mozilla-central/annotate/0d5ad6a6f814/layout/generic/nsFrameList.cpp#l129
That means 'aFrame' isn't on this frame list.  One stack frame up it means
we have a frame (rgFrame) in the provided list of child frames to push
(aRowGroups) that doesn't belong to this table frame:
http://hg.mozilla.org/mozilla-central/annotate/0d5ad6a6f814/layout/tables/nsTableFrame.cpp#l1956

The crash data doesn't name a specific line in nsTableFrame::ReflowChildren
but I'm pretty sure the PushChildren() call is the one on line 2898:
http://hg.mozilla.org/mozilla-central/annotate/0d5ad6a6f814/layout/tables/nsTableFrame.cpp#l2893

The code here seems wrong for the case the kid has an existing next-in-flow
with a different table parent.  The "if (!kidNextInFlow)" block is not run
and we put 'kidNextInFlow' into 'rowGroups' for it to be pushed, but we
don't really now this is our kid, if it isn't then it would fit perfectly
with the crash symptoms.

Comment 7

[Mats Palmgren (inactive)]

Attachment: fix

I think we should only put a newly created next-in-flow child
in the child frame array to be pushed.  If the kid already has
an existing next-in-flow, there are two cases: 1. the NIF is
already one of our kids and thus is already present in the
array; 2. the NIF belongs to a different table frame;  in both
those cases we don't want to add it.

Comment 8

[Bernd]

Mats,

Here comes the typical story, while away from desk I suspect this to be wallpaper. First I can't imagine that we have already a nif. Tables are never reflowed twice on printing. Ok there is the generated content crash, which I own, but  thats a different story. I would like to know how we try to reflow a row group twice. This needs to be stopped. Second I insist on a test case  a in crash patch. I know this is a high burden, but unless this is a top crash, where swift action is required, this needs to handled by the rules. If the burden is to high for you I will create the test case as I do not tolerate crashes in this component.

Comment 9

[Mats Palmgren (inactive)]

> First I can't imagine that we have already a nif.

Well, apparently we have one.  Even if that shouldn't occur, there's
no reason to leave the code as is IMO, since it's obviously wrong in
that case.

Anyway, it *does* occur when running the reftest
layout/reftests/bugs/421710-1.html

> Tables are never reflowed twice on printing.

Well, it actually *does* occur.  Same test.
The table we reflow twice even has a row-group with a NIF!
Fortunately, the NIF is still on the Overflow-list in this case.
It looks like we reflow the table and create a NIF for the row-group
and then decide to push the table to the next page...

> I would like to know how we try to reflow a row group twice.

This happens *all the time* in print mode.
As an example: layout/reftests/bugs/409084-1b.html (in print preview)
Run it in gdb and put a break point in nsTableRowGroupFrame::Reflow

You have:
nsTableFrame::ReflowTable
  nsTableFrame::ReflowChildren
    nsTableFrame::SetupHeaderFooterChild
      nsContainerFrame::ReflowChild
        nsTableRowGroupFrame::Reflow (this=0x7fffe4767280 ...

and then a little later:

nsTableFrame::ReflowTable
  nsTableFrame::ReflowChildren
    nsContainerFrame::ReflowChild
      nsTableRowGroupFrame::Reflow (this=0x7fffe4767280 ...

from the same nsTableFrame::ReflowChildren call.

> Second I insist on a test case  a in crash patch

I agree tests are valuable, but I disagree that it should block fixes from
landing.  After all, there are real users behind the crash stats...

Comment 10

[Mats Palmgren (inactive)]

Attachment: frame dumps, stacks for reftest layout/reftests/bugs/421710-1.html

The first frame dump is the frame tree after frame construction before reflow.
The last frame tree is when the table frame (red) enters its second reflow.

Comment 11

[Bernd]

> Second I insist on a test case  a in crash patch

>I agree tests are valuable, but I disagree that it should block fixes from
>landing.  After all, there are real users behind the crash stats...

The reason to insist is that the stack trace look to me very similar to bug 675490 and bug 642088 which  I worked on and both are fixed in firefox8. In both cases the crash reason was something completely different and if I had taken the short path, that I suspect here, none of the roots would have been eradicated. If this crash is still real then I would prefer to know what really goes wrong instead of silently closing it. Therefor a test case helps tremendously.
It also helps to verify that the patch fixes the bug and not something else. When I  tried to create a testcase from the eml file I noticed that I tried this already in Oct. 2011 and failed.

This bug could be easily a dupe of the above mentioned bugs which would make the moral argument moot as this bug might be wfm.  I checked the crash stats for setnextsibling on TB 9.0.1 and the first 10 none dup crashes of them had not removeframe but the insertframe call on stack which would be the other couple of bugs that I worked last time on like bug 695430. Further there is no crash with RemoveFrame on 9.0.1 that is table related.

Would your patch alter the behavior on layout/reftests/bugs/421710-1.html?

Comment 12

[Bernd]

This patch basically backs out bug 347367

Comment 13

[Bernd]

Mats please take the test case from bug 696640 to  fulfill the testcase requirement. Add a assert that (!NIF || reorder) to make sure that the NIF is a old one.

Comment 14

[Bernd]

and you might move with the patch to bug 696640, I believe this one is wfm.

Comment 15

[Bernd]

Comment on attachment 592448 [details] [diff] [review]
fix

as I said before I will review the patch with the test case.

Comment 16

[Mats Palmgren (inactive)]

Should be fixed by bug 696640.