Closed Bug 318592 Opened 15 years ago Closed 14 years ago

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

Categories

(Core :: Layout, defect, P1)

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: martijn.martijn, Assigned: bzbarsky)

References

Details

(5 keywords)

Crash Data

Attachments

(5 files, 1 obsolete file)

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?
Attached file testcase
Talkback ID: TB12474086G
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)
testcase also crashes OS/2 Deer Park
OS: Windows XP → All
From talkback ID:
nsQuoteNode::DepthAfter  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsQuoteList.h, line 94]
nsHTMLLinkElement::AddRef
0xf968e918
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
Maybe this is a bad crasher? (not only because it's a regression)
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?
Attached patch Perhaps like this (obsolete) — Splinter Review
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #207962 - Flags: superreview?(dbaron)
Attachment #207962 - Flags: review?(dbaron)
Martijn, could you file a separate bug on "testcase2", please?
Priority: -- → P1
Summary: Crash with evil testcase, using float:right; and .u::first-letter { letter-spacing: 50px; } → [FIX]Crash with evil testcase, using float:right; and .u::first-letter { letter-spacing: 50px; }
Target Milestone: --- → mozilla1.9alpha
I filed bug 322820 for it.
Thanks!
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?
Attachment #207962 - Flags: superreview?(dbaron)
Attachment #207962 - Flags: superreview+
Attachment #207962 - Flags: review?(dbaron)
Attachment #207962 - Flags: review+
Flags: blocking1.9a1?
Summary: [FIX]Crash with evil testcase, using float:right; and .u::first-letter { letter-spacing: 50px; } → [FIX]Crash with evil testcase, using float:right; and .u::first-letter { letter-spacing: 50px; } [@ nsQuoteList::Calc] [@ nsQuoteNode::DepthAfter] [@ nsCSSFrameConstructor::RemoveFirstLetterFrames]
Flags: blocking1.9a1? → blocking1.9+
OK, I finally sorted through this.  ReframeContainingBlock is probably more correct here, so this patch does that.
Attachment #207962 - Attachment is obsolete: true
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.
Attachment #244617 - Flags: approval1.8.1.1?
Attachment #244617 - Flags: approval1.8.0.9?
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061106 Minefield/3.0a1
Status: RESOLVED → VERIFIED
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
Attachment #244617 - Flags: approval1.8.1.1?
Attachment #244617 - Flags: approval1.8.1.1+
Attachment #244617 - Flags: approval1.8.0.9?
Attachment #244617 - Flags: approval1.8.0.9+
Fixed for 1.8.0.9, 1.8.1.1
Still crashing on the first testcase using latest 1.8.0 and 1.8 (Talkback ID: TB26622448). On trunk it looks fine.
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.
(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.
v.fixed for last two testcases.  Martijn logged bug 362155 for the remaining crash with the first testcase.
Blocks: 349095
Blocks: 342120
Crash Signature: [@ nsQuoteList::Calc] [@ nsQuoteNode::DepthAfter] [@ nsCSSFrameConstructor::RemoveFirstLetterFrames]
You need to log in before you can comment on or make changes to this bug.