Use the precomplete file to determine files to uninstall instead of the uninstall.log

RESOLVED FIXED in Firefox 35

Status

()

Firefox
Installer
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

unspecified
Firefox 36
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 wontfix, firefox34 wontfix, firefox35 fixed, firefox36 fixed, firefox37 unaffected, firefox-esr31 wontfix)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 8522632 [details] [diff] [review]
patch rev1

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 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+
(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.
Created attachment 8522665 [details] [diff] [review]
patch rev2

Changed this to use the __UNINSTALL__ define instead of checking _MOZFUNC_UN
Attachment #8522632 - Attachment is obsolete: true
Attachment #8522665 - Flags: review+
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?
I tried that when I wrote the OnStubInstallUninstall and it had issues. I'll check again.
Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/c3bf44ea6aed
Target Milestone: --- → Firefox 36
Duplicate of this bug: 577563
https://hg.mozilla.org/mozilla-central/rev/c3bf44ea6aed
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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?
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → fixed
status-firefox37: --- → unaffected
status-firefox-esr31: --- → affected
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 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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/af9b9ed74be4
status-firefox34: affected → wontfix
status-firefox35: affected → fixed
status-firefox-esr31: affected → wontfix
You need to log in before you can comment on or make changes to this bug.