Closed
Bug 1038560
Opened 10 years ago
Closed 10 years ago
Use the precomplete file to determine files to uninstall instead of the uninstall.log
Categories
(Firefox :: Installer, defect)
Tracking
()
RESOLVED
FIXED
Firefox 36
People
(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)
References
Details
Attachments
(1 file, 1 obsolete file)
18.98 KB,
patch
|
robert.strong.bugs
:
review+
lsblakk
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta-
lsblakk
:
approval-mozilla-esr31-
|
Details | Diff | Splinter Review |
This is a much better source for the files and directories and I recently noticed that new files aren't always being added to the uninstall.log possibly due to the numerous changes to app update especially with the addition of staging.
Assignee | ||
Comment 1•10 years ago
|
||
There is also some code cleanup with the removal of the NO_UNINSTALL_SURVEY define and related code as well as preparation for this to be used on install.
Attachment #8522632 -
Flags: review?(netzen)
Comment 2•10 years ago
|
||
Comment on attachment 8522632 [details] [diff] [review] patch rev1 Review of attachment 8522632 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, one question inline. ::: browser/installer/windows/nsis/uninstaller.nsi @@ +457,5 @@ > + > + ; Admin is required to delete files on reboot so only add the moz-delete if > + ; the user is an admin. After calling UAC::IsAdmin $0 will equal 1 if the user > + ; is an admin. > + UAC::IsAdmin Even if the user installed to a user directory? Not sure I fully understand this check.
Attachment #8522632 -
Flags: review?(netzen) → review+
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Brian R. Bondy [:bbondy] from comment #2) > Comment on attachment 8522632 [details] [diff] [review] > patch rev1 > > Review of attachment 8522632 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, one question inline. > > ::: browser/installer/windows/nsis/uninstaller.nsi > @@ +457,5 @@ > > + > > + ; Admin is required to delete files on reboot so only add the moz-delete if > > + ; the user is an admin. After calling UAC::IsAdmin $0 will equal 1 if the user > > + ; is an admin. > > + UAC::IsAdmin > > Even if the user installed to a user directory? Not sure I fully understand > this check. The code below it will add a file named $INSTDIR\${FileMainEXE}.moz-delete that will be removed on reboot which requires admin. The file prevents an install until after a reboot and though there is other code that tries to clean this up there are cases where it doesn't. I'm tempted to remove it entirely. There are still cases where files have been locked so I'll file another bug to make it more specific.
Assignee | ||
Comment 4•10 years ago
|
||
Changed this to use the __UNINSTALL__ define instead of checking _MOZFUNC_UN
Attachment #8522632 -
Attachment is obsolete: true
Attachment #8522665 -
Flags: review+
Comment 5•10 years ago
|
||
Comment on attachment 8522665 [details] [diff] [review] patch rev2 Review of attachment 8522665 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/installer/windows/nsis/uninstaller.nsi @@ +419,4 @@ > > + StrCpy $R2 "false" > + StrCpy $R3 "false" > + ${un.RemovePrecompleteEntries} "$R2" "$R3" Couldn't you just pass "false" "false" directly here?
Assignee | ||
Comment 6•10 years ago
|
||
I tried that when I wrote the OnStubInstallUninstall and it had issues. I'll check again.
Assignee | ||
Comment 7•10 years ago
|
||
Pushed to fx-team https://hg.mozilla.org/integration/fx-team/rev/c3bf44ea6aed
Target Milestone: --- → Firefox 36
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c3bf44ea6aed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8522665 [details] [diff] [review] patch rev2 I would like to get this and dependent bugs landed for at least Firefox 35. [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: Files are not removed on uninstall and files left behind can lead to problems with future installs. User impact if declined: Files are not removed on uninstall and files left behind can lead to problems with future installs. Fix Landed on Version: Firefox 36 Risk to taking this patch (and alternatives if risky): Small. Worst case scenario is that files won't be removed on uninstall. It has been tested thoroughly by myself and has been verified. String or UUID changes made by this patch: None
Attachment #8522665 -
Flags: approval-mozilla-esr31?
Attachment #8522665 -
Flags: approval-mozilla-beta?
Attachment #8522665 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
status-firefox33:
--- → wontfix
status-firefox34:
--- → affected
status-firefox35:
--- → affected
status-firefox36:
--- → fixed
status-firefox37:
--- → unaffected
status-firefox-esr31:
--- → affected
Comment 11•9 years ago
|
||
Comment on attachment 8522665 [details] [diff] [review] patch rev2 It's too late to take this change in Firefox 34 and ESR 31.3. We can consider it for Firefox 35 and ESR 31.4.
Attachment #8522665 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment 12•9 years ago
|
||
Comment on attachment 8522665 [details] [diff] [review] patch rev2 Taking this for Aurora (35). It doesn't meet ESR criteria and it's not clear there's much value here vs. waiting for 38.0ESR so unless there's a strong case for this from ESR deployers, let's wait.
Attachment #8522665 -
Flags: approval-mozilla-esr31?
Attachment #8522665 -
Flags: approval-mozilla-esr31-
Attachment #8522665 -
Flags: approval-mozilla-aurora?
Attachment #8522665 -
Flags: approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•