Last Comment Bug 921670 - text in Flash content is mojibake
: text in Flash content is mojibake
Status: VERIFIED FIXED
: dev-doc-needed, regression, site-compat
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Android
: -- normal with 1 vote (vote)
: mozilla29
Assigned To: George Wright (:gw280) (:gwright)
:
Mentors:
http://www.investopedia.com
: 926031 958946 (view as bug list)
Depends on:
Blocks: 848491
  Show dependency treegraph
 
Reported: 2013-09-27 16:10 PDT by Kevin Brosnan [:kbrosnan]
Modified: 2014-02-06 14:27 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
wontfix
+
wontfix
+
verified
verified
verified
26+
+


Attachments
device-2013-09-27-160641.png (273.56 KB, image/png)
2013-09-27 16:10 PDT, Kevin Brosnan [:kbrosnan]
no flags Details
device-2013-10-09-120801.png (154.06 KB, image/png)
2013-10-09 09:08 PDT, Aaron Train [:aaronmt]
no flags Details
Better screen shot (83.51 KB, image/png)
2013-10-15 09:11 PDT, Kevin Brosnan [:kbrosnan]
no flags Details
Forward port old SkFontHost_android (20.38 KB, patch)
2014-01-15 19:41 PST, George Wright (:gw280) (:gwright)
snorp: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Kevin Brosnan [:kbrosnan] 2013-09-27 16:10:31 PDT
Created attachment 811428 [details]
device-2013-09-27-160641.png

