719872 - [skia] Crash [@ SkTypeface::UniqueID] switching tabs to a Flash document

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Martijn Wargers (dead)]

I was getting this crash a couple of times on my LG Optimus Black.
You need to have the Setting Plugins to "Enabled" to test this.

Steps to reproduce:
- Visit http://people.mozilla.org/~mwargers/tests/plugins/flash/317375_flash_wmode.swf
- Switch to a different tab
- Switch back to the Flash file

I did crash a couple of times this way, but currently, it doesn't seem to crash anymore.

https://crash-stats.mozilla.com/report/index/bp-6dff8f4a-93c7-4dd8-a05e-7f8c22120120
0 	libxul.so 	SkTypeface::UniqueID 	gfx/skia/src/core/SkTypeface.cpp:56
1 	libxul.so 	SkPaint::descriptorProc 	gfx/skia/src/core/SkPaint.cpp:1380
2 	libxul.so 	SkPaint::getFontMetrics 	gfx/skia/src/core/SkPaint.cpp:1091
3 	libxul.so 	anp_getFontMetrics 	other-licenses/skia-npapi/ANPPaint.cpp:162
4 	libflashplayer.so 	libflashplayer.so@0x54ddff 	
5 	0 (deleted) 	0 @0x13f3ffe 	
6 	libflashplayer.so 	libflashplayer.so@0x54ddd3 	
7 	libflashplayer.so 	libflashplayer.so@0x532871 	
8 	libflashplayer.so 	libflashplayer.so@0x532863 	
9 	libflashplayer.so 	libflashplayer.so@0x234ddb 	
10 	libflashplayer.so 	libflashplayer.so@0x533cf7 	
11 	0 (deleted) 	0 @0x13f3ffe 	
12 	libflashplayer.so 	libflashplayer.so@0x75a606 	
13 	libflashplayer.so 	libflashplayer.so@0x234c9b 	
14 	libflashplayer.so 	libflashplayer.so@0x75a606 	
15 	libflashplayer.so 	libflashplayer.so@0x225feb 	
16 	libflashplayer.so 	libflashplayer.so@0x225fab 	
17 	libflashplayer.so 	libflashplayer.so@0x240067 	
18 	libflashplayer.so 	libflashplayer.so@0x75a606 	
19 	libflashplayer.so 	libflashplayer.so@0x75a606 	
20 	libflashplayer.so 	libflashplayer.so@0x5328d7

Comment 1

[Scoobidiver (away)]

It's currently #1 top crasher in today's build.

The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=58e933465c36&tochange=5c2bc94d359c

Comment 2

[Frank Wein [:mcsmurf]]

I get the same crash when going to http://www.bundesliga.de on a Samsung Galaxy S2 (landscape mode) and then scrolling down a bit after the page has finished loading. Plugins are enabled.

Comment 3

[Matt Woodrow (:mattwoodrow)]

I'm not familiar with this code. Where is the 'ANPPaint*' created, that libflashplayer is passing into anp_getFontMetrics?

If it's created by flash, then doesn't libflashplayer need to have been compiled against the same Skia version as we have in our tree?

Comment 4

[Joe Drew (not getting mail)]

snorp, I choose you!

Comment 5

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

dup of Bug 717060 ?

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> I'm not familiar with this code. Where is the 'ANPPaint*' created, that
> libflashplayer is passing into anp_getFontMetrics?
> 
> If it's created by flash, then doesn't libflashplayer need to have been
> compiled against the same Skia version as we have in our tree?

There are shims in other-licenses/skia-npapi that libflashplayer uses. This didn't used to crash, though, so I'd guess something changed in our Skia? Regardless, I'll take this one.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

With a local debug build I actually get the following:

Switching to Thread 21195]
0x72fab4b6 in SkFontHost::CreateTypeface(SkTypeface const*, char const*, void const*, unsigned int, SkTypeface::Style) ()
   from /Users/snorp/source/birch/objdir-android/dist/bin/libxul.so
(gdb) bt
#0  0x72fab4b6 in SkFontHost::CreateTypeface(SkTypeface const*, char const*, void const*, unsigned int, SkTypeface::Style) ()
   from /Users/snorp/source/birch/objdir-android/dist/bin/libxul.so
#1  0x72fa112e in SkTypeface::CreateFromName (name=<optimized out>, style=<optimized out>)
    at /Users/snorp/source/birch/gfx/skia/src/core/SkTypeface.cpp:65
