Last Comment Bug 854867 - Click-to-play no longer respects per-site permissions
: Click-to-play no longer respects per-site permissions
Status: RESOLVED FIXED
: regression, relnote
Product: SeaMonkey
Classification: Client Software
Component: Passwords & Permissions (show other bugs)
: SeaMonkey 2.17 Branch
: All All
: -- normal (vote)
: seamonkey2.20
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
http://mozqa.com/data/firefox/plugins...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-26 05:54 PDT by Konstantin Zemlyak
Modified: 2013-04-12 07:32 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed


Attachments
2.17 branch patch (11.23 KB, patch)
2013-03-29 07:10 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Review
2.18 branch patch (10.70 KB, patch)
2013-03-29 07:31 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Review
Addressed review comments (10.25 KB, patch)
2013-03-31 02:52 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
philip.chee: feedback+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Review
2.17 branch patch (10.62 KB, patch)
2013-04-04 16:21 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
bugspam.Callek: approval‑comm‑release+
Details | Diff | Review

Description Konstantin Zemlyak 2013-03-26 05:54:42 PDT
With plugins.click_to_play set to true SeaMonkey no longer loads flash/plugins on whitelisted domains since 2.17b1. This worked fine in 2.16 betas before.

I have tested FF beta 20.0b4 and Aurora 21.0a2 - they have click to play working as intended, so I assume it's SM only bug.

