Closed
Bug 449027
Opened 16 years ago
Closed 16 years ago
Support specifying application range for plugins in blocklist.xml
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b1
People
(Reporter: mossop, Assigned: mossop)
References
Details
(Keywords: verified1.9.0.6)
Attachments
(1 file, 1 obsolete file)
50.85 KB,
patch
|
mossop
:
review+
dveditz
:
approval1.9.0.6+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
See bug 446195 for the plugin version range handling.
Assignee | ||
Comment 2•16 years ago
|
||
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?
Assignee | ||
Comment 3•16 years ago
|
||
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)
Comment 5•16 years ago
|
||
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+
Assignee | ||
Comment 6•16 years ago
|
||
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+
Assignee | ||
Comment 7•16 years ago
|
||
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: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Assignee | ||
Comment 8•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/302e0725b17b
Comment 9•16 years ago
|
||
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.
Comment 10•16 years ago
|
||
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+
Assignee | ||
Comment 11•16 years ago
|
||
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?
Comment 13•16 years ago
|
||
Hey guys - the metrics guys were totally unaware of this change. Are we sure that AMO was ready to receive these requests, as well?
Comment 14•16 years ago
|
||
https://addons.mozilla.org/blocklist/3/%7Bec8030f7-c20a-464f-9b0e-13a3a9e97384%7D/3.1b2/Firefox/2008042309/Darwin_x86-gcc3/en-US/default/Darwin%209.2.2/default/default/blocklist.xml Service works but I think this is hurting metrics because the size of the URL has changed.
Comment 15•16 years ago
|
||
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).
Comment 16•16 years ago
|
||
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.
Updated•16 years ago
|
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Updated•16 years ago
|
Attachment #335855 -
Flags: approval1.9.0.6? → approval1.9.0.6+
Comment 17•16 years ago
|
||
Comment on attachment 335855 [details] [diff] [review] patch rev 2 Approved for 1.9.0.6, a=dveditz for release-drivers.
Assignee | ||
Comment 18•15 years ago
|
||
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
Comment 19•15 years ago
|
||
Verified for 1.9.0.6 through unit test.
Keywords: fixed1.9.0.6 → verified1.9.0.6
You need to log in
before you can comment on or make changes to this bug.
Description
•