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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

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.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Mats Palmgren (inactive)]

Attachment: wip

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...

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This makes more sense I think.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I don't think I know enough about how our overflow list stuff works to review this quickly... maybe try roc?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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?

Comment 7

[Eli Friedman]

Attachment: Minimal patch

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.

Comment 8

[Eli Friedman]

*** Bug 363272 has been marked as a duplicate of this bug. ***

Comment 9

[Frank Wein [:mcsmurf]]

See Bug 363272 for stack traces.

Comment 10

[Martijn Wargers (dead)]

*** Bug 363382 has been marked as a duplicate of this bug. ***

Comment 11

[Mats Palmgren (inactive)]

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

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 248150 [details] [diff] [review]
Minimal patch

Landed on trunk 2006-12-12 01:54 PST.

Comment 13

[Eli Friedman]

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.

Comment 14

[Martijn Wargers (dead)]

Ah, ok, Mats mentioned the patch was wallpaper, so I thought we desperately wanted to have a better fix for this.

Comment 15

[Martijn Wargers (dead)]

Maybe it's just better to mark this fixed and file a new bug whatever is meant in comment 5?

Comment 16

[chris hofmann]

sounds like this bug should get marked fixed, and a new low priority bug ought to get opened up.

Comment 17

[Mats Palmgren (inactive)]

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.