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)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 36
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- unaffected
firefox-esr31 --- wontfix

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch rev1 (obsolete) — Splinter Review
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.
Attached patch patch rev2Splinter Review
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
https://hg.mozilla.org/mozilla-central/rev/c3bf44ea6aed
Status: ASSIGNED → RESOLVED
Closed: 10 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?
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+
You need to log in before you can comment on or make changes to this bug.