394300 - Check for updates is offering incompatible updates

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

The EM update checker is now finding and offering extension updates that are incompatible with the current app version

Comment 1

[Dave Townsend [:mossop]]

If you have checkCompatiblity=true then the updates do not actually install,
however the installation ends with a compability check and an empty modal
question box with OK and Cancel buttons. Then you are left in the Installation
pane with the updates in an "Installing..." state.

Comment 2

[Dave Townsend [:mossop]]

This was introduced by bug 299716. Basic problem is some mis-typed property names in _isValidUpdate. Unfortunately the logic for _parseV20Update has also been thrown out so just those corrections would leave bug 314915 regressed.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Main problems are caused by misnamed properties in _isValidUpdate and overwriting aUpdateData.version in _parseV20Update even when looking at a potentially invalid update.

This patch removes the pseudo-updateitem UpdateData object that gets passed around for the entire process which I think is making life difficult (not least because it's property names are subtly different to nsIUpdateItem).

Instead _parseV20UpdateInfo and _parseV20Update now return a proper nsIUpdateItem for the best update entry they found. This allows us to just create a new one for each update to check in _parseV20Update while retaining the best one found so far for return later.

One minor addition is a change to _isValidUpdate to ensure that an update specifies min and max versions. I don't think it's right an update can leave those out and claim compatibility with every version of an app.

Comment 4

[Dave Townsend [:mossop]]

Attachment: testcases

Testcases for this and bug 314915. Include two test extensions, one marked as compatible with app one with toolkit. The update specifies a bunch of updated versions and we check that the highest valid version is returned.

Comment 5

[Alex Vincent [:WeirdAl]]

Comment on attachment 278962 [details] [diff] [review]
patch rev 1

Drive-by comments:

>-    if (max && gVersionChecker.compare(appExtensionsVersion, max) > 0)
>+    if (!max || gVersionChecker.compare(appExtensionsVersion, max) > 0)
>       return false;

Do we really want to require all extensions should have a maxVersion?  IIRC, this is a bit of a change (we recommended it, sure, but now we'd require it).

>   // Parses Firefox 1.0RC1+ update.rdf format
>-  _parseV20UpdateInfo: function(aDataSource, aLocalItem, aUpdateData, aUpdateCheckType) {
>+  _parseV20UpdateInfo: function(aDataSource, aLocalItem, aUpdateCheckType) {

Well, while we're here, would you care to JavaDoc this?

>-  _parseV20Update: function(aDataSource, aUpdateResource, aLocalItem, aUpdateData, aUpdateCheckType) {
>+  _parseV20Update: function(aDataSource, aUpdateResource, aLocalItem, aBestVersion, aUpdateCheckType) {

Ditto.

Comment 6

[Alex Vincent [:WeirdAl]]

Comment on attachment 278963 [details] [diff] [review]
testcases

>Index: toolkit/mozapps/extensions/test/unit/data/test_bug394300.rdf
>+<?xml version="1.0" encoding="UTF8"?>

UTF-8.

>+
>+<RDF:RDF xmlns:RDF="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>+         xmlns:em="http://www.mozilla.org/2004/em-rdf#">
>+
>+  <RDF:Description about="urn:mozilla:extension:[e-mail]">[e-mail]">
>+    <em:updates>
>+      <RDF:Seq>
>+        <RDF:li>
>+          <RDF:Description>
>+            <em:version>20</em:version>
>+            <em:targetApplication>
>+              <RDF:Description>
>+                <em:id>[e-mail]</em:id>
>+                <em:minVersion>2</em:minVersion>
>+                <em:maxVersion>2</em:maxVersion>
>+                <em:updateLink>http://localhost:4444/broken.xpi</em:updateLink>
>+              </RDF:Description>
>+            </em:targetApplication>
>+          </RDF:Description>
>+        </RDF:li>

Can you add a comment for each li and/or target-app to describe the expected results?

>Index: toolkit/mozapps/extensions/test/unit/test_bug394300.js
>+var env = Components.classes["@mozilla.org/process/environment;1"]
>+                    .getService(Components.interfaces.nsIEnvironment);
>+env.set("XPCOM_DEBUG_BREAK", "stack");

Not without a XXX comment explaining why this is necessary!  I did this in bug 299716 because I was hitting assertions for thread safety.  It's a cop-out, an evil thing to do in the context of xpcshell tests, and should be justified in writing what the problems are.  The goal is to remove this hack when we don't have any assertion failures.

>+do_import_script("netwerk/test/httpserver/httpd.js");
>+var server;
>+
>+var updateListener = {
>+  onUpdateStarted: function onUpdateStarted()
>+  {
>+  },

The casual reader would like to know which interface this is for.  I'd recommend comments, and including a |// do nothing| comment, so that it's clear this is intended.

>+function run_test()
>+  do_check_neq(updates[0]);
>+  do_check_neq(updates[1]);

do_check_neq takes two arguments.

Do you have some kind of counter to indicate that onUpdateEnded gets called after all appropriate calls to onAddonUpdateEnded and onAddonUpdateStarted?

Comment 7

[Dave Townsend [:mossop]]

(In reply to comment #5)
> (From update of attachment 278962 [details] [diff] [review])
> Drive-by comments:
> 
> >-    if (max && gVersionChecker.compare(appExtensionsVersion, max) > 0)
> >+    if (!max || gVersionChecker.compare(appExtensionsVersion, max) > 0)
> >       return false;
> 
> Do we really want to require all extensions should have a maxVersion?  IIRC,
> this is a bit of a change (we recommended it, sure, but now we'd require it).

We refuse to install an extension without min/max version so we should refuse to download an update without that information too.

Comment 8

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

Comment on attachment 278962 [details] [diff] [review]
patch rev 1

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v
>retrieving revision 1.241
>diff -u8 -p -r1.241 nsExtensionManager.js.in
>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	29 Aug 2007 08:34:36 -0000	1.241
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	30 Aug 2007 14:58:44 -0000	
>@@ -6290,70 +6252,92 @@ RDFItemUpdater.prototype = {
>           aLocalItem.id + "\r\n" +
>           "If you are an Extension developer and were expecting there to be\r\n" +
>           "updates, this could mean any number of things, since the RDF system\r\n" +
>           "doesn't give up much in the way of information when the load fails.\r\n" +
>           "\r\nTry checking that: \r\n" +
>           " 1. Your RDF File is correct - e.g. check that there is a top level\r\n" +
>           "    RDF Resource with a URI urn:mozilla:extension:{GUID}, and that\r\n" +
>           "    the <em:updates> listed all have matching GUIDs.");
>-      return;
>+      return null;
>     }
> 
>     var cu = Components.classes["@mozilla.org/rdf/container-utils;1"]
>                        .getService(Components.interfaces.nsIRDFContainerUtils);
>     if (cu.IsContainer(aDataSource, updates)) {
>       var ctr = getContainer(aDataSource, updates);
> 
>-      // Initial target version is the currently installed version
>-      aUpdateData.version = aLocalItem.version;
>+      // Track the newest update found
>+      var updatedItem = null;
> 
>       var versions = ctr.GetElements();
>       while (versions.hasMoreElements()) {
>         // There are two different methodologies for collecting version
>-        // information depending on whether or not we've bene invoked in
>+        // information depending on whether or not we've been invoked in
>         // "version updates only" mode or "version+newest" mode.
>         var version = versions.getNext().QueryInterface(Components.interfaces.nsIRDFResource);
>-        this._parseV20Update(aDataSource, version, aLocalItem, aUpdateData, aUpdateCheckType);
>-        if (aUpdateCheckType && aUpdateData.found)
>-          break;
>+        var foundItem = this._parseV20Update(aDataSource, version, aLocalItem,
>+                                             updatedItem ? updatedItem.version : aLocalItem.version,
>+                                             aUpdateCheckType);
>+        if (foundItem) {
>+          // When not checking for new versions we can bail out on the first
>+          // result.
>+          if (aUpdateCheckType)
>+            return foundItem;
>+          updatedItem = foundItem;
>+        }
>       }
>+      return updatedItem;
>     }
>+    return null;
Declare var updatedItem = null; outside of the if block and just return updatedItem.

>   },
> 
>-  _parseV20Update: function(aDataSource, aUpdateResource, aLocalItem, aUpdateData, aUpdateCheckType) {
>+  _parseV20Update: function(aDataSource, aUpdateResource, aLocalItem, aBestVersion, aUpdateCheckType) {
Instead of aBestVersion how about aNewestVersionFound since that's what it is?

>     var version = this._getPropertyFromResource(aDataSource, aUpdateResource,
>                                                 "version", aLocalItem);
>+    /* If we are looking for new versions then test whether this discovered
>+     * version is larger than any previously found update. Otherwise check
>+     * if this update is for the same version as we have installed. */
nit: Instead of larger this should be newer or greater.

>+    var result = gVersionChecker.compare(version, aBestVersion);
>+    if (aUpdateCheckType == nsIExtensionManager.UPDATE_CHECK_NEWVERSION ? result <= 0 : result != 0)
>+      return null;
>+
>     var taArc = gRDF.GetResource(EM_NS("targetApplication"));
>     var targetApps = aDataSource.GetTargets(aUpdateResource, taArc, true);
>+    
>+    // Track the best update we have found so far
>+    var bestUpdate = null;
newestUpdateItem?

r=me with those nits fixed.

Comment 9

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

Comment on attachment 278962 [details] [diff] [review]
patch rev 1

I'll review this after you've responded to Alex's comments

Comment 10

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

Dave, in thinking about best vs. newest I can go either way on that since it is really the newest compatible, etc. found. So, either way.

Comment 11

[Alex Vincent [:WeirdAl]]

By the way, which assertions are we hitting? :)

Comment 12

[Dave Townsend [:mossop]]

Attachment: patch for checkin

Comments addressed, carrying over review.

Comment 13

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

Comment on attachment 278986 [details] [diff] [review]
patch for checkin

>+  /**
>+   * Parses the Firefox 1.0RC1+ update manifest format looking for new versions
>+   * of updated compatibility information about the given add-on. Returns an
>+   * nsIUpdateItem holding the update's information if a valid update is found
>+   * or null if not.
>+   * @param   aDataSource
>+   *          The update manifest's datasource
>+   * @param   aLocalItem
>+   *          The nsIUpdateItem representing the add-on being checked for updates.
>+   * @param   aUpdateCheckType
>+   *          The type of update check being performed. See the constants in
>+   *          nsIExtensionManager
>+   */
>+  _parseV20UpdateInfo: function(aDataSource, aLocalItem, aUpdateCheckType) {
You read my mind but I thought it best to fix this after this bug was fixed. Could you change this so it has an @returns

Comment 14

[Dave Townsend [:mossop]]

Missed your last comment so checked in patch followed by additional comment fix. Will have to wait till another day to look over the unit test.

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.242; previous revision: 1.241
done

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.243; previous revision: 1.242
done

Comment 15

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

Comment on attachment 278963 [details] [diff] [review]
testcases

Dave, when you get a chance could you update the testcases? Thanks

Comment 16

[Dave Townsend [:mossop]]

Attachment: testcases rev 2

Updated testcases. Also added the pref change ready for bug 378216 landing.

Comment 17

[Alex Vincent [:WeirdAl]]

Comment on attachment 279453 [details] [diff] [review]
testcases rev 2

>+  server.registerDirectory("/", do_get_file("toolkit/mozapps/extensions/test/unit/data"));

You might need a trailing slash on the file.  However, I assume you've tested this patch as-is and it works.

r=ajvincent (unofficially)

Comment 18

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

Comment on attachment 279453 [details] [diff] [review]
testcases rev 2

looks good r=me

Comment 19

[Dave Townsend [:mossop]]

RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug394300.js,v
done
Checking in unit/test_bug394300.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug394300.js,v  <--  test_bug394300.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug394300_1/install.rdf,v
done
Checking in unit/addons/test_bug394300_1/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug394300_1/install.rdf,v  <--  install.rdf
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug394300_2/install.rdf,v
done
Checking in unit/addons/test_bug394300_2/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug394300_2/install.rdf,v  <--  install.rdf
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug394300.rdf,v
done
Checking in unit/data/test_bug394300.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug394300.rdf,v  <--  test_bug394300.rdf
initial revision: 1.1
done

Comment 20

[Tony Chung [:tchung]]

hi dave, do you have a test extension or alternate testcase i can use to verify this manually?   Thanks.