Closed
Bug 318592
Opened 19 years ago
Closed 18 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)
Tracking
()
VERIFIED
FIXED
mozilla1.9alpha1
People
(Reporter: martijn.martijn, Assigned: bzbarsky)
References
Details
(5 keywords)
Crash Data
Attachments
(5 files, 1 obsolete file)
577 bytes,
text/html
|
Details | |
754 bytes,
text/html
|
Details | |
345 bytes,
text/html
|
Details | |
8.78 KB,
patch
|
Details | Diff | Splinter Review | |
7.90 KB,
patch
|
dveditz
:
approval1.8.0.9+
dveditz
:
approval1.8.1.1+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Talkback ID: TB12474086G
Comment 2•19 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)
Reporter | ||
Comment 4•19 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•19 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•19 years ago
|
Blocks: ajax-demolisher
Reporter | ||
Comment 6•19 years ago
|
||
Maybe this is a bad crasher? (not only because it's a regression)
Assignee | ||
Comment 7•19 years ago
|
||
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?
Assignee | ||
Comment 8•19 years ago
|
||
Assignee | ||
Comment 9•19 years ago
|
||
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)
Assignee | ||
Comment 10•19 years ago
|
||
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•19 years ago
|
||
I filed bug 322820 for it.
Assignee | ||
Comment 12•19 years ago
|
||
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•19 years ago
|
Flags: blocking1.9a1?
Updated•19 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+
Assignee | ||
Comment 14•18 years ago
|
||
OK, I finally sorted through this. ReframeContainingBlock is probably more correct here, so this patch does that.
Attachment #207962 -
Attachment is obsolete: true
Assignee | ||
Comment 15•18 years ago
|
||
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?
Assignee | ||
Comment 16•18 years ago
|
||
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 17•18 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 18•18 years ago
|
||
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+
Comment 20•18 years ago
|
||
Still crashing on the first testcase using latest 1.8.0 and 1.8 (Talkback ID: TB26622448). On trunk it looks fine.
Comment 21•18 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•18 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•18 years ago
|
||
v.fixed for last two testcases. Martijn logged bug 362155 for the remaining crash with the first testcase.
Updated•14 years ago
|
Crash Signature: [@ nsQuoteList::Calc]
[@ nsQuoteNode::DepthAfter]
[@ nsCSSFrameConstructor::RemoveFirstLetterFrames]
You need to log in
before you can comment on or make changes to this bug.
Description
•