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

VERIFIED FIXED in mozilla1.9alpha1

Status

()

defect
P1
critical
VERIFIED FIXED
14 years ago
8 years ago

People

(Reporter: martijn.martijn, Assigned: bzbarsky)

Tracking

(5 keywords)

Trunk
mozilla1.9alpha1
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

14 years ago
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?
(Reporter)

Comment 1

14 years ago
Posted file testcase
Talkback ID: TB12474086G

Comment 2

14 years ago
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

14 years ago
testcase also crashes OS/2 Deer Park
OS: Windows XP → All
(Reporter)

Comment 4

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

Comment 5

14 years ago
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
(Reporter)

Updated

14 years ago
(Reporter)

Comment 6

14 years ago
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?
Posted 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
(Reporter)

Comment 11

14 years ago
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+

Updated

13 years ago
Flags: blocking1.9a1?

Updated

13 years ago
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
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Reporter)

Comment 17

13 years ago
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.

Comment 21

13 years ago
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.
(Reporter)

Comment 22

13 years ago
(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

13 years ago
v.fixed for last two testcases.  Martijn logged bug 362155 for the remaining crash with the first testcase.
(Reporter)

Updated

12 years ago
Blocks: 349095
(Reporter)

Updated

12 years ago
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.