665360 - Crash [@ gfxMixedFontFamily::ReplaceFontEntry(gfxFontEntry*, gfxFontEntry*) ]

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Bob Clary [:bc] (inactive)]

1. http://www.eastoncycling.com/en-us/mountain/products/bars/all-mountain/ea70-mb-281 in Nightly on Windows XP
2. Crash

Ted's exploitable breakpad anaylyser gives this a high exploitability which actually matches the use of deleted memory.

+		this	0x097fc460 {mHdr=0xdddddddd }	const nsTArray_base<nsTArrayDefaultAllocator> * const

  // @return The number of elements in the array.
  size_type Length() const {
=>    return mHdr->mLength;
  }

>	xul.dll!nsTArray_base<nsTArrayDefaultAllocator>::Length()  Line 140 + 0x5 bytes	C++
 	xul.dll!gfxMixedFontFamily::ReplaceFontEntry(gfxFontEntry * aOldFontEntry=0x097fc028, gfxFontEntry * aNewFontEntry=0x097f60c8)  Line 116 + 0xb bytes	C++
 	xul.dll!nsUserFontSet::ReplaceFontEntry(gfxProxyFontEntry * aProxy=0x097fc028, gfxFontEntry * aFontEntry=0x097f60c8)  Line 688	C++
 	xul.dll!gfxUserFontSet::OnLoadComplete(gfxProxyFontEntry * aProxy=0x097fc028, const unsigned char * aFontData=0x00000000, unsigned int aLength=21812, unsigned int aDownloadStatus=0)  Line 560	C++
 	xul.dll!nsFontFaceLoader::OnStreamComplete(nsIStreamLoader * aLoader=0x098ae110, nsISupports * aContext=0x00000000, unsigned int aStatus=0, unsigned int aStringLen=21812, const unsigned char * aString=0x0a0c6c00)  Line 220 + 0x20 bytes	C++
 	xul.dll!nsStreamLoader::OnStopRequest(nsIRequest * request=0x098ad8cc, nsISupports * ctxt=0x00000000, unsigned int aStatus=0)  Line 125 + 0x3e bytes	C++
 	xul.dll!nsCORSListenerProxy::OnStopRequest(nsIRequest * aRequest=0x098ad8cc, nsISupports * aContext=0x00000000, unsigned int aStatusCode=0)  Line 623	C++

I was able to reproduce the crash but not the deleted memory in debug Aurora.
I was not able to reproduce the crash in debug Beta.
I was not able to reproduce with nightly Nightly or Aurora builds.

This was found from crash automation from bp-5279a5dd-12aa-4dcc-bb1d-471152110616 which matches the detected crash.

Comment 1

[Daniel Veditz [:dveditz]]

What about a debug nightly, or MallocScribble on mac?

Comment 2

[Bob Clary [:bc] (inactive)]

Yes, the stack in comment 0 is from a debug windows Nightly build. I tried today's debug Nightly mac build with scribble enabled and still did not crash. I just reproduced the crash with today's debug Nightly on windows xp with the same deleted memory in mHdr. I have it in visual studio at the moment and will leave it there for a bit if you need me to be a debugging bot.

Note when I say nightly I mean an opt build downloaded from ftp.m.o regardless of branch. When I say Nightly (cap N) it is a trunk build.

Comment 3

[Daniel Veditz [:dveditz]]

Maybe something specific to WinXP fonts? Or specifically the fonts installed on your test system?

Comment 4

[Johnny Stenback (:jst)]

John, can you look into this?

Comment 5

[Bob Clary [:bc] (inactive)]

Maybe just XP. I don't see it in Windows 7 with the automation. These are pretty generic vms.

Comment 6

[John Daggett (:jtd)]

