Last Comment Bug 751177 - Cannot uninstall natively-installed webapps whose names contain unicode characters on Windows
: Cannot uninstall natively-installed webapps whose names contain unicode chara...
Status: VERIFIED FIXED
[blocking-webrtdesktop1+], [qa!]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: 15 Branch
: x86_64 Windows 7
: P1 critical
: Firefox 15
Assigned To: Tim Abraldes [:TimAbraldes] [:tabraldes]
: Jason Smith [:jsmith]
:
Mentors:
: 756496 (view as bug list)
Depends on: 752756
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-02 07:55 PDT by Jason Smith [:jsmith]
Modified: 2016-02-04 15:00 PST (History)
8 users (show)
jsmith: in‑moztrap-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 - Write "webapp.ini" and "shortcuts_log.ini" as UTF-16LE (1.73 KB, patch)
2012-05-16 11:55 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
no flags Details | Diff | Splinter Review
Patch v2 - Specify "UTF-16LE" instead of just "UTF-16" (1.74 KB, patch)
2012-05-16 18:57 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
felipc: review+
Details | Diff | Splinter Review
Patch v3 - Updating to match patch v3 in bug 752756 (1.77 KB, patch)
2012-05-21 17:36 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
timabraldes: review+
timabraldes: checkin+
Details | Diff | Splinter Review

Description Jason Smith [:jsmith] 2012-05-02 07:55:36 PDT
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] 2012-05-02 12:21:11 PDT
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] 2012-05-02 12:45:45 PDT
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] 2012-05-07 14:51:08 PDT
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] 2012-05-08 12:36:10 PDT
I imagine that this blocks Kilimanjaro
Comment 5 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-05-16 11:55:05 PDT
Created attachment 624474 [details] [diff] [review]
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] 2012-05-16 18:57:16 PDT
Created attachment 624623 [details] [diff] [review]
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 me!) 2012-05-16 19:08:15 PDT
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 [:bsmedberg] 2012-05-17 06:14:31 PDT
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 2012-05-18 11:29:19 PDT
Asa, can you tell us if this is blocking k9o?
Comment 10 Asa Dotzler [:asa] 2012-05-18 11:34:19 PDT
(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] 2012-05-18 11:35:36 PDT
(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 12 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-05-18 16:03:24 PDT
*** Bug 756496 has been marked as a duplicate of this bug. ***
Comment 13 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-05-21 17:36:14 PDT
Created attachment 625831 [details] [diff] [review]
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 me!) 2012-05-21 17:44:06 PDT
that's totally fine
Comment 15 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-05-22 12:39:30 PDT
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] 2012-05-22 14:39:34 PDT
Tested on OS X 10.7 - No regressions detected. Need to still test on Windows.
Comment 17 Jason Smith [:jsmith] 2012-05-22 14:59:36 PDT
Tested on Win XP & 7: Looks good.
Comment 18 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-05-23 10:15:02 PDT
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] 2012-05-24 09:36:18 PDT
https://hg.mozilla.org/mozilla-central/rev/97a2a24bbc1d
Comment 20 Jason Smith [:jsmith] 2012-05-25 15:37:23 PDT
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] 2012-05-30 11:33:33 PDT
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] 2012-06-04 08:39:26 PDT
Per Sheila, there is no need to track blocking k9o work for a specific Firefox release at this point.

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