Last Comment Bug 650639 - Firefox 6.0a1 Crash [@ nsUserFontSet::ReplaceFontEntry ]
: Firefox 6.0a1 Crash [@ nsUserFontSet::ReplaceFontEntry ]
Status: RESOLVED FIXED
: crash, regression, reproducible
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: ---
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
http://www.exotissimo.com/travel/viet...
: 651180 (view as bug list)
Depends on: 653977
Blocks: 532972 633299 642734
  Show dependency treegraph
 
Reported: 2011-04-17 08:46 PDT by chris hofmann
Modified: 2013-12-27 14:33 PST (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, ensure loaders are cancelled when updating the font set (10.32 KB, patch)
2011-04-27 07:31 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Review
patch, ensure loaders are cancelled when updating the font set - improved (15.30 KB, patch)
2011-04-27 10:05 PDT, Jonathan Kew (:jfkthame)
dbaron: review+
Details | Diff | Review

Description chris hofmann 2011-04-17 08:46:51 PDT
possible new regression on the nightly channel.  currently #8 top crash there.

not sure about the component here, probably needs to change to something better.

https://crash-stats.mozilla.com/report/index/7b0b6db1-4f18-47ac-88e4-baaf22110417

Frame 	Module 	Signature [Expand] 	Source
0 	XUL 	nsUserFontSet::ReplaceFontEntry 	h:105
1 	XUL 	gfxUserFontSet::OnLoadComplete 	gfx/thebes/gfxUserFontSet.cpp:564
2 	XUL 	nsFontFaceLoader::OnStreamComplete 	layout/style/nsFontFaceLoader.cpp:228
3 	XUL 	nsStreamLoader::OnStopRequest 	netwerk/base/src/nsStreamLoader.cpp:125
4 	XUL 	nsStreamListenerTee::OnStopRequest 	netwerk/base/src/nsStreamListenerTee.cpp:71
5 	XUL 	nsHttpChannel::OnStopRequest 	netwerk/protocol/http/nsHttpChannel.cpp:4116
6 	XUL 	nsInputStreamPump::OnStateStop 	netwerk/base/src/nsInputStreamPump.cpp:578
7 	XUL 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:403
8 	XUL 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:114
9 	XUL 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:618
10 	XUL 	NS_ProcessPendingEvents_P 	nsThreadUtils.cpp:200
11 	XUL 	nsBaseAppShell::NativeEventCallback 	widget/src/xpwidgets/nsBaseAppShell.cpp:130
12 	XUL 	nsAppShell::ProcessGeckoEvents 	widget/src/cocoa/nsAppShell.mm:399
13 	CoreFoundation 	__CFRunLoopDoSources0 	
14 	CoreFoundation 	__CFRunLoopRun 	
15 	CoreFoundation 	CFRunLoopRunSpecific 	
16 	HIToolbox 	HIToolbox@0x2e7ed 	
17 	HIToolbox 	HIToolbox@0x2e550 	
18 	HIToolbox 	HIToolbox@0x2e4ab 	
19 	AppKit 	_DPSNextEvent 	
20 	AppKit 	-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] 	
21 	AppKit 	-[NSApplication run] 	
22 	XUL 	nsAppShell::Run 	widget/src/cocoa/nsAppShell.mm:746
23 	XUL 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:218
24 	XUL 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3754
25 	firefox-bin 	main 	browser/app/nsBrowserApp.cpp:158
26 	firefox-bin 	firefox-bin@0x953

more reports at:

https://crash-stats.mozilla.com/report/list?signature=nsUserFontSet::ReplaceFontEntry

users talk mostly of visiting AMO and this comment:
  I've been loading a page with webfonts. It crashed before fonts have been  
  loaded.

some of the code near the top of the stack changed recently.

0138798a072a
2011-04-12 11:53 +0100	Jonathan Kew - bug 633299 - don't discard font entries for @font-face rules that haven't changed. r=dbaron

crashes started appearing just after that in builds  from the 13th

Crash count for 20110411-crashdata.csv        0
Crash count for 20110412-crashdata.csv        0
Crash count for 20110413-crashdata.csv        2
	\N fx_bld:6.0a1 20110413090915	comment:\N
	http://c8l.ca/14y fx_bld:6.0a1 20110413090934	comment:\N
