Bug 1413295 followup: Address reliability and paveover install problems

RESOLVED FIXED in Firefox 58

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mhowell, Assigned: mhowell)

Tracking

58 Branch
Firefox 58
Unspecified
Windows
Points:
---

Firefox Tracking Flags

(firefox57 unaffected, firefox58 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Several issues with the bug 1413295 patch caused it to be backed out from 57 in bug 1414416. Those issues need to be addressed for 58.
(Assignee)

Updated

a year ago
Depends on: 1413295
(Assignee)

Comment 1

a year ago
Robert, I realize I'm sending you one installer review after another, and I'm sorry for that, but you're the best person to be doing these I'm afraid.
Attachment #8926560 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 2

a year ago
Bug 1413295 completely failed to address paveover installs, which led to some confusing behaviors that QA identified; most of the time a second shortcut would be created for the same install. This patch should avoid that confusion.
Attachment #8926565 - Flags: review?(robert.strong.bugs)
Comment on attachment 8926560 [details] [diff] [review]
Part 1 - Streamline PostUpdate shortcut renaming code

>diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
>--- a/browser/installer/windows/nsis/shared.nsh
>+++ b/browser/installer/windows/nsis/shared.nsh
>@@ -334,140 +334,60 @@
>     ${If} ${FileExists} "$QUICKLAUNCH\${BrandShortName}.lnk"
>       ShellLink::SetShortCutWorkingDirectory "$QUICKLAUNCH\${BrandShortName}.lnk" \
>                                              "$INSTDIR"
>     ${EndIf}
>   ${EndUnless}
> !macroend
> !define ShowShortcuts "!insertmacro ShowShortcuts"
> 
>-; Update the branding information on all shortcuts our installer created,
>+; Update the branding name on all shortcuts our installer created,
nit: unnecessary comma

> ; to convert from BrandFullName (which is what we used to name shortcuts)
>-; to BrandShortName (which is what we now name shortcuts). Also update the
>-; icon if it's been changed.
>-; This should only be called sometime after both MigrateStartMenuShortcut
>-; and MigrateTaskBarShurtcut, and it assumes SHCTX is set correctly.
>-!macro UpdateShortcutBranding
>+; to BrandShortName (which is what we now name shortcuts). We only rename
>+; desktop and start menu shortcuts, because touching taskbar pins often
>+; (but inconsistently) triggers various broken behaviors in the shell.
>+; This should only be called sometime after MigrateStartMenuShortcut,
Don't forget to file a bug to remove MigrateStartMenuShortcut

>+; and it assumes SHCTX is set correctly.
>+!macro UpdateShortcutsBranding
>+  ${UpdateOneShortcutBranding} "STARTMENU" "$SMPROGRAMS"
>+  ${UpdateOneShortcutBranding} "DESKTOP" "$DESKTOP"
>+!macroend
>+!define UpdateShortcutsBranding "!insertmacro UpdateShortcutsBranding"
>+
>+!macro UpdateOneShortcutBranding LOG_SECTION SHORTCUT_DIR
>+  ; Only try to rename the shortcuts found in the shortcuts log, to avoid
>+  ; blowing away a name that the user created.
>   ${GetLongPath} "$INSTDIR\uninstall\${SHORTCUTS_LOG}" $R9
>   ${If} ${FileExists} "$R9"
>     ClearErrors
>-    ; The entries in the shortcut log are numbered, but we never actually
>-    ; create more than one shortcut (or log entry) in each location.
>-    ReadINIStr $R8 "$R9" "STARTMENU" "Shortcut0"
>+    ; The shortcuts log contains a numbered list of entries for each section,
>+    ; but we never actually create more than one.
>+    ReadINIStr $R8 "$R9" "${LOG_SECTION}" "Shortcut0"
>     ${IfNot} ${Errors}
>-      ${If} ${FileExists} "$SMPROGRAMS\$R8"
<snip>
>-        ShellLink::GetShortCutTarget "$DESKTOP\$R8"
>+      ${If} ${FileExists} "${SHORTCUT_DIR}\$R8"
Would be good to check if the new shortcut file name doesn't already exist here so it doesn't fall through to trying to rename it to a file that already exists.

>+        ShellLink::GetShortCutTarget "${SHORTCUT_DIR}\$R8"
>         Pop $R7
>         ${GetLongPath} "$R7" $R7
>         ${If} $R7 == "$INSTDIR\${FileMainEXE}"
>-          ShellLink::GetShortCutIconLocation "$DESKTOP\$R8"
<snip>
>-              CreateShortcut "$QUICKLAUNCH\User Pinned\TaskBar\$R8" "$R7"
>-            ${EndIf}
>+        ${AndIf} $R8 != "${BrandShortName}.lnk"
>+          ClearErrors
>+          Rename "${SHORTCUT_DIR}\$R8" "${SHORTCUT_DIR}\${BrandShortName}.lnk"
>+          ${IfNot} ${Errors}
>+            ; Update the shortcut log manually instead of calling LogShortcut
>+            ; because it would add a Shortcut1 entry, and we really do want to
>+            ; overwrite the existing entry 0, since we just renamed the file.
>+            WriteINIStr "$R9" "${LOG_SECTION}" "Shortcut0" \
>+                        "${BrandShortName}.lnk"
>           ${EndIf}
>         ${EndIf}
>       ${EndIf}
>     ${EndIf}
>   ${EndIf}
> !macroend
>-!define UpdateShortcutBranding "!insertmacro UpdateShortcutBranding"
>+!define UpdateOneShortcutBranding "!insertmacro UpdateOneShortcutBranding"

r=me with that
Attachment #8926560 - Flags: review?(robert.strong.bugs) → review+
Comment on attachment 8926565 [details] [diff] [review]
Part 2 - Support renaming shortcuts during paveover installs

Discussed these changes with mhowell over irc and I don't think the check for user renamed or created shortcuts should be done in this bug if at all and asked him to remove that code. Also, the existence of the destination file should be checked beforehand the same as my comment regarding the previous patch and that Win 7 is the minimum platform now so ${If} ${AtLeastWin7} is not needed.
Attachment #8926565 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 5

a year ago
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #3)
> Don't forget to file a bug to remove MigrateStartMenuShortcut

Filed bug 1415743. Patches with the other changes are coming up.
(Assignee)

Comment 6

a year ago
Minor changes, r+ carried over.
Attachment #8926560 - Attachment is obsolete: true
Attachment #8926662 - Flags: review+
(Assignee)

Comment 7

a year ago
Attachment #8926565 - Attachment is obsolete: true
Attachment #8926663 - Flags: review?(robert.strong.bugs)
Comment on attachment 8926663 [details] [diff] [review]
Part 2 - Support renaming shortcuts during paveover installs

diff --git a/browser/installer/windows/nsis/installer.nsi b/browser/installer/windows/nsis/installer.nsi
>--- a/browser/installer/windows/nsis/installer.nsi
>+++ b/browser/installer/windows/nsis/installer.nsi
>@@ -490,27 +490,50 @@ Section "-Application" APP_IDX
>     UAC::ExecCodeSegment $0
>   ${EndUnless}
> 
>   ; UAC only allows elevating to an Admin account so there is no need to add
>   ; the Start Menu or Desktop shortcuts from the original unelevated process
>   ; since this will either add it for the user if unelevated or All Users if
>   ; elevated.
>   ${If} $AddStartMenuSC == 1
>-    CreateShortCut "$SMPROGRAMS\${BrandShortName}.lnk" "$INSTDIR\${FileMainEXE}"
>-    ${If} ${FileExists} "$SMPROGRAMS\${BrandShortName}.lnk"
>-      ShellLink::SetShortCutWorkingDirectory "$SMPROGRAMS\${BrandShortName}.lnk" \
>-                                           "$INSTDIR"
>-      ${If} ${AtLeastWin7}
>-      ${AndIf} "$AppUserModelID" != ""
>-        ApplicationID::Set "$SMPROGRAMS\${BrandShortName}.lnk" "$AppUserModelID" "true"
>+    ; See if there's an existing shortcut for this installation using the old
>+    ; name that we should just rename, instead of creating a new shortcut.
>+    ; We could do this renaming even when $AddStartMenuSC is false; the idea
>+    ; behind not doing that is to interpret "false" as "don't do anything
>+    ; involving start menu shortcuts at all." We could also try to do this for
>+    ; both shell contexts, but that won't typically accomplish anything.
>+    ${If} ${FileExists} "$SMPROGRAMS\${BrandFullName}.lnk"
>+      ShellLink::GetShortCutTarget "$SMPROGRAMS\${BrandFullName}.lnk"
>+      Pop $0
>+      ${GetLongPath} "$0" $0
>+      ${If} $0 == "$INSTDIR\${FileMainEXE}"
>+      ${AndIfNot} ${FileExists} "$SMPROGRAMS\${BrandShortName}.lnk"
>+        Rename "$SMPROGRAMS\${BrandFullName}.lnk" \
>+               "$SMPROGRAMS\${BrandShortName}.lnk"
>+        ; Update the shortcut log manually instead of calling LogShortcut
>+        ; because it would add a Shortcut1 entry, and we really do want to
>+        ; overwrite the existing entry 0, since we just renamed the file.
Please add this same comment for the second case below

Unless I'm mistaken, the following calls that happen previously will add Shortcut1=${BrandShortName}.lnk since Shortcut0=${BrandFullName}.lnk would already exist on a pave over and the call to WriteINIStr below will then change Shortcut0=${BrandFullName}.lnk to Shortcut0=${BrandShortName}.lnk.
  ${LogStartMenuShortcut} "${BrandShortName}.lnk"
  ${LogDesktopShortcut} "${BrandShortName}.lnk"

>+        WriteINIStr "$INSTDIR\uninstall\${SHORTCUTS_LOG}" "STARTMENU" \
>+                    "Shortcut0" "${BrandShortName}.lnk"
>+        ${LogMsg} "Renamed existing shortcut to $SMPROGRAMS\${BrandShortName}.lnk"
>       ${EndIf}
>-      ${LogMsg} "Added Shortcut: $SMPROGRAMS\${BrandShortName}.lnk"
>     ${Else}
>-      ${LogMsg} "** ERROR Adding Shortcut: $SMPROGRAMS\${BrandShortName}.lnk"
>+      CreateShortCut "$SMPROGRAMS\${BrandShortName}.lnk" "$INSTDIR\${FileMainEXE}"
>+      ${If} ${FileExists} "$SMPROGRAMS\${BrandShortName}.lnk"
>+        ShellLink::SetShortCutWorkingDirectory "$SMPROGRAMS\${BrandShortName}.lnk" \
>+                                               "$INSTDIR"
>+        ${If} "$AppUserModelID" != ""
>+          ApplicationID::Set "$SMPROGRAMS\${BrandShortName}.lnk" \
>+                             "$AppUserModelID" "true"
>+        ${EndIf}
>+        ${LogMsg} "Added Shortcut: $SMPROGRAMS\${BrandShortName}.lnk"
>+      ${Else}
>+        ${LogMsg} "** ERROR Adding Shortcut: $SMPROGRAMS\${BrandShortName}.lnk"
>+      ${EndIf}
>     ${EndIf}
>   ${EndIf}
> 
>   ; Update lastwritetime of the Start Menu shortcut to clear the tile cache.
>   ; Do this for both shell contexts in case the user has shortcuts in multiple
>   ; locations, then restore the previous context at the end.
>   ${If} ${AtLeastWin8}
>     SetShellVarContext all
>@@ -520,27 +543,41 @@ Section "-Application" APP_IDX
>     ${If} $TmpVal == "HKLM"
>       SetShellVarContext all
>     ${ElseIf} $TmpVal == "HKCU"
>       SetShellVarContext current
>     ${EndIf}
>   ${EndIf}
> 
>   ${If} $AddDesktopSC == 1
>-    CreateShortCut "$DESKTOP\${BrandShortName}.lnk" "$INSTDIR\${FileMainEXE}"
>-    ${If} ${FileExists} "$DESKTOP\${BrandShortName}.lnk"
>-      ShellLink::SetShortCutWorkingDirectory "$DESKTOP\${BrandShortName}.lnk" \
>-                                             "$INSTDIR"
>-      ${If} ${AtLeastWin7}
>-      ${AndIf} "$AppUserModelID" != ""
>-        ApplicationID::Set "$DESKTOP\${BrandShortName}.lnk" "$AppUserModelID"  "true"
>+    ${If} ${FileExists} "$DESKTOP\${BrandFullName}.lnk"
>+      ShellLink::GetShortCutTarget "$DESKTOP\${BrandFullName}.lnk"
>+      Pop $0
>+      ${GetLongPath} "$0" $0
>+      ${If} $0 == "$INSTDIR\${FileMainEXE}"
>+      ${AndIfNot} ${FileExists} "$DESKTOP\${BrandShortName}.lnk"
>+        Rename "$DESKTOP\${BrandFullName}.lnk" \
>+               "$DESKTOP\${BrandShortName}.lnk"
>+        WriteINIStr "$INSTDIR\uninstall\${SHORTCUTS_LOG}" "DESKTOP" \
>+                    "Shortcut0" "${BrandShortName}.lnk"
>+        ${LogMsg} "Renamed existing shortcut to $DESKTOP\${BrandShortName}.lnk"
>       ${EndIf}
>-      ${LogMsg} "Added Shortcut: $DESKTOP\${BrandShortName}.lnk"
>     ${Else}
>-      ${LogMsg} "** ERROR Adding Shortcut: $DESKTOP\${BrandShortName}.lnk"
>+      CreateShortCut "$DESKTOP\${BrandShortName}.lnk" "$INSTDIR\${FileMainEXE}"
>+      ${If} ${FileExists} "$DESKTOP\${BrandShortName}.lnk"
>+        ShellLink::SetShortCutWorkingDirectory "$DESKTOP\${BrandShortName}.lnk" \
>+                                               "$INSTDIR"
>+        ${If} "$AppUserModelID" != ""
>+          ApplicationID::Set "$DESKTOP\${BrandShortName}.lnk" \
>+                             "$AppUserModelID" "true"
>+        ${EndIf}
>+        ${LogMsg} "Added Shortcut: $DESKTOP\${BrandShortName}.lnk"
>+      ${Else}
>+        ${LogMsg} "** ERROR Adding Shortcut: $DESKTOP\${BrandShortName}.lnk"
>+      ${EndIf}
>     ${EndIf}
>   ${EndIf}
> 
>   ; If elevated the Quick Launch shortcut must be added from the unelevated
>   ; original process.
>   ${If} $AddQuickLaunchSC == 1
>     ${Unless} ${AtLeastWin7}
>       ClearErrors

Clearing review for the issue above that I suspect exists.
Attachment #8926663 - Flags: review?(robert.strong.bugs)
iirc that would only affect the full installer and not the stub
(Assignee)

Comment 10

a year ago
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #8)
> Please add this same comment for the second case below

Done.

> Unless I'm mistaken, the following calls that happen previously will add
> Shortcut1=${BrandShortName}.lnk since Shortcut0=${BrandFullName}.lnk would
> already exist on a pave over and the call to WriteINIStr below will then
> change Shortcut0=${BrandFullName}.lnk to Shortcut0=${BrandShortName}.lnk.

This isn't an issue because earlier on, in the InstallStartCleanup section, we're blowing away the existing shortcut log (by deleting the entire uninstall directory).
Attachment #8926663 - Attachment is obsolete: true
Attachment #8926683 - Flags: review?(robert.strong.bugs)
(In reply to Matt Howell [:mhowell] from comment #10)
> Created attachment 8926683 [details] [diff] [review]
> Part 2 - Support renaming shortcuts during paveover installs
> 
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #8)
> > Please add this same comment for the second case below
> 
> Done.
Thanks

> 
> > Unless I'm mistaken, the following calls that happen previously will add
> > Shortcut1=${BrandShortName}.lnk since Shortcut0=${BrandFullName}.lnk would
> > already exist on a pave over and the call to WriteINIStr below will then
> > change Shortcut0=${BrandFullName}.lnk to Shortcut0=${BrandShortName}.lnk.
> 
> This isn't an issue because earlier on, in the InstallStartCleanup section,
> we're blowing away the existing shortcut log (by deleting the entire
> uninstall directory).
Then you shouldn't need the call to WriteIniStr and it should be removed... right?
Comment on attachment 8926683 [details] [diff] [review]
Part 2 - Support renaming shortcuts during paveover installs

r=me with the removal of the calls to WriteIniStr or an explanation as to why it is necessary even with the calls that set those values in previous code.
Attachment #8926683 - Flags: review?(robert.strong.bugs) → review+
(Assignee)

Comment 13

a year ago
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #11)
> Then you shouldn't need the call to WriteIniStr and it should be removed...
> right?

No, because we've also already created the Shortcut0 entries in the new log by this point, via the calls you were mentioning. I could move those calls down here to where the shortcuts are created, but I was trying to mess with as little as I could get away with in this patch.
But they are created with these calls
  ${LogStartMenuShortcut} "${BrandShortName}.lnk"
  ${LogDesktopShortcut} "${BrandShortName}.lnk"

the LogDesktopShortcut macro translates to the following when called 
WriteINIStr "$INSTDIR\uninstall\${SHORTCUTS_LOG}" "$DESKTOP" "Shortcut0" "${BrandShortName}.lnk"

and then you update it with this call (DESKTOP only shown but it applies to STARTMENU too)
>+        WriteINIStr "$INSTDIR\uninstall\${SHORTCUTS_LOG}" "DESKTOP" \
>+                    "Shortcut0" "${BrandShortName}.lnk"

Which just writes the same value to it.
(Assignee)

Comment 15

a year ago
... that is a good point. I'll take 'em out.
(Assignee)

Comment 16

a year ago
Only change is the removal of the above-discussed WriteINIStr calls, r+ carried over.
Attachment #8926683 - Attachment is obsolete: true
Attachment #8926688 - Flags: review+
Matt, just wanted to point out that the only time the shortcuts will be renamed when using the full installer when paving over an install is when the startmenu and desktop shortcut creation isn't unchecked. Not sure if that is a case you care about but the rename code could be moved outside of those blocks along with checks for the shortcut being for that install location if you want to do it.
(Assignee)

Comment 19

a year ago
Yeah, I thought about that and left a comment about it; I decided to interpret the box being unchecked as "don't do anything to mess with any shortcuts in that location." It's something of an edge case, but I thought that seemed reasonable.
That makes sense. I think that is the same reason this shouldn't change shortcuts that have been renamed or created (can't differentiate between the two) by the user as well.
https://hg.mozilla.org/mozilla-central/rev/7ff201a34ffc
https://hg.mozilla.org/mozilla-central/rev/4bdb96f48bb6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.