751177 - Cannot uninstall natively-installed webapps whose names contain unicode characters on Windows

Status: VERIFIED FIXED

(Firefox Graveyard :: Web Apps, defect, P1)

Description

[Jason Smith [:jsmith]]

Steps:

1. Install an application with an app name that has Japanese characters in the front and end in Win 7 (Example: Ed's app at http://ed.agadak.net/app/#)
2. Uninstall the application

Expected:

A prompt should appear allowing the app to be uninstalled. Confirming the uninstall should uninstall the app from the machine.

Actual:

An error is thrown from Win 7 saying that uninstall ran into problems trying to get rid of the app. Selecting "uninstall with recommended settings" does nothing. Trying to uninstall the app again a few more times still does nothing. Seen this issue on Win 7 32-bit and 64-bit.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

The uninstaller reads the app filename out of webapprt.ini which lives in the app's installation dir.  It looks for "${AppFilename}.exe" in the app's installation directory and aborts immediately if it does not find it there.

Running a procmon trace reveals that the uninstaller is looking for "Ed Agadak テスト.exe" rather than "Ed Agadak テスト.exe" which means that it is treating the filename string as cp-1252 (on my machine).

I'll see if I can convince the uninstaller to treat the INI entries as utf-8, but I have a sinking feeling that we may have to use the registry instead - https://blogs.msdn.com/b/oldnewthing/archive/2007/11/26/6523907.aspx

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

The uninstaller works if webapp.ini is encoded as UTF-16 with a BOM.  However, adding a BOM (even if the encoding is still UTF-8) breaks the app with this error message [https://mxr.mozilla.org/mozilla-central/source/webapprt/win/webapprt.cpp?rev=4c1aa39b85a9#478] since nsINIParser apparently doesn't like files with BOMs.

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

The fact that NSIS uninstallers can only read Unicode INI files if they are encoded as UTF-16le with a BOM means that we're going to have to write our "shortcuts_log.ini" in that format. Firefox's "shortcuts_log.ini" is encoded that way because its NSIS installer is responsible for writing the file.  We don't have an NSIS installer for webapps; the native-installation code is part of Firefox and is written in javascript.  We either need to extend nsIINIParserWriter to be able to write UTF-16le INI files with BOMs, or convert the file after writing it.

Since we have to solve this problem for shortcuts_log.ini, I'm tempted to just move the executable name (which is only used by the uninstaller) from webapp.ini to shortcuts_log.ini.  Another option is to write a 1-line text file to the "uninstall" folder that has the executable name in it (and is encoded in UTF-16le with a BOM), then read that file from the uninstaller.  An even further option might be to call MultiByteToWideString from the NSIS uninstaller to convert the executable name from UTF-8.

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I imagine that this blocks Kilimanjaro

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1 - Write "webapp.ini" and "shortcuts_log.ini" as UTF-16LE

This patch works with Patch v1 of bug 752756.  With both patches applied, Ed's app installs, runs, and uninstalls correctly.

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2 - Specify "UTF-16LE" instead of just "UTF-16"

It turns out there was an issue with the v1 patch in bug 752756, so please use the v2 patch there for testing.  The v2 patch in bug 752756 only accepts "UTF-16LE" (not "UTF-16") so I'm updating this patch to match.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 624623 [details] [diff] [review]
Patch v2 - Specify "UTF-16LE" instead of just "UTF-16"

It's weird that we have to specify the file twice (once in createINIParser and once in writeFile), due to pre-existence of the optional argument aFile in writeFile. But changing createINIParser to have the argument as optional seems it will be overly complicated for not much benefit. If Benjamin is fine with the slight duplication on this edge case, then it's fine by me!

Comment 8

[Benjamin Smedberg]

Comment on attachment 624623 [details] [diff] [review]
Patch v2 - Specify "UTF-16LE" instead of just "UTF-16"

You don't need to specify it again, just pass "null". Although I think the parameters are going to change in the other bug...

Comment 9

[Sheila Mooney]

Asa, can you tell us if this is blocking k9o?

Comment 10

[Asa Dotzler [:asa]]

(In reply to Sheila Mooney from comment #9)
> Asa, can you tell us if this is blocking k9o?

We need *a* solution. If we can enforce something at the Marketplace that prevents distribution of apps with names with characters that fail in this scenario, let's do that for k9o and get a real fix later. If we cannot or do not want to try Marketplace enforcement, we should get this fixed in product for k9o. 

I'm marking this a blocker and we can unblock if we find a Marketplace solution.

Comment 11

[Asa Dotzler [:asa]]

(In reply to Asa Dotzler [:asa] from comment #10)
> (In reply to Sheila Mooney from comment #9)
> > Asa, can you tell us if this is blocking k9o?
> 
> We need *a* solution. If we can enforce something at the Marketplace that
> prevents distribution of apps with names with characters that fail in this
> scenario, let's do that for k9o and get a real fix later. If we cannot or do
> not want to try Marketplace enforcement, we should get this fixed in product
> for k9o. 
> 
> I'm marking this a blocker and we can unblock if we find a Marketplace
> solution.

I should have looked at the status closer. With a reviewed patch in hand, let's not bother the Marketplace folks and instead just land the fix in code. If for some reason we cannot get the code fix to work, let's then talk about pushing off to the Marketplace.

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v3 - Updating to match patch v3 in bug 752756

Carrying forward r+ (felipe let me know if there's a reason not to carry forward)

Comment 14

[:Felipe Gomes (needinfo for replies!)]

that's totally fine

Comment 15

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Pushed to try along with the final patch for bug 752756:
  https://tbpl.mozilla.org/?tree=Try&rev=102cb00af075

jsmith will do a quick verification that this patch works and doesn't break INI processing:
  1) Install Ed's app on Windows and Mac
  2) Verify that Ed's app launches on Windows and Mac
  3) Uninstall Ed's app

If all looks good I'll land this tonight.

Comment 16

[Jason Smith [:jsmith]]

Tested on OS X 10.7 - No regressions detected. Need to still test on Windows.

Comment 17

[Jason Smith [:jsmith]]

Tested on Win XP & 7: Looks good.

Comment 18

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 625831 [details] [diff] [review]
Patch v3 - Updating to match patch v3 in bug 752756

https://hg.mozilla.org/integration/mozilla-inbound/rev/97a2a24bbc1d

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/97a2a24bbc1d

Comment 20

[Jason Smith [:jsmith]]

This works if the app is installed on the build with this patch implemented, but will not work if the app was installed on a build with this patch not implemented. 

Is this case, should the user be forced to reinstall the app to uninstall it on the latest build (to handle version B below)? Or should we reopen this bug to say version B has to be supported? Tim?

Test Case Used:

1. Go to http://ed.agadak.net/app/
2. Install Ed's app (should see Japanese characters used)
3. Go to add or remove programs
4. Uninstall the app

Version A of Test Case: App installed on the 5/25 nightly build
Version B of Test Case: App installed with 5/20 nightly build

Tim - If no work needs to be done here, move this to verified fixed. If work still needs to be done, reopen the bug.

Comment 21

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

This patch is expected only to fix new installations: Apps that have already been installed will continue to act the way they always have.  Reinstalling a broken app using the latest nightly is a good way to make them uninstallable.

One additional side-effect of this patch: Apps that are installed by a version of Firefox that includes this patch will not be runnable if Firefox is downgraded to a version that does not include this patch.

Comment 22

[Alex Keybl [:akeybl]]

Per Sheila, there is no need to track blocking k9o work for a specific Firefox release at this point.