Crash count for 20110414-crashdata.csv        4
	\N fx_bld:6.0a1 20110414030535	comment:Loading addons.mozilla.org
	\N fx_bld:6.0a1 20110414030535	comment:\N
	http://slides.html5rocks.com/#slide-orientation fx_bld:6.0a1 20110414030535	comment:\N
	http://www.20minutos.es/ fx_bld:6.0a1 20110413090934	comment:\N
Crash count for 20110415-crashdata.csv        4
	http://blog.mozilla.com/blog/2011/02/08/mozilla-firefox-4-beta-now-including-do-not-track-capabilities/ fx_bld:6.0a1 20110413090915	comment:\N
	http://whiteappleer.tw/2011/04/15/mac-app-store-the-unarchiver-app/ fx_bld:6.0a1 20110414030535	comment:\N
	https://duckduckgo.com/?q=planet+xml+xsl+animation fx_bld:6.0a1 20110414030535	comment:\N
	https://mozillademos.org/demos/planetarium/demo.html fx_bld:6.0a1 20110414030535	comment:\N
Crash count for 20110416-crashdata.csv       17
	http://blog.mozilla.com/ fx_bld:6.0a1 20110415030523	comment:\N
	http://falkvinge.net/2011/04/14/european-court-of-justice-to-outlaw-internet-filtering-esp-for-copyright-enforcement/ fx_bld:6.0a1 20110415030523	comment:\N
	http://feastofvsenses.com/piwik/index.php?module=CoreHome&action=index&idSite=1&period=day&date=yesterday#module=Dashboard&action=embeddedIndex&idSite=1&period=day&date=yesterday fx_bld:6.0a1 20110415030523	comment:\N
	http://listen.grooveshark.com/#/ fx_bld:6.0a1 20110415030523	comment:\N
	http://localhost:3000/ fx_bld:6.0a1 20110416030532	comment:\N
	http://localhost:3000/archive fx_bld:6.0a1 20110415030523	comment:I've been loading a page with webfonts. It crashed before fonts have been loaded.
	http://localhost:3000/archive fx_bld:6.0a1 20110415030523	comment:\N
	http://localhost:3000/archive fx_bld:6.0a1 20110415030523	comment:\N
	http://localhost:3000/archive fx_bld:6.0a1 20110416030532	comment:\N
	http://mzl.la/hBFNBw fx_bld:6.0a1 20110416030532	comment:\N
	http://ourphotos.com:8888/wp/?p=16 fx_bld:6.0a1 20110415030523	comment:\N
	http://vojtol.pl/#home-glowna fx_bld:6.0a1 20110416030532	comment:\N
	http://vojtol.pl/szkocja-rozne#107 fx_bld:6.0a1 20110415030523	comment:\N
	http://www.bbc.co.uk/iplayer/console/b00zzqdz fx_bld:6.0a1 20110415030523	comment:\N
	http://www.facebook.com/extern/login_status.php?
          api_key=111132365592157&
          app_id=111132365592157&
          channel_url=http static.ak.fbcdn.net
          http%253A%252F%252Flisten.grooveshark.com%2 
          fx_bld:6.0a1 20110415030523	comment:\N
	http://www.pointlessone.org/ fx_bld:6.0a1 20110415030523	comment:\N
	https://addons.mozilla.org/fr/firefox/search/?sort=updated&cat=all fx_bld:6.0a1 20110415030523	comment:\N
Comment 1 Jonathan Kew (:jfkthame) 2011-04-17 09:03:43 PDT
Probably a regression from bug 633299; I'll look into it. Any reliable STR would be most helpful.
Comment 2 chris hofmann 2011-04-18 10:28:10 PDT
looks like there are these 4 forms of the stack.

....Signature number: 14-nsUserFontSet::ReplaceFontEntry
https://bugzilla.mozilla.org/buglist.cgi?bug_id=650639
______ distribution of 11 different stacks, looking at top 10 frames
      5  stacks like
