Closed
Bug 118968
Opened 23 years ago
Closed 23 years ago
Print Preview crashes on tinderbox
Categories
(Core :: Printing: Output, defect)
Tracking
()
VERIFIED
FIXED
mozilla0.9.8
People
(Reporter: rods, Assigned: rods)
Details
(Keywords: crash)
Attachments
(3 files, 2 obsolete files)
11.52 KB,
text/plain
|
Details | |
731 bytes,
patch
|
Details | Diff | Splinter Review | |
8.55 KB,
patch
|
attinasi
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Severity: normal → critical
Target Milestone: --- → mozilla0.9.8
Assignee | ||
Comment 4•23 years ago
|
||
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•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•23 years ago
|
||
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•23 years ago
|
||
I found a small issue with setting the cached value for skipping the reflow.
Attachment #65429 -
Attachment is obsolete: true
Comment 7•23 years ago
|
||
Attachment 65432 [details] [diff] (the reflow optimization) looks fine to me.
Assignee | ||
Comment 8•23 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•23 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•23 years ago
|
||
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•23 years ago
|
||
I'm going to defer to dbaron & hyatt, since I've got one foot on a plane.
Comment 12•23 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•23 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•23 years ago
|
||
sr=hyatt
Comment 15•23 years ago
|
||
a=brendan@mozilla.org on drivers' behalf for checkin to 0.9.8. /be
Blocks: 115520
Keywords: mozilla0.9.8+
Assignee | ||
Comment 16•23 years ago
|
||
fixed
No longer blocks: 115520
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Keywords: mozilla0.9.8+
Resolution: --- → FIXED
Comment 17•23 years ago
|
||
Rod, can you verify? thanks...
You need to log in
before you can comment on or make changes to this bug.
Description
•