616426 - crash [@ nsThebesFontMetrics::GetMetrics ] because we hard code our font search path to /system/fonts instead of $ANDROID_ROOT/fonts

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Scoobidiver (away)]

It is #12 top crasher in Fennec 4.0b3pre for the last week.
It happens at startup (uptime=0).

Signature	nsThebesFontMetrics::GetMetrics
UUID	42aa125a-f07f-4d37-a53f-351352101201
Time 	2010-12-01 17:51:22.830736
Uptime	0
Install Age	19586 seconds (5.4 hours) since version was first installed.
Product	Fennec
Version	4.0b3pre
Build ID	20101201041953
Branch	2.0
OS	Linux
OS Version	0.0.0 Linux 2.6.35.9-wildmonks #315 PREEMPT Fri Nov 26 16:47:39 PST 2010 armv7l
CPU	arm
CPU Info	
Crash Reason	SIGSEGV
Crash Address	0xffffffff

Frame 	Module 	Signature [Expand] 	Source
0 	libxul.so 	nsThebesFontMetrics::GetMetrics 	gfx/src/thebes/nsThebesFontMetrics.cpp:112
1 	libxul.so 	nsThebesFontMetrics::GetExternalLeading 	gfx/src/thebes/nsThebesFontMetrics.cpp:190
2 	libxul.so 	nsHTMLReflowState::CalcLineHeight 	layout/generic/nsHTMLReflowState.cpp:2100
3 	libxul.so 	nsHTMLReflowState::CalcLineHeight 	layout/generic/nsHTMLReflowState.cpp:2160
4 	libxul.so 	nsBlockReflowState::nsBlockReflowState 	layout/generic/nsBlockReflowState.cpp:146
5 	libxul.so 	nsBlockFrame::Reflow 	layout/generic/nsIFrame.h:1267
6 	libxul.so 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:743
7 	libxul.so 	nsCanvasFrame::Reflow 	layout/generic/nsCanvasFrame.cpp:498
8 	libxul.so 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:743
9 	libxul.so 	nsHTMLScrollFrame::ReflowScrolledFrame 	layout/generic/nsGfxScrollFrame.cpp:518
10 	libxul.so 	nsHTMLScrollFrame::ReflowContents 	layout/generic/nsGfxScrollFrame.cpp:609
11 	libxul.so 	nsHTMLScrollFrame::Reflow 	layout/generic/nsGfxScrollFrame.cpp:817
12 	libxul.so 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:743
13 	libxul.so 	ViewportFrame::Reflow 	layout/generic/nsViewportFrame.cpp:294
14 	libxul.so 	PresShell::DoReflow 	layout/base/nsPresShell.cpp:7787
15 	libxul.so 	PresShell::ProcessReflowCommands 	layout/base/nsPresShell.cpp:7891
16 	libxul.so 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:4884
17 	libxul.so 	nsDocument::FlushPendingNotifications 	nsCOMPtr.h:492
18 	libxul.so 	nsDocument::FlushPendingNotifications 	nsIDocument.h:463
19 	libxul.so 	nsDocLoader::DocLoaderIsEmpty 	uriloader/base/nsDocLoader.cpp:773
20 	libxul.so 	nsDocLoader::OnStopRequest 	uriloader/base/nsDocLoader.cpp:706
21 	libxul.so 	nsLoadGroup::RemoveRequest 	netwerk/base/src/nsLoadGroup.cpp:680
22 	libxul.so 	nsDocument::DoUnblockOnload 	nsCOMPtr.h:492
23 	libxul.so 	nsDocument::UnblockOnload 	content/base/src/nsDocument.cpp:7201
24 	libxul.so 	nsBindingManager::DoProcessAttachedQueue 	nsCOMPtr.h:492
25 	libxul.so 	nsRunnableMethodImpl<void , true>::Run 	nsThreadUtils.h:347
26 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:626
27 	libxul.so 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:250
28 	libxul.so 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:111
29 	libxul.so 	mozilla::ipc::MessagePumpForChildProcess::Run 	ipc/glue/MessagePump.cpp:230
30 	libxul.so 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:220
31 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:512
32 	libxul.so 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:198
33 	libxul.so 	XRE_RunAppShell 	toolkit/xre/nsEmbedFunctions.cpp:631
34 	libxul.so 	mozilla::ipc::MessagePumpForChildProcess::Run 	ipc/glue/MessagePump.cpp:222
35 	libxul.so 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:220
36 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:512
37 	libxul.so 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp:510
38 	libmozutils.so 	ChildProcessInit 	other-licenses/android/APKOpen.cpp:691
39 	plugin-container 	main 	ipc/app/MozillaRuntimeMainAndroid.cpp:69
40 	libc.so 	libc.so@0xd60a 	

More reports at:
http://crash-stats.mozilla.com/report/list?range_value=4&range_unit=weeks&signature=nsThebesFontMetrics%3A%3AGetMetrics&version=Fennec%3A4.0b3pre

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Need STR

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

How are we determining that this is a startup crash?

Comment 3

[Josh Matthews [:jdm]]

Not a startup crash; content processes don't report uptime currently.

Comment 4

[Scoobidiver (away)]

It is #9 top crasher in Fennec 4.0b3 for the last week.

Comment 5

[Scoobidiver (away)]

It is #3 top crasher and #1 unresolved top crasher in Fennec 4.0b3 over the last week.

Comment 6

[Scoobidiver (away)]

It is #1 top crasher in 4.0b5 (see bug 636430 comment 4).

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

this rang some bells, so I went searching through my bugmail. The original implementation of the freetype backend for WinMo hit this, as you can see in bug 462908 comment 15. In that bug stuart said we shouldn't ever return null here so we shouldn't need a null check.

