AppUserModelID never gets set if it's already generated

RESOLVED FIXED in Firefox 14

Status

()

Toolkit
NSIS Installer
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
mozilla15
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox12 affected, firefox13+ affected, firefox14+ fixed, firefox15 fixed, firefox-esr1014+ wontfix, status1.9.2 unaffected)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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?
(Assignee)

Comment 1

5 years ago
Created attachment 610393 [details] [diff] [review]
fix
Assignee: nobody → jmathies
Attachment #610393 - Flags: review?(robert.bugzilla)
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 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}
>
Attachment #610393 - Flags: review?(robert.bugzilla) → review+
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.
status1.9.2: --- → unaffected
status-firefox-esr10: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?

Comment 5

5 years ago
(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?
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

5 years ago
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.
tracking-firefox13: ? → +
tracking-firefox14: ? → +
(Assignee)

Comment 8

5 years ago
Created attachment 625803 [details] [diff] [review]
updated patch
Attachment #610393 - Attachment is obsolete: true
Attachment #625803 - Flags: review?(robert.bugzilla)
Attachment #625803 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e1856b5952
https://hg.mozilla.org/mozilla-central/rev/92e1856b5952
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
tracking-firefox-esr10: ? → 14+
(Assignee)

Comment 11

5 years ago
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.
Attachment #625803 - Flags: approval-mozilla-beta?
Attachment #625803 - Flags: approval-mozilla-aurora?
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.
Attachment #625803 - Flags: approval-mozilla-beta?
Attachment #625803 - Flags: approval-mozilla-beta-
Attachment #625803 - Flags: approval-mozilla-aurora?
Attachment #625803 - Flags: approval-mozilla-aurora+
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.
(Assignee)

Comment 14

5 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/e9efce4842b8
status-firefox14: affected → fixed
status-firefox15: affected → fixed
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)
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.
status-firefox-esr10: affected → wontfix
You need to log in before you can comment on or make changes to this bug.