As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
Last Comment Bug 665360 - Crash [@ gfxMixedFontFamily::ReplaceFontEntry(gfxFontEntry*, gfxFontEntry*) ]
: Crash [@ gfxMixedFontFamily::ReplaceFontEntry(gfxFontEntry*, gfxFontEntry*) ]
: crash, regression, reproducible, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: mozilla8
Assigned To: Jonathan Kew (:jfkthame)
: Milan Sreckovic [:milan]
: 680475 (view as bug list)
Depends on:
Blocks: 532972
  Show dependency treegraph
Reported: 2011-06-19 10:19 PDT by Bob Clary [:bc:]
Modified: 2015-10-16 11:37 PDT (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

possible band-aid (1.51 KB, patch)
2011-07-02 09:48 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
jst: approval‑mozilla‑beta+
Details | Diff | Splinter Review
patch for 1.9.2 branch (2.25 KB, patch)
2011-08-29 11:42 PDT, Jonathan Kew (:jfkthame)
christian: approval1.9.2.23+
Details | Diff | Splinter Review

Description User image Bob Clary [:bc:] 2011-06-19 10:19:00 PDT
1. 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 User image Daniel Veditz [:dveditz] 2011-06-22 17:15:17 PDT
What about a debug nightly, or MallocScribble on mac?
Comment 2 User image Bob Clary [:bc:] 2011-06-22 18:38:51 PDT
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 User image Daniel Veditz [:dveditz] 2011-06-23 13:41:06 PDT
Maybe something specific to WinXP fonts? Or specifically the fonts installed on your test system?
Comment 4 User image Johnny Stenback (:jst, 2011-06-23 13:41:39 PDT
John, can you look into this?
Comment 5 User image Bob Clary [:bc:] 2011-06-23 13:46:59 PDT
Maybe just XP. I don't see it in Windows 7 with the automation. These are pretty generic vms.
Comment 6 User image John Daggett (:jtd) 2011-06-23 19:47:35 PDT
(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 User image Johnny Stenback (:jst, 2011-06-30 14:00:29 PDT
Jonathan, sounds like this needs attention from you, reassigning.
Comment 8 User image Jonathan Kew (:jfkthame) 2011-07-02 05:36:16 PDT
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 User image Jonathan Kew (:jfkthame) 2011-07-02 09:48:16 PDT
Created attachment 543614 [details] [diff] [review]
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 User image Johnny Stenback (:jst, 2011-07-07 14:01:26 PDT
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 User image Johnny Stenback (:jst, 2011-08-04 13:29:09 PDT
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 User image Bob Clary [:bc:] 2011-08-04 13:37:51 PDT
fwiw, I am still seeing related crashes in automation and could retest within a matter of hours once this lands.
Comment 13 User image Jonathan Kew (:jfkthame) 2011-08-09 00:56:05 PDT
Pushed to mozilla-inbound:

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 15 User image Daniel Veditz [:dveditz] 2011-08-18 13:48:25 PDT
bc: please retest and see if the band-aide helped any.
Comment 16 User image Daniel Veditz [:dveditz] 2011-08-18 14:07:20 PDT
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 User image Daniel Veditz [:dveditz] 2011-08-18 14:08:46 PDT
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 User image Bob Clary [:bc:] 2011-08-18 14:23:46 PDT
I only see this on beta now. Appeared fixed (papered over?) on Nightly and Aurora. It is already on Aurora isn't it?
Comment 19 User image Zack Weinberg (:zwol) 2011-08-19 14:41:29 PDT
*** Bug 680475 has been marked as a duplicate of this bug. ***
Comment 20 User image Zack Weinberg (:zwol) 2011-08-19 14:44:38 PDT
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.*%2C%20gfxFontEntry*%29&version=Firefox%3A6.0
Comment 21 User image Johnny Stenback (:jst, 2011-08-22 14:38:11 PDT
Comment on attachment 543614 [details] [diff] [review]
possible band-aid

Approved for beta per today's triage meeting.
Comment 22 User image chris hofmann 2011-08-22 14:46:51 PDT
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

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//
   3 javascript:''//

urls for testing
  13 \N
   3 javascript:''
   2 file:///D:/portfolio/index.html
Comment 23 User image Jonathan Kew (:jfkthame) 2011-08-23 01:43:02 PDT
Pushed to mozilla-beta:
Comment 24 User image Bob Clary [:bc:] 2011-08-23 04:24:39 PDT
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 User image Jonathan Kew (:jfkthame) 2011-08-23 04:32:14 PDT
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 User image Johnny Stenback (:jst, 2011-08-25 13:30:18 PDT
Ok, marking this FIXED then!
Comment 27 User image christian 2011-08-29 10:19:27 PDT
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
Comment 28 User image Jonathan Kew (:jfkthame) 2011-08-29 10:58:17 PDT
(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 User image Jonathan Kew (:jfkthame) 2011-08-29 11:42:25 PDT
Created attachment 556625 [details] [diff] [review]
patch for 1.9.2 branch
Comment 30 User image christian 2011-09-09 10:30:25 PDT
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 User image Jonathan Kew (:jfkthame) 2011-09-10 07:19:28 PDT
Comment 32 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 15:21:38 PDT
qa-, Bob can you please verify this is fixed? Thanks
Comment 33 User image Bob Clary [:bc:] 2011-09-22 15:43:45 PDT
ashughes: i retested my urls and did not see the crash on 1.9.2, beta/7, aurora/8, nightly/9.
Comment 34 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 17:41:13 PDT
Thanks Bob. I'm calling this verified.

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