Last Comment Bug 457514 - Crash [@ NeedFrameFor] with floating :first-letter
: Crash [@ NeedFrameFor] with floating :first-letter
Status: RESOLVED FIXED
: [sg:critical?]
: assertion, crash, testcase, verified1.9.0.16, verified1.9.1
Product: Core
Classification: Components
Component: Layout: Floats
: Trunk
: x86 Mac OS X
: P2 critical (vote)
: mozilla1.9.2
Assigned To: Timothy Nikkel (:tn)
: layout.floats
:
:
: 306663 503936
  Show dependency treegraph
 
Reported: 2008-09-28 01:41 PDT by Jesse Ruderman
Modified: 2010-02-13 12:16 PST (History)
18 users (show)
roc: wanted1.9.2+
roc: wanted1.9.1+
dveditz: blocking1.9.0.16+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
tnikkel: in‑testsuite?
See Also:
Crash Signature:
[@ NeedFrameFor]
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta2-fixed
  .6+
  .6-fixed


Attachments
testcase (488 bytes, text/html)
2008-09-28 01:41 PDT, Jesse Ruderman
no flags Details
patch (1.60 KB, patch)
2009-10-15 02:24 PDT, Timothy Nikkel (:tn)
bzbarsky: review+
roc: approval1.9.2+
Details | Diff | Splinter Review
patch for 1.9.1 and 1.9.0 (1.57 KB, patch)
2009-10-17 19:04 PDT, Timothy Nikkel (:tn)
bzbarsky: review+
dveditz: superreview+
dveditz: approval1.9.1.6+
dveditz: approval1.9.0.16+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2008-09-28 01:41:11 PDT
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].
Comment 1 Boris Zbarsky (:bz) 2008-11-14 17:48:38 PST
OK.  So we crash in NeedFrameFor because aParentFrame is 0xdddddddd.  No idea why that is, yes.
Comment 2 Bernd 2009-03-27 09:28:29 PDT
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.
Comment 3 Brandon Sterne (:bsterne) 2009-10-05 16:26:16 PDT
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();
Comment 4 Timothy Nikkel (:tn) 2009-10-15 02:24:15 PDT
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.
Comment 5 Jesse Ruderman 2009-10-15 09:20:43 PDT
Bug 448615 covers the assertions, I think.
Comment 6 Boris Zbarsky (:bz) 2009-10-15 09:44:33 PDT
Comment on attachment 406414 [details] [review]
patch

Ah, nice catch!  r=bzbarsky
Comment 7 Timothy Nikkel (:tn) 2009-10-15 17:51:24 PDT
We'll want this on all branches.
Comment 8 Timothy Nikkel (:tn) 2009-10-17 19:04:33 PDT
Created attachment 406892 [details] [review]
patch for 1.9.1 and 1.9.0

Boris, can you give this a quick once over?
Comment 9 Boris Zbarsky (:bz) 2009-10-17 19:50:08 PDT
Comment on attachment 406892 [details] [review]
patch for 1.9.1 and 1.9.0

Looks good.
Comment 10 Daniel Veditz 2009-10-23 10:31:10 PDT
Is there any reason this hasn't landed on mozilla-central for testing yet?
Comment 11 Timothy Nikkel (:tn) 2009-10-24 02:14:37 PDT
http://hg.mozilla.org/mozilla-central/rev/1d8209258b30
Comment 12 Timothy Nikkel (:tn) 2009-10-31 00:50:26 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cc13042643ca
Comment 13 Timothy Nikkel (:tn) 2009-10-31 19:39:25 PDT
Reminder to myself to add bug 503936 as crashtest when landing this on the remaining branches.
Comment 14 Daniel Veditz 2009-11-02 14:54:23 PST
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
Comment 16 Boris Zbarsky (:bz) 2009-11-09 09:08:01 PST
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
Comment 17 Al Billings [:abillings] 2009-11-10 16:44:43 PST
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.
Comment 18 Al Billings [:abillings] 2009-11-20 16:51:03 PST
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.
Comment 19 Daniel Veditz 2010-02-13 12:16:13 PST
Does not affect 1.8.1.22

Note You need to log in before you can comment on or make changes to this bug.