1583129 - Do not distinguish between "Program Files" and "Program Files (x86)" on Windows when checking the install directory

Status: RESOLVED FIXED

(Thunderbird :: Installer, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1542025 +++

Magnus and I discussed the issue and agreed that we should proceed along the lines of bug 1542025 comment #36 (quote):

Updating from 32 to 64 bit causes a change of install location "Program Files" vs "Program Files (x86)",
so it is not an "upgrade", it is a new install. [...]
So we need a technical solution to migrate profiles to 64 bit versions [...].
Perhaps we need to equate "Program Files" and "Program Files (x86)" as the same location for the purposes profile selection.

Looking though this log
https://hg.mozilla.org/mozilla-central/log/tip/toolkit/profile/nsToolkitProfileService.cpp
I have the impression that the " (x86)" could just be stripped here:
https://searchfox.org/mozilla-central/rev/e3fc8f8970491aef14d3212b2d052942f4d29818/toolkit/xre/nsXREDirProvider.cpp#1193

Dave, could you please comment on this approach.

Note: It's unfortunate that TB is doing the 32bit to 64bit migration so late. If we had done it at the time Firefox did theirs, most users would be on 64bit by now and the "profile per install" wouldn't be an issue.

Comment 1

[Wayne Mery (:wsmwk)]

This sounds like a very good direction

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1583129-strip-x68.patch

Totally untested, but compiles ;-) - I don't know whether it's the right spot to add such code. I'd just like to know whether you'd accept a patch along these lines.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1583129-strip-x68.patch

Grrr, trailing spaces. Sorry about the noise.

Comment 4

[Dave Townsend [:mossop]]

I don't think what you're doing here is correct but let me explain what I see here:

The legacy install hash is aimed at correcting an error where the install hash was generated from a string that didn't match the string case of the actual path to the install on disk. generated before we know we have the correct case of the application install path. This affects case insensitive filesystems, if your command line for launching Firefox doesn't match the actual case of the path on disk then Firefox still launches and before bug 1555319 we'd use the given command line, not the path on disk to generate the hash. This caused the obvious problem that using different command lines to the same install would make us think you were using different installs.

So the legacy install hash may or may not match the correct install hash. If it doesn't and if there is a default profile for the legacy hash then we make it the default profile for the correct hash.

If I understand what you want, users previously had 32-bit Thunderbird installed at C:\Program Files (x86)\Thunderbird... and now have 64-bit Thunderbird installed at C:\Program Files\Thunderbird... and you want to migrate the profile that was the default for the x86 version to the new install path.

So you start the new version of Thunderbird. The install hash is going to be generated from C:\Program Files\Thunderbird... The legacy install hash is going to be generated from a string that only varies by case from C:\Program Files\Thunderbird... It already doesn't contain the "(x86)" that you look to remove in this patch (and worth noting, the install paths never end with "(x86)" either way). What you probably want to do instead is add the "(x86)" on to the string used to generate the legacy hash to get a hash for the old location.

Slightly complicating matters, the program folder names are localized. I do not know if the x86 version reliably ends with "(x86)".

I think the way to go about this is to get the current program files directory with a call to SHGetKnownFolderPath for FOLDERID_ProgramFiles, compare it case insensitively to the start of the legacy path and if there is a match replace it with the path for FOLDERID_ProgramFilesX86 (this should match on 32-bit and differ on 64-bit). Then the legacy hash should be that of the install from the x86 directory.

The downside of doing this where you are is that I think this change is likely to break the detection of a string case change (and as it turns out junction point mappings) which is what bug 1555319 was intended to fix. If retaining that is desirable for Thunderbird then there would need to be a more complicated fix here.

Note also there is a bug with this legacy hash stuff that I'm investigating (bug 1580442). My suspicion is that there isn't much point in fixing it at this stage but if it does get fixed it might change the code around here a little.

Comment 5

[Jorg K (CEST = GMT+2)]

Thanks for the comment. I see that my little snippet was foolish, but at least it was in the right function ;-)

I'll see what I can come up with.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1583129-use-x86-path.patch

OK, here's basically what you said, just a bit reversed. If the install patch already starts with the x68 path, we do nothing. If it doesn't, but starts with the regular program files patch, we replace that with the x68 path. Debug shows:

=== Install: C:\Program Files\Thunderbird Daily-self
=== Prog Files x86: C:\Program Files (x86)
=== Prog Files: C:\Program Files
=== Install (new): C:\Program Files (x86)\Thunderbird Daily-self

So how about that?

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 9097483 [details] [diff] [review]
1583129-use-x86-path.patch

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

This looks fine to me (obviously with the printf statements removed). As I mentioned earlier this will break the fix for bug 1555319 in some cases, if that doesn't bother you then that's fine, just be aware.

Please add a short comment to the top, something like:

```
Convert a 64-bit install path to what would have been the 32-bit install path to allow users to migrate their profiles from one to the other.
```

