Closed Bug 880675 Opened 11 years ago Closed 11 years ago

Stop caching blocklisting state in a writable nsIPluginTag.blocklisted

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla24

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(1 file)

This turned out to be a a prerequisite for bug 875454 in which I accidentally broke hard-blocklisting. Currently the blocklist service caches the blocklist state of plugins by setting nsIPluginTag.blocklisted. This is weird firstly because new plugins may not be immediately blocklisted, but also because it adds a level of indirection that doesn't need to be there.

This may at one point have been intended as a performance optimization, so that loading plugins wouldn't necessarily load the blocklist service. But now that we have CtP blocks, we always load the blocklist service anyway and query it during plugin initialization.

This patch required some refactoring of tests which previously were setting .blocklisted to check plugin behaviors. These tests now load fake blocklist data (suppressing the blocklist UI to avoid hangs).
Attachment #759735 - Flags: review?(jschoenick)
Attachment #759735 - Flags: review?(bmcbride)
Attachment #759735 - Flags: review?(jschoenick) → review+
Comment on attachment 759735 [details] [diff] [review]
Remove the writable nsIPluginTag.blocklisted property, which is a poorly-constructed cache of the blocklist data, r?jschoenick r?unfocused

Review of attachment 759735 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +30,5 @@
>  const PREF_BLOCKLIST_INTERVAL         = "extensions.blocklist.interval";
>  const PREF_BLOCKLIST_LEVEL            = "extensions.blocklist.level";
>  const PREF_BLOCKLIST_PINGCOUNTTOTAL   = "extensions.blocklist.pingCountTotal";
>  const PREF_BLOCKLIST_PINGCOUNTVERSION = "extensions.blocklist.pingCountVersion";
> +const PREF_BLOCKLIST_SUPPRESSUI       = "extensions.blocklist.suppressUI";

If possible, I really want to avoid adding prefs like this to the blocklist - especially if they're just intended for tests. Can you not just listen for the dialog to appear and silently close it (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/social/browser_blocklist.js#126) or temporarily override nsIWindowWatcher (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/social/browser_blocklist.js#126) ?
Attachment #759735 - Flags: review?(bmcbride) → review-
I can, if you insist, and I will do it if that gets this done more quickly. But I in fact strongly object to that in general. You're basically coupling these tests to the internal implementation of the blocklist, which will probably change its behavior in the future (there really shouldn't be a prompt for blocking plugins in general). In the xpcshell tests I could possibly do this by mocking out the blocklist service entirely. But in the b-c tests, I'd much prefer a "testing mode" flag to the blocklist service to suppress the UI, rather than hacking the window watcher to auto-close the window.
Flags: needinfo?(bmcbride)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> But in the b-c tests, I'd much prefer a "testing mode" flag to the blocklist
> service to suppress the UI, rather than hacking the window watcher to auto-close the
> window.

Yeah, that seems better to me too.
Eh, yea, ok - I don't feel that strongly either way.
Flags: needinfo?(bmcbride)
Attachment #759735 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/ea3191239e8e
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 882339
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: