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

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

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.

Comment 1

[Dave Townsend [:mossop]]

Attachment: Test extension

Test extension

Comment 2

[Dave Townsend [:mossop]]

Attachment: Update rdf

The is a copy of the update rdf that the test extension updates from.

Comment 3

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

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.

Comment 4

[Dave Townsend [:mossop]]

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.

Comment 5

[Dave Townsend [:mossop]]

Attachment: 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).

Comment 6

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

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!

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated to trunk.

Comment 8

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

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!

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch for checkin

Added those comments, carrying over review, ready for checkin.

Comment 10

[Dave Townsend [:mossop]]

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

Comment 11

[Alex Vincent [:WeirdAl]]

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.

Comment 12

[Dave Townsend [:mossop]]

This is tested by the testcase for bug 394300

Comment 13

[Tony Chung [:tchung]]

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