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

VERIFIED FIXED in Firefox 15

Status

Firefox Graveyard
Web Apps
P1
critical
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: jsmith, Assigned: TimAbraldes)

Tracking

15 Branch
Firefox 15
x86_64
Windows 7
Bug Flags:
in-moztrap -

Details

(Whiteboard: [blocking-webrtdesktop1+], [qa!])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Whiteboard: [marketplace-beta=]
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
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
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.
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.
Depends on: 752756
I imagine that this blocks Kilimanjaro
blocking-kilimanjaro: --- → ?
Summary: Cannot uninstall an application with Japanese characters on the front and back of an app name in Win 7 → Cannot uninstall natively-installed webapps whose names contain unicode characters on Windows
(Reporter)

Updated

5 years ago
Blocks: 731054
(Reporter)

Updated

5 years ago
Priority: -- → P1
Whiteboard: [marketplace-beta=] → [marketplace-beta=] [blocking-webrtdesktop1+]
(Reporter)

Updated

5 years ago
Target Milestone: --- → Firefox 15
(Reporter)

Updated

5 years ago
Whiteboard: [marketplace-beta=] [blocking-webrtdesktop1+] → [blocking-webrtdesktop1+]
(Reporter)

Updated

5 years ago
tracking-firefox15: --- → ?
(Reporter)

Updated

5 years ago
No longer blocks: 731054
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.
Attachment #624474 - Flags: review?(felipc)
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.
Attachment #624474 - Attachment is obsolete: true
Attachment #624474 - Flags: review?(felipc)
Attachment #624623 - Flags: review?(felipc)
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!
Attachment #624623 - Flags: review?(felipc) → review+
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

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

Comment 10

5 years ago
(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.
blocking-kilimanjaro: ? → +

Comment 11

5 years ago
(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.
Duplicate of this bug: 756496
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)
Attachment #624623 - Attachment is obsolete: true
Attachment #625831 - Flags: review+
that's totally fine
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.
(Reporter)

Comment 16

5 years ago
Tested on OS X 10.7 - No regressions detected. Need to still test on Windows.
(Reporter)

Comment 17

5 years ago
Tested on Win XP & 7: Looks good.
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
Attachment #625831 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/97a2a24bbc1d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Whiteboard: [blocking-webrtdesktop1+] → [blocking-webrtdesktop1+], [qa+]
(Reporter)

Comment 20

5 years ago
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.
Whiteboard: [blocking-webrtdesktop1+], [qa+] → [blocking-webrtdesktop1+], [qa!]
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.
Status: RESOLVED → VERIFIED
Per Sheila, there is no need to track blocking k9o work for a specific Firefox release at this point.
tracking-firefox15: ? → -
(Reporter)

Updated

5 years ago
Flags: in-moztrap?(jsmith)
(Reporter)

Updated

5 years ago
QA Contact: jsmith
(Reporter)

Updated

5 years ago
Flags: in-moztrap?(jsmith) → in-moztrap-
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.