Print Preview crashes on tinderbox

VERIFIED FIXED in mozilla0.9.8

Status

()

Core
Printing: Output
--
critical
VERIFIED FIXED
16 years ago
16 years ago

People

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

Tracking

({crash})

Trunk
mozilla0.9.8
x86
Windows 2000
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

16 years ago
 
(Assignee)

Comment 1

16 years ago
Created attachment 64135 [details]
stack trace
(Assignee)

Updated

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

Updated

16 years ago
Keywords: crash

Comment 2

16 years ago
->m099
Target Milestone: mozilla0.9.8 → mozilla0.9.9
(Assignee)

Comment 3

16 years ago
taking
Assignee: karnaze → rods
(Assignee)

Comment 4

16 years ago
Created attachment 65429 [details] [diff] [review]
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.
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
(Assignee)

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
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!
(Assignee)

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.
(Assignee)

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.
(Assignee)

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?

r=attinasi
Attachment #65611 - Flags: review+
(Assignee)

Comment 13

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

16 years ago
sr=hyatt
a=brendan@mozilla.org on drivers' behalf for checkin to 0.9.8.

/be
Blocks: 115520
Keywords: mozilla0.9.8+
(Assignee)

Comment 16

16 years ago
fixed
No longer blocks: 115520
Status: ASSIGNED → RESOLVED
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
verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.