Last Comment Bug 740233 - AppUserModelID never gets set if it's already generated
: AppUserModelID never gets set if it's already generated
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: NSIS Installer (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla15
Assigned To: Jim Mathies [:jimm]
:
: Matt Howell [:mhowell]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-28 18:46 PDT by Jim Mathies [:jimm]
Modified: 2012-07-11 12:49 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
affected
+
fixed
fixed
14+
wontfix
unaffected


Attachments
fix (831 bytes, patch)
2012-03-28 18:46 PDT, Jim Mathies [:jimm]
robert.strong.bugs: review+
Details | Diff | Splinter Review
updated patch (1.02 KB, patch)
2012-05-21 15:49 PDT, Jim Mathies [:jimm]
robert.strong.bugs: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Jim Mathies [:jimm] 2012-03-28 18:46:11 PDT
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] 2012-03-28 18:46:49 PDT
Created attachment 610393 [details] [diff] [review]
fix
Comment 2 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-18 12:58:35 PDT
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 [:rstrong] (use needinfo to contact me) 2012-05-18 14:01:14 PDT
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 [:rstrong] (use needinfo to contact me) 2012-05-18 14:21:06 PDT
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] 2012-05-18 16:06:40 PDT
(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 [:rstrong] (use needinfo to contact me) 2012-05-21 13:46:15 PDT
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] 2012-05-21 15:13:42 PDT
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] 2012-05-21 15:49:26 PDT
Created attachment 625803 [details] [diff] [review]
updated patch
Comment 10 Ed Morley [:emorley] 2012-05-22 06:29:39 PDT
https://hg.mozilla.org/mozilla-central/rev/92e1856b5952
Comment 11 Jim Mathies [:jimm] 2012-05-24 08:52:00 PDT
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 2012-05-24 14:25:42 PDT
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 [:rstrong] (use needinfo to contact me) 2012-05-24 15:52:06 PDT
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 15 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-03 12:19:04 PDT
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 2012-07-11 12:49:35 PDT
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.

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