Last Comment Bug 711618 - implement basic click to play permission model
: implement basic click to play permission model
Status: RESOLVED FIXED
[secr:curtisk]
: privacy
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- enhancement with 4 votes (vote)
: mozilla14
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
Mentors:
: 737508 (view as bug list)
Depends on: 761840 549697 711552 743429 748701
Blocks: cuts-addons 744620 click-to-play
  Show dependency treegraph
 
Reported: 2011-12-16 14:29 PST by David Keeler [:keeler] (use needinfo?)
Modified: 2012-12-09 07:25 PST (History)
69 users (show)
jaws: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch v1 (8.41 KB, patch)
2011-12-19 11:56 PST, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Splinter Review
patch v2 (13.92 KB, patch)
2012-01-02 17:41 PST, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Splinter Review
Patch for bug (13.43 KB, patch)
2012-04-04 23:16 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug v2 (18.64 KB, patch)
2012-04-05 17:25 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Tests for patch (6.44 KB, patch)
2012-04-05 17:26 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Tests for patch v2 (7.41 KB, patch)
2012-04-05 18:08 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
Details | Diff | Splinter Review
Patch for bug (part 2, platform changes) (5.58 KB, patch)
2012-04-10 15:53 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug (part 2, platform changes) (2.44 KB, patch)
2012-04-10 21:13 PDT, Jared Wein [:jaws] (please needinfo? me)
jaas: review+
Details | Diff | Splinter Review
Patch for bug v3 (21.02 KB, patch)
2012-04-11 14:28 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug v4 (22.44 KB, patch)
2012-04-13 16:34 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
blassey.bugs: approval‑mozilla‑central+
Details | Diff | Splinter Review

Description David Keeler [:keeler] (use needinfo?) 2011-12-16 14:29:01 PST
+++ This bug was initially created as a clone of Bug #711552 +++

limi: "If you've activated a click-to-play plugin on a certain URL 3 times, make it activate on subsequents loads *of that page* (to handle the intranet/bank Java app use case)", "…and the way to clear the "click to play -> automatic after 3 times" is to change the state of that plugin to either always on or always off and then back to click-to-play. This avoids having a management interface for the list of URLs that will always load. :P"

gkn: "When a page is removed from the history (and isn't bookmarked) it should also be removed from this list."

Question: what's a good way to store this list of URLs/plugins?
Comment 1 Jesse Ruderman 2011-12-16 15:27:08 PST
This should be per-origin, not per-URL.
Comment 2 Tom Lowenthal [:StrangeCharm] 2011-12-16 15:30:08 PST
I'm not convinced. I go to youtube a lot, and I'll probably choose to play at least three videos on youtube, but I don't want youtube videos to load automatically.
Comment 3 Ian Melven :imelven 2011-12-16 17:02:40 PST
This reminds me of bug 682455 and it does have security consequences since we know that plugins are often a frequent (successful) target for exploitation. 

Also, don't we already have about:permissions for a management interface for this ?
Comment 4 Jesse Ruderman 2011-12-16 17:07:26 PST
If a site repeatedly provides useful content through plugins, it's not especially likely to attack you the 4th time. And the user is likely to click through, like they did the first 3 times.

