Closed Bug 449027 Opened 12 years ago Closed 11 years ago

Support specifying application range for plugins in blocklist.xml

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.1b1

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Keywords: verified1.9.0.6)

Attachments

(1 file, 1 obsolete file)

Currently if a plugin entry exists in blocklist.xml then it always matches regardless of the application version. The blocklist service is configured to return plugin items or not based on the application requesting. This will mean the blocklist is not current immediately after a change in application version.

We should support a similar targetApplication part for pluginItem that we do for emItem. Given that we now have versions on plugins we can probably just support the full versionRange stuff in exactly the same manner.

Firefox 3 would ignore the versionRange tag which would mean plugins would be blocked regardless though, we might want to backport some kind of fix to make that deal better.
See bug 446195 for the plugin version range handling.
Depends on: 446195
It's probably necessary that we take this on branch, without it there is no way to properly support upgrading the application where plugins installed should be blocked in the new version of the app but not in the current version.
Flags: wanted1.9.0.x?
Attached patch patch rev 1 (obsolete) — Splinter Review
This patch allows including a versionRange elements in pluginItem exactly as in emItem. If there is no versionRange then the pluginItem is assumed to match all versions of all apps. If there are any there then at least one must match, same as for emItem.

The bulk of this patch moves the testing of versionRange into the BlocklistItemData object so it can be shared by both items. Previously without minVersion/maxVersion we would use 0/* as their values. This does not sit so well with plugins which can on some platforms provide arbitrary version strings which may be considered less than 0. Instead this keeps a null for undefined items and simply ignores them from version checks.

Included in the patch are a bunch of tests of the blocklist system covering both plugins and extensions, checking the blocklist service correctly loads and applies the downloaded blocklist. The tests are identical for extensions and plugins. One set tests when the application ID is used in the targetApplication and the other the toolkit ID. The tests also check that the blocklist notification dialog is launched to display the newly blocked items.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #334512 - Flags: review?(robert.bugzilla)
Depends on: 451215
Duplicate of this bug: 446195
No longer depends on: 446195
Comment on attachment 334512 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/src/nsBlocklistService.js b/toolkit/mozapps/extensions/src/nsBlocklistService.js
>--- a/toolkit/mozapps/extensions/src/nsBlocklistService.js
>+++ b/toolkit/mozapps/extensions/src/nsBlocklistService.js
>...
>   _checkPluginsList: function() {
>     if (!this._addonEntries)
>       this._loadBlocklist();
>+    var appVersion = gApp.version;
>+    var toolkitVersion = gApp.platformVersion;
>     var phs = Cc["@mozilla.org/plugin/host;1"].
>               getService(Ci.nsIPluginHost);
>-    phs.getPluginTags({ }).forEach(this._checkPlugin, this);
>+    var plugins = phs.getPluginTags({});
>+    for (var i = 0; i < plugins.length; i++) {
>+      plugins[i].blocklisted = this._isPluginBlocklisted(plugins[i], appVersion,
>+                                                         toolkitVersion);
>+    }
style nit: You don't use braces elsewhere when using a for loop that contains a single statement.

Seems like you could just pass gApp.version / gApp.platformVersion and get rid of the two vars.


>@@ -767,64 +765,132 @@ function BlocklistItemData(versionRangeE

> BlocklistItemData.prototype = {
>-/**
>- * Retrieves a version range (e.g. minVersion and maxVersion) for a
>- * blocklist item's targetApplication element.
>- * @param   targetAppElement
>- *          A targetApplication blocklist element.
>- * @returns An array of JS objects with the following properties:
>- *          "minVersion"  The minimum version in a version range (default = 0).
>- *          "maxVersion"  The maximum version in a version range (default = *).
>- */
>+  /**
>+   * Tests if an item is included in this blocklist range
What is a blocklist range?

>+  /**
>+   * Checks if a version falls in between a min and max version.
>+   * @param   version
>+   *          The version to test.
>+   * @param   minVersion
>+   *          The minimum version. If null it is assumed that version is always
>+   *          larger.
>+   * @param   maxVersion
>+   *          The maximum version. If null it is assumed that version is always
>+   *          smaller.
>+   */
>+  matchesRange: function(version, minVersion, maxVersion) {
>+    if (minVersion && gVersionChecker.compare(version, minVersion) < 0)
>+      return false;
>+    if (maxVersion && gVersionChecker.compare(version, maxVersion) > 0)
>+      return false;
>+    return true;
hmmm... "Checks if a version falls in between a min and max version."
Since either can be null the summary should reflect that.

>+  /**
>+   * Tests if there is a matching range for the given application and version.
>+   * @param   appID
>+   *          The application ID to test for.
>+   * @param   appVersion
>+   *          The version of the application to test for.
>+   * @returns True if this version range covers the application version given.
>+   */
>+  matchesAppRange: function(appID, appVersion) {
>+    var blTargetApp = this.targetApps[appID];
>+    if (!blTargetApp)
>+      return false;
>+
>+    for (var x = 0; x < blTargetApp.length; ++x) {
>+      if (this.matchesRange(appVersion, blTargetApp[x].minVersion, blTargetApp[x].maxVersion))
>+        return true;
>+    }
>+
>+    return false;
>+  },
This is also used for TOOLKIT_ID so the function name |matchesAppRange|, the summary stating "given application", etc. is not correct. I know we still have parts of the EM that don't call this out specifically - getUpdatedTargetAppInfo for example - but it would be good to not add more code that does this. At the very least this should be mentioned.

Would it be worth it to have it just check TOOLKIT_ID without the second call? For that matter you could then remove matchesAppRange and perform the check in includesItem.

r=me with those nits picked or reasons not to
Attachment #334512 - Flags: review?(robert.bugzilla) → review+
Attached patch patch rev 2Splinter Review
Addressed all the comments, with a few notes:

(In reply to comment #5)
> This is also used for TOOLKIT_ID so the function name |matchesAppRange|, the
> summary stating "given application", etc. is not correct. I know we still have
> parts of the EM that don't call this out specifically - getUpdatedTargetAppInfo
> for example - but it would be good to not add more code that does this. At the
> very least this should be mentioned.

Changed that to matchesTargetRange.

> Would it be worth it to have it just check TOOLKIT_ID without the second call?
> For that matter you could then remove matchesAppRange and perform the check in
> includesItem.

The logic works out somewhat simpler when you can just return the result from the separate function, and I want to keep the checking the same for both cases. I already discovered a bug with the previous implementation where it isn't correctly handling the toolkit id case when doing separate checks.
Attachment #334512 - Attachment is obsolete: true
Attachment #335855 - Flags: review+
I've checked this in without the pref change so it still uses the old blocklist form. I'll land the pref change once bug 451215 makes the new schema available
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Hey Dave, can you hook me up with some testcases to test ranges?   Or is there a easy way i can take existing plugins, and manipulate the blocklist.xml to test this?  

Unit tests would be great too, thanks.
Dave must have forgot to set in‑testsuite+. The tests for this were landed at the same time and they do pass.
Flags: in-testsuite+
Comment on attachment 335855 [details] [diff] [review]
patch rev 2

I would like us to take this on branch if possible. This fixes a potential problem for users switching from Firefox 3 to Firefox 3.1. Without this users may have a blocklist that does not contain correct data for Firefox 3.1 and so plugins we would expect to be blocked might not be and vice versa.

The patch is well tested and has been baking on trunk for a while now.

The actual risk of a problem is low right now however since all of our plugin blocks are for Firefox 3.0 - 3.*, but if that changes then we would hit the problem.
Attachment #335855 - Flags: approval1.9.0.6?
well stocked tests.   setting to verified.
Status: RESOLVED → VERIFIED
Hey guys - the metrics guys were totally unaware of this change. Are we sure that AMO was ready to receive these requests, as well?
I'm not seeing any structural change to the bloclist version 2 and version 3 URL templates other than the version number changing so I don't think that it will cause us any new development work, but we *have* to be informed of any changes to the structure of any URL requests to services that are part of metrics (currently AUS, Blocklist, AMO VersionCheck, AMO /downloads/file/ and bouncer downloads.

One of the few hardcoded items in my request processing is the version of the request because normally if you guys change the URL, it is likely that it might have impact to our processing that requires new work (e.g. you add a new field to the template).
I opened Bug 469806 in the hopes that someone might be able to implement a test that would keep this kind of change from surprising metrics after the fact again.
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Attachment #335855 - Flags: approval1.9.0.6? → approval1.9.0.6+
Comment on attachment 335855 [details] [diff] [review]
patch rev 2

Approved for 1.9.0.6, a=dveditz for release-drivers.
Checked into CVS

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.342; previous revision: 1.341
done
Checking in toolkit/mozapps/extensions/src/nsBlocklistService.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsBlocklistService.js,v  <--  nsBlocklistService.js
new revision: 1.13; previous revision: 1.12
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug449027.js,v
done
Checking in toolkit/mozapps/extensions/test/unit/test_bug449027.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug449027.js,v  <--  test_bug449027.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug449027_app.xml,v
done
Checking in toolkit/mozapps/extensions/test/unit/data/test_bug449027_app.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug449027_app.xml,v  <--  test_bug449027_app.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug449027_toolkit.xml,v
done
Checking in toolkit/mozapps/extensions/test/unit/data/test_bug449027_toolkit.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug449027_toolkit.xml,v  <--  test_bug449027_toolkit.xml
initial revision: 1.1
done
Keywords: fixed1.9.0.6
Verified for 1.9.0.6 through unit test.
Blocks: 597982
You need to log in before you can comment on or make changes to this bug.