0|0|XUL|nsUserFontSet::ReplaceFontEntry
0|1|XUL|gfxUserFontSet::OnLoadComplete
0|2|XUL|nsFontFaceLoader::OnStreamComplete
0|3|XUL|nsStreamLoader::OnStopRequest
0|4|XUL|NS_InvokeByIndex_P
0|5|XUL|XPCWrappedNative::CallMethod
0|6|XUL|XPC_WN_CallMethod
0|7|XUL|js::Interpret
0|8|XUL|js::RunScript
0|9|XUL|js::Invoke

      3  stacks like
0|0|XUL|nsUserFontSet::ReplaceFontEntry
0|1|XUL|gfxUserFontSet::OnLoadComplete
0|2|XUL|nsFontFaceLoader::OnStreamComplete
0|3|XUL|nsStreamLoader::OnStopRequest
0|4|XUL|nsStreamListenerTee::OnStopRequest
0|5|XUL|nsHttpChannel::OnStopRequest
0|6|XUL|nsInputStreamPump::OnStateStop
0|7|XUL|nsInputStreamPump::OnInputStreamReady
0|8|XUL|nsInputStreamReadyEvent::Run
0|9|XUL|nsThread::ProcessNextEvent

      2  stacks like
0|0|XUL|nsUserFontSet::ReplaceFontEntry
0|1|XUL|gfxUserFontSet::OnLoadComplete
0|2|XUL|nsFontFaceLoader::OnStreamComplete
0|3|XUL|nsStreamLoader::OnStopRequest
0|4|XUL|nsStreamListenerTee::OnStopRequest
0|5|XUL|nsStreamListenerTee::OnStopRequest
0|6|XUL|nsHttpChannel::OnStopRequest
0|7|XUL|nsInputStreamPump::OnStateStop
0|8|XUL|nsInputStreamPump::OnInputStreamReady
0|9|XUL|nsInputStreamReadyEvent::Run

      1  stacks like
0|0|libxul.so|nsUserFontSet::ReplaceFontEntry
0|1|libxul.so|gfxUserFontSet::OnLoadComplete
0|2|libxul.so|nsFontFaceLoader::OnStreamComplete
0|3|libxul.so|nsStreamLoader::OnStopRequest
0|4|libxul.so|nsCORSListenerProxy::OnStopRequest
0|5|libxul.so|nsStreamListenerWrapper::OnStopRequest
0|6|libxul.so|
0|7|libxul.so|XPCWrappedNative::CallMethod
0|8|libxul.so|XPC_WN_CallMethod
0|9|libxul.so|js::Interpret
Comment 3 Bob Clary [:bc:] 2011-04-18 15:12:22 PDT
http://www.exotissimo.com/travel/vietnam/tours/

In automation I got:

Operating system: Windows NT
                  5.1.2600 Service Pack 3
CPU: x86
     GenuineIntel family 6 model 44 stepping 2
     1 CPU

Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
Crash address: 0xffffffff82940030

Thread 0 (crashed)
 0  xul.dll!nsTArray_base<nsTArrayDefaultAllocator>::Length() [nsTArray.h : 139 + 0x5]
    eip = 0x100de73c   esp = 0x0012ce90   ebp = 0x0012ce94   ebx = 0x00000001
    esi = 0x01990068   edi = 0x00000000   eax = 0x045c8068   ecx = 0x82940030
    edx = 0x0358c368   efl = 0x00210202
    Found by: given as instruction pointer in context
 1  xul.dll!gfxMixedFontFamily::ReplaceFontEntry(gfxFontEntry *,gfxFontEntry *) [gfxUserFontSet.h : 102 + 0xa]
    eip = 0x10602144   esp = 0x0012ce9c   ebp = 0x0012ceac
    Found by: call frame info
 2  xul.dll!nsUserFontSet::ReplaceFontEntry(gfxProxyFontEntry *,gfxFontEntry *) [nsFontFaceLoader.cpp : 686 + 0x16]
    eip = 0x106020e0   esp = 0x0012ceb4   ebp = 0x0012cec4
    Found by: call frame info
 3  xul.dll!gfxUserFontSet::OnLoadComplete(gfxFontEntry *,unsigned char const *,unsigned int,unsigned int) [gfxUserFontSet.cpp : 564 + 0x1a]
    eip = 0x119f09b1   esp = 0x0012cecc   ebp = 0x0012d550
    Found by: call frame info