(In reply to comment #4)
> John, can you look into this?

I'm flying today, I won't be able to look at this until next week.  The location of the crash indicates a downloadable font, it's not specific to fonts on XP.  But I think this is something jkew already fixed, maybe it just needs to get pushed to a difference branch?  Jonathan, can you comment?

Comment 7

[Johnny Stenback (:jst)]

Jonathan, sounds like this needs attention from you, reassigning.

Comment 8

[Jonathan Kew [:jfkthame]]

The recent fixes I'm aware of in this area are bug 655138 and bug 650639, but both of those are present in Aurora as well as trunk code at this point.

Crash-stats indicates this is still happening on aurora (6.0a2) as well as trunk, plus a variety of earlier versions (4.0.1, even 3.x). Interestingly, the most recent trunk build that shows up is from 06/24 (a week ago, at this point), but I don't see anything in the pushlog from around that time that jumps out as a likely explanation for why the crashes might have stopped.

Also, crash-stats doesn't list any occurrences on FF5.0. I don't have any explanation for that, when it's happening in both 4.0.1 and 6.0a2.

My guess is that the crashes occur when the gfxFontFamily associated with a proxy font entry has been deleted prior to the font load completing, but I haven't figured out an exact scenario that would lead to this - let alone managed to reproduce it. Although we've made some changes in this area lately (especially bug 633299 and followup fixes), I don't think that accounts for the problem here, as we see this crash signature even in much older releases.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: possible band-aid

I have been running a current Aurora build on Windows for a while hoping to reproduce this, but haven't yet had any success. However, something we could try as a possible band-aid would be to make the nsFontFaceLoader hold a reference to the font family, as well as to the actual proxy on whose behalf it is loading. (Note that the proxy font entry does *not* have a strong reference to its family, only a weak pointer.) If there's some race condition or similar whereby the family can get deleted right before the load completes, this should ensure that it remains valid long enough to avoid this crash.

Unless you have any better ideas, John, I'd suggest we try landing this - I don't see how it could do any harm, at least - and see whether it has any effect on crash-stats.

Comment 10

[Johnny Stenback (:jst)]

Seems unlikely that we'll take this for 6, or that we'll even know if it's fixed or not. Tracking for newer versions though.

Comment 11

[Johnny Stenback (:jst)]

John, can you review here? We'd love to get this band-aid bug in to see if it helps, which based on what Marcia tells me we'd know whether it helped or not in a matter of days.

Comment 12

[Bob Clary [:bc] (inactive)]

fwiw, I am still seeing related crashes in automation and could retest within a matter of hours once this lands.

Comment 13

[Jonathan Kew [:jfkthame]]

Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/89842a2a40c6

Let's keep this bug open after this merges to m-c, at least until we have time to see whether this affects the crashiness at all.

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/89842a2a40c6

Comment 15

[Daniel Veditz [:dveditz]]

bc: please retest and see if the band-aide helped any.

Comment 16

[Daniel Veditz [:dveditz]]

I don't see any crashes in 8.0 builds after the Aug 9 checkin. There were only a handful before, but enough in 8 and 7 that I feel pretty good that we've got a real fix here.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 543614 [details] [diff] [review]
possible band-aid

This patch seems to have fixed the security hole in Firefox 8. It's safe and should be landed on Firefox 7 beta.

Comment 18

[Bob Clary [:bc] (inactive)]

I only see this on beta now. Appeared fixed (papered over?) on Nightly and Aurora. It is already on Aurora isn't it? http://hg.mozilla.org/releases/mozilla-aurora/rev/89842a2a40c6

Comment 20

[Zack Weinberg (:zwol)]

This is in the top fifty crashes for Firefox 6; the band-aid should IMO be considered if we do a patch for that release.  And it should definitely be landed on beta ASAP.

https://crash-stats.mozilla.com/report/list?range_value=7&range_unit=days&date=2011-08-18&signature=gfxMixedFontFamily%3A%3AReplaceFontEntry%28gfxFontEntry*%2C%20gfxFontEntry*%29&version=Firefox%3A6.0

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 543614 [details] [diff] [review]
possible band-aid

Approved for beta per today's triage meeting.

Comment 22

[chris hofmann]

yeah, lets get this fixed quickly and in as many releases as possible.

signature has jumped to over 180 crashes per day after the release of 6.0

         gfxMixedFontFamily::ReplaceFontEntry.gfxFontEntry...gfxFontEntry..
date     total    breakdown by build
         crashes  count build, count build, ...

20110818 187  	174 6.02011081116, 
        	4 7.0a22011081604, 	3 3.5.12009071509, 
        	2 7.02011081615, 	2 6.02011080403, 
        	1 8.0a12011072003, 	1 3.6.132010120307, 

and is visible on wide variety of sites.

domains of sites
  16 file://
  13 \N//
   5 http://www.ibahia.com
   4 http://www.probikekit.com
   4 http://www.blogger.com
   3 javascript:''//
   3 http://www.xcid.net
   3 http://www.lggetconnected.com
   3 http://www.eastoncycling.com
   2 http://www.conceptclub.ru
   2 http://veldev.com
   2 http://tracking.traviangames.com
   2 http://swetozar-online.ru
   2 http://ilumalite.syntaxmarketing.com.au
   2 http://fr.novopress.info
   2 http://feedproxy.google.com
   2 http://demos.foxymoron.co.in
   2 http://conceptclub.ru
   2 http://192.168.0.100
   2 http://178.79.144.232
   1 https://www.change.org
   1 https://plus.google.com
   1 https://opdagelse.telenor.dk
   1 https://ib.nab.com.au
   1 https://docs.google.com
   1 http://yfrog.com
   1 http://www.youtube.com
   1 http://www.yellowpagesmyanmar.net

urls for testing
  13 \N
   3 javascript:''
   2 http://www.xcid.net/contacto/survey.php?action=manage
   2 http://swetozar-online.ru/catalog/result/
   2 http://demos.foxymoron.co.in/lastwilderness/
   2 http://192.168.0.100/sliced/inne/Crimson%20Oak/DW%20Studio/blog.html
   2 file:///D:/portfolio/index.html
   1 https://www.change.org/login
   1 https://plus.google.com/u/0/_/notifications/frame?hl=en&origin=https%3A%2F%2Fmail.google.com&jsh=r%3Bgc%2F22954852-97da52d8#pid=23&id=gbsf&parent=https%3A%2F%2Fmail.google.com&rpctoken=82175096&_methods=onError%2ConInfo%2ChideNotificationWidget%2CpostShar
   1 https://opdagelse.telenor.dk/Bliv-underholdt/Musik/Fa-WiMP/Aktiver-WiMP/
   1 https://ib.nab.com.au/nabib/payments_transferList.ctl
   1 https://docs.google.com/a/captiway.com/?tab=oo#home
   1 http://yfrog.com/h6athizj
   1 http://www.youtube.com/watch?v=HyQqZh2Yx8I&feature=mr_meh&list=ULFHp2KgyQUFk&index=21&playnext=0
   1 http://www.yellowpagesmyanmar.net/list_bycat.php?page=2&cid=133&hcid=&loc=1
   1 http://www.xcid.net/
   1 http://www.web-web-11.info/unicalle//index.php?option=com_diplomas&view=diploma1&Itemid=8
   1 http://www.vodafonefreezone.com/Article.aspx?PageID=108&PageName=senle-ben-paketleri
   1 http://www.turkcell.de/internet
   1 http://www.thenorthface.cl/catalogo/hombres/footwear/hiking/2016_m-syncline-gtx
   1 http://www.thedeyjaycollection.com/shop/
   1 http://www.theatre-octogone.ch/11/index.php?page=saison-theatre
   1 http://www.sputnik-centar.hr/
   1 http://www.sounddepo.com/2011/
   1 http://www.silent-guilde.fr/
   1 http://www.sanitarium.com.au/products/vegetarian
   1 http://www.ray-ban.com/germany/store-locator

Comment 23

[Jonathan Kew [:jfkthame]]

Pushed to mozilla-beta:
http://hg.mozilla.org/releases/mozilla-beta/rev/b8d41ea97ce8

Comment 24

[Bob Clary [:bc] (inactive)]

beta does not appear to crash on my previous list of urls. This should be FIXED and the stati set to fixed, right?

Comment 25

[Jonathan Kew [:jfkthame]]

If you were able to reproduce it consistently before, and can confirm it's fixed, that's great; please mark accordingly. I was never able to reproduce the crash locally, so intended to wait and see what crash-stats showed...

Comment 26

[Johnny Stenback (:jst)]

Ok, marking this FIXED then!

Comment 27

[LegNeato]

Does this affect 1.9.2? It looks like it does as the crash signature shows up on 3.6.x. If so we'll need to take it for 1.9.2.21

Comment 28

[Jonathan Kew [:jfkthame]]

(In reply to Christian Legnitto [:LegNeato] from comment #27)
> Does this affect 1.9.2?

It almost certainly does. The 1.9.2 crash reports look as though they're coming from the same code path. We've never really explained the underlying cause, but the precautionary patch here seems to have stopped the crashes, as far as I can tell.

The patch needs minor adjustment to apply on 1.9.2; I'm just building locally to check it's OK, then will post it here.

Comment 29

[Jonathan Kew [:jfkthame]]

Attachment: patch for 1.9.2 branch

Comment 30

[LegNeato]

Comment on attachment 556625 [details] [diff] [review]
patch for 1.9.2 branch

Thanks! Approved for releases/mozilla-1.9.2. Please land on the default branch asap

Comment 31

[Jonathan Kew [:jfkthame]]

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

Comment 32

[u279076]

qa-, Bob can you please verify this is fixed? Thanks

Comment 33

[Bob Clary [:bc] (inactive)]

ashughes: i retested my urls and did not see the crash on 1.9.2, beta/7, aurora/8, nightly/9.

Comment 34

[u279076]

Thanks Bob. I'm calling this verified.