392150 - Clean the old updates directory on uninstall / in-place upgrade

Status: RESOLVED FIXED

(Firefox :: Installer, defect, P1)

Description

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

Similar to the VirtualStore cleanup performed by the fix for bug 387385

Comment 1

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

Drivers, if a user has a Software Update pending and runs the installer or for whatever reason Software Update doesn't cleanup the update files the app may be downgraded (see bug 376305). I think this is reason enough to block with a priority of P3.

Comment 3

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

Why was this ever a P3? This requires beta exposure ...

Comment 4

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

To be clear, the reason why I think this is a Big Deal is that now that we're being less aggressive about fetching updates, in terms of popping UI up in front of users, I fear it's more likely that someone may have gotten an update and not realized it.

This concern might not be valid, though, as the situations would be:

User has 3.0.1, updater fetches 3.0.2, user downloads 3.0.2, no change
User has 3.0.1, updated fetches 3.0.2, user downloads 3.0.3, regressed

but after it regresses, won't it then fetch the newer update?

Comment 5

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

I should have a patch today after finishing the testing of the patch for bug 380015

Comment 6

[Mike Connor [:mconnor]]

Rob, what's up with this?

Comment 7

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

Turns out that I needed a Vista system since the update dir is located in a different dir for Vista. It is a bit more complicated than I first thought and I am working on it right now.

Comment 8

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

Attachment: patch rev1

mconnor... any chance on you reviewing this?

Comment 9

[Seth Spitzer]

Comment on attachment 310900 [details] [diff] [review]
patch rev1

r=sspitzer

two questions:

1)  RmDir /r "$R6"

eyeballing the code, it looks like we'll never generate a bad value for $R6, but would it be worth double checking that $R9 and $R8 are not empty?  

I hate to miss seeing a scenario and this code could remove $LOCALAPPDATA\*

Comment 10

[Seth Spitzer]

> two questions:

sorry, just one.

Comment 11

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

I'm testing a patch with the additional checks and will submit shortly

Comment 12

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

Attachment: patch w/ comments addressed

carrying forward r+

Comment 13

[Seth Spitzer]

Comment on attachment 311024 [details] [diff] [review]
patch w/ comments addressed

thanks for adding the "IfFileExists "$R6\updates" +1 end" sanity check.

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 311024 [details] [diff] [review]
patch w/ comments addressed

a1.9b5=mconnor on behalf of drivers for checkin during baking period

Comment 15

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

fixing summary... we don't clean out the updates dir after software update since it contains the update history log and software update manages the mar files.

Comment 16

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

Checked in to trunk

Checking in mozilla/browser/installer/windows/nsis/uninstaller.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/uninstaller.nsi,v  <--  uninstaller.nsi
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.38; previous revision: 1.37
done

Comment 17

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

Attachment: followup patch - not required for beta 5 but I'd like to land it

Seth, this should really be called from installer.nsi since the profile path is app specific

Comment 18

[Seth Spitzer]

Comment on attachment 311048 [details] [diff] [review]
followup patch - not required for beta 5 but I'd like to land it

r=sspitzer,

sorry for not catching the Mozilla\Firefox in toolkit/mozapps/installer/windows/nsis/common.nsh during my other review.

thanks for the fix.

Comment 19

[Damon Sicore (:damons)]

Comment on attachment 311048 [details] [diff] [review]
followup patch - not required for beta 5 but I'd like to land it

a1.9beta5+=damons

Comment 20

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

Comment on attachment 311048 [details] [diff] [review]
followup patch - not required for beta 5 but I'd like to land it

Followup patch checked in to trunk

Checking in mozilla/browser/installer/windows/nsis/installer.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/installer.nsi,v  <--  installer.nsi
new revision: 1.41; previous revision: 1.40
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.39; previous revision: 1.38
done