Last Comment Bug 719872 - [skia] Crash [@ SkTypeface::UniqueID] switching tabs to a Flash document
: [skia] Crash [@ SkTypeface::UniqueID] switching tabs to a Flash document
Status: VERIFIED FIXED
[native-crash]
: crash, crashreportid, regression, topcrash
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Android
: -- critical (vote)
: mozilla12
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
: Milan Sreckovic [:milan]
Mentors:
http://people.mozilla.org/~mwargers/t...
: 717060 (view as bug list)
Depends on:
Blocks: 721741
  Show dependency treegraph
 
Reported: 2012-01-20 09:55 PST by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2012-02-01 09:54 PST (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Fix Skia font backend on Android (10.29 KB, patch)
2012-01-25 17:22 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Splinter Review
Fix Skia font backend on Android (19.60 KB, patch)
2012-01-26 22:28 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
matt.woodrow: review+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2012-01-20 09:55:48 PST
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) 2012-01-21 00:22:55 PST
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] 2012-01-21 07:09:09 PST
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) 2012-01-21 17:12:50 PST
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) 2012-01-21 21:20:27 PST
snorp, I choose you!
Comment 5 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-01-22 16:08:13 PST
dup of Bug 717060 ?
Comment 6 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-23 07:22:27 PST
(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) (jwillcox@mozilla.com) 2012-01-23 11:01:34 PST
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 8 Doug Turner (:dougt) 2012-01-23 14:11:59 PST
*** Bug 717060 has been marked as a duplicate of this bug. ***
Comment 9 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-23 14:26:06 PST
Ok, this appears to be the problem:

changeset:   84849:8de271eee34b
user:        Matt Woodrow <mwoodrow@mozilla.com>
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) 2012-01-23 14:53:48 PST
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) (jwillcox@mozilla.com) 2012-01-24 06:31:34 PST
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) 2012-01-24 12:51:36 PST
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) (jwillcox@mozilla.com) 2012-01-25 17:22:50 PST
Created attachment 591663 [details] [diff] [review]
Fix Skia font backend on Android
Comment 14 Cristian Nicolae (:xti) 2012-01-26 02:30:17 PST
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) 2012-01-26 02:49:59 PST
(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) (jwillcox@mozilla.com) 2012-01-26 21:21:41 PST
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) (jwillcox@mozilla.com) 2012-01-26 22:00:59 PST
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) (jwillcox@mozilla.com) 2012-01-26 22:28:59 PST
Created attachment 592060 [details] [diff] [review]
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) 2012-01-30 13:25:44 PST
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 22 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-01-31 14:13:11 PST
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) 2012-02-01 09:54:58 PST
Today's build does not seem to crash with the repro steps in Comment 22.

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