1415647 - Bug 1413295 followup: Address reliability and paveover install problems

Status: RESOLVED FIXED

(Firefox :: Installer, enhancement, P1)

Description

[Molly Howell (she/her) (no longer active)]

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.

Comment 1

[Molly Howell (she/her) (no longer active)]

Attachment: Part 1 - Streamline PostUpdate shortcut renaming code

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.

Comment 2

[Molly Howell (she/her) (no longer active)]

Attachment: Part 2 - Support renaming shortcuts during paveover installs

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.

Comment 3

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

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

Comment 4

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

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.

Comment 5

[Molly Howell (she/her) (no longer active)]

(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.

Comment 6

[Molly Howell (she/her) (no longer active)]

Attachment: Part 1 - Streamline PostUpdate shortcut renaming code

Minor changes, r+ carried over.

Comment 7

[Molly Howell (she/her) (no longer active)]

Attachment: Part 2 - Support renaming shortcuts during paveover installs

Comment 8

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

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.

Comment 9

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

iirc that would only affect the full installer and not the stub

Comment 10

[Molly Howell (she/her) (no longer active)]

Attachment: 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.

> 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).

Comment 11

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

(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 12

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

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.

Comment 13

[Molly Howell (she/her) (no longer active)]

(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.

Comment 14

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

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.

Comment 15

[Molly Howell (she/her) (no longer active)]

... that is a good point. I'll take 'em out.

Comment 16

[Molly Howell (she/her) (no longer active)]

Attachment: Part 2 - Support renaming shortcuts during paveover installs

Only change is the removal of the above-discussed WriteINIStr calls, r+ carried over.

Comment 17

[Molly Howell (she/her) (no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff201a34ffc6a98c6433759ed21c910629e39ec
Bug 1415647 Part 1 - Streamline PostUpdate shortcut renaming code. r=rstrong

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bdb96f48bb68c7552b7051a9e5edc3bd238b7d0
Bug 1415647 Part 2 - Support renaming shortcuts during paveover installs. r=rstrong

Comment 18

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

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.

Comment 19

[Molly Howell (she/her) (no longer active)]

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.

Comment 20

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

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.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7ff201a34ffc
https://hg.mozilla.org/mozilla-central/rev/4bdb96f48bb6