Closed Bug 921670 Opened 11 years ago Closed 10 years ago

text in Flash content is mojibake

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla29
Tracking Flags:
Tracking Status
firefox24 --- unaffected
firefox25 + wontfix
firefox26 + wontfix
firefox27 + verified
firefox28 --- verified
firefox29 --- verified
relnote-firefox --- 26+
fennec + ---

People

(Reporter: kbrosnan, Assigned: gw280)

References

(
URL
)

Details

(Keywords: dev-doc-needed, regression, site-compat)

Attachments

(2 files, 2 obsolete files)

Better screen shot
83.51 KB, image/png
Details
Forward port old SkFontHost_android
20.38 KB, patch
snorp
: review+
bajaj
: approval-mozilla-aurora+
bajaj
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached image device-2013-09-27-160641.png (obsolete) — 
Flash content is displaying mojibake instead of text. The stock chart in the screenshot is Flash.
(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?
Flags: needinfo?(kbrosnan)
Keywords: qawanted
Nevermind, I think I just remembered you're on PTO. Hopefully Aaron can help out here.
Flags: needinfo?(kbrosnan) → needinfo?(aaron.train)
Flags: needinfo?(aaron.train)
Keywords: qawanted
Attached image device-2013-10-09-120801.png (obsolete) — 
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?
George, does this look related your skia font wrapping?
Flags: needinfo?(gwright)
Second to last beta coming up, would love to find out if there's anything low risk we can do before Wednesday.
tracking-fennec: ? → ---
status-firefox24: unaffected → ---
status-firefox25: affected → ---
status-firefox26: affected → ---
status-firefox27: affected → ---
tracking-firefox25: ? → ---
Unsure how duping a bug cleared all those flags...
tracking-fennec: --- → ?
status-firefox24: --- → unaffected
status-firefox25: --- → affected
status-firefox26: --- → affected
status-firefox27: --- → affected
tracking-firefox25: --- → ?
(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
(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?
Flags: needinfo?(gwright) → needinfo?(kbrosnan)
Attached image Better screen shot — 
The text inside the Flash chart is not displayed correctly.
Attachment #811428 - Attachment is obsolete: true
Attachment #814941 - Attachment is obsolete: true
Flags: needinfo?(kbrosnan)
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.
Keywords: regressionwindow-wanted
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.
Assignee: nobody → gwright
tracking-firefox25: ? → +
tracking-fennec: ? → 25+
Worth a relnote?

"Text in Flash elements may display as boxes on Samsung devices."
relnote-firefox: --- → ?
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.
Flags: needinfo?(gwright)
Sure, I'll look at it tomorrow.
Flags: needinfo?(gwright)
tracking-fennec: 25+ → +
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.
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.
Blocks: 848491
Any update on comment 15?
Flags: needinfo?(gwright)
Not really, I couldn't reproduce it on my device :( Can I get detailed steps to reproduce?
Flags: needinfo?(gwright)
(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.
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
OK great, I will get a nexus 4 then :)
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.
status-firefox26: affected → wontfix
This is an common issue in the support forums.
tracking-firefox27: --- → ?
Any chance of getting this fixed on Firefox 27? This is a very visible regression.
Flags: needinfo?(gwright)
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?
Flags: needinfo?(gwright)
Component: General → Graphics
Product: Firefox for Android → Core
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.
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 ?
Flags: needinfo?(gwright)
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.
Flags: needinfo?(gwright)
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.
Attachment #8360842 - Flags: review?(snorp) → review+
(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?
(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.
https://hg.mozilla.org/mozilla-central/rev/c62b23f22aea
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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
Attachment #8360842 - Flags: approval-mozilla-beta?
Attachment #8360842 - Flags: approval-mozilla-aurora?
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.
Attachment #8360842 - Flags: approval-mozilla-beta?
Attachment #8360842 - Flags: approval-mozilla-beta+
Attachment #8360842 - Flags: approval-mozilla-aurora?
Attachment #8360842 - Flags: approval-mozilla-aurora+
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)
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
status-firefox28: fixed → verified
status-firefox29: fixed → verified
>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?
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.
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.
I'll add this to https://developer.mozilla.org/en-US/Firefox/Releases/25/Site_Compatibility for the record, along with other regressions.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: