Bugzilla@Mozilla – Bug 457514
Crash [@ NeedFrameFor] with floating :first-letter
Last modified: 2010-02-13 12:16:13 PST
Summon comment box
Created attachment 340777 [details] testcase This testcase triggers ###!!! ASSERTION: Should have Reset() before destruction!: 'mCurrentWord.Length() == 0', file /Users/jruderman/central/content/base/src/nsLineBreaker.cpp, line 51 (like bug 448615), then crashes [@ NeedFrameFor].
OK. So we crash in NeedFrameFor because aParentFrame is 0xdddddddd. No idea why that is, yes.
the testcase reports in !exploitable > > Exploitability Classification: PROBABLY_EXPLOITABLE > Recommended Bug Title: Probably Exploitable - Data from Faulting Address > controls Code Flow starting at gklayout!NeedFrameFor+0x11 > (Hash=0x67336e00.0x705c1723) > > The data from the faulting address is later used as the target for a branch.
This still crashes trunk, but with a slightly different stack. Using a trunk debug build from 20090929 I hit two assertions (same one as Jesse plus a second) before the crash, which now happens in ContentAppended: ###!!! 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 Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0xf0dea91f 0x10e6565a in nsCSSFrameConstructor::ContentAppended (this=0xf131320, aContainer=0x150fa530, aNewIndexInContainer=2) at /build/m-c/src/layout/base/nsCSSFrameConstructor.cpp:6344 6344 nsIAtom* frameType = parentFrame->GetType();
Created attachment 406414 [details] [review] patch The assertions are a separate issue; they can be triggered without the crash, and the crash can happen without the assertions. This is to fix the crash. When we insert the "T", the parent frame gets set to the first letter frame in GetInsertionPrevSibling. Then when we adjust the parent for first-letter style, we use its parent which is the containing block, and so we end up trying to insert the "T" with the div as the parent and it ends up just wrong. Instead we need to get the parent of the placeholder frame and use that as parent.
Bug 448615 covers the assertions, I think.
Comment on attachment 406414 [details] [review] patch Ah, nice catch! r=bzbarsky
We'll want this on all branches.
Created attachment 406892 [details] [review] patch for 1.9.1 and 1.9.0 Boris, can you give this a quick once over?
Comment on attachment 406892 [details] [review] patch for 1.9.1 and 1.9.0 Looks good.
Is there any reason this hasn't landed on mozilla-central for testing yet?
http://hg.mozilla.org/mozilla-central/rev/1d8209258b30
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cc13042643ca
Reminder to myself to add bug 503936 as crashtest when landing this on the remaining branches.
Comment on attachment 406892 [details] [review] patch for 1.9.1 and 1.9.0 sr=dveditz Approved for 1.9.1.6 and 1.9.0.16, a=dveditz for release-drivers
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f6d7973c6182 And the test from bug 503936 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/59a416680c6c
Checking in nsCSSFrameConstructor.cpp; /cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v <-- nsCSSFrameConstructor.cpp new revision: 1.1488; previous revision: 1.1487 Checking in layout/base/crashtests/503936-1.html; /cvsroot/mozilla/layout/base/crashtests/503936-1.html,v <-- 503936-1.html initial revision: 1.1 Checking in layout/base/crashtests/crashtests.list; /cvsroot/mozilla/layout/base/crashtests/crashtests.list,v <-- crashtests.list new revision: 1.103; previous revision: 1.102
Verified for 1.9.1.6 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.6pre) Gecko/20091110 Shiretoko/3.5.6pre using the testcase. Testcase crashes with 1.9.1.5 easily.
Verified for 1.9.0.16 using testcase and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2009111921 GranParadiso/3.0.16pre (.NET CLR 3.5.30729). Crashes 1.9.0.15 cleanly.
Does not affect 1.8.1.22