Closed Bug 362210 Opened 13 years ago Closed 10 years ago

[reflow branch] Crash on closing print preview when at least two pages are involved

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: sharparrow1)

References

(Depends on 1 open bug)

Details

(Keywords: crash, testcase)

Attachments

(4 files)

I crash on closing print preview when at least 2 pages are shown on print preview.

I tried to get a backtrace with a debug build, but I didn't manage to get a useful stack.
Attached file testcase
Assignee: nobody → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
Attached patch wipSplinter Review
It seems we have flipped the DIRTY bit test compared to trunk:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsPageFrame.cpp&rev=3.162&root=/cvsroot&mark=102#83
This fixes the crash but it still doesn't print more than one page...
Attached patch Patch rev. 1Splinter Review
This makes more sense I think.
Attachment #246939 - Flags: superreview?(bzbarsky)
Attachment #246939 - Flags: review?(bzbarsky)
I don't think I know enough about how our overflow list stuff works to review this quickly... maybe try roc?
Everywhere else we do things differently: when frame A reflows child B, and B is incomplete, then A creates the continuation for B (say B') and then puts B' on A's overflow list. I think we should do that here too, where A is an nsSimplePageSequenceFrame and B is the nsPageFrame.
Attachment #246939 - Flags: superreview?(bzbarsky)
Attachment #246939 - Flags: review?(bzbarsky)
(In reply to comment #2)
> It seems we have flipped the DIRTY bit test compared to trunk:

That's not flipped.  We now have a dirty bit set if we would have, in the past, had a StyleChange or Initial reflow reason, or maybe also a Dirty reflow reason, depending on what Dirty was used for on that particular frame (since Dirty basically meant "custom", if my memory is corrrect).

What if you remove the test entirely?
Attached patch Minimal patchSplinter Review
This fix isn't correct in the general case, but it's good enough to fix print preview.

I'm not completely sure when this isn't equivalent to the dirty test for print preview.  For print preview, there should be an initial reflow, and after that nothing should get marked dirty. We certainly shouldn't be creating a continuation twice, though, so I think this preserves what the test was supposed to do.
Attachment #248150 - Flags: review?(dbaron)
*** Bug 363272 has been marked as a duplicate of this bug. ***
See Bug 363272 for stack traces.
*** Bug 363382 has been marked as a duplicate of this bug. ***
Comment on attachment 248150 [details] [diff] [review]
Minimal patch

I think we should take this as wallpaper for now at least, so we
don't block further Print Preview testing and stop the dupes.
We can leave the bug open to implement what roc suggested in comment 5 later.
r=mats
Attachment #248150 - Flags: superreview?(roc)
Attachment #248150 - Flags: review?(dbaron)
Attachment #248150 - Flags: review+
Attachment #248150 - Flags: superreview?(roc) → superreview+
Comment on attachment 248150 [details] [diff] [review]
Minimal patch

Landed on trunk 2006-12-12 01:54 PST.
Flags: blocking1.9?
re: blocking1.9? flag:

The wallpaper isn't great, but fixing it propertly isn't a high priority.  There isn't really much to gain until we try to implement dynamic pagination.
Ah, ok, Mats mentioned the patch was wallpaper, so I thought we desperately wanted to have a better fix for this.
Flags: blocking1.9? → blocking1.9-
Maybe it's just better to mark this fixed and file a new bug whatever is meant in comment 5?
sounds like this bug should get marked fixed, and a new low priority bug ought to get opened up.
I looked at the current code and it does what is said in comment 5.
Marking this bug fixed, and filed bug 511536 for the improvement
mentioned in comment 7.
Assignee: matspal → sharparrow1
Status: NEW → RESOLVED
Closed: 10 years ago
Depends on: 511536
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.