368661 - on install, remove the updates directory, updates.xml and the active-update.xml file

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[(not reading, please use [e-mail] instead)]

on installer, remove the updates directory and the active-update.xml file

this is something that I believe juan or jay pointed out recently.

what happens if you:

1) install firefox
2) do 'check for updates'
3) download the mar, but don't apply it

exit

run an previously downloaded fx installer, and install on top of your firefox directory that contains the pending update.

the uninstaller does this (see http://lxr.mozilla.org/seamonkey/source/browser/installer/windows/nsis/uninstaller.nsi#241), but not the installer (according to robert, who just eyeballed the code).

we should also remove updates.xml, which is the history of the updates.  as robert says, we should remove all three (active-update.xml, updates.xml, updates/), as when you run the installer, you are in a fresh state.

This is something I think we'd want to fix for 2.0.0.2, so asking for blocking.

Comment 1

[(not reading, please use [e-mail] instead)]

this would also affect tbird, so cc mscott.

Comment 2

[(not reading, please use [e-mail] instead)]

taking.

I chatted with robert about this, and the fix would be to http://lxr.mozilla.org/seamonkey/source/browser/installer/windows/nsis/installer.nsi (and again, to http://lxr.mozilla.org/seamonkey/source/mail/installer/windows/nsis/installer.nsi and http://lxr.mozilla.org/seamonkey/source/calendar/installer/windows/nsis/installer.nsi)

Comment 3

[Jay Patel [:jay]]

Seth:  I did not get a chance to test this today, but based on your code review, it looks like a pave over install might cause some issues as I mentioned today, so I will be sure to test this tomorrow.  Do we want to fix this on both branches (will it be easy to fix with the 1.5 installer and the 2.0 nsis installer?)

Comment 4

[(not reading, please use [e-mail] instead)]

yes, a "pave over" install could cause problems.

As for the MOZILLA_1_8_0_BRANCH, I'm not sure if it is a problem with the xpinstaller.  I haven't tested, but it might be.  

I'm going to start with the NSIS installer based fixed for trunk and MOZILLA_1_8_BRANCH.

Perhaps dveditz knows more about the old xpinstaller code?

Comment 5

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

It looks like
deleteThisFolder("Program", "updates");
and
deleteThisFile("Program", "updates.xml");
deleteThisFile("Program", "active-update.xml");

could be called to accomplish this... perhaps after calling performInstall() in browser.jst?

dveditz knows much more than I about this code so I may be way off.

Comment 6

[(not reading, please use [e-mail] instead)]

confirming that on a "pave over" install, the active-update.xml, updates.xml and pending update (in updates/0) are not removed, so that when we start up after intstalling, apply the pending update and if you look at the update history, it shows all the old history.

last night robert suggested a fix for the NSIS installer, so I'm looking into that first.

Comment 7

[Jay Patel [:jay]]

I just tested this too and confirmed the same thing Seth saw.  However, I also am seeing some odd behavior with one of my test cases:

1. Install 1.5.0.8
2. Update to 1.5.0.9 (on "release" channel"
3. Check for update (offers 2.0.0.1)
4. Download update but don't apply (choose Later)
5. Install 2.0.0.0 on top of the original 1.5.0.8 directory
6. Check for update (offers 2.0.0.1) BUT it appears that the active-update.xml and updates.xml don't change from the previous update attempt (still see 1.5.0.9 -> 2.0.0.1 snippet info).
7. Apply update and end up with 2.0.0.1 BUT updates.xml still shows the 1.5.0.9 -> 2.0.0.1 (NOT 2.0.0.0 -> 2.0.0.1)


So I'm wondering which snippet that second update attempt is really using.  I have not tried to retest with live http headers to confirm, but Juan is trying this scenario also.

Comment 8

[Jay Patel [:jay]]

Forgot to mention that in step #3, I changed the update channel to "releasetest" and then back to "release" in step #6. 

Comment 9

[juan becerra [:juanb]]

I'm unable to repeat the steps above. After choosing to apply the update later, and attempting to install on top of it, I find that on launching FF the applying of the update will take place as if the 2.0 installation had not taken place. I tried a couple of times, one on default location, and one on a custom folder.

Comment 10

[(not reading, please use [e-mail] instead)]

in response to jay's comment #7:

You did not see the updater, so you did not see the pave over install bug.  But I can explain why.

The build that downloaded the mar was 1.5.0.9, which does not have my fix.  (1.5.0.10 does).  So, the channel attribute in your active-update.xml file (it was not set) did not match the app.update.channel pref value ("releasetest").

Because the two don't match, when FF2000 starts up, because the channels don't match, we threw away the update on disk.

To reproduce the pave over problem, you'd need to do it with:

    * 1.5.0.10 (since it has my fix), or
    * 2.0.0.x -> 2.0.0.x (since both use the channel attribute)

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: patch

robert, sorry for the delay.  what do you think?

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: oops, wrong patch (same idea, better comment)

Comment 13

[(not reading, please use [e-mail] instead)]

here's how I tested my patch:

1)  run ./firefox-3.0a2pre.en-US.win32.installer.exe, install minefield to a directory "foo", and start up minefield.
2)  set app.update.url.override to http://www.sspitzer.org/emtpy.xml
3)  get the empty update, but don't apply it
4)  exit minefield
5)  re-run ./firefox-3.0a2pre.en-US.win32.installer.exe, pave over "foo"
6)  before you get to the "start up minefield" stage of the installer, you will see updates/, updates.xml and active-update.xml get removed, which is what we want.