Flash content is displaying mojibake instead of text. The stock chart in the screenshot is Flash.
Comment 1 Alex Keybl [:akeybl] 2013-10-08 06:41:55 PDT
(In reply to Kevin Brosnan [:kbrosnan] from comment #0)
> Created attachment 811428 [details]
> device-2013-09-27-160641.png
> 
> Flash content is displaying mojibake instead of text. The stock chart in the
> screenshot is Flash.

What set of devices/content/versions/etc. are impacted? With the Summit behind us, will you have the time to identify a regression window?
Comment 2 Alex Keybl [:akeybl] 2013-10-09 08:39:00 PDT
Nevermind, I think I just remembered you're on PTO. Hopefully Aaron can help out here.
Comment 3 Aaron Train [:aaronmt] 2013-10-09 09:08:47 PDT
Created attachment 814941 [details]
device-2013-10-09-120801.png

Not entirely sure what I'm looking for here. The chart looks fine to me? Where is mojibake in the screenshot?

From my Galaxy SIV zoomed in on the chart on Nightly

WFM?
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2013-10-10 10:56:29 PDT
George, does this look related your skia font wrapping?
Comment 5 Alex Keybl [:akeybl] 2013-10-14 07:52:56 PDT
Second to last beta coming up, would love to find out if there's anything low risk we can do before Wednesday.
Comment 6 Kevin Brosnan [:kbrosnan] 2013-10-14 14:01:20 PDT
*** Bug 926031 has been marked as a duplicate of this bug. ***
Comment 7 Kevin Brosnan [:kbrosnan] 2013-10-14 14:08:41 PDT
Unsure how duping a bug cleared all those flags...
Comment 8 David Lawrence [:dkl] 2013-10-14 21:55:46 PDT
(In reply to Kevin Brosnan [:kbrosnan] from comment #7)
> Unsure how duping a bug cleared all those flags...

Glitch in the recent tracking flag migration performed over the weekend. I am going through the list restoring the ones that were acciddently cleared. Thanks for putting these back for us.

dkl
Comment 9 George Wright (:gw280) (:gwright) 2013-10-15 09:03:45 PDT
(In reply to Aaron Train [:aaronmt] from comment #3)
> Created attachment 814941 [details]
> device-2013-10-09-120801.png
> 
> Not entirely sure what I'm looking for here. The chart looks fine to me?
> Where is mojibake in the screenshot?
> 
> From my Galaxy SIV zoomed in on the chart on Nightly

I'm also not sure what the bug is here. Kevin, can you provide details please?
Comment 10 Kevin Brosnan [:kbrosnan] 2013-10-15 09:11:13 PDT
Created attachment 817214 [details]
Better screen shot

The text inside the Flash chart is not displayed correctly.
Comment 11 Kevin Brosnan [:kbrosnan] 2013-10-15 17:54:36 PDT
I can't get a regression range due to bug 890272 a Flash crash on activation from July-3 till July-17 where bug 890272 was fixed. This was introduced somewhere in that two week period.
Comment 12 Alex Keybl [:akeybl] 2013-10-16 07:07:05 PDT
We'll track, but this is looking more and more like a wontfix for FF25. Hoping George Wright can take a look and compare to a Desktop flash instance of the same, for reproducibility.
Comment 13 Kevin Brosnan [:kbrosnan] 2013-10-24 17:40:26 PDT
Worth a relnote?

"Text in Flash elements may display as boxes on Samsung devices."
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2013-11-06 18:23:06 PST
George - is this on your radar for the upcoming week or two?  Firefox 26 is in beta now and while this has already shipped in FF25 it would still be good to get a fix given the popularity of Samsung devices.
Comment 15 George Wright (:gw280) (:gwright) 2013-11-07 09:01:39 PST
Sure, I'll look at it tomorrow.
Comment 16 barney.qrp 2013-11-15 01:28:55 PST
Update, I carried out a complete wipe of my Galaxy S3 and installed FF25 and flash 11.1.115.81 and the problem had gone. I then installed FF26 and all still fine. I then did an official OS upgrade to Jelly Bean 4.3 and all was still fine. I can not pin point it but it is almost certainly a 3rd party app causing this behaviour, I am working my way 1 app at a time to try and find it.
Comment 17 Kevin Brosnan [:kbrosnan] 2013-11-18 15:55:09 PST
changeset:   137328:28957db0a941
user:        George Wright <gw@gwright.org.uk>
date:        Thu Apr 25 20:47:06 2013 -0400
summary:     Bug 848491 - Re-apply bug 687188 - Expand the gradient cache by 2 to store 0/1 colour stop values for clamping.


This appears to be device independent.
Comment 18 Kevin Brosnan [:kbrosnan] 2013-11-19 09:09:15 PST
Any update on comment 15?
Comment 19 George Wright (:gw280) (:gwright) 2013-11-19 09:34:11 PST
Not really, I couldn't reproduce it on my device :( Can I get detailed steps to reproduce?
Comment 20 George Wright (:gw280) (:gwright) 2013-11-19 09:35:32 PST
(In reply to Kevin Brosnan [:kbrosnan] from comment #17)
> changeset:   137328:28957db0a941
> user:        George Wright <gw@gwright.org.uk>
> date:        Thu Apr 25 20:47:06 2013 -0400
> summary:     Bug 848491 - Re-apply bug 687188 - Expand the gradient cache by
> 2 to store 0/1 colour stop values for clamping.
> 
> 
> This appears to be device independent.

Also there's no way that patch is causing this regression. It's likely caused by a related patch, though, which was to update Skia.
Comment 21 Kevin Brosnan [:kbrosnan] 2013-11-19 18:40:03 PST
I can reproduce this on a Nexus 4 running Android 4.3 and a HTC t-mobile G2 among other phones. Yes I made a slight mistake in the bisect, the issue lies between  http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5f652fba751d&tochange=28957db0a941
Comment 22 George Wright (:gw280) (:gwright) 2013-11-19 19:34:11 PST
OK great, I will get a nexus 4 then :)
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2013-11-22 20:08:32 PST
We're now past the point of taking speculative fixes for FF26 or waiting for George to get a device and investigate.  Not marking this tracking for FF27, but a low risk uplift might be considered so please nominate when there's a patch.
Comment 24 Kevin Brosnan [:kbrosnan] 2013-11-25 10:00:21 PST
This is an common issue in the support forums.
Comment 25 Kevin Brosnan [:kbrosnan] 2013-12-10 10:18:31 PST
Any chance of getting this fixed on Firefox 27? This is a very visible regression.
Comment 26 George Wright (:gw280) (:gwright) 2013-12-10 13:06:50 PST
So here's the problem - we are currently updating our in-tree version of Skia as the one we are currently using is seriously old. The fix for this would be different on the old Skia vs. the new Skia, and I'd have to fix it anyway with the new Skia (which is what I was planning on doing). 

As far as I'm aware, there is no plan right now to land the new Skia on 27 (I believe we're targeting 28), and given our limited team resources I'm not too fond of fixing this same issue twice on different Skia revisions, as all the font management code has changed upstream.

What do we want to do here? Would it be completely crazy to try and target the Skia update for 27?
Comment 27 Milan Sreckovic [:milan] 2013-12-11 12:07:28 PST
George is going to investigate a "non upstream update of Skia" solution.  Doing a full update in Beta (or even Aurora) is not the safest approach, but neither is letting this bug stay in the released product for six months.  Keep you posted.
Comment 28 Aaron Train [:aaronmt] 2014-01-12 08:54:25 PST
*** Bug 958946 has been marked as a duplicate of this bug. ***
Comment 29 bhavana bajaj [:bajaj] 2014-01-13 11:25:50 PST
NI on George to see if there are any updates here given we are only a few beta's away before Firefox 27 is shipped ?
Comment 30 George Wright (:gw280) (:gwright) 2014-01-15 19:38:47 PST
I have a hacky fix. It's not ideal but it'll serve to bridge the gap until we can find a more optimal/permanent solution.
Comment 31 George Wright (:gw280) (:gwright) 2014-01-15 19:41:10 PST
https://tbpl.mozilla.org/?tree=Try&rev=cb247d615a35
Comment 32 George Wright (:gw280) (:gwright) 2014-01-15 19:41:41 PST
Created attachment 8360842 [details] [diff] [review]
Forward port old SkFontHost_android
Comment 33 James Willcox (:snorp) (jwillcox@mozilla.com) 2014-01-16 06:31:31 PST
Comment on attachment 8360842 [details] [diff] [review]
Forward port old SkFontHost_android

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

Ok modulo the one issue. Ugly though.

::: gfx/skia/moz.build
@@ +238,4 @@
>      ]
>      # left out of UNIFIED_SOURCES for now to avoid having to patch skia. Should revisit eventually.
>      SOURCES += [
> +        'src/ports/SkFontHost_android_old.cpp',

Is this really on for Android and Gonk both? Should probably only be for Android.
Comment 34 Milan Sreckovic [:milan] 2014-01-16 07:49:25 PST
(In reply to George Wright (:gw280) from comment #30)
> I have a hacky fix. It's not ideal but it'll serve to bridge the gap until
> we can find a more optimal/permanent solution.

The optimal solution exists once we update Skia to upstream, or is it still as ugly there?
Comment 35 George Wright (:gw280) (:gwright) 2014-01-20 20:10:48 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/c62b23f22aea
Comment 36 George Wright (:gw280) (:gwright) 2014-01-20 20:13:02 PST
(In reply to Milan Sreckovic [:milan] from comment #34)
> (In reply to George Wright (:gw280) from comment #30)
> > I have a hacky fix. It's not ideal but it'll serve to bridge the gap until
> > we can find a more optimal/permanent solution.
> 
> The optimal solution exists once we update Skia to upstream, or is it still
> as ugly there?

Already spoke to Milan offline about this, but posting the reply here for all to see; current skia doesn't fix this issue and in fact makes it worse, as the font code has mostly been refactored upstream. Until we assess the extent of the changes and how it affects font selection on Android, we don't know what the optimal solution will be. My guess is that it will be either to fix the Linux fonthost that is maintained upstream to do font selection the way we'd like it to be done, or implement the skia-npapi ANPTypeface methods using gfxFont then returning an SkTypeface wrapping the cairo_font_face_t that is returned.
Comment 37 Carsten Book [:Tomcat] - PTO-back Sept 4th 2014-01-21 04:21:51 PST
https://hg.mozilla.org/mozilla-central/rev/c62b23f22aea
Comment 38 George Wright (:gw280) (:gwright) 2014-01-21 10:29:03 PST
Comment on attachment 8360842 [details] [diff] [review]
Forward port old SkFontHost_android

[Approval Request Comment]
Regression caused by (bug #): 903993
User impact if declined: Some fonts won't display correctly in Flash content
Testing completed (on m-c, etc.): m-c tests + local flash tests
Risk to taking this patch (and alternatives if risky): Should be low risk; this code has been shipped before
String or IDL/UUID changes made by this patch: None
Comment 39 bhavana bajaj [:bajaj] 2014-01-22 12:28:21 PST
Comment on attachment 8360842 [details] [diff] [review]
Forward port old SkFontHost_android

Approving given this has been a common issue in support forums. Also requesting QA verification here.
Comment 41 Paul Feher 2014-01-24 00:47:56 PST
Verified fixed using Nexus 4 (Android 4.4.2) on:
 27.0 Beta 9 (2014-01-24)
 Nightly 29.0a1 (2014-01-23)
 Aurora 28.0a2 (2014-01-23)
Comment 42 Páll Haraldsson 2014-02-05 06:17:28 PST
>Verified fixed using Nexus 4 (Android 4.4.2) on:
> 27.0 Beta 9 (2014-01-24)

I wander since this is before the merge date isn't this fixed/or should be, in current (mobile) Firefox and https://www.mozilla.org/en-US/mobile/27.0/releasenotes/

needs to be updated?

I also saw:
Target Milestone: 	mozilla29

I'm not putting pressure on, keep up the good work. Maybe the fix got disabled for a reason?
Comment 43 Kevin Brosnan [:kbrosnan] 2014-02-05 12:05:04 PST
Bugzilla can be hard to decode. Target milestone is the release that the check was first checked into, in this case Firefox 29 was nightly. The "Tracking Flags:" section status-firefox# shows when/if a fix was uplifted. In this case it shows that it was uplifted to Firefox 27 and verified to work.
Comment 44 Kevin Brosnan [:kbrosnan] 2014-02-06 14:17:36 PST
I don't think this needs docs. It was a couple release regression. Users should just update to a current version from the Play Store.
Comment 45 Kohei Yoshino [:kohei] 2014-02-06 14:27:44 PST
I'll add this to https://developer.mozilla.org/en-US/Firefox/Releases/25/Site_Compatibility for the record, along with other regressions.

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