locally I got:

>	xul.dll!nsRefPtr<gfxFontEntry>::get()  Line 1098 + 0x3 bytes	C++
 	xul.dll!nsRefPtr<gfxFontEntry>::operator gfxFontEntry *()  Line 1112	C++
 	xul.dll!gfxMixedFontFamily::ReplaceFontEntry(gfxFontEntry * aOldFontEntry=0x079838e0, gfxFontEntry * aNewFontEntry=0x045922b8)  Line 104 + 0x16 bytes	C++
 	xul.dll!nsUserFontSet::ReplaceFontEntry(gfxProxyFontEntry * aProxy=0x079838e0, gfxFontEntry * aFontEntry=0x045922b8)  Line 687	C++
 	xul.dll!gfxUserFontSet::OnLoadComplete(gfxFontEntry * aFontToLoad=0x079838e0, const unsigned char * aFontData=0x00000000, unsigned int aLength=23608, unsigned int aDownloadStatus=0)  Line 565	C++
 	xul.dll!nsFontFaceLoader::OnStreamComplete(nsIStreamLoader * aLoader=0x0566b958, nsISupports * aContext=0x00000000, unsigned int aStatus=0, unsigned int aStringLen=23608, const unsigned char * aString=0x05237cb0)  Line 228 + 0x20 bytes	C++
