457514 - Crash [@ NeedFrameFor] with floating :first-letter

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect, P2)

Description

[Jesse Ruderman]

Attachment: 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 [:bzbarsky]]

OK.  So we crash in NeedFrameFor because aParentFrame is 0xdddddddd.  No idea why that is, yes.

Comment 2

[Bernd]

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)]

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 (:tnikkel)]

Attachment: 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]

Bug 448615 covers the assertions, I think.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 406414 [details] [diff] [review]
patch

Ah, nice catch!  r=bzbarsky

Comment 7

[Timothy Nikkel (:tnikkel)]

We'll want this on all branches.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: patch for 1.9.1 and 1.9.0

Boris, can you give this a quick once over?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 406892 [details] [diff] [review]
patch for 1.9.1 and 1.9.0

Looks good.

Comment 10

[Daniel Veditz [:dveditz]]

Is there any reason this hasn't landed on mozilla-central for testing yet?

Comment 11

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/1d8209258b30

Comment 12

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cc13042643ca

Comment 13

[Timothy Nikkel (:tnikkel)]

Reminder to myself to add bug 503936 as crashtest when landing this on the remaining branches.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 406892 [details] [diff] [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 15

[Timothy Nikkel (:tnikkel)]

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

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 - ex-MoCo]]

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 - ex-MoCo]]

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 [:dveditz]]

Does not affect 1.8.1.22

Comment 20

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c503b13bbb01

Comment 21

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/c503b13bbb01