::: toolkit/xre/nsXREDirProvider.cpp
@@ +1207,5 @@
> +      if (SUCCEEDED(hres)) {
> +        printf("=== Prog Files: %s\n", NS_ConvertUTF16toUTF8(nsDependentString(path)).get());
> +        if (StringBeginsWith(installPath, nsDependentString(path),
> +            nsCaseInsensitiveStringComparator())) {
> +          installPath.Replace(0, wcslen(path), nsDependentString(pathX86));

You created an  identical nsDependentString earlier, just create one and reuse.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1583129-use-x86-path.patch (v1 b)

OK, I addressed the comments. Formatting by clang-format ;-)

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 9098361 [details] [diff] [review]
1583129-use-x86-path.patch (v1 b)

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

LGTM

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5460d334b460
Calculate legacy hash using x86 program file path for Thunderbird. r=mossop

Comment 11

[Jorg K (CEST = GMT+2)]

Dave, thinking about this away from the keyboard a bit, these questions arose:

Assume a user for TB 60.9 x64 who got the version from Mozilla's FTP server since it was never officially advertised. This user now updates to TB 68 x64 with this patch. Will that cause any trouble since with the patch we assume a x86 installation path that the user never had?

Or in the case were a TB 68.0 x64 user upgrades to TB 68 x64 with this patch. Will that cause trouble?

I guess I'd like to understand better when the GetLegacyInstallHash() function is used.

Comment 12

[Dave Townsend [:mossop]]

(In reply to Jorg K (GMT+2) from comment #11)

Dave, thinking about this away from the keyboard a bit, these questions arose:

Assume a user for TB 60.9 x64 who got the version from Mozilla's FTP server since it was never officially advertised. This user now updates to TB 68 x64 with this patch. Will that cause any trouble since with the patch we assume a x86 installation path that the user never had?

Or in the case were a TB 68.0 x64 user upgrades to TB 68 x64 with this patch. Will that cause trouble?

I guess I'd like to understand better when the GetLegacyInstallHash() function is used.

GetLegacyInstallHash is only used if there is not a default profile for the current install directory. So if they've been running 68 they should already have a default profile and so this won't affect anything.

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/5460d334b460

Comment 14

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/ab5afeaca26fabebd545a1486151351273c70f93 on THUNDERBIRD_68_VERBRANCH
EDIT: Evil trick to remove this from my "comm-esr68 checkin-needed" query: releases/comm-esr68/rev

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to Dave Townsend [:mossop] (he/him) from comment #4)

If I understand what you want, users previously had 32-bit Thunderbird installed at C:\Program Files (x86)\Thunderbird... and now have 64-bit Thunderbird installed at C:\Program Files\Thunderbird... and you want to migrate the profile that was the default for the x86 version to the new install path.

Yes, this is what we want. So today, after building a version of TB 68 with the patch, I tried this:

I installed TB 60.9 32bit in it's default install location, C:\Program Files (x86)\Mozilla Thunderbird. I removed all profiles from the AppData directory and ran that "old" 32bit version. It created a new profile, called xzy.default.

Then I installed a version of TB 68 64bit which contained the modification into its default install location, C:\Program Files\Mozilla Thunderbird and ran it. Sadly, it didn't recognise the profile and create a new one, abc.default-release.

EDIT: I repeated the exercise to check installs.ini and profiles.ini. installs.ini shows one install only with the .default-release profile. profiles.ini equally shows one install, but there's also a reference to the .default profile.

So looks like we missed something. I certainly miss the understanding of how all this hangs together any when those hashes are calculated. Obviously in TB 60 none of this existed, so it's TB 68 which decides to calculate something and reject the pre-existing profile.

In bug 1542025 comment #42 I had already tried an "unpatched" 32 bit version of TB 68 to upgrade from TB 60. Obviously that would have installed into the existing location and then the existing profile was recognised.

Comment 16

[Jorg K (CEST = GMT+2)]

Another experiment:

I installed TB 60.9 32bit in it's default install location, C:\Program Files (x86)\Mozilla Thunderbird. I removed all profiles from the AppData directory and ran that "old" 32bit version. It created a new profile, called xzy.default.

Then I installed a version of TB 68 64bit which contained the modification into the 32bit location, C:\Program Files\Mozilla Thunderbird (x86) and ran it. It recognised the existing profile.

Then I ran the same version of TB 68 64bit which contained the modification from C:\Program Files\Mozilla Thunderbird, and it also recognised the existing profile.

At the end, installs.ini and profiles.ini show two installs, both with the same profile.

Comment 17

[Jorg K (CEST = GMT+2)]

In continuation of comment #16 I did another experiment with TB 68.1.1 64bit without the patch. When running it form the C:\Program Files\Mozilla Thunderbird location, it does NOT recognise the profile that was recognised when TB 68.1.1 64bit was first run from the (x86) location. So the patch does improve the situation.

We still need to fix this scenario:
TB 60.x 32bit installed in the (x86) location, only one profile.

TB 68 64bit installed in the non-(x86) location: Doesn't recognise the only profile.

Comment 18

[Jorg K (CEST = GMT+2)]

This bug is becoming confusing. I've filed bug 1587067 for the FF/TB 60 32bit to FF/TB 68 64bit upgrade issue. It's the same problem for FF.