318592 - [FIX]Crash with evil testcase, using float:right; and .u::first-letter { letter-spacing: 50px; } [@ nsQuoteList::Calc] [@ nsQuoteNode::DepthAfter] [@ nsCSSFrameConstructor::RemoveFirstLetterFrames]

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

See upcoming testcase.
It crashes for me when hovering over the text.
It doesn't crash always (but most of the time). If it doesn't crash directly try reloading and hover again over the text.

Doesn't crash in 2005-03-22 build, crashes in 2005-02-23 build:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-03-22+07%3A00%3A00&maxdate=2005-03-23+08%3A00%3A00&cvsroot=%2Fcvsroot
Maybe regression from bug 263825?

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Talkback ID: TB12474086G

Comment 2

[Ian Graham]

Also crashes Firefox 1.5 -- TB12496055E
(Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5)

Comment 3

[Felix Miata]

testcase also crashes OS/2 Deer Park

Comment 4

[Martijn Wargers (dead)]

From talkback ID:
nsQuoteNode::DepthAfter  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsQuoteList.h, line 94]
nsHTMLLinkElement::AddRef
0xf968e918

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase2 (crashes on load)

This is a different testcase that I minimised from a different crasher.
It has the same regression period, so probably the same bug.

Talkback data from TB12496055E:
nsCSSFrameConstructor::RemoveFirstLetterFrames  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 12779]
nsCSSFrameConstructor::RemoveLetterFrames  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 12845]
nsCSSFrameConstructor::ContentRemoved  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9814]
nsCSSFrameConstructor::ReinsertContent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9492]
PresShell::CharacterDataChanged  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 5398]
nsGenericDOMDataNode::SetData  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsGenericDOMDataNode.cpp, line 354]
nsCommentNode::SetData  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsCommentNode.cpp, line 59]
nsHTMLFormElement::AddRef
0xf9a8e918

Comment 6

[Martijn Wargers (dead)]

Maybe this is a bad crasher? (not only because it's a regression)

Comment 7

[Boris Zbarsky [:bzbarsky]]

So I don't know what happened on the day this regressed, but just tracing through this for the first testcase (or rather a more minimal version of it), I see the following:

1) When the display on the span changes, we reframe the closest block (since we're creating an {ib} split.  This means we end up with dirty quotes.
2) From EndUpdate, we process the dirty quotes.  This means calling SetText() on the anonymous text node for the quote (the end quote, to be exact).
3) We end up in nsCSSFrameConstructor::CharacterDataChanged
4) Since the text is a child of a letter frame, we actually remove and reinsert the content node in question to make things work right.
5) Reinserting the content node means inserting a child into the first inline in an {ib} split.
6) This makes nsCSSFrameConstructor::ContentInserted actually reinsert the block containing the whole {ib} split(!)
7) That causes us to remove that whole block from the frame tree, including the start quote, and hence to mark the quotes as dirty.

At that point we assert, have quote state that's not consistent with the actual frame tree, and later crash because we've not removed quotes that correspond to deleted frames.

I'd think that in step 6 we should perhaps be calling NeedSpecialFrameReframe or something, instead of just blindly nuking the world?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Somewhat reduced testcase

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Perhaps like this

This fixes the first and third testcase; the second seems to be a different bug (different asserts, etc).

I suppose I could have just checked for aInReinsertContent here, but this seemed like the more correct thing to do in general.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Martijn, could you file a separate bug on "testcase2", please?

Comment 11

[Martijn Wargers (dead)]

I filed bug 322820 for it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Thanks!

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 207962 [details] [diff] [review]
Perhaps like this

These changes seem fine to me -- I think much of this code doesn't make sense either before or after the changes, although I haven't looked closely enough to remember all the issues.

One question, though.  Should a call to NeedSpecialFrameReframe be followed by ReframeContainingBlock or ReinsertContent?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

OK, I finally sorted through this.  ReframeContainingBlock is probably more correct here, so this patch does that.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch version of the same thing

I think this should be reasonably safe to take on branch... This should only affect cases when we're dealing with first-letter, and it just makes deal with them more safely.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 17

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061106 Minefield/3.0a1

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 244617 [details] [diff] [review]
Branch version of the same thing

approved for 1.8/1.8.0 branch, a=dveditz for drivers

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8.0.9, 1.8.1.1

Comment 20

[juan becerra [:juanb]]

Still crashing on the first testcase using latest 1.8.0 and 1.8 (Talkback ID: TB26622448). On trunk it looks fine.

Comment 21

[Jay Patel [:jay]]

No crashes with latest 1.8.0 and 1.8 nightly builds for testcase2 and the somewhat reduced testcase.  Are those the ones we care most about?  

If so, we can mark this verified.  If not, and if the first testcase crash is a concern, we need to remove the "fixed1.8.x.x" keywords and have another look.

Comment 22

[Martijn Wargers (dead)]

(In reply to comment #21)
> If so, we can mark this verified.  If not, and if the first testcase crash is a
> concern, we need to remove the "fixed1.8.x.x" keywords and have another look.

Ok, I filed bug 362155 for the branch specific crash for the first testcase, so we can leave this bug alone if we want to.

Comment 23

[Jay Patel [:jay]]

v.fixed for last two testcases.  Martijn logged bug 362155 for the remaining crash with the first testcase.