1643612 - The Refresh Firefox option from the uninstaller is removed under certain steps

Status: NEW

(Firefox :: Installer, defect, P2)

Description

[Andrei Purice]

Attachment: Refresh Firefox

Affected Versions:
Nightly 79.0a1 (2020-06-04)
Beta 78.0b3

Affected Platforms:
Windows 7/10

Preconditions:
Have the Profile Manager set up to open Firefox on launch.

Steps to reproduce:

1. Reach the Firefox installation folder or Add/Remove programs from Windows.
2. Click on the uninstall option.
3. Select the refresh option from the uninstall wizard.
4. After the Profile Manager pops-up, choose to create a new profile using special/ASCII characters such as "†" (alt + 0134), "¤" (alt + 01444), etc. and launch it.
5. After the browser is launched close it.
6. Repeat the uninstall process and check the uninstall wizard.

Expected Result:
The Refresh Firefox option is available and working accordingly.

Actual Result:
The Refresh Firefox option is missing from the uninstaller prompt.

Regression-Range:
Not a regression this is part of the new Refresh option implementation for the uninstaller.

Notes:

1. In order for the option to be visible again the user has to delete all the profiles, reinstall Firefox or start Firefox with a new profile that doesn't contain the special character.
2. Given this issue needs custom profile setup with special characters, QA doesn't consider it as a high severity issue especially that this is not normal user behavior.

Comment 1

[Andrei Purice]

Attachment: Install before and after

Comment 2

[Nick Alexander :nalexander [he/him]]

What a fascinating ticket. P2 'cuz this isn't top-of-mind; S3 'cuz even though there's no real work-around, it seems unlikely that we have non-ASCII characters in a non-trivial number of profiles.

Comment 3

[Nick Alexander :nalexander [he/him]]

Actually, S4.

Comment 4

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

A simpler STR is:

1. Create the profile with the name containing † (Alt-0134)
2. Set it as default (this probably happened automatically in 1)
3. Run the uninstaller

The refresh page isn't shown.

Firefox reads and writes installs.ini and profiles.ini with our own code, which uses UTF-8. The NSIS ReadINIStr and the underlying GetPrivateProfileStringW assumes the file is Windows-1250 (or whatever the system code page is, I guess? unless there is a UTF-16 BOM), so it gets junk for the profile path (including converting the 0x80 byte to the unicode equivalent 0x20AC). Fundamentally it's just wrong to assume these are proper Windows INI files. This should be a problem with the refresh prompt in the stub installer, as well.

It's unusual to have a non-default name for a profile, and the paths are usually relative, so I agree with :nalexander that this probably isn't too serious. In the worst case here we're only reading the INI wrong and deciding not to show something inessential.

I'm worried about the other ways we touch the INI files elsewhere in our NSIS code, though, :mhowell could this be a problem with something else we're still doing?

Comment 5

[Molly Howell (she/her) (mostly inactive)]

There is one more place I can think of that I believe would be broken because of this same problem, which is here where we read the install path from the full installer configuration INI. In that case we have no guarantee at all what encoding that file is going to be in, so we might or might not end up doing terrible things depending on that.

Otherwise, the installers don't do a lot of reading of INI files that can have arbitrary content, like profile names; it's mostly things that the installer itself wrote, like the shortcuts log or the InstallOptions files, or at least things that have known fixed contents, like application.ini.

The only other exceptions I can find are one specific function that we should think about removing where we convert the encoding of a specific distributuion.ini from CP-1252 to UTF-8, and also this function which is never called and which we should remove.

I agree with the triaging that's been done here, but for when we do want a solution, I wonder if we could continue reading the string as we are, but then repair the encoding ourselves using a combination of manually twiddling the bytes we know are mangled followed by MultiByteToWideChar. That may be impossible in practice though because what we need to do could depend on what the system code page is, so the only way out might be to ship our own INI parser.

Comment 6

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to Molly Howell (she/her) [:mhowell] from comment #5)

I wonder if we could continue reading the string as we are, but then repair the encoding ourselves using a combination of manually twiddling the bytes we know are mangled followed by MultiByteToWideChar. That may be impossible in practice though because what we need to do could depend on what the system code page is, so the only way out might be to ship our own INI parser.

I thought we could go back to the system code page with WideCharToMultiByte CP_ACP, then MultiByteToWideChar CP_UTF8, which seems to work for single byte code pages like 1252, etc. But I don't think it will work work with true multi-byte code pages like 932.

We could do GetPrivateProfileStringA, I think that would pass the bytes along directly, and then MultiByteToWideChar CP_UTF8. But this is going to be more of an ordeal of coding in NSIS and testing than I think is justified right now.