Comment 4 Bob Clary [:bc:] 2011-04-19 22:48:41 PDT
*** Bug 651180 has been marked as a duplicate of this bug. ***
Comment 5 Jonathan Kew (:jfkthame) 2011-04-26 11:13:06 PDT
I tried beating on some webfonts examples but have not yet managed to reproduce this (and simple inspection of the code didn't lead to an obvious cause). If anyone has an example that triggers this with any consistency, I'd love to hear about it.
Comment 6 Bob Clary [:bc:] 2011-04-26 11:21:06 PDT
http://www.exotissimo.com/travel/vietnam/tours/ reproduces for me on winxp and mc. I have it in vc debugger atm if you need a remote debugging bot.
Comment 7 Marcia Knous [:marcia - use ni] 2011-04-26 14:23:32 PDT
Reproduces for me using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110426 Firefox/6.0a1 as well.
Comment 8 Jonathan Kew (:jfkthame) 2011-04-27 04:52:12 PDT
I finally reproduced this (it's sensitive to the speed/timing of font downloads), and should have a patch up shortly. Confirmed it is a regression from bug 633299; it occurs because it's possible for a font entry to get removed from the font set (and from its parent family) while a download is in progress; then when the download completes and tries to update the relevant family, it's not there.

This didn't happen prior to bug 633299 because we used to completely destroy the user font set (and in the process, cancel all its downloaders) whenever the @font-face rules change.
Comment 9 Jonathan Kew (:jfkthame) 2011-04-27 07:31:51 PDT
Created attachment 528589 [details] [diff] [review]
patch, ensure loaders are cancelled when updating the font set

In intensive testing on the http://www.exotissimo.com/ site mentioned above, I can no longer trigger a crash with this patch. (It was sporadic for me previously, depending on the timing of stylesheet and font downloads, I believe.)
Comment 10 Jonathan Kew (:jfkthame) 2011-04-27 10:05:50 PDT
Created attachment 528619 [details] [diff] [review]
patch, ensure loaders are cancelled when updating the font set - improved

Better version of the patch - functionally equivalent to the original, but it's cleaner to reset the proxy's loading state from nsFontFaceLoader::Cancel().

Also eliminates more of the static_cast<gfxProxyFontEntry*> usage from the code; an nsFontFaceLoader is always created referring to a proxy entry, so we can simply declare it as such rather than checking its mIsProxy field and casting in various places. And letting the compiler do better type-checking makes for less error-prone code going forward. :)
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-27 11:15:39 PDT
The only interesting bits seem to be the changes to 
nsFontFaceLoader::Cancel() and nsUserFontSet::UpdateRules(); everything
else seems to be the type changes, which look good.

As far as the change in UpdateRules -- does this cause us to stop loads for fonts that we still want to continue loading because they'll still be in the font set?  Or were those loads not actually useful anyway?

And in the change to Cancel() -- what's the lifetime of these proxy entries?  Do they outlast the particular attempt to load?
Comment 12 Jonathan Kew (:jfkthame) 2011-04-27 11:43:45 PDT
(In reply to comment #11)
> As far as the change in UpdateRules -- does this cause us to stop loads for
> fonts that we still want to continue loading because they'll still be in the
> font set?  Or were those loads not actually useful anyway?

Yes, I think it could cancel loads that we end up re-doing, and in this case it's not optimal to cancel them (although we'll still be in much better shape than we were prior to bug 633299, as we don't discard fonts that have actually been loaded). But I'm a bit nervous of trying to be smarter about it to try and preserve these, and potentially leaving corner cases that still break (and that are hard to test/verify as they're so timing-sensitive). I'm more comfortable doing the simple fix for this crash, and then if we want to optimize behavior further, do that at leisure in a followup.

> And in the change to Cancel() -- what's the lifetime of these proxy entries? 
> Do they outlast the particular attempt to load?

Yes, in the case where the rule (and hence proxy entry) survives the font-set update. The proxy works its way through its list of sources, and if we cancel a loader, we want it to retry that same source next time rather than move on to the next (as we'd do when calling LoadNext on a proxy that is already in the loading state). Resetting the state to not-loading means it will start again at the current position in its source list.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-27 12:14:42 PDT
Comment on attachment 528619 [details] [diff] [review]
patch, ensure loaders are cancelled when updating the font set - improved

ok, sounds good, r=dbaron
Comment 14 Jonathan Kew (:jfkthame) 2011-04-27 22:14:50 PDT
Landed on mozilla-central:
  http://hg.mozilla.org/mozilla-central/rev/a9dd6038db6a

I'm expecting this to resolve these crashes, but will leave the bug open for now until we see crash-stats from the next Nightly build.
Comment 15 Jonathan Kew (:jfkthame) 2011-05-01 10:25:11 PDT
Crash-stats shows no crashes in builds later than 20110427, for either this signature or the Windows version of the crash, shown in bug 651180; accordingly, marking this as fixed.
Comment 16 Robert Sayre 2011-05-17 14:46:32 PDT
please nominate for mozilla-beta.
Comment 17 Marcia Knous [:marcia - use ni] 2011-05-17 14:48:51 PDT
Jonathan: I see a handful of crashes on current Mac trunk builds - http://tinyurl.com/68wuac4. Should I file a new bug for this?
Comment 18 Jonathan Kew (:jfkthame) 2011-05-18 23:50:57 PDT
Comment on attachment 528619 [details] [diff] [review]
patch, ensure loaders are cancelled when updating the font set - improved

Nominating for mozilla-beta as per comment #16; this fixes a potential crash. Assuming we take this patch, we _must_ also take Bug 653977 to avoid the disappearing-text regression described there.
Comment 19 Jonathan Kew (:jfkthame) 2011-05-18 23:53:51 PDT
(In reply to comment #17)
> Jonathan: I see a handful of crashes on current Mac trunk builds -
> http://tinyurl.com/68wuac4. Should I file a new bug for this?

Yes, please do, and cc me; I'll try to investigate what could be happening there.
Comment 20 Jonathan Kew (:jfkthame) 2011-05-19 00:10:59 PDT
(In reply to comment #18)
> Nominating for mozilla-beta as per comment #16; this fixes a potential
> crash. Assuming we take this patch, we _must_ also take Bug 653977 to avoid
> the disappearing-text regression described there.

Hmm, on looking again, I don't think this is applicable to m-b at all. Bug 633299 landed on trunk _after_ the merge to Aurora on 4/11, and therefore none of this is present in m-a / m-b.
Comment 21 Asa Dotzler [:asa] 2011-05-19 14:48:11 PDT
Comment on attachment 528619 [details] [diff] [review]
patch, ensure loaders are cancelled when updating the font set - improved

unsetting the approval request as this doesn't affect Aurora/Beta

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