Print Preview crashes on tinderbox

VERIFIED FIXED in mozilla0.9.8



Printing: Output
16 years ago
16 years ago


(Reporter: rods (gone), Assigned: rods (gone))



Windows 2000

Firefox Tracking Flags

(Not tracked)



(3 attachments, 2 obsolete attachments)



16 years ago

Comment 1

16 years ago
Created attachment 64135 [details]
stack trace


16 years ago
Severity: normal → critical
Target Milestone: --- → mozilla0.9.8


16 years ago
Keywords: crash

Comment 2

16 years ago
Target Milestone: mozilla0.9.8 → mozilla0.9.9

Comment 3

16 years ago
Assignee: karnaze → rods

Comment 4

16 years ago
Created attachment 65429 [details] [diff] [review]

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.


16 years ago

Comment 5

16 years ago
Created attachment 65432 [details] [diff] [review]
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
> *** Registering nsLayoutModule components (all right -- a generic module!)
<	nsBoxToBlockAdaptor	0	47	6	0	0	53
>	nsBoxToBlockAdaptor	0	36	6	0	0	42
<	      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
<	       nsBlockFrame	32	0	4	0	0	36
>	       nsBlockFrame	22	0	4	0	0	26
<		 nsBoxFrame	1	52	2	0	0	55
>		 nsBoxFrame	1	47	2	0	0	50
<	       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

16 years ago
Created attachment 65435 [details] [diff] [review]
replaces first patch (nsSimplePageSeq.cpp)

I found a small issue with setting the cached value for skipping the reflow.
Attachment #65429 - Attachment is obsolete: true

Comment 7

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

Comment 8

16 years ago
This fix keeps us from crashing by filtering out the extra initial reflows. I
have filed a bug on the real problem: Bug 120652
Target Milestone: mozilla0.9.9 → mozilla0.9.8

Comment 9

16 years ago
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

16 years ago
Created attachment 65611 [details] [diff] [review]
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.
Attachment #65435 - Attachment is obsolete: true

Comment 11

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

Comment 12

16 years ago
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?

Attachment #65611 - Flags: review+

Comment 13

16 years ago
I fixed the comments, and I did set the aIsChrome to false but it is really

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

16 years ago
sr=hyatt on drivers' behalf for checkin to 0.9.8.

Blocks: 115520
Keywords: mozilla0.9.8+

Comment 16

16 years ago
No longer blocks: 115520
Last Resolved: 16 years ago
Keywords: mozilla0.9.8+
Resolution: --- → FIXED

Comment 17

16 years ago
Rod,  can you verify? thanks...

Comment 18

16 years ago
You need to log in before you can comment on or make changes to this bug.