after you start up minefield, you won't get the updater.exe, and in the updates history UI you will see "no updates installed yet".

if you were to do this with a minefield or bonecho build without the fix, you would not see those files and directory removed (in step 60, and you would get the updater.exe process.

note, my test mar (that empty.xml points to) is empty, so we fail to apply it (as we have code in updater.cpp that avoids applying an update with "no actions".)

you could, of course, try all this with a nightly builds which have real updates on the AUS server.

Comment 14

[(not reading, please use [e-mail] instead)]

Attachment: for calendar

Comment 15

[(not reading, please use [e-mail] instead)]

Attachment: for tbird

Comment 16

[(not reading, please use [e-mail] instead)]

Comment on attachment 253528 [details] [diff] [review]
for tbird

scott, we'd want this on trunk and MOZILLA_1_8_BRANCH, assuming you and robert agree it is the correct fix.

Comment 17

[(not reading, please use [e-mail] instead)]

Comment on attachment 253527 [details] [diff] [review]
for calendar

lilmatt, are you shipping off the MOZILLA_1_8_BRANCH as well?  if so, you'd want this.

Comment 18

[Scott MacGregor]

Comment on attachment 253528 [details] [diff] [review]
for tbird

approving for the branch, this doesn't effect firefox at all.

Comment 19

[Matthew (lilmatt) Willis]

Comment on attachment 253527 [details] [diff] [review]
for calendar

r=lilmatt with a space between the # and "Remove", and with that comment wrapped at 80 chars

Comment 20

[(not reading, please use [e-mail] instead)]

thanks mscott / lilmatt.

I took my comment style from:

#Remove Talkback files from old location (in case user updates from 1.0.x)

but, I'll fix it to include a space and be less than 80 chars.

Comment 21

[(not reading, please use [e-mail] instead)]

for 1.5.0.x, I got lucky (thanks to bsmedberg)

dveditz pointed me to mozilla/toolkit/mozapps/installer/make-installjsremoves.pl

for firefox and tbird, on the MOZILLA_1_8_0_BRANCH we have:

mozilla/browser/installer/removed-files.in
mozilla/mail/installer/removed-files.in 

from my fx build log:

/usr/bin/perl /cygdrive/c/builds/150x/mozilla/toolkit/mozapps/installer/make-ins
talljsremoves.pl ../removed-files > instgen/removed-files.js

...

  -Iinstgen/removed-files.js \
  /cygdrive/c/builds/150x/mozilla/browser/installer/windows/browser.jst > instge
n/browser.jst

once I confirm this is used by tbird as well (by looking at the build log), I will attach a patch.

question for lilmatt:  there is no mozilla/calendar/installer/removed-files.in on the MOZILLA_1_8_0_BRANCH.  Is that because there was no calendar back then?

Comment 22

[(not reading, please use [e-mail] instead)]

from dveditz, in regards to rstrong original suggestion:

> you do NOT want to put anything after performInstall(), it won't be executed

I didn't know that, so I figured i'd share with everyone else.

thanks for the tip, dan.

Comment 23

[(not reading, please use [e-mail] instead)]

Attachment: good for trunk, MOZILLA_1_8_0_BRANCH (except for calendar), MOZILLA_1_8_BRANCH

carrying over lilmatt and mscott's reviews, but would still like to hear from robert.

Comment 24

[(not reading, please use [e-mail] instead)]

note, from my 1.5 tree, my browser.jst file contains:

function removeOldFiles() {
...
  deleteThisFile("Program", "uninstall/UninstallFirefox.exe");
  deleteThisFile("Program", "uninstall/UninstallDeerPark.exe");
  deleteThisFolder("Program", "updates");
  deleteThisFile("Program", "active-update.xml");
  deleteThisFile("Program", "updates.xml");
}

which is what we want.

Comment 25

[Jay Patel [:jay]]

Let's get this reviewed asap so we can approve it for landing on the branches.

Comment 26

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

(In reply to comment #25)
> Let's get this reviewed asap so we can approve it for landing on the branches.
Jay and Seth, since this needs to be reviewed asap I think it makes sense for someone else to review it besides me since I am working on the remaining Vista bugs for 1.9.1.2... true?

Comment 27

[(not reading, please use [e-mail] instead)]

Comment on attachment 253543 [details] [diff] [review]
good for trunk, MOZILLA_1_8_0_BRANCH (except for calendar), MOZILLA_1_8_BRANCH

good point, robert.  sending review to dveditz, who I've already been chatting with about this.

note, I have r=mscott/lilmatt already.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 253543 [details] [diff] [review]
good for trunk, MOZILLA_1_8_0_BRANCH (except for calendar), MOZILLA_1_8_BRANCH

r=dvedotz

Comment 29

[Jay Patel [:jay]]

Comment on attachment 253543 [details] [diff] [review]
good for trunk, MOZILLA_1_8_0_BRANCH (except for calendar), MOZILLA_1_8_BRANCH

Approved for both branches, a=jay

Comment 30

[(not reading, please use [e-mail] instead)]

fixed on trunk and MOZILLA_1_8_BRANCH

Comment 31

[(not reading, please use [e-mail] instead)]

fixed on MOZILLA_1_8_0_BRANCH (for firefox and mail, no calendar on that branch)

Comment 32

[(not reading, please use [e-mail] instead)]

I need to back this out, as this is the wrong fix.

the fix will affect both software update and the NSIS / xpinstall installer.

Comment 33

[(not reading, please use [e-mail] instead)]

here's why this is wrong:

we use the same file when generating a mar, for updates.

see http://lxr.mozilla.org/mozilla/source/tools/update-packaging/common.sh#65

backing out now from trunk, MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH

Comment 34

[(not reading, please use [e-mail] instead)]

Attachment: patch for browser for trunk

Comment 35

[(not reading, please use [e-mail] instead)]

Attachment: patch, per robert

Comment 36

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

Comment on attachment 253727 [details] [diff] [review]
patch, per robert

punt on calendar and get r= from lilmatt.

lilmatt - if you want to give me r rights on installer changes please let me know.

Comment 37

[(not reading, please use [e-mail] instead)]

Comment on attachment 253727 [details] [diff] [review]
patch, per robert

seeking approval for MOZILLA_1_8_BRANCH

Comment 38

[(not reading, please use [e-mail] instead)]

Attachment: patch for the MOZILLA_1_8_0_BRANCH

Comment 39

[Jay Patel [:jay]]

Comment on attachment 253727 [details] [diff] [review]
patch, per robert

Approved for 1.8 branch, a=jay for browser... will leave it up to lilmatt and mscott for the r= so Seth can land this for cal and mail.

Comment 40

[Jay Patel [:jay]]

Seth:  I'm assuming you're still looking into what we want to do on 1.8.0, since the new patch won't work there.  I'll be keeping an eye on bugmail for a few more hours if you and/or Rob get a patch together and reviewed... in case you want to land this tonight.

Comment 41

[Scott MacGregor]

Comment on attachment 253732 [details] [diff] [review]
patch for the MOZILLA_1_8_0_BRANCH

thanks seth!

Comment 42

[Jay Patel [:jay]]

Comment on attachment 253732 [details] [diff] [review]
patch for the MOZILLA_1_8_0_BRANCH

Approved for 1.8.0 branch, a=jay.  Two out of three reviews ain't bad.  Looks good to me, so hopefully dveditz can give the r= after the fact.

Comment 43

[(not reading, please use [e-mail] instead)]

fixed on trunk, MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH for browser and mail.

fix not landed for calender (on trunk / MOZILLA_1_8_BRANCH), waiting for lilmatt

Comment 44

[Matthew (lilmatt) Willis]

Comment on attachment 253727 [details] [diff] [review]
patch, per robert

r=lilmatt

Comment 45

[Daniel Veditz [:dveditz]]

Comment on attachment 253732 [details] [diff] [review]
patch for the MOZILLA_1_8_0_BRANCH

post facto r=dveditz

You may have wanted to comment why this isn't in the normal removeOldFiles function with the other deletes, even a "see bug 368331)". I guess cvs blame will have to be enough.

Comment 46

[(not reading, please use [e-mail] instead)]

lilmatt / dveditz, thanks for the reviews.

I've landed the calendar fixes to both trunk and MOZILLA_1_8_BRANCH.

Comment 47

[Daniel Veditz [:dveditz]]

(In reply to comment #22)
> > you do NOT want to put anything after performInstall(), it won't be executed

Script after that point still runs, of course, but any install-ish commands that make changes to the user's machine (anything that would end up noted in the install.log file) will return errors after that point.

The reason is that the startInstall()/performInstall() calls form a transaction and the install actions in the middle aren't done until the performInstall() call. This allows an install that runs into trouble to call cancelInstall() instead and bail out rather than leave the user in a half-installed and possibly non-running state.

Comment 48

[(not reading, please use [e-mail] instead)]

note to mscott:  I failed to land the mail part of this fix on the trunk until just now.

Comment 49

[Carsten Book [:Tomcat]]

Verified fixed for trunk, 1.8.1.2 and 1.8.0.10 for browser and mail.

Using: (for browser)
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.10pre) Gecko/20070208 Firefox/1.5.0.10pre ID:2007020809
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a3pre) Gecko/2007020804 Minefield/3.0a3pre
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2pre) Gecko/2007020804 BonEcho/2.0.0.2pre

