Closed Bug 408493 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: Layout, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: jruderman, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, crash, testcase)

Crash Data

Attachments

(6 files, 2 obsolete files)

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
Attached file 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.
Assignee: nobody → mats.palmgren
Status: NEW → ASSIGNED
Flags: in-testsuite?
Flags: blocking1.9?
OS: Mac OS X → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9 M11
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Attached patch Patch rev. 1 (diff -w) (obsolete) — Splinter Review
Look in all continuations for the letter frame.
Attachment #293344 - Flags: superreview?(bzbarsky)
Attachment #293344 - Flags: review?(bzbarsky)
Comment on attachment 293344 [details] [diff] [review]
Patch rev. 1 (diff -w)

Makes sense, but do we need something similar in RecoverLetterFrames?
Attachment #293344 - Flags: superreview?(bzbarsky)
Attachment #293344 - Flags: superreview+
Attachment #293344 - Flags: review?(bzbarsky)
Attachment #293344 - Flags: review+
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
Attached patch Patch rev. 2Splinter Review
(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)
Attachment #293343 - Attachment is obsolete: true
Attachment #293344 - Attachment is obsolete: true
Attachment #293479 - Flags: superreview?(bzbarsky)
Attachment #293479 - Flags: review?(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?
For the reftests, the about:blank one should just be a crashtest, right?
Comment on attachment 293479 [details] [diff] [review]
Patch rev. 2 (diff -w)

r+sr=bzbarsky
Attachment #293479 - Flags: superreview?(bzbarsky)
Attachment #293479 - Flags: superreview+
Attachment #293479 - Flags: review?(bzbarsky)
Attachment #293479 - Flags: review+
(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?)
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Depends on: 436392
Crash Signature: [@ IsPercentageAware]
You need to log in before you can comment on or make changes to this bug.