Spinoff of bug 353804 in order to split up the bug into manageable pieces and to have a bug / patch that is more likely to make it onto a branch.

I'm going to add a bunch of tests to nsIUpdateManager in this bug

Dave, this changes the test names which I've been wanting to do for a while and cleans up the tests a tad in preparation for the new tests.

Comment on attachment 369866 [details] [diff] [review] Test cleanup and prep gonna do a bit more

Dave, I'm separating the test data cleanup patch from the rest to make this easier to review. This patch renames a couple of the test data files and removes the xml files since the next patch generates the xml files as needed in the tests.

Dave, besides a lot of cleanup this also adds the functions to head_update.js for the new tests.

This includes one generic test for nsIUpdateManager and one test specific for this bug. I'll add more as time permits.

I didn't clean this up as much as it should be since I'd like to try to get this for 3.5 as well as 3.0.x

Comment on attachment 370319 [details] [diff] [review] test cleanup There is a problem with test_0110_PatchApply.js... I'll resubmit after I figure out what it is

had an extra backslash in the app.update.url.override pref

Comment on attachment 370311 [details] [diff] [review] test data cleanup I'm going to move the test cleanup over to bug 487346 in the hope to get this fixed sooner

Comment on attachment 373020 [details] [diff] [review] patch w/ tests rev3 - requires patch from bug 487346 Generally looks good, a few comment changes and one issue that I think needs answering first. >diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in > // Dig through the update history to find the patch that was just > // installed and update its metadata. >- for (var i = 0; i < um.updateCount; ++i) { >- var umUpdate = um.getUpdateAt(i); >- if (umUpdate && umUpdate.version == update.version && >- umUpdate.buildID == update.buildID) { >- umUpdate.statusText = update.statusText; >- break; >- } >- } >+ um.activeUpdate = update; You're not really updating the patch's metadata anymore I think, adjust the comment accordingly. > downloadUpdate: function AUS_downloadUpdate(update, background) { > if (!update) > throw Cr.NS_ERROR_NULL_POINTER; >+ >+ let ai = Cc["@mozilla.org/xre/app-info;1"]. >+ getService(Ci.nsIXULAppInfo); >+ let vc = Cc["@mozilla.org/xpcom/version-comparator;1"]. >+ getService(Ci.nsIVersionComparator); >+ // Don't download the update if the update's version is less than the >+ // current application's version. >+ if (update.version && vc.compare(update.version, ai.version) < 0) { >+ LOG("UpdateService", "downloadUpdate - removing update for previous " + >+ "application version " + update.version); >+ cleanupActiveUpdate(); >+ return STATE_NONE; >+ } Shouldn't this be <=? If we're already running 3.5.1 we don't need to download the update to 3.5.1. You have a test for this too, maybe I'm missing something?. The r- is pending an answer to this. > // Otherwise add it to the front of the list. >- if (update) >- this._updates = [update].concat(this._updates); >+ this._updates = [update].concat(this._updates); > }, |this._updates.unshift(update)| is probably simpler >+ /// Don't write updates that have a temporary state to the updates.xml file. Only need two slashes