Comment 8

[Karl Tomlinson (:karlt)]

Could some systems have no fonts of the type searched?
Or have the fonts in a different location, perhaps.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

my current working theory is that some devices don't have Droid Sans

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

bug 633109 implies there are some fonts we're not finding

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

looking through the android font code, this implies to me that "Droid Sans" will always be there:
https://www.google.com/codesearch/p?hl=en#nScvmqlsOdU/Development/Depends/skia/src/ports/SkFontHost_android.cpp&q=FileTypeface&d=5&l=409

however, if ANDROID_ROOT isn't "/system" we may be looking in the wrong place for it:
https://www.google.com/codesearch/p?hl=en#nScvmqlsOdU/Development/Depends/skia/src/ports/SkFontHost_android.cpp&q=FileTypeface&d=5&l=36

Comment 12

[Alon Zakai (:azakai)]

As mentioned in bug 633109 comment 2, at least on my galaxy s ANDROID_ROOT is /system, as expected. But perhaps elsewhere it isn't. Would be easy to check both /system and ANDROID_ROOT I guess.

Comment 13

[Alon Zakai (:azakai)]

Attachment: patch

Patch to look in $ANDROID_ROOT, if it differs from /system.

Comment 14

[Alon Zakai (:azakai)]

Attachment: patch without whitespace

For convenience here is a version of that patch without whitespace (the patch adds indentation so more looks changed that actually is).

Comment 15

[Alon Zakai (:azakai)]

Attachment: patch v2

Comment 16

[Alon Zakai (:azakai)]

http://hg.mozilla.org/mozilla-central/rev/c56df99c1c75

Comment 17

[John Daggett (:jtd)]

I think it might make sense to annotate the crashdump if there's a situation where zero fonts are found.  Seems like we're guessing at the root cause here.  If the root cause really was no fonts in the directory it would be pretty easy to figure that out.  Other scenarios are possible here, all the fonts failed to load, none of the files had extensions '.ttf', yadda, yadda.

I would suggest tracking the data below.  If the total fonts loaded is 0, then annotate the crashdump with that info:

- whether the $root/system dir was found
- how many files were found
- how many .ttf files were found
- how many fonts failed to load

This way, if the crash still does occur at least we'll have a better idea as to which scenario is causing the crash.  Kinda sucky that manufacturers can hack the Skia code to load fonts from random places.

Comment 18

[Alon Zakai (:azakai)]

Logging that data sounds good to me.

Would we want this for 4.0?

Comment 19

[Matt Brubeck (:mbrubeck)]

I am seeing this crash in the content process in todays nightly build (from m-c 3570861040e9) when loading this URL on Samsusng Galaxy Tab (100% reproducible):
https://support.mozilla.com/en-US/questions?tagged=mobile

Crash report:
https://crash-stats.mozilla.com/report/index/bp-00defcb3-4a26-4dac-a5a2-5d5972110311

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: follow up patch

Alon's patch has us looking /system rather than /system/fonts, which means we're not finding any fonts.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: follow up patch

this adds the closedir() as jdn suggested on irc

Comment 22

[Alon Zakai (:azakai)]

Good catch!

Comment 23

[Jonathan Kew [:jfkthame]]

Comment on attachment 518785 [details] [diff] [review]
follow up patch

Looks sane to me, go for it!

Though actually, the name "FindFontsInDirectory" is a bit confusing, as it's not actually the fonts dir that's being passed in... maybe the method should really be called "FindFontFilesInFontsSubdirOfDirectory". Awfully verbose, though.

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: follow up v2

Comment 25

[Doug Turner (:dougt)]

Comment on attachment 518808 [details] [diff] [review]
follow up v2

Stealing review.  seriously regression -- we could either push this fix or back out the other one.  I think we should just land this follow up.  jfkthame, feel free to review post landing.


   PR_GetEnv("ANDROID_ROOT");

please put a comment in the code (similar to comment #11).

Comment 26

[Jonathan Kew [:jfkthame]]

(In reply to comment #25)
> Comment on attachment 518808 [details] [diff] [review]
> follow up v2
> 
> Stealing review.  seriously regression -- we could either push this fix or back
> out the other one.  I think we should just land this follow up.  jfkthame, feel
> free to review post landing.

Thanks for taking this - looks good to me. Let's get it landed asap!

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/529e0a66b709

Comment 28

[Josh Matthews [:jdm]]

(In reply to comment #17)
> I think it might make sense to annotate the crashdump if there's a situation
> where zero fonts are found.  Seems like we're guessing at the root cause here. 
> If the root cause really was no fonts in the directory it would be pretty easy
> to figure that out.  Other scenarios are possible here, all the fonts failed to
> load, none of the files had extensions '.ttf', yadda, yadda.
> 
> I would suggest tracking the data below.  If the total fonts loaded is 0, then
> annotate the crashdump with that info:
> 
> - whether the $root/system dir was found
> - how many files were found
> - how many .ttf files were found
> - how many fonts failed to load
> 
> This way, if the crash still does occur at least we'll have a better idea as to
> which scenario is causing the crash.  Kinda sucky that manufacturers can hack
> the Skia code to load fonts from random places.

Filed bug 641601.

Comment 29

[Scoobidiver (away)]

There have been one crash after the fix in 4.0b6pre/20110315:
bp-ae1f4c28-4266-48b0-9c9e-ef84f2110316

Comment 30

[Alon Zakai (:azakai)]

I guess there are multiple causes of this crash, we fixed part but not all so far.

Comment 31

[Brad Lassey [:blassey] (use needinfo?)]

let's update this bug's summary to reflect what it actually fixed and track any other crashes in another bug