Closed Bug 460389 Opened 17 years ago Closed 16 years ago

Crash [@ nsBlockFrame::DoRemoveFrame] with -moz-column, floating first letter, rtl

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jruderman, Unassigned)

References

(Depends on 1 open bug)

Details

(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical])

Crash Data

Attachments

(2 files, 2 obsolete files)

testcase (crashes Firefox when loaded)
217 bytes, text/html
Details
possible patch
1.27 KB, patch
Details | Diff | Splinter Review
###!!! ASSERTION: prev sibling not in line list: 'Not Reached', file /Users/jruderman/central/layout/generic/nsBlockFrame.cpp, line 4821 ###!!! ASSERTION: unknown out of flow frame type: 'disp->mDisplay == NS_STYLE_DISPLAY_POPUP', file /Users/jruderman/central/layout/generic/nsHTMLReflowState.cpp, line 503 ###!!! ASSERTION: Should have Reset() before destruction!: 'mCurrentWord.Length() == 0', file /Users/jruderman/central/content/base/src/nsLineBreaker.cpp, line 51 Crash: nsBlockFrame::DoRemoveFrame calls nsIFrame::GetNextSibling with this=0xdddddddd. These assertions also appear in bug 448615, bug 436982, and bug 429865. Beware: the testcase is made of pure, concentrated evil.
Whiteboard: [sg:critical?]
Crashes on Linux as well. Crash ID: 07314fae-9c12-11dd-afcb-001a4bd43ed6 Build ID: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081016 Minefield/3.1b2pre
OS: Mac OS X → All
Hardware: PC → All
Whiteboard: [sg:critical?] → [sg:critical]
Interestingly, *all* the assertions come before the disabling of the style sheet.
The first assertion is the result of something quite evil: #2 0x00007fab97e9da6d in nsBlockFrame::AddFrames (this=0x2ec3de8, aFrameList=0x2d2c8a0, aPrevSibling=0x2ec8328) at /home/dbaron/builds/mozilla-central/mozilla/layout/generic/nsBlockFrame.cpp:4815 #3 0x00007fab97e9dc73 in nsBlockFrame::InsertFrames (this=0x2ec3de8, aListName=0x1f998f8, aPrevFrame=0x2ec8328, aFrameList=0x2d2c8a0) at /home/dbaron/builds/mozilla-central/mozilla/layout/generic/nsBlockFrame.cpp:4754 #4 0x00007fab97e91a40 in SplitInlineAncestors (aFrame=<value optimized out>) at /home/dbaron/builds/mozilla-central/mozilla/layout/base/nsBidiPresUtils.cpp:146 Here, SplitInlineAncestors is splitting a *floating* :first-letter frame, and AddFrames is not happy about aPrevSibling being an out-of-flow frame rather than something in the line.
Attached patch possible patch — — Splinter Review
This fixes the crash, and the assertions other than the following two that remain: ###!!! ASSERTION: Should have been cleared: 'mBreakSinks.IsEmpty()', file /home/dbaron/builds/mozilla-central/mozilla/layout/generic/nsTextFrameThebes.cpp, line 619 ###!!! ASSERTION: Should have Reset() before destruction!: 'mCurrentWord.Length() == 0', file /home/dbaron/builds/mozilla-central/mozilla/content/base/src/nsLineBreaker.cpp, line 51 I need to look at what else other than SplitInlineAncestors uses eBidiInlineContainer and think about whether this is the right change for both SplitInlineAncestors and for any other callers.
The two remaining assertions are bug 448615.
Attached patch patch (obsolete) — — Splinter Review
I actually think it's correct to go with the more limited fix here; I think my previous patch is incorrect. I also added some reftests for the cases I was worried about breaking even with that (and I didn't break them).
Attachment #358675 - Flags: superreview?(roc)
Attachment #358675 - Flags: review?(uriber)
I should probably add borders to those reftests, though...
Attachment #358675 - Flags: superreview?(roc) → superreview+
Attached patch patch (obsolete) — — Splinter Review
Updating patch just to fix up the weird delete metadata that was in it from my renaming the reftests, so that nobody accidentally imports that stuff (which can really mess up a tree).
Assignee: nobody → dbaron
Attachment #358675 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #358736 - Flags: review?(uriber)
Attachment #358675 - Flags: review?(uriber)
Hmm. Actually, this seems to be causing layout/reftests/first-letter/399941-8.html to fail.
The other thing I need to think about is what should be conditioned on: * being a letter frame * being a letter frame that is for the :first-letter (rather than the continuation * being a floating letter frame (each is a subset of the previous)
Comment on attachment 358736 [details] [diff] [review] patch r+=me. IIRC we have several outstanding nasty issues with bidi and floating first-letter (bug 416831, bug 413085, bug 429865, bug 444484 come up in a quick search). It might be worth checking if/how this fix affects them.
Attachment #358736 - Flags: review+
For what it's worth, "possible patch" is identical to my patch in bug 429865, which as Simon pointed out is probably not the right thing to do... Definitely worth checking whether this patch fixes that bug too.
Blocks: 429865
So the reason for the regression is that nsFirstLetterFrame::Reflow assumes that the first letter frame has only one child. In this case, that's wrong for the continuation part. But I could also imagine it being wrong for the first-letter part.
... and in the normal case that works because we only extend the first letter continuation to the end of the first text node. Fixing bug 365131 would make this a lot easier.
I'm putting this back in the nobody pile; I really don't know how to proceed here.
Assignee: dbaron → nobody
Thoughts in slightly more detail: we could teach nsFirstLetterFrame to reflow more than one child, but I suspect that would break some of the basic cases, since the first-letter boundaries are found during reflow, not frame construction (I think, at least for the non-floating cases). I'm also not really sure what :first-letter should do in some of these bidi cases. It would probably help if we figured out what was in the first letter at frame construction time, then allowed :first-letter to have more than one child, and made at least non-floating first-letters derived from inline frames. I'm also not sure to what extent we're following CSS 2.1 and the current css3-selectors draft versus the probably-better proposal in the 2002 CR of css3-selectors (I think it was in that draft). Or maybe that difference in how the pseudo-element nested with the real elements was for first-line, not first letter. I don't really remember.
That said, I'll land the tests from the patch as with-first-letter-*.
Status: ASSIGNED → NEW
Bug 491547 has a lot in common with this. The patch here prevents the crash there, but causes a regression in rendering, for the same reason as described in comment 13. The testcase there is in fact an example of the first-letter part of a first-letter frame having more than one child. Normally that doesn't occur because of another bug: we don't create a first-letter that crosses text run boundaries (bug 393985).
Still crashes on trunk.
Blocks: 491547
Doesn't seem to crash trunk anymore, but still triggers a bunch of assertions. dbaron, any current thoughts on first-letter in frame construction vs. reflow? ###!!! ASSERTION: prev sibling not in line list: 'Not Reached', file /build/m-c/src/layout/generic/nsBlockFrame.cpp, line 4748 ###!!! ASSERTION: unknown out of flow frame type: 'disp->mDisplay == NS_STYLE_DISPLAY_POPUP', file /build/m-c/src/layout/generic/nsHTMLReflowState.cpp, line 529 ###!!! ASSERTION: Should have been cleared: 'mBreakSinks.IsEmpty()', file /build/m-c/src/layout/generic/nsTextFrameThebes.cpp, line 650 ###!!! ASSERTION: Should have been cleared: 'mBreakSinks.IsEmpty()', file /build/m-c/src/layout/generic/nsTextFrameThebes.cpp, line 650 ###!!! ASSERTION: Should have Reset() before destruction!: 'mCurrentWord.Length() == 0', file /build/m-c/src/content/base/src/nsLineBreaker.cpp, line 51 ###!!! ASSERTION: unknown out of flow frame type: 'disp->mDisplay == NS_STYLE_DISPLAY_POPUP', file /build/m-c/src/layout/generic/nsHTMLReflowState.cpp, line 529 ###!!! ASSERTION: unknown out of flow frame type: 'disp->mDisplay == NS_STYLE_DISPLAY_POPUP', file /build/m-c/src/layout/generic/nsHTMLReflowState.cpp, line 529 ###!!! ASSERTION: unknown out of flow frame type: 'disp->mDisplay == NS_STYLE_DISPLAY_POPUP', file /build/m-c/src/layout/generic/nsHTMLReflowState.cpp, line 529 ###!!! ASSERTION: unknown out of flow frame type: 'disp->mDisplay == NS_STYLE_DISPLAY_POPUP', file /build/m-c/src/layout/generic/nsHTMLReflowState.cpp, line 529 ###!!! ASSERTION: unknown out of flow frame type: 'disp->mDisplay == NS_STYLE_DISPLAY_POPUP', file /build/m-c/src/layout/generic/nsHTMLReflowState.cpp, line 529 ###!!! ASSERTION: out-of-flow on wrong child list: '!(childFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)', file /build/m-c/src/layout/base/nsCSSFrameConstructor.cpp, line 6965
No longer blocks: 491547
Depends on: 491547
Should be fixed by bug 491547.
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Crash Signature: [@ nsBlockFrame::DoRemoveFrame]
Can this be opened up? It was fixed a long time ago now.
Group: core-security
Added crashtest https://hg.mozilla.org/integration/mozilla-inbound/rev/058768a8c209
Flags: in-testsuite? → in-testsuite+
Depends on: 780985
Since the fix for this landed an assert was landed in bug 508473 that this testcase fails. So I marked it as asserting in the manifest and filed bug 780985 to cover fixing the assert. https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6bc1dcd073 https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d35dd85c62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: