1152552 - Unable to install apps with names that contain chars outside range of U+0080/U+0100 - U+FFFF

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P1)

Description

[(not reading bugmail) Nick Desaulniers [:\n]]

The app appears to install, (the onsuccess callback fires), but the app isn't actually installed, an amongst a large stack trace I see:

/Users/Nicholas/Library/Caches/TemporaryItems/tmpicon-5.png
Error: Unable to write image to file /Users/Nicholas/Library/Caches/TemporaryItems/Mykzilla  allizkyM.app/Contents.cns9u
  /Users/Nicholas/Library/Caches/TemporaryItems/Mykzilla  allizkyM.app/Contents
[Parent 87501] WARNING: char16_t out of char range; high bits of data lost: 0x2013: file /Users/Nicholas/mozilla/mozilla-central-git/js/xpconnect/src/XPCConvert.cpp, line 363
/Users/Nicholas/Library/Caches/TemporaryItems/tmpicon-6.png
Error: Unable to write image to file /Users/Nicholas/Library/Caches/TemporaryItems/Mykzilla  allizkyM.app/Contents.qLdi3
  /Users/Nicholas/Library/Caches/TemporaryItems/Mykzilla  allizkyM.app/Contents

This is a debug build of nightly with the latest commit from today:

commit 75871f42cb7ef38ffcf2491c1ec2e42835bae37f
Merge: 014bb16 599d0d9
Author: Ryan VanderMeulen <[e-mail]>
Date:   Wed Apr 8 12:38:35 2015 -0400

This happens both from a e10s window AND non e10s window.

Comment 1

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: Full stack trace

Comment 2

[(not reading bugmail) Nick Desaulniers [:\n]]

Oddly, I am able to install Kumar's app, but not Myk's.

Kumar's: http://webapprt-win.paas.allizom.org/
Myk's: http://www.mykzilla.org/app/

I wonder if the temp app file, which looks like it gets its name from the <title> having special characters has anything to do with this:

➜  mozilla-central-git git:(master) ✗ ls ~/Library/Caches/TemporaryItems/
Mykzilla ? allizkyM.app           tmpicon-1.png           tmpicon.png

Comment 3

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: GDB Back traces

Comment 4

[(not reading bugmail) Nick Desaulniers [:\n]]

sure enough, the stack trace complains about:

[Parent 87570] WARNING: char16_t out of char range; high bits of data lost: 0x2013: file /Users/Nicholas/mozilla/mozilla-central-git/js/xpconnect/src/XPCConvert.cpp, line 363

0x2013 in JavaScript is `"\u2013" === "–"` yields true, and that hyphen is indeed the one in Myk's title!  This is preventing the temporary app from being saved correctly!

Error: Unable to write image to file /Users/Nicholas/Library/Caches/TemporaryItems/Mykzilla  allizkyM.app/Contents.qLdi3
  /Users/Nicholas/Library/Caches/TemporaryItems/Mykzilla  allizkyM.app/Contents

^ notice it says `Mykzilla  allizkyM.app/Contents` when it should say `Mykzilla – allizkyM.app/Contents`.

The simple fix is for Myk to not have characters in the <title> outside of the range of U+0080/U+0100 - U+FFFF as defined here: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#356 but this has bigger implications for webapprt!

Comment 5

[(not reading bugmail) Nick Desaulniers [:\n]]

Also, gdb logs the character as 8211 which is decimal of 0x2013, what the "%x" format flag prints the char as, as seen in the stack trace (stderr): http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#362

Comment 6

[(not reading bugmail) Nick Desaulniers [:\n]]

I'm quite sure that this is the function that is failing [0].  It puts a bunch of things in a temp folder, than tries the install and cleanup.  The temp dir is created, but never cleaned up, which is odd, because the error handling seems correct.  Using the browser debugger [1], it is difficult to trace execution because the dev tools (incorrectly?) jump into some other source file related to promises when simply stepping over the next statement...  Also, to the debugger, this file [0] appears to be named NativeApp.jsm which I assume is some black magic voodoo since the only NativeApp.jsm I see in MXR [2] is not the same source I see in the debugger, but [0] is?  Oh, they're concatenated.  Look at that.


[0] http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/MacNativeApp.js#49
[1] https://developer.mozilla.org/en-US/docs/Debugging_JavaScript#JavaScript_Debugger
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/NativeApp.jsm

Comment 7

[(not reading bugmail) Nick Desaulniers [:\n]]

We fail on the call to `yield this._getIcon(tmpDir);` and that's where I see the messages printed to stderr from Comment 4. [0]

[0] http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/MacNativeApp.js#84

Comment 8

[(not reading bugmail) Nick Desaulniers [:\n]]

_getIcon fails twice when calling `yield this._processIcon(mimeType, icon, aTmpDir);`. [0][1]

In _processIcon, we invoke the process `sips` [2] which leads me to believe this is an OSX specific bug, since according to a comment in a test [3] we do this only for OSX.

I'll have to keep chasing the rabbit down the rabbit hole, but it looks like `tmpIconPath` [4] contains the bad character, `/Users/Nicholas/Library/Caches/TemporaryItems/Mykzilla – allizkyM.app/Contents/Resources/appicon.icns`.  I'm not sure yet whether the nsIProcess is barfing, or sips itself.  Either way, it seems bad that we conditionally reject a promise [5], but then don't ever check the yielded value for any errors that may have occurred [0][1].

[0] http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/NativeApp.jsm#187
[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/NativeApp.jsm#195
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/MacNativeApp.js#321
[3] http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/tests/test_hosted_icons.xul#70
[4] http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/MacNativeApp.js#323
[5] http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/MacNativeApp.js#311

Comment 9

[Marco Castelluccio [:marco]]

> Either way, it seems bad that we conditionally reject a promise [5], but then don't ever check the yielded value for any errors that may have occurred [0][1].

Aren't we propagating the promise rejection from [5] to the _getIcon caller?

Comment 10

[(not reading bugmail) Nick Desaulniers [:\n]]

It's possible (frankly, I don't fully understand how errors are propagated through generators and our Promise implementation (Promise.defer()?)).  I do know that eventually, the onsuccess callback is called, then onerror should have been, since the app was not actually installed.

Comment 11

[(not reading bugmail) Nick Desaulniers [:\n]]

Setting a break point as soon as we enter C++, `nsProcess::RunAsync` [0], from JS [1], the warning `[Parent 58390] WARNING: char16_t out of char range; high bits of data lost: 0x2013: file /Users/Nicholas/mozilla/mozilla-central-git/js/xpconnect/src/XPCConvert.cpp, line 363` has already been printed.  In the JS, I console.log before calling process.runAsync, and it's immediately before the warning.

It looks like we'll have to sanitize the temp dir [2] to only contain charcodes < \u0080.

Note to self for later:

function printUnicode (str) {
  for (var i = 0; i < str.length; ++i)
    console.log(String.charCodeAt(str[i]).toString(16))
}

[0] http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsProcessCommon.cpp#354
[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/MacNativeApp.js#321
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/MacNativeApp.js#71

Comment 12

[(not reading bugmail) Nick Desaulniers [:\n]]

str = 'Mykzilla – allizkyM.app/Contents'
// \u0080 === 8*16 === 128
str.split('').filter(function (char) { return String.charCodeAt(char) < 128 }).join('')

Comment 13

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: patch v1

This works, but on OSX the app name is now missing unicode characters, which will be a problem for non-ascii locales... maybe there's a better way to sanitize just the second arg to getFile [0] rather than modify this.appNameAsFileName?

[0] http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/MacNativeApp.js#71

Comment 14

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: patch v2

indeed!

Comment 15

[Marco Castelluccio [:marco]]

Comment on attachment 8596282 [details] [diff] [review]
patch v2

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

Can you also write a test for this?

I guess the problem isn't the content of the <title> element but the manifest.name property, you could verify it by installing "app2" from www.mykzilla.org/app/.

Comment 16

[(not reading bugmail) Nick Desaulniers [:\n]]

I can install app2 from the installed version of mykzilla, even!  Where should the test go, what kind of test should it be, and will I be able to run it or will I be blocked by: https://bugzilla.mozilla.org/show_bug.cgi?id=1125394 ?

Comment 17

[Marco Castelluccio [:marco]]

The test should go in toolkit/webapps/tests, it should be a simple test like test_hosted.xul.
You can directly copy test_hosted.xul and modify the name of the testing app to include an "invalid" character.

You should be able to run these tests despite bug 1125394 because they're simple chrome mochitests and not webapprt tests.

Note that during updates we're using a subdirectory of the installation directory, so SIPS could fail during updates as well (because the installation directory could contain these characters).
To fix the problem both for installations and updates, you could simply modify _processIcon ( http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/MacNativeApp.js#294) instead of modifying the name of the tmp directory:
 - Copy the icon in a "safe" temporary directory
 - Run SIPS with this icon
 - Copy the resulting icon in aDir

(To copy the files, you can use OS.File: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_the_main_thread)

Comment 18

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: test_non_ascii_app_name___test_hosted.diff

Marco, I simply copied toolkit/webapps/tests/test_hosted.xul to a new file, toolkit/webapps/tests/test_non_ascii_app_name.xul and made a few changes.  The test doesn't seem to run correctly when I run `./mach mochitest toolkit/webapps/tests/test_non_ascii_app_name.xul`.  What am I doing wrong?

Comment 19

[Marco Castelluccio [:marco]]

You also need to add it to the tests list in toolkit/webapps/tests/chrome.ini.

Comment 20

[(not reading bugmail) Nick Desaulniers [:\n]]

indeed, the test catches the failure during upgrade.  I'll rewrite the patch to handle this at install and upgrade.  Good catch!

Comment 21

[(not reading bugmail) Nick Desaulniers [:\n]]

Running into the need for a mkdir -p.  Moving the sanitization code into toolkit/webapps/MacNativeApp.js _processIcon works for installs because the install path is something like good/good/good/needs_sanitation, but fails for updates since the directory is good/good/good/needs_sanitation/update and in that case trying to create good/good/good/sanitzed/update fails because good/good/good/sanitzed does not yet exist.  OS.File.makeDir takes an optional `from` key value pair, but I'm afraid it won't be portable to Windows. [0]  Actually, now that I think about it, this bug is OSX specific because this only affects forking out to sips.  Ok, let me try that.

[0] https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_the_main_thread#OS.File.makeDir%28%29

Comment 22

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: patch v3

even better than trying to sanitize the dir name, just use the system temp dir!

Comment 23

[Marco Castelluccio [:marco]]

Comment on attachment 8598301 [details] [diff] [review]
patch v3

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

::: toolkit/webapps/MacNativeApp.js
@@ +307,5 @@
>        // icon was successfully converted.
>        OS.File.exists(tmpIconPath).then((aExists) => {
>          if (aExists) {
> +          OS.File.move(tmpIconPath, finalIconPath).then(
> +            () => deferred.resolve(), err => deferred.reject(err));

Very nit: *() => deferred.resolve() could simply be *deferred.resolve*.

::: toolkit/webapps/tests/test_non_ascii_app_name.xul
@@ +1,1 @@
> +<?xml version="1.0"?>

I assume this is a copy of test_hosted.xul with a different app name, so I'm not looking at it in detail.

Comment 24

[(not reading bugmail) Nick Desaulniers [:\n]]

(In reply to Marco Castelluccio [:marco] from comment #23)
> Very nit: *() => deferred.resolve() could simply be *deferred.resolve*.

Excellent catch!

> I assume this is a copy of test_hosted.xul with a different app name, so I'm
> not looking at it in detail.

Yes, see the diff between the two tests that I attached.

Comment 25

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: patch v4

Comment 26

[(not reading bugmail) Nick Desaulniers [:\n]]

The diff file was created by the command:

diff toolkit/webapps/tests/test_non_ascii_app_name.xul toolkit/webapps/tests/test_hosted.xul > test_non_ascii_app_name___test_hosted.diff

Comment 27

[Marco Castelluccio [:marco]]

Comment on attachment 8600064 [details] [diff] [review]
patch v4

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

Thank you!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

1) checkin-needed is a bug keyword (and plays more nicely with our bug marking tools)
2) please provide a link to a green Try run for this patch

Comment 29

[(not reading bugmail) Nick Desaulniers [:\n]]

@Ryan, why did I not need a try run for https://bugzilla.mozilla.org/show_bug.cgi?id=768521#c19 ?  Was it because there was no new test added there?  How do I push to try?  Can I do that from a git mirror of m-c?

Comment 30

[Ryan VanderMeulen [:RyanVM]]

(In reply to Nick Desaulniers [:\n] from comment #29)
> @Ryan, why did I not need a try run for
> https://bugzilla.mozilla.org/show_bug.cgi?id=768521#c19 ?  Was it because
> there was no new test added there?

Because I looked at the patch and decided that a small pref-only change likely didn't need a Try push. Our normal policy is to only push checkin-needed patches that have a Try link per the dev-platform announcement from about a year ago, so this was an exception to that rule.
https://groups.google.com/d/msg/mozilla.dev.platform/9w0-Bh_3vVI/xWebYK64GdwJ

> How do I push to try?  Can I do that from a git mirror of m-c?

Pushing to try from git should work fine, but I have no experience with it myself. Try asking in #developers. Otherwise, the link below might help:
https://wiki.mozilla.org/ReleaseEngineering/TryServer

Comment 31

[(not reading bugmail) Nick Desaulniers [:\n]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a02da320a0c1

Comment 32

[Ryan VanderMeulen [:RyanVM]]

For future reference, checkin-needed is generally set via the bug keyword rather than as a whiteboard message. Looks good to land, though :)

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/39e681c51045

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39e681c51045