70730 - Printing page crashes Mozilla

Status: VERIFIED FIXED

(Core :: Printing: Output, defect, P1)

Description

[Oliver Klee]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; 0.9) Gecko/20010301
BuildID:    2001030110

Trying to print this page crashes Mozilla (before anything gets printed).

Reproducible: Always

Steps to Reproduce:
1. Visit URL.
2. Print the page ("All").

Actual Results:  Crash.

Expected Results:  No crash, and the page printed.

Mozilla does not crash and prints ok if I print only the selection.

Comment 1

[Oliver Klee]

Attachment: Dr. Watson log.

Comment 2

[Oliver Klee]

Adding crash keyword.

Comment 3

[Oliver Klee]

Attachment: Slightly reduced testcase that still crashes

Comment 4

[Boris Zbarsky [:bzbarsky]]

crashes in Linux as well, with a CVS build from 2001-03-02.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Linux stack trace

Comment 6

[dcone (gone)]

Attachment: reduced test case

Comment 7

[dcone (gone)]

Here is the bug.. if I can help let me know.

Comment 8

[Marc Attinasi]

Thanks for the testcase. 

Comment 9

[Marc Attinasi]

Looks like there is some case where a line with the state bit
NS_BLOCK_FRAME_HAS_OUTSIDE_BULLET does not have an mBullet data member set.
Interestingly, there appears to be some inconsistency between testing for
mBullet and HasOutsideBullet (which tests for the state bit
NS_BLOCK_FRAME_HAS_OUTSIDE_BULLET). If I check for the mBullet in addition to
the HasOutsideBullet() check I can get rid of the crash (and the printout looks
correct!), but I'd like to learn why this discrepency exists.

Comment 10

[Marc Attinasi]

Attachment: patch to stop crash: checks for mBullet in addition to HaveOutsideBullet()

Comment 11

[Marc Attinasi]

Don, can you please review the patch? I'm still looking to see why it is
happening that the flag for having a bullet and the data member are out of
synch, but this fixes the crash and makes a good printout (I tested the testcase
only).

Comment 12

[dcone (gone)]

Patch looks good.  r=dcone.
thanks marc

Comment 13

[Marc Attinasi]

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

Comment 14

[Marc Attinasi]

Further refinement of patch: Added assertion in HaveOutsideBullet to detect as
early as possible when the mBullet and the NS_BLOCK_FRAME_HAS_OUTSIDE_BULLET
flag get out of whack.

Chris, can you please take a look and possibly grant me your sr=? Thanks.

Comment 15

[Marc Attinasi]

Attachment: Refined patch: added assertion to HaveOutsideBullet and some more mBullet checks

Comment 16

[Chris Waterson]

sr=waterson. Any clue why we're getting out of sync?

Comment 17

[Marc Attinasi]

Not yet, but it only seems to happen in printing. I'm guessing so far that it is
a timing issue: the bullet frame is getting destroyed before the line somehow -
this is because the printout has all of the bullets anyway, but that is just a
guess.

Comment 18

[Marc Attinasi]

Fix checked in: nsBlockFrame.h/cpp

Comment 19

[Chris Waterson]

cf. bug 71181

Comment 20

[Chris Waterson]

Had to revert changes due to bug 71181.

Comment 21

[Marc Attinasi]

Accepting again.

Comment 22

[Marc Attinasi]

OK, I made a really bad assumption about mBullet in the Destroy() method. Prior
to my change, the Destroy method was only deleting the mBullet member if the
HAS_OUTSIDE_BULLET flag is set in the frame's state bits. My change was to
always delete mBullet if non-null. However, the mBullet is also in the frame's
child list if it is an INSIDE bullet, so this was causing a double delete (once
when the child frames were cleaned up, once when mBullet itself was deleted). So
the proper fix is simply to check for HAS_OUTSIDE_BULLET && mBullet in Destroy,
rather than just checking mBullet.

There is still a mystery to solve around why we sometimes get a case of the
HAS_OUTSIDE_BULLET being set but mBullet is null, and I have put in an assertion
to try and detect that early, but I still do not know what is up there (and it
appears to only happen in printing AFAICT).

Comment 23

[Marc Attinasi]

Attachment: New Patch to fix crash (and avoid regressions...)

Comment 24

[Chris Waterson]

Is the last patch in lieu of the other patch? Or in addition to it? If the last 
patch is The Whole Patch, it looks ok to me, (s)r=waterson. If both patches are 
req'd, then lemme know; I'll need to grok it better...

Comment 25

[Marc Attinasi]

Attachment: General Test Case (inside and outside bullets)

Comment 26

[karnaze (gone)]

Moving to mozilla0.9

Comment 27

[Marc Attinasi]

Fixed (again).

Comment 28

[Boris Zbarsky [:bzbarsky]]

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

Comment 29

[sujay]

verified in 3/20 build.

Comment 30

[Oliver Klee]

This now works for me too.

Comment 31

[Chris Waterson]

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