Extension update can identify the wrong version as being the newest.

RESOLVED FIXED in mozilla1.9alpha7

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

1.8.0 Branch
mozilla1.9alpha7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

13 years ago
The update for extension seems to make assumptions about the ordering of the update rdf for an extension. It looks like it generally assumes that the last version listed in the file is the newest version.

As an example I will attach an empty extension and the update rdf that it points to. If you install the extension and then attempt to update it firefox will claim to have found a newer version, version 3. However looking at the update file it is clear that there is a version 4 available.

From testing with the real extension that this problem was seen with I know that was the exctension actually version 3 then it would correctly find version 4 in that case.

Also, moving the update identifier for version 4 at the top of the file to be after version 3 means that version 4 is always identified as the latest version.
(Assignee)

Comment 1

13 years ago
Created attachment 201736 [details]
Test extension

Test extension
(Assignee)

Comment 2

13 years ago
Created attachment 201737 [details]
Update rdf

The is a copy of the update rdf that the test extension updates from.
This should probably be done in the next re-write - I could have sworn the docs mentioned that the orderring was significant but can't find any that do so.
(Assignee)

Comment 4

13 years ago
Having to maintain an order in the update rdf is somewhat of a pain for people like me who (foolishly) have a semi-automated update system. It means basically implementing the version checker logic and so of course is susceptible to changes and bugs in it.
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 5

11 years ago
Created attachment 267607 [details] [diff] [review]
patch rev 1

This changes the parsing loop to check whether each potential update is a valid update when the update version is the same as the current version (in version update only case) or the update version is larger than the last largest seen version (which is initilised to the current version).
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #267607 - Flags: review?(robert.bugzilla)
Comment on attachment 267607 [details] [diff] [review]
patch rev 1

Dave, this looks good. Can you fix all of the tabs in the patch, update it for the recent changes on trunk from bug 335942, and re-request? Thanks!
Attachment #267607 - Flags: review?(robert.bugzilla) → review-
(Assignee)

Comment 7

11 years ago
Created attachment 270429 [details] [diff] [review]
patch rev 2

Updated to trunk.
Attachment #267607 - Attachment is obsolete: true
Attachment #270429 - Flags: review?(robert.bugzilla)
Comment on attachment 270429 [details] [diff] [review]
patch rev 2

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v
>retrieving revision 1.223
>diff -u8 -p -r1.223 nsExtensionManager.js.in
>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	25 Jun 2007 21:38:19 -0000	1.223
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	30 Jun 2007 16:25:25 -0000	
>@@ -6036,30 +6036,30 @@ ExtensionItemUpdater.prototype = {
>       return true;
>     }
>     else if (this._updateCheckType == nsIExtensionManager.UPDATE_SYNC_COMPATIBILITY)
>       this._emDS.updateTargetAppInfo(aLocalItem.id, aRemoteItem.minAppVersion,
>                                      aRemoteItem.maxAppVersion);
>     return false;
>   },
>   
>-  _isValidUpdate: function(aLocalItem, aRemoteItem) {
>+  _isValidUpdate: function(aLocalItem, aVersion, aMinAppVersion, aMaxAppVersion) {
nit: it isn't clear what aVersion, aMinAppVersion, and aMaxAppVersion are from now that they aren't associated with aRemoteItem. Either comment the function or use param names that describe what they are.


>@@ -6477,52 +6470,55 @@ RDFItemUpdater.prototype = {
>...
>   _parseV20Update: function(aDataSource, aUpdateResource, aLocalItem, aUpdateData, aUpdateCheckType) {
>     var version = this._getPropertyFromResource(aDataSource, aUpdateResource, 
>                                                 "version", aLocalItem);
>     var taArc = gRDF.GetResource(EM_NS("targetApplication"));
>     var targetApps = aDataSource.GetTargets(aUpdateResource, taArc, true);
>     while (targetApps.hasMoreElements()) {
>       var targetApp = targetApps.getNext().QueryInterface(Components.interfaces.nsIRDFResource);
>       var id = this._getPropertyFromResource(aDataSource, targetApp, "id", aLocalItem);
>       if (id != this._updater._appID)
>         continue;
>       
>-      var result = gVersionChecker.compare(version, aLocalItem.version);
>+      var result = gVersionChecker.compare(version, aUpdateData.version);
>       if (aUpdateCheckType == nsIExtensionManager.UPDATE_CHECK_NEWVERSION ? result > 0 : result == 0) {
nit: a comment describing this if statement would be a good thing.

With those fixed r=me. Thanks!
Attachment #270429 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 9

11 years ago
Created attachment 271122 [details] [diff] [review]
patch for checkin

Added those comments, carrying over review, ready for checkin.
Attachment #270429 - Attachment is obsolete: true
Attachment #271122 - Flags: review+
(Assignee)

Comment 10

11 years ago
Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.226; previous revision: 1.225
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta1
Mossop, can we talk?  I have to undo half the change rstrong noted in comment 8 part 1 for bug 299716, because I have to check ID as well.  It's better in this case to pass aRemoteItem, so I can check that too.

Updated

11 years ago
Flags: in-testsuite?
(Assignee)

Comment 12

11 years ago
This is tested by the testcase for bug 394300
Flags: in-testsuite? → in-testsuite+

Comment 13

11 years ago
Dave, do you have an updated testcase you can attach that is supported for the trunk? i want to confirm the fix.

The test extension attached in comment 1 complains about not working for: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008011104 Minefield/3.0b3pre
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.