Would you be happier if the permission went away after 30 days of not being used?
Comment 5 Ian Melven :imelven 2011-12-16 17:39:44 PST
(In reply to Jesse Ruderman from comment #4)
> If a site repeatedly provides useful content through plugins, it's not
> especially likely to attack you the 4th time. And the user is likely to
> click through, like they did the first 3 times.

ok, these are good points. 

(In reply to David Keeler from comment #0)

"…and the way to clear the "click to play -> automatic after 3 times" is to change the state of that plugin to either always on or always off and then back to click-to-play.

(In reply to David Keeler from comment #0)
> +++ This bug was initially created as a clone of Bug #711552 +++
> 
> limi: "If you've activated a click-to-play plugin on a certain URL 3 times,
> make it activate on subsequents loads *of that page* (to handle the
> intranet/bank Java app use case)", "…and the way to clear the "click to play
> -> automatic after 3 times" is to change the state of that plugin to either
> always on or always off and then back to click-to-play. This avoids having a
> management interface for the list of URLs that will always load. :P"

does this mean disable and re-enable the plugin in the Add-On Manager ? 

> gkn: "When a page is removed from the history (and isn't bookmarked) it
> should also be removed from this list."
 
what's the link between clearing history and resetting plugin click to play permissions ?
Comment 6 David Keeler [:keeler] (use needinfo?) 2011-12-17 11:35:08 PST
(In reply to Ian Melven :imelven from comment #5)

> > limi: "If you've activated a click-to-play plugin on a certain URL 3 times,
> > make it activate on subsequents loads *of that page* (to handle the
> > intranet/bank Java app use case)", "…and the way to clear the "click to play
> > -> automatic after 3 times" is to change the state of that plugin to either
> > always on or always off and then back to click-to-play. This avoids having a
> > management interface for the list of URLs that will always load. :P"
> 
> does this mean disable and re-enable the plugin in the Add-On Manager ?

That's my understanding. It would be something like "click to play" -> "disable" -> "click to play" (or "click to play" -> "enable" -> "click to play"). This would reset all of the sites for that plugin.

> > gkn: "When a page is removed from the history (and isn't bookmarked) it
> > should also be removed from this list."
>  
> what's the link between clearing history and resetting plugin click to play
> permissions ?

My understanding is if a page is removed from history, a plugin that had been click-to-played on that page >= 3 times will go back to click-to-play. However, if this feature is on a per-site basis rather than a per-URL basis, this won't work unless all pages from that site are removed from history.
Comment 7 Jim Jeffery not reading bug-mail 1/2/11 2011-12-17 13:36:17 PST
(In reply to David Keeler from comment #6)

> > 
> > does this mean disable and re-enable the plugin in the Add-On Manager ?
> 
> That's my understanding. It would be something like "click to play" ->
> "disable" -> "click to play" (or "click to play" -> "enable" -> "click to
> play"). This would reset all of the sites for that plugin.
> 

Wait ? What?  You mean to use the feature one will have to open the Addon manager ?  I had assumed there would be a marker or something on the site visited similar to the way Flashblock works for example that can be 'clicked' to play a flash object.  If one has to open the Addon manager every-time one wants to view a flash video for each site this feature will be virtually useless as it will be become quickly very annoying to jump to Addon manager.  I understand that after 3 times it will be automatic, but still for each site to have to leap around to 'click to play' ?  And as stated above what about those users that never want flash objects to play unless clicked ?  Some flash ads are so poorly written they 'eat cpu' thus the reason for full-time blocking.
Comment 8 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-12-18 00:39:14 PST
I think users will understand "always load this plugin for this site" much better than they will understand "it works automatically after three times."

Especially in China it is common to have site-specific plugins. For example, banks and many e-commerce sites (eBay and amazon.com analogs) have plugins specific to their site. These are sites that are used by basically everybody in China. In particular, it is common to use plugins to implement a "secure login" form that attempts to prevent keyloggers from stealing the user's password. AFAICT, the login form is often on multiple pages of the site, and users expect to be able to log in easily on any page of the site. We are working now to make these plugins work better (e.g. bug 611253) and be less confusing to end-users, to help with adoption of Firefox in China; my concern is that the proposed policy will make things more confusing than necessary, and make new Firefox users think Firefox is less convenient than other browsers.

I think that having a per-origin auto-load setting and making it an explicit choice ("enable now" and "always enable for this site") would be a lot less confusing. (And, the same applies to geo-location prompting.)

We should not be afraid of site-specific settings. With WebAPIs, we are going to be asking users to enable MANY features on a per-site/origin basis. We have to make a usable UI for site-specific settings anyway; in fact, I thought we already had a UI design for site-specific settings ready to go. We might as well start doing it with this (and geolocation).
Comment 9 David Keeler [:keeler] (use needinfo?) 2011-12-19 11:56:24 PST
Created attachment 582909 [details] [diff] [review]
patch v1

I agree that having the option to "load this time" or "always load for this site" make more sense. Here's a patch for how that could work.
limi/UX team: input?
Comment 10 Tom Lowenthal [:StrangeCharm] 2011-12-21 03:43:17 PST
(In reply to Jesse Ruderman from comment #4)
> If a site repeatedly provides useful content through plugins, it's not
> especially likely to attack you the 4th time. And the user is likely to
> click through, like they did the first 3 times.

Blocking plugins can help resist attacks, but that's not the only reason. As comment 7 points out, performance is a very good reason not to load plugins media by default. Another reason is that it can just be annoying: I hate opening a tab in the background and having sound start to play.

Any non-obvious behavior like changing the default after three accepts, or only being able to remove the preference by removing the site from history creates a bad UX. We should default to well-understood concepts like Allow Once / Allow Always, and integrate this feature into the up-and-coming site-specific preferences manager which currently lives at about:permissions.
Comment 11 Jesse Ruderman 2011-12-24 15:25:38 PST
Yeah, I'd prefer an explicit "always" action over this automatic behavior. And we need plugin activation to be based on context menus rather than single-left-click anyway, for basic safety, so there's plenty of space to put an "always" option.

(It's fine with me if left-click opens a context menu in this case, fwiw.)
Comment 12 David Keeler [:keeler] (use needinfo?) 2012-01-02 17:41:44 PST
Created attachment 585342 [details] [diff] [review]
patch v2

Here's the latest:
* this builds on bug 711552 (so, no hanger unless necessary, etc.)
* adds the option to always play a given plugin for the current site (still left/right click / menu-based)
* when loading a plugin, if the user has indicated that it should always play on that site, load it and only it (this means that new invisible plugins won't be played, but old ones that were present when the permission was set will be)
Comment 13 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-01-11 14:59:25 PST
need to schedule a secteam review
Comment 14 Jesse Ruderman 2012-01-11 19:53:45 PST
"Left-click menu" is the right UI when this is being used as a security measure. But for some plugins/users, it might be used only as an anti-annoyance measure, in which case it would be good to have a "Left-click play" implemented as well.
Comment 15 Justin Dolske [:Dolske] 2012-02-27 16:55:27 PST
(In reply to Jesse Ruderman from comment #14)
> "Left-click menu" is the right UI when this is being used as a security
> measure.

You mean right-click (context) menu, I assume?

But more generally, as a security measure it just needs to be some place in chrome where context can't get at (or easily trick a user into doing)
Comment 16 Jesse Ruderman 2012-02-27 18:26:25 PST
I mean that left-clicking should do the same thing as right-clicking.

Chrome UI would be even more secure, yes.
Comment 17 Jared Wein [:jaws] (please needinfo? me) 2012-03-23 10:34:31 PDT
*** Bug 737508 has been marked as a duplicate of this bug. ***
Comment 18 Jared Wein [:jaws] (please needinfo? me) 2012-04-04 23:16:08 PDT
Created attachment 612462 [details] [diff] [review]
Patch for bug

This implements a basic permissions model of click to play plugins on a per-site basis through the plugin popup-notification doorhanger. Settings can be modified through about:permissions.

This patch doesn't implement an expiring permission or a rolling window for the permissions as has been discussed in the mailing lists [1]. There is also no ability to remember settings through the overlay.

We could land this patch as-is as an intermediate step on our way to click-to-play plugins and refine our permissions model when we reach a decision that we are happy with.

Curtis, are you OK with landing this as-is as an intermediate step?

[1] https://groups.google.com/forum/#!topic/mozilla.dev.security/DzCnFBMIPNI
Comment 19 Dão Gottwald [:dao] 2012-04-05 02:57:41 PDT
Permissions are handled in Page Info > Permissions. about:permissions as a work in progress is still hidden from the user.
Comment 20 Ian Melven :imelven 2012-04-05 10:45:03 PDT
(In reply to Jared Wein [:jaws] from comment #18)
> Created attachment 612462 [details] [diff] [review]
> Patch for bug
> 
> This implements a basic permissions model of click to play plugins on a
> per-site basis through the plugin popup-notification doorhanger. Settings
> can be modified through about:permissions.
> 
> This patch doesn't implement an expiring permission or a rolling window for
> the permissions as has been discussed in the mailing lists [1]. There is
> also no ability to remember settings through the overlay.
> 
> We could land this patch as-is as an intermediate step on our way to
> click-to-play plugins and refine our permissions model when we reach a
> decision that we are happy with.
> 
> Curtis, are you OK with landing this as-is as an intermediate step?
> 
> [1] https://groups.google.com/forum/#!topic/mozilla.dev.security/DzCnFBMIPNI

FWIW, personally i don't see a problem with landing this if it's disabled by default. I think having a prototype to show and discuss will help get UX and Product feedback and help us get closer to a decision on what we actually want click to play to be. 

I do think the feature shouldn't be enabled in a release until it's closer to what's been discussed. 

Thanks for your continued awesome work on this, Jared :)
Comment 21 Ian Melven :imelven 2012-04-05 10:51:14 PDT
(In reply to Ian Melven :imelven from comment #20)
> I think having a prototype to show and discuss will help get UX and
> Product feedback and help us get closer to a decision on what we actually
> want click to play to be. 

and hopefully some user testing/feedback too, of course !
Comment 22 Jared Wein [:jaws] (please needinfo? me) 2012-04-05 17:25:28 PDT
Created attachment 612754 [details] [diff] [review]
Patch for bug v2

This patch makes plugin permissions accessible from the Page Info > Permissions dialog.
Comment 23 Jared Wein [:jaws] (please needinfo? me) 2012-04-05 17:26:12 PDT
Created attachment 612755 [details] [diff] [review]
Tests for patch

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=6dc9c22a6534
Comment 24 Jared Wein [:jaws] (please needinfo? me) 2012-04-05 18:08:07 PDT
Created attachment 612767 [details] [diff] [review]
Tests for patch v2

Fixed the two test failures in browser_permissions.js.
Comment 25 Willy_ Foo_Foo 2012-04-06 10:57:01 PDT
Thanks for this! too

i also have traded Flashblock for plugins.click_to_play;true

But by when it will be stable enough to test?
will it have the option to control(enable on demand) for all types of plugins?
& what about say a page has two flash player windows but want to play only the second one
will it be possible ? (flashblock allows this)
Comment 26 Jared Wein [:jaws] (please needinfo? me) 2012-04-08 23:02:19 PDT
Comment on attachment 612754 [details] [diff] [review]
Patch for bug v2

I'm going to update nsObjectLoadingContent so it automatically activates the plugin (and doesn't dispatch the PluginClickToPlay event) if the ownerDocument.location has permissions or if the ownerWindow.top.location has permissions.

This will help users who visits YouTube.com, gives permissions so plugins will auto-activate when browsing YouTube.com, and then visit another site who has a YouTube iframe.
Comment 27 Jared Wein [:jaws] (please needinfo? me) 2012-04-10 15:53:45 PDT
Created attachment 613804 [details] [diff] [review]
Patch for bug (part 2, platform changes)

Don't dispatch PluginClickToPlay if the permissions are set to Always Allow for the current site.
Comment 28 Jared Wein [:jaws] (please needinfo? me) 2012-04-10 15:55:19 PDT
Comment on attachment 612754 [details] [diff] [review]
Patch for bug v2

I talked with Felipe more about this and he mentioned that changing how we respect the permissions could create scenarios where some plugins auto-activate while others don't.

I've put the platform changes in part 2 of the patches for this bug.
Comment 29 :Felipe Gomes (needinfo me!) 2012-04-10 20:52:14 PDT
Comment on attachment 612754 [details] [diff] [review]
Patch for bug v2

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

- it's not clear yet to me that the _reshowNotification case is correct in all situations, I'll look at it again on the next patch iteration

- the pageinfo and aboutPermissions selectors should only display if the feature is enabled, otherwise setting a site-based value would be misleading as it would have no effect. But I like how the global entry on about:permissions control the pref.

- only using testPermission once per page would be nice although I now think it's too much early optimization to worry about that, specially if trying to do that also for the backend patch (the change would be simple in the front-end)

::: browser/base/content/browser.js
@@ +7246,5 @@
>    _handleClickToPlayEvent: function PH_handleClickToPlayEvent(aPlugin) {
>      let doc = aPlugin.ownerDocument;
>      let browser = gBrowser.getBrowserForDocument(doc.defaultView.top.document);
> +    let topURI = Services.io.newURI(browser.contentWindow.location.href, null, null);
> +    let pluginsPermission = Services.perms.testPermission(topURI, "plugins");

no need to create a new uri here, just use browser.currentURI

@@ +7250,5 @@
> +    let pluginsPermission = Services.perms.testPermission(topURI, "plugins");
> +    let overlay = doc.getAnonymousElementByAttribute(aPlugin, "class", "mainBox");
> +
> +    if (browser._clickToPlayPluginsActivated ||
> +        pluginsPermission == Ci.nsIPermissionManager.ALLOW_ACTION) {

the ALLOW_ACTION condition can go away as it should never happen with the platform changes

@@ +7255,5 @@
>        let objLoadingContent = aPlugin.QueryInterface(Ci.nsIObjectLoadingContent);
>        objLoadingContent.playPlugin();
>        return;
> +    } else if (pluginsPermission == Ci.nsIPermissionManager.DENY_ACTION) {
> +      overlay.style.visibility = "hidden";

can you create a class and set hidden through css? or does the anonymous content make this not simple to do?

@@ +7281,5 @@
>        gPluginHandler._showClickToPlayNotification(browser);
>    },
>  
>    _showClickToPlayNotification: function PH_showClickToPlayNotification(aBrowser) {
>      aBrowser._clickToPlayDoorhangerShown = true;

this should only be set after the return

@@ +7285,5 @@
>      aBrowser._clickToPlayDoorhangerShown = true;
>      let contentWindow = aBrowser.contentWindow;
> +
> +    let topURI = Services.io.newURI(aBrowser.contentWindow.location.href, null, null);
> +    let pluginsPermission = Services.perms.testPermission(topURI, "plugins");

same here and below (about aBrowser.currentURI)

@@ +7310,5 @@
> +      callback: function () {
> +        Services.perms.add(cwURI, "plugins", Ci.nsIPermissionManager.DENY_ACTION);
> +        let notification = PopupNotifications.getNotification("click-to-play-plugins", gBrowser.selectedBrowser);
> +        if (notification)
> +          notification.remove();

no need to remove the notification, it will be automatically removed
Comment 30 Jared Wein [:jaws] (please needinfo? me) 2012-04-10 21:13:52 PDT
Created attachment 613867 [details] [diff] [review]
Patch for bug (part 2, platform changes)

These platform changes will auto-activate plugins if the permissions allow it for the site without firing unnecessary PluginClickToPlay events.
Comment 31 Willy_ Foo_Foo 2012-04-11 05:30:00 PDT
Guys will this be also implemented so that ogg or web-m to require to be licked to be activated
this is needed to stop loading unnecessary plugins on any page(inbuilt or external) especially under linux
& also speeds up Firefox
Comment 32 Matthew N. [:MattN] 2012-04-11 10:57:44 PDT
(In reply to beelzebub360 from comment #31)
> Guys will this be also implemented so that ogg or web-m to require to be
> licked to be activated

This bug is just for plug-ins and built-in ogg/webm support is not considered a plug-in.  While not identical because we will still fetch some metadata, if you want to always click-to-play media, you can set media.autoplay.enabled to false on about:config.
Comment 33 Jared Wein [:jaws] (please needinfo? me) 2012-04-11 14:28:54 PDT
Created attachment 614173 [details] [diff] [review]
Patch for bug v3

Thanks for the thorough review. I found and fixed an issue with reshowNotifications if plugins had already been activated on the page.

(In reply to Felipe Gomes (:felipe) from comment #29)
> @@ +7255,5 @@
> >        let objLoadingContent = aPlugin.QueryInterface(Ci.nsIObjectLoadingContent);
> >        objLoadingContent.playPlugin();
> >        return;
> > +    } else if (pluginsPermission == Ci.nsIPermissionManager.DENY_ACTION) {
> > +      overlay.style.visibility = "hidden";
> 
> can you create a class and set hidden through css? or does the anonymous
> content make this not simple to do?

Using a class here would break instances where getAnonymousElementByAttribute(elem, "class", ...) is used.
 
> @@ +7281,5 @@
> >        gPluginHandler._showClickToPlayNotification(browser);
> >    },
> >  
> >    _showClickToPlayNotification: function PH_showClickToPlayNotification(aBrowser) {
> >      aBrowser._clickToPlayDoorhangerShown = true;
> 
> this should only be set after the return

I intentionally placed this before any of the early returns since if plugins are set to be denied for the page, there is no sense in checking this on each PluginClickToPlay event.
Comment 34 Josh Aas 2012-04-11 17:50:55 PDT
Comment on attachment 613867 [details] [diff] [review]
Patch for bug (part 2, platform changes)

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

I'm not really reviewing for the bigger-picture experience here, code-wise this looks fine.
Comment 35 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-04-12 23:47:36 PDT
It looks like the permission is just "plug-ins" as opposed to "plug-in Foo". I think plug-ins should be enabled on a per plug-in type basis. It's relatively rare for a site to use multiple different plug-ins, so the case where a user wants to enable e.g. both Flash and Java for a site should be rare. However, if a site has managed to bait me into enabling Flash for the site in order to watch a video, I don't want Java-based exploit kits that someone has dropped on the site to activate, too.
Comment 36 :Felipe Gomes (needinfo me!) 2012-04-12 23:49:28 PDT
Comment on attachment 614173 [details] [diff] [review]
Patch for bug v3

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

Note: there are carriage returns in both patches (some \r already got in with bug 711552, you can fix them all in this landing)

::: browser/base/content/browser.js
@@ +7279,5 @@
> +    let pluginNeedsActivation = plugins.some(function(plugin) {
> +      let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +      return !objLoadingContent.activated;
> +    });
> +    if (plugins.length && pluginNeedsActivation)

no need to check for length, plugins.some() should return false for an empty array

@@ +7289,5 @@
>      let contentWindow = aBrowser.contentWindow;
> +
> +    let pluginsPermission = Services.perms.testPermission(aBrowser.currentURI, "plugins");
> +    if (pluginsPermission == Ci.nsIPermissionManager.DENY_ACTION)
> +      return;

instead of checking this here, check on both callers and don't call it if permission == DENY. That's because _handleClickToPlay already have the information so it's a waste to immediately test the permission again. But you'll then need to test it at the reshowClickToPlay function

::: browser/components/preferences/aboutPermissions.js
@@ +361,5 @@
> +    }
> +    return this.ALLOW;
> +  },
> +  set plugins(aValue) {
> +    let value = (aValue != this.DENY);

plugins don't have a global deny, so this is always true. the correct should be aValue != this.ALLOW

@@ +407,1 @@
>  

I believe you also need to add an entry for the default plugin value here: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/aboutPermissions.js#283

@@ +445,5 @@
>      this._observersInitialized = true;
>      Services.obs.notifyObservers(null, "browser-permissions-preinit", null);
> +
> +    document.getElementById("plugins-pref-item").hidden =
> +      !Services.prefs.getBoolPref("plugins.click_to_play");

the plugins entry should always be visible in the global pane, as it's just controlling the pref. It needs to be hidden from the site-specific views if the pref is in its default state ("Allow")
Comment 37 mdew 2012-04-13 04:22:17 PDT
As a suggestion to ease people complaining and bitching there favourite websites aren't working.. can we can some pre-added domains like

youtube.com
hulu.com
netflix.com
.. etc
Comment 38 Daniel Cater 2012-04-13 04:31:31 PDT
(In reply to mdew from comment #37)
> As a suggestion to ease people complaining and bitching there favourite
> websites aren't working.. can we can some pre-added domains like
> 
> youtube.com
> hulu.com
> netflix.com
> .. etc

One of the main reasons I use click-to-play (in Nightly and Chrome) is that I can open up lots of YouTube videos and different tabs and they won't all start playing or buffering at once. I would therefore suggest not shipping a default whitelist.

If you disagree, please file a new bug as a followup to this one as that isn't the focus here.
Comment 39 Jared Wein [:jaws] (please needinfo? me) 2012-04-13 16:34:00 PDT
Created attachment 614951 [details] [diff] [review]
Patch for bug v4

Fixed the issues that you mentioned and changed about:permissions to use the plugin icon.
Comment 40 :Felipe Gomes (needinfo me!) 2012-04-13 18:17:54 PDT
Comment on attachment 614951 [details] [diff] [review]
Patch for bug v4

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

::: browser/base/content/browser.js
@@ +7262,5 @@
>        if (aEvent.button == 0 && aEvent.isTrusted)
>          gPluginHandler.activatePlugins(aEvent.target.ownerDocument.defaultView.top);
>      }, true);
>  
>      if (!browser._clickToPlayDoorhangerShown)

if (!browser._clickToPlayDoorhangerShown &&
     pluginsPermission != Ci.nsIPermissionManager.DENY_ACTION)
Comment 41 Dão Gottwald [:dao] 2012-04-14 08:48:48 PDT
(In reply to Henri Sivonen (:hsivonen) from comment #35)
> It looks like the permission is just "plug-ins" as opposed to "plug-in Foo".
> I think plug-ins should be enabled on a per plug-in type basis. It's
> relatively rare for a site to use multiple different plug-ins, so the case
> where a user wants to enable e.g. both Flash and Java for a site should be
> rare. However, if a site has managed to bait me into enabling Flash for the
> site in order to watch a video, I don't want Java-based exploit kits that
> someone has dropped on the site to activate, too.

You should bring this up at the security review.
Comment 42 Ian Melven :imelven 2012-04-14 10:05:18 PDT
(In reply to Henri Sivonen (:hsivonen) from comment #35)
> It looks like the permission is just "plug-ins" as opposed to "plug-in Foo".
> I think plug-ins should be enabled on a per plug-in type basis. It's
> relatively rare for a site to use multiple different plug-ins, so the case
> where a user wants to enable e.g. both Flash and Java for a site should be
> rare. However, if a site has managed to bait me into enabling Flash for the
> site in order to watch a video, I don't want Java-based exploit kits that
> someone has dropped on the site to activate, too.

I think this is a must have for this feature in terms of security - I updated the use cases at https://wiki.mozilla.org/Opt-in_activation_for_plugins to make this behavior explicit.
Comment 43 Justin Dolske [:Dolske] 2012-04-14 16:27:08 PDT
(In reply to Henri Sivonen (:hsivonen) from comment #35)
> It looks like the permission is just "plug-ins" as opposed to "plug-in Foo".

Correct. This is essentially a lightweight followup to the initial CTP landing, creating a basic set of permissions to make testing less of a headache. It's not the last word on CTP permissions, and future work will be needed to figure out what the right permissions granularity is -- both in terms of UX and Security.
Comment 44 Jared Wein [:jaws] (please needinfo? me) 2012-04-17 16:59:30 PDT
Comment on attachment 614951 [details] [diff] [review]
Patch for bug v4

This and the other patches for this bug are for click-to-play plugins on desktop Firefox. The part 2 patches include platform changes that fix a couple issues with our detection of activated plugins and also auto-enables plugins on the platform side if the permissions are set to "always allow" plugins on the site.

I tested these patches (and the patch for bug 742619) with Fennec Native and didn't find any issues.

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=c9990f15d47e
Comment 47 Paul Silaghi, QA [:pauly] 2012-04-30 06:25:11 PDT
How can I reset the white/black list for the "always/never activate plugins for this site" option ?
Comment 48 Jared Wein [:jaws] (please needinfo? me) 2012-04-30 06:40:53 PDT
(In reply to Paul Silaghi [QA] from comment #47)
> How can I reset the white/black list for the "always/never activate plugins
> for this site" option ?

Please see the paragraph about managing your permissions here:
http://msujaws.wordpress.com/2012/04/20/site-specific-permissions-for-firefox-opt-in-plugins/
Comment 49 Thomas Ludwig 2012-05-07 08:20:02 PDT
I've tested this new feature in FF 14 (on Kubuntu 12.04), and it works very well. Congratulations! It's also compatible with Noscript: Even if a site is whitelisted in Noscript (with its default settings), plugins are blocked by CTP. That's how it should be - great!

However, there is one problem: CTP (or executing plugins in general) does not work if javascript is not allowed for that website. I realize that most FF users simply use the default settings where javascript is allowed by default. Nevertheless, Noscript is a very popular add-on (and there are some others to control active content). If users of those add-ons stumble upon websites that contain plugins but JS is blocked, CTP simply doesn't work - the CTP placeholders aren't even displayed.

That's why I suggest the following enhancement: If JS is blocked on a website that contains plugins and
1. the user clicks the CTP placeholder: allow JS temporarily (this requires that displaying the placeholder is technically possible even if JS is blocked);
2. the user selects "Always activate plugins for this site": Display a hint "Please allow javascript for this site" (or something similar).

Implementing such an enhancement would improve the acceptance of CTP as it would sail around some trouble for users of above mentioned add-ons.
Comment 50 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-05-07 08:37:33 PDT
I filed bug 752515 on the issue of JS being disabled. It's better to have separate bug reports on things like that or they won't be tracked.
Comment 51 neil@parkwaycc.co.uk 2012-12-09 07:25:30 PST
Comment on attachment 612767 [details] [diff] [review]
Tests for patch v2

>+  Services.perms.removeAll();
You got lucky here that none of the rest of your tests needs any of the predefined permissions installed by the test harness...

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