Way to reproduce this bug on clean profile:
1) Open about:config, set plugins.click_to_play to true
2) Open some page with plugins, for example http://www.youtube.com/watch?v=v0l_KHjXg_s
3) Click doorhanger, in dropdown list next to button choose "Always activate plugins for this site"
4) Plugins should load and video should start to play
5) Reload page
6a) Plugins should load and video should start to play
6b) instead I observe the same page as in 2, with disabled plugins.
Comment 1 neil@parkwaycc.co.uk 2013-03-26 17:56:44 PDT
This regression was caused by bug 746374 which switched from having a single "plugin" permission to a uniquely named permission for each plugin, which means we have to whitelist all the plugins on the page. (And of course the permissions won't display nicely in Data Manager - ideas KaiRo?)
Comment 2 Ian Neal 2013-03-26 18:15:56 PDT
Some sort of row per plugin? Is there a reverse lookup mechanism?
Comment 3 Robert Kaiser (not working on stability any more) 2013-03-26 19:01:20 PDT
(In reply to neil@parkwaycc.co.uk from comment #1)
> And of course the
> permissions won't display nicely in Data Manager

I think we should have a separate bug for that - but that's probably not too hard.
Comment 4 neil@parkwaycc.co.uk 2013-03-29 07:10:38 PDT
Created attachment 731146 [details] [diff] [review]
2.17 branch patch

I happened to use a 2.17 tree to write this patch. It doesn't apply to trunk, so I'll merge it and attach the merged version for review.
Comment 5 neil@parkwaycc.co.uk 2013-03-29 07:31:21 PDT
Created attachment 731152 [details] [diff] [review]
2.18 branch patch

Applies to trunk too :-)

* Moves the permission check into canActivatePlugin because each plugin has its own check; pageshow event no longer makes a global permission check
* Added setPermissionForPlugins convenience method
* Updated setupPluginClickToPlay to check the plugin's specific permission
Comment 6 Ian Neal 2013-03-30 16:06:37 PDT
Comment on attachment 731152 [details] [diff] [review]
2.18 branch patch

Not tested yet, just doing code inspection.
>+++ b/suite/common/bindings/notification.xml
>       <method name="canActivatePlugin">
>         <parameter name="objLoadingContent"/>
>         <body>
>           <![CDATA[
>+            if (objLoadingContent.actualType) {
>+              var ph = Components.classes["@mozilla.org/plugin/host;1"]
>+                                 .getService(Components.interfaces.nsIPluginHost);
>+              var permissionString = ph.getPermissionStringForType(objLoadingContent.actualType);
>+              const nsIPermissionManager = Components.interfaces.nsIPermissionManager;
>+              var pm = Components.classes["@mozilla.org/permissionmanager;1"]
>+                                 .getService(nsIPermissionManager);
>+              var pluginPermission = pm.testPermission(this.activeBrowser.currentURI, permissionString);
>+              if (pluginPermission == nsIPermissionManager.DENY_ACTION)
>+                return false;
>+            }
>+
>             var nsIObjectLoadingContent = Components.interfaces.nsIObjectLoadingContent;
>             return !objLoadingContent.activated &&
>                    objLoadingContent.pluginFallbackType !== nsIObjectLoadingContent.PLUGIN_PLAY_PREVIEW;
>           ]]>
>         </body>
>       </method>
This seems very similar to:
>       <method name="setupPluginClickToPlay">
>         <parameter name="pluginElement"/>
>         <body>
>           <![CDATA[
>             var doc = pluginElement.ownerDocument;
>             var overlay = doc.getAnonymousElementByAttribute(pluginElement, "class", "mainBox");
> 
>+            var objLoadingContent = pluginElement.QueryInterface(Components.interfaces.nsIObjectLoadingContent);
>+            if (objLoadingContent.actualType) {
>+              var ph = Components.classes["@mozilla.org/plugin/host;1"]
>+                                 .getService(Components.interfaces.nsIPluginHost);
>+              var permissionString = ph.getPermissionStringForType(objLoadingContent.actualType);
>+              const nsIPermissionManager = Components.interfaces.nsIPermissionManager;
>+              var pm = Components.classes["@mozilla.org/permissionmanager;1"]
>+                                 .getService(nsIPermissionManager);
>+              var pluginPermission = pm.testPermission(this.activeBrowser.currentURI, permissionString);
>+              if (pluginPermission == nsIPermissionManager.DENY_ACTION) {
>+                overlay.style.visibility = "hidden";
>+                return;
>+              }
>+            }
>+
Would it make sense to have some sort of helper to remove the code duplication?
Comment 7 neil@parkwaycc.co.uk 2013-03-30 16:22:40 PDT
(In reply to Ian Neal from comment #6)
> (From update of attachment 731152 [details] [diff] [review])
> >       <method name="canActivatePlugin">
> >         <parameter name="objLoadingContent"/>
> >         <body>
> >           <![CDATA[
> >+            if (objLoadingContent.actualType) {
...
> >+              if (pluginPermission == nsIPermissionManager.DENY_ACTION)
> >+                return false;
> >+            }
> This seems very similar to:
> >       <method name="setupPluginClickToPlay">
> >         <parameter name="pluginElement"/>
> >         <body>
> >           <![CDATA[
> >             var doc = pluginElement.ownerDocument;
> >             var overlay = doc.getAnonymousElementByAttribute(pluginElement, "class", "mainBox");
> > 
> >+            var objLoadingContent = pluginElement.QueryInterface(Components.interfaces.nsIObjectLoadingContent);
> >+            if (objLoadingContent.actualType) {
...
> >+              if (pluginPermission == nsIPermissionManager.DENY_ACTION) {
> >+                overlay.style.visibility = "hidden";
> >+                return;
> Would it make sense to have some sort of helper to remove the code
> duplication?
[The two pieces of code were originally copied from attachment 685690 [details] [diff] [review] where they were written slightly differently. I didn't like the way the code for canActivatePlugin was written so I tweaked the style to mirror setupPluginClickToPlay. I since notice that subsequent patches have landed which ideally I should merge in to this patch too.]
I could probably make setupPluginClickToPlay call canActivatePlugin directly.
Comment 8 Konstantin Zemlyak 2013-03-31 01:28:02 PDT
Small nit, shouldn't "var nsIObjectLoadingContent = Components.interfaces.nsIObjectLoadingContent;" be const, not var?
Comment 9 neil@parkwaycc.co.uk 2013-03-31 02:52:14 PDT
Created attachment 731623 [details] [diff] [review]
Addressed review comments
Comment 10 Philip Chee 2013-04-03 08:49:15 PDT
Comment on attachment 731623 [details] [diff] [review]
Addressed review comments

f=me
I tested with the URL from Bug 746374 Comment 18:
http://mozqa.com/data/firefox/plugins/plugin-test.html
Comment 11 neil@parkwaycc.co.uk 2013-04-04 16:06:03 PDT
Pushed comm-central changeset 3e75505e7166.
Comment 12 neil@parkwaycc.co.uk 2013-04-04 16:21:08 PDT
Comment on attachment 731623 [details] [diff] [review]
Addressed review comments

[Approval Request Comment]
Regression caused by (bug #): 746374
User impact if declined: Unable to save click-to-play plugin permissions
Testing completed (on m-c, etc.): Landed on c-c
Risk to taking this patch (and alternatives if risky): Not a trivial patch but passed both feedback and review
String changes made by this patch: None
Comment 13 neil@parkwaycc.co.uk 2013-04-04 16:21:36 PDT
Created attachment 733617 [details] [diff] [review]
2.17 branch patch

Requesting release approval just in case...
[Approval Request Comment]
Regression caused by (bug #): 746374
User impact if declined: Unable to save click-to-play plugin permissions
Testing completed (on m-c, etc.): Landed on c-c
Risk to taking this patch (and alternatives if risky): Awaiting review; alternative is to remove the UI that saves the permission
String changes made by this patch: None
Comment 14 Jens Hatlak (:InvisibleSmiley) 2013-04-04 23:07:28 PDT
Comment on attachment 731623 [details] [diff] [review]
Addressed review comments

Realistically I'm not going to get to this any time soon. Luckily Philip's f+ is more than enough.
Comment 15 Ian Neal 2013-04-06 08:01:14 PDT
Comment on attachment 733617 [details] [diff] [review]
2.17 branch patch

I'll leave Callek to do the a=
Comment 16 neil@parkwaycc.co.uk 2013-04-11 16:00:40 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/89781801c00c
https://hg.mozilla.org/releases/comm-beta/rev/acd8f84ffcdf

I'll let whoever it was who pushed to comm-release to update the bug...
Comment 17 Philip Chee 2013-04-12 07:32:55 PDT
> I'll let whoever it was who pushed to comm-release to update the bug...
Callek did it with a candlestick in the library.
http://hg.mozilla.org/releases/comm-release/rev/9ee1b229d0fb

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