(for trunk)
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.10pre) Gecko/20070208 Thunderbird/1.5.0.10pre ID:2007020806
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2pre) Gecko/20070208 Thunderbird/2.0pre ID:2007020804

also done tests on Fedora Fc6

I tested several updates and used the steps the reproduce in this bug. The Update directory, updates.xml and active-update.xml file were removed on a new latest build full installation like expected.

Comment 50

[elegant_dice]

This bug is still around.

It just happened on my Grandad's computer.
Win XP Home,  upgrading from 2.0.0.6 to 2.0.0.7 (I believe)

I was called in after it started to repeatedly give the "One or more files could not be updated" message.  I had to kill Firefox from the taskmanager.  I guess Firefox had attempted to upgrade itself from the normal user account.

I logged out and logged in as administrator, and tried to run firefox.  It appeared as a tiny window (top-left), and if you maximized it, the window contents were completely grey.  No interaction was possible.   Maybe the regular user was able to partially upgrade firefox?

I killed it, uninstalled and reinstalled off the internet.  The admin's firefox started working again.

I logged out, logged back in as the normal user, but the "Cannot be updated" message continued to cause trouble.

I deleted the localuser\localsettings\etc\updates.xml folders and files, and tried again... success.

however, this fix was way beyond the abilities of my Grandad.  This bug needs to be resolved otherwise his computer may be crippled in the future by the next upgrade.

thanks
Paul

Comment 51

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

Did you experience this while running the windows installer or during software update?

Comment 52

[elegant_dice]

The initial install by Grandad would've been done via the automatic software update.

When I logged in as admin and tried to run firefox (unsuccessfully), i was hoping to use the software update.  I couldn't, so i used the windows installer.

Then i logged out and in as User.  I did not try to run the windows installer as User.  Instead I just ran firefox, where it continued to complain not being able to apply updates.  This was after it had been updated, and all instances of firefox had closed.


Ideally, firefox should ask user to log on as admin so that firefox can install updates.  Or something that basically babysits the user through the process.

Comment 53

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

This bug was about the Windows installer removing those files and has nothing to do with Software Update which is where you experienced a problem.

Comment 54

[elegant_dice]

agreed, bug https://bugzilla.mozilla.org/show_bug.cgi?id=368661 seems to be more appropriate.

sorry!

Comment 55

[elegant_dice]

agreed, bug https://bugzilla.mozilla.org/show_bug.cgi?id=374900 seems to be more appropriate.

sorry!