#2  0x72e07374 in anp_createFromName (name=0x0, s=0) at /Users/snorp/source/birch/other-licenses/skia-npapi/ANPTypeface.cpp:32
#3  0x81d4e598 in ?? ()
#4  0x81d4e598 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

The 'name' passed in anp_createFromName isn't actually null. It's actually "Curlz MT" when I print it out. If I had to guess, I'd say the new Skia is not coping well when it can't find a given font.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ok, this appears to be the problem:

changeset:   84849:8de271eee34b
user:        Matt Woodrow <[e-mail]>
date:        Thu Jan 19 17:55:27 2012 +1300
summary:     Bug 716415 - Follow to include changes that I forgot to qref. r=bustage

diff --git a/gfx/skia/Makefile.in b/gfx/skia/Makefile.in
--- a/gfx/skia/Makefile.in
+++ b/gfx/skia/Makefile.in
@@ -301,24 +301,21 @@
        include/ports/SkTypeface_mac.h \
        $(NULL)
 CPPSRCS += \
        SkFontHost_mac_coretext.cpp \
        SkTime_Unix.cpp \
        $(NULL)
 endif
 
 ifeq (android,$(MOZ_WIDGET_TOOLKIT))
 CPPSRCS += \
-       SkFontHost_FreeType.cpp \
-       SkFontHost_android.cpp \
-       SkFontHost_gamma.cpp \
-       FontHostConfiguration_android.cpp \
+       SkFontHost_none.cpp \
        SkMMapStream.cpp \
        SkTime_Unix.cpp \
        $(NULL)

We need the font stuff to work (as it was before). What was the reason for using the null backend here?

Comment 10

[Matt Woodrow (:mattwoodrow)]

Sorry, I didn't realize there was other code depending on our skia codebase.

FontHostConfiguration_android (required by SkFontHost_android.cpp) has a dependency on expat.h (and XML parser).

Their code is expecting XML_Char to be defined as char*, while our implementation of expat (see expat_config.h) is using PRUnichar.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ugh, and PRUnichar is two bytes, I presume? Do we have a plan for working around this problem? s/XML_Char/char*/ maybe in the one file maybe?

Comment 12

[Matt Woodrow (:mattwoodrow)]

Yep.

I'm not sure, the XML parser callbacks pass 2 byte chars, and the skia implementations of those callbacks are using one byte chars.

I can't think of anything other than rewriting the majority of that file. Unless there is a system version of the XML parser that we can link to instead.

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix Skia font backend on Android

Comment 14

[Cristian Nicolae (:xti)]

This crash still occurs on the latest Nightly build.

I opened and enabled flash plugin for http://gotmilk.com. While the page content is loading, Fennec just crashes without performing any additional steps.

--
Mozilla/5.0 (Android;Linux armv7l;rv:12.0a1)Gecko/20120125
Firefox/12.0a1 Fennec/12.0a1
Device: Samsung Galaxy S
OS: Android 2.2

Comment 15

[Scoobidiver (away)]

(In reply to Cristian Nicolae (:xti) from comment #14)
> This crash still occurs on the latest Nightly build.
The patch hasn't been reviewed.

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 591663 [details] [diff] [review]
Fix Skia font backend on Android

bsmedberg: Matt wasn't comfortable reviewing the stringy bits of the patch, and Brad suggested that you might be able to.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

We have a problem here. The xml files it tries to read don't exist on older Android. I'm going to patch it to work with that (fallback to whatever it did in older versions).

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix Skia font backend on Android

Ok, this one just falls back to the hardcoded list that Skia used before and works everywhere

Comment 19

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 592060 [details] [diff] [review]
Fix Skia font backend on Android

Review of attachment 592060 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good for now, as long as you add a patch file to update.sh :)

I'll talk to the google guys too, and see if they are aware of this (possibly unintentional) android version requirement.

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b735935813b2
http://hg.mozilla.org/integration/mozilla-inbound/rev/8e80c7006eba

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b735935813b2
https://hg.mozilla.org/mozilla-central/rev/8e80c7006eba

Comment 22

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Need to wait until tomorrow to verify; Was able to hit this bug on Samsung Galaxy S II with http://www.kongozan.com/live/index2.html and tapping on the two flash plugins.

Comment 23

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Today's build does not seem to crash with the repro steps in Comment 22.