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

RESOLVED FIXED in mozilla1.9beta3

Status

()

Core
Layout
P4
critical
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: mats)

Tracking

(Blocks: 1 bug, {assertion, crash, testcase})

Trunk
mozilla1.9beta3
assertion, crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 293309 [details]
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
(Assignee)

Comment 1

11 years ago
Created attachment 293342 [details]
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
(Assignee)

Updated

11 years ago
Flags: in-testsuite?
Flags: blocking1.9?
OS: Mac OS X → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9 M11
(Assignee)

Comment 2

11 years ago
Created attachment 293343 [details] [diff] [review]
Patch rev. 1
(Assignee)

Comment 3

11 years ago
Created attachment 293344 [details] [diff] [review]
Patch rev. 1 (diff -w)

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+

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
(Assignee)

Comment 5

11 years ago
Created attachment 293472 [details]
Testcase #2 (doesn't crash, demonstrates bug in RecoverLetterFrames)
(Assignee)

Comment 6

11 years ago
Created attachment 293473 [details] [diff] [review]
reftest.diff (covers both issues)
(Assignee)

Comment 7

11 years ago
Created attachment 293474 [details] [diff] [review]
Patch rev. 2
(Assignee)

Comment 8

11 years ago
Created attachment 293479 [details] [diff] [review]
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)
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+
(Assignee)

Comment 12

11 years ago
(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?)
(Assignee)

Comment 13

11 years ago
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
Last Resolved: 11 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.