485624 - Downloads in progress for previous releases should be canceled on startup instead of resumed

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

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.

Comment 1

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

Attachment: tests in progress

Comment 2

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

Attachment: patch in progress

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

Comment 3

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

Attachment: tests in progress

Comment 4

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

Attachment: Test cleanup and prep

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 5

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

Comment on attachment 369866 [details] [diff] [review]
Test cleanup and prep

gonna do a bit more

Comment 6

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

Attachment: test data cleanup

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.

Comment 7

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

Attachment: test cleanup

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

Comment 8

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

Attachment: new tests

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

Comment 9

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

Attachment: patch rev1

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 10

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

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

Comment 11

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

Attachment: test cleanup rev2

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

Comment 12

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

Attachment: additional test to verify downloads are resumed for the same app version

Comment 15

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

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 16

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

Attachment: patch w/ tests rev2 - requires patch from bug 487346

Comment 18

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

Attachment: patch w/ tests rev3 - requires patch from bug 487346

Comment 19

[Dave Townsend [:mossop]]

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

Comment 20

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

(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 21

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

Attachment: patch rev4 with comments addressed (checked in)

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 373142 [details] [diff] [review]
patch rev4 with comments addressed (checked in)

Right, had forgotten about nightlies, shame.

Comment 23

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

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

Comment 24

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

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.

Comment 25

[Nick Thomas [:nthomas] (UTC+12)]

(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 ?

Comment 26

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

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 27

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

Attachment: patch for 1.9.1 - requires patch from Bug 487346 (checked in)

Comment 28

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

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

a191=beltzner

Comment 29

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

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

Comment 30

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

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

Comment 32

[Daniel Veditz [:dveditz]]

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 33

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

Attachment: patch for the 1.9.0 branch with updated tests from Bug 487346

Comment 34

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

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.

Comment 35

[Daniel Veditz [:dveditz]]

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 36

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

Attachment: updated tests for the 1.9.0 branch (updated pre-existing tests checked in)

Comment 37

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

Attachment: patch for the 1.9.0 branch (checked in)

Comment 38

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

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

Comment 39

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

Attachment: new test data files for the 1.9.0 branch (checked in)

Comment 40

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

Attachment: backported tests for 1.9.0 branch (checked in)

Comment 41

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

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 42

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

Attachment: Tests for when this bug is fixed (checked in)

Comment 43

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

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 44

[Daniel Veditz [:dveditz]]

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 45

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

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 46

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

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 47

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

Attachment: followup for 1.9.0 branch (checked in)

Comment 48

[Dave Townsend [:mossop]]

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 49

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

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 50

[Daniel Veditz [:dveditz]]

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

Comment 51

[Samuel Sidler (old account; do not CC)]

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

Comment 52

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

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

Comment 53

[Martin Stránský [:stransky] (ni? me)]

We don't want this for 1.8, right?

Comment 54

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

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

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 56

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

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.

Comment 57

[James Andrewartha]

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

Comment 58

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

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

Comment 59

[James Andrewartha]

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.