Last Comment Bug 795397 - click-to-play blocklisting: respect the "plugins" permission
: click-to-play blocklisting: respect the "plugins" permission
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla18
Assigned To: David Keeler [:keeler] (use needinfo?)
: Benjamin Smedberg [:bsmedberg]
Depends on:
Blocks: click-to-play
  Show dependency treegraph
Reported: 2012-09-28 10:51 PDT by David Keeler [:keeler] (use needinfo?)
Modified: 2012-12-26 23:42 PST (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (7.24 KB, patch)
2012-09-28 12:58 PDT, David Keeler [:keeler] (use needinfo?)
jaas: review+
jaws: review+
Details | Diff | Splinter Review
patch v2 (7.19 KB, patch)
2012-10-01 10:34 PDT, David Keeler [:keeler] (use needinfo?)
dkeeler: review+
Details | Diff | Splinter Review
patch v2 with commit message (7.20 KB, patch)
2012-10-03 10:54 PDT, David Keeler [:keeler] (use needinfo?)
dkeeler: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description David Keeler [:keeler] (use needinfo?) 2012-09-28 10:51:44 PDT
Initially I thought it would be a good idea for a click-to-play-blocklisted plugin to always be click-to-play, regardless of the "plugins" permission value. (The reasoning being, if a user has in the past said "always allow plugins" on a site, and then we make a plugin click-to-play, they should be made aware of the security implications of running that plugin.) However, since non-blocklist click-to-play isn't a first-class feature yet anyway, this a) won't happen to many users and b) isn't the right behavior anyway for the rest of users.
Ideally, we would have a more sophisticated plugins-permission manager that would handle all of these edge cases and actually differentiate between types of plugins and whether or not a plugin was vulnerable when it was allowed to run, but we're a ways away from that. In the meantime, it would be best if the permissions simply worked as expected.
Comment 1 David Keeler [:keeler] (use needinfo?) 2012-09-28 12:58:30 PDT
Created attachment 666028 [details] [diff] [review]

Josh - if you could review the nsObjectLoadingContent changes, that'd be great.
Comment 2 David Keeler [:keeler] (use needinfo?) 2012-09-28 12:59:27 PDT
Comment on attachment 666028 [details] [diff] [review]

And Jared, if you could review the test changes, that would also be great.
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-09-28 13:47:54 PDT
Comment on attachment 666028 [details] [diff] [review]

Review of attachment 666028 [details] [diff] [review]:

r+ for the test changes.
Comment 4 John Schoenick [:johns] 2012-09-28 15:12:28 PDT
Comment on attachment 666028 [details] [diff] [review]

Review of attachment 666028 [details] [diff] [review]:

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2525,5 @@
> +  // set the fallback reason
> +  // (if it's click-to-play, it might be because of the blocklist)
> +  uint32_t state;
> +  nsresult rv = pluginHost->GetBlocklistStateForType(mContentType.get(), &state);
> +  NS_ENSURE_SUCCESS(rv, false);

nit, if this call fails for some reason, we might return false here without setting aReason (an out param), which would result in callers using uninitialized values for fallback reasons.
Comment 5 Josh Aas 2012-10-01 06:57:38 PDT
Comment on attachment 666028 [details] [diff] [review]

Review of attachment 666028 [details] [diff] [review]:

r+ with a fix for the nit :johns pointed out
Comment 6 David Keeler [:keeler] (use needinfo?) 2012-10-01 10:34:25 PDT
Created attachment 666608 [details] [diff] [review]
patch v2

Fixed it so the aReason gets set. Carrying over r+.
Try run: (only running on one platform to conserve try resources and because there are no platform-specific changes. If this blows up in my face I'll go back to running on everything :)
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-10-01 18:27:25 PDT

A real commit message would have been nice :-(
Comment 8 Ed Morley [:emorley] 2012-10-02 08:59:43 PDT
Comment 9 David Keeler [:keeler] (use needinfo?) 2012-10-02 09:18:33 PDT
(In reply to Ryan VanderMeulen from comment #7)
> A real commit message would have been nice :-(

Sorry about that :-/
Comment 10 David Keeler [:keeler] (use needinfo?) 2012-10-02 09:19:46 PDT
... and the target milestone got reset. I'm doing really well lately...
Comment 11 David Keeler [:keeler] (use needinfo?) 2012-10-03 10:54:55 PDT
Created attachment 667549 [details] [diff] [review]
patch v2 with commit message

[Approval Request Comment]
Bug caused by (feature/regressing bug #): blocklist/click-to-play plugins (bug 760625)
User impact if declined: the "always activate plugins" option in the popup notification won't work for blocklisted plugins
Testing completed (on m-c, etc.): tested on m-c
Risk to taking this patch (and alternatives if risky): low (potentially could regress click-to-play plugins, but we've seen no evidence of that yet)
String or UUID changes made by this patch: none
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-10-05 18:57:02 PDT
Comment 13 Paul Silaghi, QA [:pauly] 2012-12-19 06:29:01 PST
Is there a way to undo these settings?
Comment 14 David Keeler [:keeler] (use needinfo?) 2012-12-19 09:46:31 PST
(In reply to Paul Silaghi [QA] from comment #13)
> Is there a way to undo these settings?

At the moment, Tools -> Clear Recent History -> Site Preferences :-/
I'm working on more accessible ui in bug 775857 and bug 821892.
Comment 15 John Schoenick [:johns] 2012-12-19 12:56:38 PST
(In reply to Paul Silaghi [QA] from comment #13)
> Is there a way to undo these settings?

You can also find the site in question in about:permissions and select "forget this site"
Comment 16 Paul Silaghi, QA [:pauly] 2012-12-20 00:53:00 PST
The plugin permissions are remembered. Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0b4

But I personally think this shouldn't land until bug 775857 and bug 821892 are fixed. Our users won't know to undo their settings. Clicking "Never activate plugins" by mistake on youtube or facebook will have a big negative impact.
Comment 17 David Keeler [:keeler] (use needinfo?) 2012-12-21 14:02:11 PST
Actually, I looked into this a bit more. Bug 746374 made the change that caused "Page Info" and about:preferences to not work for this. That landed in 20, so until then, these two methods work just fine for undoing "always-" or "never activate for this site".
Comment 18 David Keeler [:keeler] (use needinfo?) 2012-12-21 14:24:31 PST
(In reply to David Keeler from comment #17)
> about:preferences

by which I mean about:permissions, of course
Comment 19 Paul Silaghi, QA [:pauly] 2012-12-26 23:42:14 PST
Please see

Note You need to log in before you can comment on or make changes to this bug.