(In reply to comment #19) > (From update of attachment 373020 [details] [diff] [review]) > Generally looks good, a few comment changes and one issue that I think needs > answering first. > > >diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in > > > // Dig through the update history to find the patch that was just > > // installed and update its metadata. > >- for (var i = 0; i < um.updateCount; ++i) { > >- var umUpdate = um.getUpdateAt(i); > >- if (umUpdate && umUpdate.version == update.version && > >- umUpdate.buildID == update.buildID) { > >- umUpdate.statusText = update.statusText; > >- break; > >- } > >- } > >+ um.activeUpdate = update; > > You're not really updating the patch's metadata anymore I think, adjust the > comment accordingly. It sets the patch's metadata without digging through the history so I changed this to // Update the patch's metadata. > > > downloadUpdate: function AUS_downloadUpdate(update, background) { > > if (!update) > > throw Cr.NS_ERROR_NULL_POINTER; > >+ > >+ let ai = Cc["@mozilla.org/xre/app-info;1"]. > >+ getService(Ci.nsIXULAppInfo); > >+ let vc = Cc["@mozilla.org/xpcom/version-comparator;1"]. > >+ getService(Ci.nsIVersionComparator); > >+ // Don't download the update if the update's version is less than the > >+ // current application's version. > >+ if (update.version && vc.compare(update.version, ai.version) < 0) { > >+ LOG("UpdateService", "downloadUpdate - removing update for previous " + > >+ "application version " + update.version); > >+ cleanupActiveUpdate(); > >+ return STATE_NONE; > >+ } > > Shouldn't this be <=? If we're already running 3.5.1 we don't need to download > the update to 3.5.1. You have a test for this too, maybe I'm missing > something?. The r- is pending an answer to this. Nightly updates are equal so it can't prevent updates to the same version > > // Otherwise add it to the front of the list. > >- if (update) > >- this._updates = [update].concat(this._updates); > >+ this._updates = [update].concat(this._updates); > > }, > > |this._updates.unshift(update)| is probably simpler > > >+ /// Don't write updates that have a temporary state to the updates.xml file. > > Only need two slashes fixed

Comment on attachment 373142 [details] [diff] [review] patch rev4 with comments addressed (checked in) Right, had forgotten about nightlies, shame.

Pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/a0b731e5e5ad

Comment on attachment 373142 [details] [diff] [review] patch rev4 with comments addressed (checked in) Drivers, this fixes the typical case for official release versions where an update is started in the background by user1, user1 quits, user2 installs a newer version, user1 laucnhes Firefox and continues to download / apply the older version. This has happened several times as can be seen by the dupes.

(In reply to comment #20) > (In reply to comment #19) > > Shouldn't this be <=? If we're already running 3.5.1 we don't need to download > > the update to 3.5.1. You have a test for this too, maybe I'm missing > > something?. The r- is pending an answer to this. > Nightly updates are equal so it can't prevent updates to the same version The buildID is increasing all the time, can't distinguish nightlies by that ?

I've considered it and I may at some point utilize it... the problem I have with using it is that the format has changed previously and the format change could break update.

Comment on attachment 373752 [details] [diff] [review] patch for 1.9.1 - requires patch from Bug 487346 (checked in) a191=beltzner

Pushed to mozilla-1.9.1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2176123cd7c0

Tests pushed to mozilla-1.9.1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/932cce645a2d

This might not be as necessary for the 1.9.0 branch as bug 313057, but when a user is stuck in that state there might be a newer version with security fixes they aren't getting because they're busy downloading some old thing. Then again, maybe the user account that was able to upgrade while this one is still downloading an old version will beat them to the next version as well.

Comment on attachment 380551 [details] [diff] [review] patch for the 1.9.0 branch with updated tests from Bug 487346 Looks like the new tests cause leaks with the existing tests (the new tests don't leak) in a debug build on 1.9.0 though these same tests don't leak on 1.9.1 and trunk... I'm going to refactor the tests so the existing tests don't leak on 1.9.0 and resubmit.

We'll look at approving this patch for 1.9.0 when you're happy with it, but we don't need to hold/block the next release for this if we get bug 313057.

Comment on attachment 381255 [details] [diff] [review] updated tests for the 1.9.0 branch (updated pre-existing tests checked in) Checked in the pre-existing tests that have been updated Checking in mozilla/toolkit/mozapps/update/test/unit/head_update.js; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/head_update.js,v <-- head_up date.js new revision: 1.2; previous revision: 1.1 done Checking in mozilla/toolkit/mozapps/update/test/unit/tail_update.js; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/tail_update.js,v <-- tail_up date.js new revision: 1.2; previous revision: 1.1 done Checking in mozilla/toolkit/mozapps/update/test/unit/test_0010_general.js; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0010_general.js,v <-- t est_0010_general.js new revision: 1.2; previous revision: 1.1 done Checking in mozilla/toolkit/mozapps/update/test/unit/test_0020_general.js; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0020_general.js,v <-- t est_0020_general.js new revision: 1.2; previous revision: 1.1 done Checking in mozilla/toolkit/mozapps/update/test/unit/test_0030_general.js; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0030_general.js,v <-- t est_0030_general.js new revision: 1.2; previous revision: 1.1 done Checking in mozilla/toolkit/mozapps/update/test/unit/test_0040_general.js.in; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0040_general.js.in,v <-- test_0040_general.js.in new revision: 1.3; previous revision: 1.2 done

Backported tests checked in to 1.9.0 RCS file: /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0110_general.js ,v done Checking in mozilla/toolkit/mozapps/update/test/unit/test_0110_general.js; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0110_general.js,v <-- t est_0110_general.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0111_general.js ,v done Checking in mozilla/toolkit/mozapps/update/test/unit/test_0111_general.js; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0111_general.js,v <-- t est_0111_general.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/mozapps/update/test/unit/data/aus-0110_genera l-1.mar,v done Checking in mozilla/toolkit/mozapps/update/test/unit/data/aus-0110_general-1.mar ; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/data/aus-0110_general-1.mar,v <-- aus-0110_general-1.mar initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/mozapps/update/test/unit/data/aus-0110_genera l-2.mar,v done Checking in mozilla/toolkit/mozapps/update/test/unit/data/aus-0110_general-2.mar ; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/data/aus-0110_general-2.mar,v <-- aus-0110_general-2.mar initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/mozapps/update/test/unit/data/aus-0110_genera l_ref_image1.png,v done Checking in mozilla/toolkit/mozapps/update/test/unit/data/aus-0110_general_ref_i mage1.png; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/data/aus-0110_general_ref_imag e1.png,v <-- aus-0110_general_ref_image1.png initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/mozapps/update/test/unit/data/aus-0110_genera l_ref_image2.png,v done Checking in mozilla/toolkit/mozapps/update/test/unit/data/aus-0110_general_ref_i mage2.png; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/data/aus-0110_general_ref_imag e2.png,v <-- aus-0110_general_ref_image2.png initial revision: 1.1 done

Comment on attachment 381256 [details] [diff] [review] patch for the 1.9.0 branch (checked in) The updated / new tests have all passed on the 1.9.0 branch so this is good to go.

Comment on attachment 381256 [details] [diff] [review] patch for the 1.9.0 branch (checked in) Approved for 1.9.0.12, a=dveditz for release-drivers

Comment on attachment 381256 [details] [diff] [review] patch for the 1.9.0 branch (checked in) Checking in mozilla/toolkit/mozapps/update/content/history.js; /cvsroot/mozilla/toolkit/mozapps/update/content/history.js,v <-- history.js new revision: 1.8; previous revision: 1.7 done Checking in mozilla/toolkit/mozapps/update/content/updates.xml; /cvsroot/mozilla/toolkit/mozapps/update/content/updates.xml,v <-- updates.xml new revision: 1.35; previous revision: 1.34 done Checking in mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in; /cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v <-- nsUpda teService.js.in new revision: 1.153; previous revision: 1.152 done

Comment on attachment 381358 [details] [diff] [review] Tests for when this bug is fixed (checked in) Checking in mozilla/toolkit/mozapps/update/test/unit/test_0060_manager.js; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0060_manager.js,v <-- t est_0060_manager.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0061_manager.js ,v done Checking in mozilla/toolkit/mozapps/update/test/unit/test_0061_manager.js; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0061_manager.js,v <-- t est_0061_manager.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0062_manager.js ,v done Checking in mozilla/toolkit/mozapps/update/test/unit/test_0062_manager.js; /cvsroot/mozilla/toolkit/mozapps/update/test/unit/test_0062_manager.js,v <-- t est_0062_manager.js initial revision: 1.1 done

Comment on attachment 383321 [details] [diff] [review] followup for 1.9.0 branch (checked in) This is the branch applicable version of the patch in bug 496917

Comment on attachment 383321 [details] [diff] [review] followup for 1.9.0 branch (checked in) Followup for 1.9.0 to fix Bug 496917 on branch

Comment on attachment 383321 [details] [diff] [review] followup for 1.9.0 branch (checked in) Approved for 1.9.0.12, a=dveditz for release-drivers

(Make sure you add the keyword to bug 496917.)

Comment on attachment 383321 [details] [diff] [review] followup for 1.9.0 branch (checked in) Checking in mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in; /cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v <-- nsUpda teService.js.in new revision: 1.154; previous revision: 1.153 done

We don't want this for 1.8, right?

I don't think it is worth backporting it to 1.8 so no.

Robert, does the automated test cover all areas of this bug? How can we manually verify the fix?

The xpcshell test does cover this bug. If you'd like to verify this manually you can start a download and after it has started exit. Verify that the update.status file contains the string downloading... if it doesn't it downloaded too quickly and you will have to try again. Open the active-update.xml file and change the value for extensionVersion to a version that is less than the version you are currently running and save the file. Start Firefox and verify that the update.mar and update.status file have been deleted.

I'm running 3.5.3, and Firefox is downloading 3.5.3 for me. Reopen or a new bug?

New but how do you know it is downloading 3.5.3 (we just released 3.5.4)?

The Help menu said "Downloading 3.5.3" as did the dialog that popped up. I'm not in front of the computer any more, I'll open a new bug with more information when I am.