358108 - if updates.xml contains both minor and major updates, and major comes first, major update will *not* trump minor

Status: RESOLVED WONTFIX

(Toolkit :: Application Update, defect)

Description

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

scenario where if updates.xml contains both minor and major updates, major update will *not* trump minor.

I found this while creating a sample updates.xml for http://litmus.mozilla.org/show_test.cgi?id=2676 

to see the problem, you'd need a snippet like this:

<updates>
  <update type="major" version="3.5" ...></update>
  <update type="minor" version="2.5" ...></update>
</updates>

the cause of this code is http://lxr.mozilla.org/mozilla1.8.0/source/toolkit/mozapps/update/src/nsUpdateService.js.in#1288

where initialize newestMinor and newestMajor to the first update.

var newestMinor = updates[0], newestMajor = updates[0];

the problem is when we do this, we initial newestMinor to have a version of 3.5,  so we'll fail to set minorUpdate, as we are looking for the most

note, the bad code does not cause any problems if our updates.xml contains all minor updates or all major updates.  Those can be in any order.

But if there are mixed minor and majors updates in a single updates.xml file, we 1508 and 2.0 would require then to be sorted, into order for minor to trump major.
 
I don't think this stops us for shipping 1508, but we should fix it for 1509,2001, and trunk for when we change AUS to emit both minor and major updates in a single updates.xml file, we should not need to require them to be sorted.

my apologies for never catching this bug in the code before now.

Comment 1

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

Attachment: a snippet that exposes the bug

Comment 2

[Daniel Veditz [:dveditz]]

The summary of the bug makes it sounds like you *want* the file to be order dependent, but the text makes it sound like that's a problem. Should the summary be re-written to "updates.xml major update should not trump minor regardless of order" ?

In any case, no patch and the server isn't likely to support two updates in the 1.5.0.9 time-frame anyway. Not blocking for now but we'd look at a patch.

Comment 3

[Anbo Motohiko]

Attachment: attempt to fix

nsIVersionComparator::compare seems to be able to handle null parameter, but I don't take that way.

Comment 4

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

Comment on attachment 248700 [details] [diff] [review]
attempt to fix

>--- nsUpdateService.js.in.bak	2006-11-08 07:54:28.000000000 +0900
>+++ nsUpdateService.js.in	2006-12-15 09:54:54.000000000 +0900
>@@ -1332,16 +1332,18 @@ UpdateService.prototype = {
>     
>     // Choose the newest of the available minor and major updates. 
>     var majorUpdate = null, minorUpdate = null;
>-    var newestMinor = updates[0], newestMajor = updates[0];
>+    var newestMinor = null, newestMajor = null;
> 
>     var vc = Components.classes["@mozilla.org/xpcom/version-comparator;1"]
>                        .getService(Components.interfaces.nsIVersionComparator);
>     for (var i = 0; i < updates.length; ++i) {
>       if (updates[i].type == "major" && 
>-          vc.compare(newestMajor.version, updates[i].version) <= 0)
>+          (!newestMajor ||
>+           vc.compare(newestMajor.version, updates[i].version) <= 0))
>         majorUpdate = newestMajor = updates[i];
>       if (updates[i].type == "minor" && 
>-          vc.compare(newestMinor.version, updates[i].version) <= 0)
>+          (!newestMinor ||
>+           vc.compare(newestMinor.version, updates[i].version) <= 0))
>         minorUpdate = newestMinor = updates[i];
>     }
It seems as if this could be rewritten so that newestMinor and newestMajor aren't needed. Somthing along the lines of

>+          (!minorUpdate ||
>+           vc.compare(minorUpdate.version, updates[i].version) <= 0))
>         minorUpdate = updates[i];

for both majorUpdate and minorUpdate.

If that is the case please change it and rerequest review... if not let me know and I'll finish reviewing this patch. Thanks!

Comment 5

[Anbo Motohiko]

Attachment: updated per review comment

This patch treats non-"major" update as "minor".
In both nsUpdateService.js.in and toolkit/mozapps/update/content/updates.js, we always check update type (severity) is major or not.

Comment 6

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

Comment on attachment 251273 [details] [diff] [review]
updated per review comment

I'm going take this review from robert.

Comment 7

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

sorry for the bug spam, re-assigning bugs back to default owner if I'm not working actively on them.

Comment 8

[AndrewM]

Is this still a problem on Trunk? Should it be fixed for Firefox 3?

Comment 9

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

> Is this still a problem on Trunk? 

Yes.

> Should it be fixed for Firefox 3?

No, we can ship without this, so removing blocking request.

Comment 10

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

Comment on attachment 251273 [details] [diff] [review]
updated per review comment

clearing review request, until we decide we actually want / need this.

Comment 12

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

I'm more tempted to get rid of the concept of major and minor from the update code since we don't use that in practice anymore.