766604 - Installing an app with UTF-8 characters does not show correct characters to the user on the shortcut, shortcut message, and app in about:apps

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Jason Smith [:jsmith]]

Steps:

1. Go to http://ed.agadak.net/app/
2. Install Ed's app that should contain Japanese characters in the name

Expected:

The Japanese characters should be shown correctly on the shortcut message, the shortcut itself, and the app in about:apps.

Actual:

See screenshots. Garbled incorrect characters are shown instead of the correct Japanese characters.

Comment 1

[Jason Smith [:jsmith]]

Attachment: UTF 8 Weird Chars 1

Comment 2

[Jason Smith [:jsmith]]

Attachment: UTF 8 Weird Chars 2

Comment 3

[Wesley Johnston (:wesj)]

I think we're mangling these when we save them to disk. Need to dig more.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

This converts the title to UTF-8 before sending it to Java. That works, but I'm not exactly sure why. Hoping maybe you know mbrubeck? The downloaded manifest can be in any encoding. That's used to show the install prompt, which works just fine.

We save it as UTF-8:

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#248

Then, we read that back from the disk.

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#166

AFAICT, the string should be UTF-8 at that point, but I'm not sure how to query it exactly (the nsIChannel reports nothing).

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#116

When we finally send this to Java, it expects UTF-16 which may be our problem? Except this patch just converts to UTF-8....

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 645468 [details] [diff] [review]
WIP Patch

Wrong patch....

Comment 6

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 645469 [details] [diff] [review]
WIP Patch

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

::: mobile/android/chrome/content/browser.js
@@ +5962,5 @@
>  
> +          // The manifest is stored as UTF-8. This is a responseText value from reading the manifest file, which
> +          // should be UTF-8. sendMessageToJava expects UTF-16 strings, so I'm not sure why UTF-8 fixes this
> +          let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].createInstance(Ci.nsIScriptableUnicodeConverter);
> +          converter.charset = "UTF-8";

If I understand correctly, this is converting *from* UTF-8 to UTF-16.  Our internal Unicode strings are always UTF-16, so ConvertToUnicode always converts *to* UTF-16, and ConvertFromUnicode always converts *from* UTF-16.  So this code looks exactly correct, except for the comment.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch

Whoops. Forgot about this. Here's the same patch then, with the comment fixed.

Comment 9

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d2206c84e31

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 647320 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Always present
User impact if declined: Apps using UTF-8 characters will have them scrambled
Testing completed (on m-c, etc.): landed on mc today
Risk to taking this patch (and alternatives if risky: Low risk. Mobile only. Should only help.
String or UUID changes made by this patch: None.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d2206c84e31

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eea777847e40