118968 - Print Preview crashes on tinderbox

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[rods (gone)]

Comment 1

[rods (gone)]

Attachment: stack trace

Comment 2

[karnaze (gone)]

->m099

Comment 3

[rods (gone)]

taking

Comment 4

[rods (gone)]

Attachment: patch

For some reason as yet undetermined, box layout changes a resize reflow back
into an initial reflow so page layout for Print Preview ends up getting
multiple initial reflows. This patch filters those out.

Comment 5

[rods (gone)]

Attachment: patch for possible reflow optimization

I found this. It seems to skip the extra resize reflow a lot both in regular
layout and in PP layout. Everything still seems to work ok and look ok. 

Here is a diff of two runs with and without the optimization when loading the
mozilla "start" page. It reduces reflow in nsBoxToBlockAdaptor and nsBoxFrame.

Note the overall reflows are reduced by 36:

P:\mozilla\dist\WIN32_D.OBJ\bin>diff log1.txt log2.txt
2a3
> *** Registering nsLayoutModule components (all right -- a generic module!)
75c76
<	nsBoxToBlockAdaptor	0	47	6	0	0	53
---
>	nsBoxToBlockAdaptor	0	36	6	0	0	42
78,79c79,80
<	      ViewportFrame	1	52	2	0	0	55
<	     nsRootBoxFrame	1	52	2	0	0	55
---
>	      ViewportFrame	1	47	2	0	0	50
>	     nsRootBoxFrame	1	47	2	0	0	50
82c83
<	       nsBlockFrame	32	0	4	0	0	36
---
>	       nsBlockFrame	22	0	4	0	0	26
84c85
<		 nsBoxFrame	1	52	2	0	0	55
---
>		 nsBoxFrame	1	47	2	0	0	50
86c87
<	       Grand Totals	47	203	23	0	0	273
---
>	       Grand Totals	37	177	23	0	0	237


*** When loading the CSS2 page it reduced the number of reflows by 381, so the
longer the document the more reflows that are saved!

Comment 6

[rods (gone)]

Attachment: replaces first patch (nsSimplePageSeq.cpp)

I found a small issue with setting the cached value for skipping the reflow.

Comment 7

[Chris Waterson]

Attachment 65432 [details] [diff] (the reflow optimization) looks fine to me.

Comment 8

[rods (gone)]

This fix keeps us from crashing by filtering out the extra initial reflows. I
have filed a bug on the real problem: Bug 120652

Comment 9

[Chris Waterson]

Since evaughan is no longer around, could you spend a bit of time and try to
figure out why box is converting the reflow reason to eReflowReason_Initial?
(hyatt may have some insight.) Hacks like this have a nasty way of living forever.

Comment 10

[rods (gone)]

Attachment: full patch with optimization

This is the correct full fix for the problem. 

When reflowing a HTML document where the content model is benig created
The nsGfxScrollFrame will get an Initial reflow when the body is opned by the
content sink.

But there isn't enough content to really reflow very much of the document
so it never needs to do layout for the scrollbars

So later other reflows happen and these are Incremental reflows, and then the
scrollbars get reflowed. The important point here is that when they reflowed
the ReflowState inside the BoxLayoutState contains an "Incrtemental"  reason
and never a "Initial" reason.

When it reflows for Print Preview, the content model is already full
constructed and it lays out the entire document at that time. When it return
back here it discovers it needs scrollbars not this is a problem because the
ReflowState insode the BoxLayoutState still has a "Initial" reason and if it
does a Layout it is essentially asking everything to reflow yet again with an
"Initial" reason. 

This causes a lot of problems especially for tables.
 
The solution for this is to change the ReflowState's reason from Initial to
Resize and let all the frames do what is necessary for a resize refow. Now, we
only need to do this when it is doing PrintPreview and we need only do it for
HTML documents and NOT chrome.

At this time it doesn't appear to be any reason for setting the state back to
initial afterward.

Also, we do not want to call "GetAscent" when doing an Intial in PP.

Comment 11

[Chris Waterson]

I'm going to defer to dbaron & hyatt, since I've got one foot on a plane.

Comment 12

[Marc Attinasi]

Comment on attachment 65611 [details] [diff] [review]
full patch with optimization

The nice mondo comment has some typos - please re-read it and clean up the
grammar and spelling. Also, since you do set the state value back, you can
remove the part about not needing to do that.  

IsInitialReflowForPrintPreview should initialize the aIsChrome arg to PR_FALSE

Can you put an explanation of why you don't call GetAscent if this is the
initial PP reflow for non-chrome docs?

r=attinasi

Comment 13

[rods (gone)]

I fixed the comments, and I did set the aIsChrome to false but it is really
indeterminate.

Also, the best explaination I give for not doing the GetAscent, is that it
appears to have a vaild value from the r.height, and if called, it crashes.

Comment 14

[David Hyatt]

sr=hyatt

Comment 15

[Brendan Eich [:brendan]]

a=[e-mail] on drivers' behalf for checkin to 0.9.8.

/be

Comment 16

[rods (gone)]

fixed

Comment 17

[sujay]

Rod,  can you verify? thanks...

Comment 18

[sujay]

verified.