408493 - "ASSERTION: null frame is not allowed" and abort [@ IsPercentageAware] with :first-letter, moz-column

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase kills Firefox.

###!!! ASSERTION: null frame is not allowed: 'aFrame', file /Users/jruderman/trunk/mozilla/layout/generic/nsLineLayout.cpp, line 675

###!!! ABORT: file /Users/jruderman/trunk/mozilla/layout/generic/nsLineLayout.cpp, line 677

Comment 1

[Mats Palmgren (inactive)]

Attachment: Frame dumps

The problem is that the letter frame isn't in the first continuation
of the block, so RemoveLetterFrames() doesn't find it.
ContentRemoved() is trying to remove the text frame (blue), and it
succeeds but the problem is that 'parentFrame' is still the letter
frame (red) so we're leaving a letter frame without a child...
which causes a null pointer crash later when reflowed.

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (diff -w)

Look in all continuations for the letter frame.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 293344 [details] [diff] [review]
Patch rev. 1 (diff -w)

Makes sense, but do we need something similar in RecoverLetterFrames?

Comment 5

[Mats Palmgren (inactive)]

Attachment: Testcase #2 (doesn't crash, demonstrates bug in RecoverLetterFrames)

Comment 6

[Mats Palmgren (inactive)]

Attachment: reftest.diff (covers both issues)

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -w)

(In reply to comment #4)
> Makes sense, but do we need something similar in RecoverLetterFrames?

Good point, this fixes RecoverLetterFrames() in the same way.
(No need to review RemoveLetterFrames() again, it's the same as in rev. 1)

BTW, do we really need AddStateBits(NS_BLOCK_HAS_FIRST_LETTER_STYLE)
in RecoverLetterFrames()?  isn't something seriously wrong if we're
trying to recover ::first-letter in a frame that's missing that bit?
(it was added in bug 362901)

Comment 9

[Boris Zbarsky [:bzbarsky]]

> BTW, do we really need AddStateBits(NS_BLOCK_HAS_FIRST_LETTER_STYLE)
> in RecoverLetterFrames()?

At the time, I think we did.  We might have cleaned up things since then; can we turn that into an assert instead?

Comment 10

[Boris Zbarsky [:bzbarsky]]

For the reftests, the about:blank one should just be a crashtest, right?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 293479 [details] [diff] [review]
Patch rev. 2 (diff -w)

r+sr=bzbarsky

Comment 12

[Mats Palmgren (inactive)]

(In reply to comment #9)
> At the time, I think we did.  We might have cleaned up things since then; can
> we turn that into an assert instead?

I'll add an XXX comment with that question in the code for now...

(In reply to comment #10)
> For the reftests, the about:blank one should just be a crashtest, right?

I considered that, but even though we crashed this time we might not do
so in the future.  Comparing with about:blank catches a future bug
where we fail to remove the frame but does not crash for some reason.
(unlikely, but regression tests shouldn't exclude unlikely things, right?)

Comment 13

[Mats Palmgren (inactive)]

mozilla/layout/base/nsCSSFrameConstructor.cpp 	1.1451
mozilla/layout/reftests/bugs/408493-1.html 	1.1
mozilla/layout/reftests/bugs/408493-2.html 	1.1
mozilla/layout/reftests/bugs/408493-2-ref.html 	1.1
mozilla/layout/reftests/bugs/reftest.list 	1.318 

-> FIXED