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

RESOLVED WONTFIX

Status

()

Toolkit
Application Update
RESOLVED WONTFIX
11 years ago
5 months ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Unassigned)

Tracking

Trunk
Points:
---
Bug Flags:
blocking1.8.1.1 -
wanted1.8.1.x +
blocking1.8.0.9 -
wanted1.8.0.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [vista], URL)

Attachments

(2 attachments, 1 obsolete attachment)

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.
Summary: scenario where if updates.xml contains both minor and major updates, major update will *not* trump minor → if updates.xml contains both minor and major updates, and major comes first, major update will *not* trump minor
Created attachment 243551 [details]
a snippet that exposes the bug
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
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.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1-
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9-
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+

Comment 3

11 years ago
Created attachment 248700 [details] [diff] [review]
attempt to fix

nsIVersionComparator::compare seems to be able to handle null parameter, but I don't take that way.
Attachment #248700 - Flags: review?(robert.bugzilla)

Updated

11 years ago
Whiteboard: [vista]
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

11 years ago
Created attachment 251273 [details] [diff] [review]
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.
Attachment #248700 - Attachment is obsolete: true
Attachment #251273 - Flags: review?(robert.bugzilla)
Attachment #248700 - Flags: review?(robert.bugzilla)
Comment on attachment 251273 [details] [diff] [review]
updated per review comment

I'm going take this review from robert.
Attachment #251273 - Flags: review?(robert.bugzilla) → review?(sspitzer)

Updated

11 years ago
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
sorry for the bug spam, re-assigning bugs back to default owner if I'm not working actively on them.
Assignee: sspitzer → nobody

Comment 8

10 years ago
Is this still a problem on Trunk? Should it be fixed for Firefox 3?
Flags: blocking-firefox3?
Attachment #251273 - Flags: review?(sspitzer) → review?
> 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.
Flags: blocking-firefox3?
Comment on attachment 251273 [details] [diff] [review]
updated per review comment

clearing review request, until we decide we actually want / need this.
Attachment #251273 - Flags: review?
(Assignee)

Updated

10 years ago
Product: Firefox → Toolkit
Duplicate of this bug: 552888
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.
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.