Closed
Bug 1415647
Opened 7 years ago
Closed 7 years ago
Bug 1413295 followup: Address reliability and paveover install problems
Categories
(Firefox :: Installer, enhancement, P1)
Tracking
()
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox57 | --- | unaffected |
firefox58 | --- | fixed |
People
(Reporter: mhowell, Assigned: mhowell)
References
(Regressed 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
mhowell
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mhowell
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•7 years 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•7 years 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 3•7 years ago
|
||
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 4•7 years ago
|
||
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•7 years 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•7 years ago
|
||
Minor changes, r+ carried over.
Attachment #8926560 -
Attachment is obsolete: true
Attachment #8926662 -
Flags: review+
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8926565 -
Attachment is obsolete: true
Attachment #8926663 -
Flags: review?(robert.strong.bugs)
Comment 8•7 years ago
|
||
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)
Comment 9•7 years ago
|
||
iirc that would only affect the full installer and not the stub
Assignee | ||
Comment 10•7 years 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)
Comment 11•7 years ago
|
||
(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•7 years ago
|
||
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•7 years 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.
Comment 14•7 years ago
|
||
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•7 years ago
|
||
... that is a good point. I'll take 'em out.
Assignee | ||
Comment 16•7 years 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+
Assignee | ||
Comment 17•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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.
Comment 20•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7ff201a34ffc
https://hg.mozilla.org/mozilla-central/rev/4bdb96f48bb6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•