740233 - AppUserModelID never gets set if it's already generated

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Jim Mathies [:jimm]]

Found this testing on win8 today. If the value is already stored in the registry, InitHashAppModelId returns an empty string for AppUserModelID. We seem to expect this value to be valid on an upgrade but maybe this was intentional?

Comment 1

[Jim Mathies [:jimm]]

Attachment: fix

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 610393 [details] [diff] [review]
fix

I haven't had time to go over the code yet to check if this was intentional. :(

Jim, do you have any reason to think that it was intentional?

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 610393 [details] [diff] [review]
fix

>diff --git a/toolkit/mozapps/installer/windows/nsis/common.nsh b/toolkit/mozapps/installer/windows/nsis/common.nsh
>--- a/toolkit/mozapps/installer/windows/nsis/common.nsh
>+++ b/toolkit/mozapps/installer/windows/nsis/common.nsh
>@@ -6937,17 +6937,21 @@
>             WriteRegStr HKLM "$R8" "$R9" "$AppUserModelID"
>             ${If} ${Errors}
>               ClearErrors
>               WriteRegStr HKLM "$R8" "$R9" "$AppUserModelID"
I am fairly certain this should be HKCU

>               ${If} ${Errors}
>                 StrCpy $AppUserModelID "error"
>               ${EndIf}
>             ${EndIf}
>+          ${Else}
>+            StrCpy $AppUserModelID $R7
>           ${EndIf}
>+        ${Else}
>+          StrCpy $AppUserModelID $R7
>         ${EndIf}
>       ${EndIf}
> 
>       end:
>       ${If} "$AppUserModelID" == "error"
>         StrCpy $AppUserModelID ""
>       ${EndIf}
>

Comment 4

[Robert Strong (they/them - no direct email)]

I think we should take this on Beta since it is a simple fix for what is likely to be the majority of the taskbar grouping bugs. Just requesting tracking for now and wfter this has baked for a while we'll request approval with the usual info.

Comment 5

[Alex Keybl [:akeybl]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #4)
> I think we should take this on Beta since it is a simple fix for what is
> likely to be the majority of the taskbar grouping bugs. Just requesting
> tracking for now and wfter this has baked for a while we'll request approval
> with the usual info.

My only concern here is whether we'd actually hear about regressions from people using the beta windows installer prior to release. Am I being overly cautious?

Comment 6

[Robert Strong (they/them - no direct email)]

For this specific change I believe it is a safe and simple fix and the concerns with taking it are rather minor. The concerns can be mitigated at least to some extent with manual testing of the different scenarios prior to asking for approval.

Comment 7

[Alex Keybl [:akeybl]]

Tracking for release in case we're able to get this change nominated and landed in prior to tomorrow's go-to-build. Otherwise, let's get this into FF14 first.

Comment 8

[Jim Mathies [:jimm]]

Attachment: updated patch

Comment 9

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92e1856b5952

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/92e1856b5952

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 625803 [details] [diff] [review]
updated patch

[Approval Request Comment]
Bug caused by: bug 577867
User impact if declined: see below
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

We should take this on aurora for sure, just to get it out. Beta wouldn't be bad either. This bug affects users who can't elevate when they install. Window grouping on the taskbar may not work correctly without this fix.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 625803 [details] [diff] [review]
updated patch

approved for aurora, but it's too late in the cycle to risk this on beta. we won't have a chance to ensure this gets set between updates before release.

Comment 13

[Robert Strong (they/them - no direct email)]

Just so you know, the new code in this case runs after the first update so we don't need two updates to test whether the fix works. Having said that, I'm ok with holding off on this on Beta.

Comment 14

[Jim Mathies [:jimm]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/e9efce4842b8

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Can this be landed as is to the ESR branch?  If so, please nominate otherwise we'll be looking for the ESR version of this to land before next Tuesday (July 10)

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

On closer inspection, this doesn't really meet the ESR criteria so I'm going to set it to wontfix for this coming release. If enterprise users request this, we can revisit, otherwise it will come with ESR17.