408314 - uninstaller leaves behind empty install directory

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

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

spinoff of bug 407008

Comment 1

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

Attachment: patch

Comment 2

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

Attachment: patch rev2

When uninstalling the working dir is being set to the $INSTDIR which prevents its removal so this patch sets it back to $EXEDIR which is the default. When installing it is already set to $INSTDIR as one of the first actions. It turns out that on XP NSIS isn't auto creating sub-directories during the in use check so I removed the in use checks for jar files. This shouldn't be an issue since if the jar files are in use the dll's that are checked prior to the jar files should also be in use so it will catch it anyway.

Comment 3

[Seth Spitzer]

Comment on attachment 310335 [details] [diff] [review]
patch rev2

r=sspitzer, on robert's request.

the patch looks good, thanks for also giving me some background over the phone.

one suggestion for the "Example Usage:" comment,

+ *  ; that there are no more files in the $INSTDIR directory to check.
  *  Push "end"
- *  Push "chrome\toolkit.jar"

is it worth explaining why you shouldn't / can't list the .jar files (as we know they can be in use and you've seen copy can fail on XP?)

also, to confirm my undertanding, this isn't an issue for mail's shared.nsh and suite's shared.nsh because neither of those products do the "file in use check on (un)install", right?

Comment 4

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

I can add a comment that subdir's don't work on XP.

You are correct that mail and suite both don't use that yet.

Comment 5

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

Comment on attachment 310335 [details] [diff] [review]
patch rev2

Drivers: this patch fixes a problem where the install dir is left behind on uninstall and incorrectly telling the user that a reboot might be necessary. It is fairly simple and I'm fine with landing either before or after Beta 5

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310335 [details] [diff] [review]
patch rev2

a1.9=beltzner

Comment 7

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

Attachment: patch as checked in with comment fix

Comment 8

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

Checked in to trunk

Checking in mozilla/browser/installer/windows/nsis/shared.nsh;
/cvsroot/mozilla/browser/installer/windows/nsis/shared.nsh,v  <--  shared.nsh
new revision: 1.19; previous revision: 1.18
done
Checking in mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh;
/cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh,v  <--  common.nsh
new revision: 1.36; previous revision: 1.35
done

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Robert, I can verify this bug when removing Minefield via the Software panel. No reboot window is shown. But running the helper.exe application directly it doesn't work. The reboot window still appears. Is this the expecting behavior?

Comment 10

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

Henrik, what specifically "doesn't work"? This bug was for the install dir being left behind when uninstalling from Add / Remove Programs and I suspect that what you are seeing is due to the dir being opened by windows explorer which could easily cause directories not to be removed and require a reboot. Also, we don't try to remove the install dir on reboot due to the possibility that the user installed into a "bad" location such as "Program Files", a root dir, etc.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Robert, you are right. I had accidentally open the uninstall folder while removing Minefield. That's why it doesn't work. But what I wonder is why other uninstallers are capable of removing the folder anyway. Due to this is not a common it is working fine.

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032304 Minefield/3.0b5pre