Click-to-play no longer respects per-site permissions

RESOLVED FIXED in seamonkey2.20

Status

SeaMonkey
Passwords & Permissions
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Konstantin Zemlyak, Assigned: neil@parkwaycc.co.uk)

Tracking

({regression, relnote})

SeaMonkey 2.17 Branch
seamonkey2.20
regression, relnote

SeaMonkey Tracking Flags

(seamonkey2.17 fixed, seamonkey2.18 fixed, seamonkey2.19 fixed)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Keywords: regression
(Assignee)

Comment 1

4 years ago
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?)
Assignee: nobody → neil
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All

Comment 2

4 years ago
Some sort of row per plugin? Is there a reverse lookup mechanism?

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
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
Attachment #731152 - Flags: review?(iann_bugzilla)
Attachment #731152 - Flags: feedback?(philip.chee)
Attachment #731152 - Flags: feedback?(jh)

Comment 6

4 years ago
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?
(Assignee)

Comment 7

4 years ago
(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.
(Reporter)

Comment 8

4 years ago
Small nit, shouldn't "var nsIObjectLoadingContent = Components.interfaces.nsIObjectLoadingContent;" be const, not var?
(Assignee)

Updated

4 years ago
status-seamonkey2.17: --- → affected
status-seamonkey2.18: --- → affected
status-seamonkey2.19: --- → affected
Keywords: relnote
(Assignee)

Comment 9

4 years ago
Created attachment 731623 [details] [diff] [review]
Addressed review comments
Attachment #731146 - Attachment is obsolete: true
Attachment #731152 - Attachment is obsolete: true
Attachment #731152 - Flags: review?(iann_bugzilla)
Attachment #731152 - Flags: feedback?(philip.chee)
Attachment #731152 - Flags: feedback?(jh)
Attachment #731623 - Flags: review?(iann_bugzilla)
Attachment #731623 - Flags: feedback?(philip.chee)
Attachment #731623 - Flags: feedback?(jh)

Comment 10

4 years ago
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
Attachment #731623 - Flags: feedback?(philip.chee) → feedback+

Updated

4 years ago

Updated

4 years ago
Attachment #731623 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 11

4 years ago
Pushed comm-central changeset 3e75505e7166.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.20
(Assignee)

Comment 12

4 years ago
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
Attachment #731623 - Flags: approval-comm-beta?
Attachment #731623 - Flags: approval-comm-aurora?
(Assignee)

Comment 13

4 years ago
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
Attachment #733617 - Flags: review?(iann_bugzilla)
Attachment #733617 - Flags: approval-comm-release?
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.
Attachment #731623 - Flags: feedback?(jh)

Updated

4 years ago
Attachment #731623 - Flags: approval-comm-beta?
Attachment #731623 - Flags: approval-comm-beta+
Attachment #731623 - Flags: approval-comm-aurora?
Attachment #731623 - Flags: approval-comm-aurora+

Comment 15

4 years ago
Comment on attachment 733617 [details] [diff] [review]
2.17 branch patch

I'll leave Callek to do the a=
Attachment #733617 - Flags: review?(iann_bugzilla) → review+

Updated

4 years ago
Attachment #733617 - Flags: approval-comm-release? → approval-comm-release+
(Assignee)

Comment 16

4 years ago
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...
status-seamonkey2.18: affected → fixed
status-seamonkey2.19: affected → fixed

Comment 17

4 years ago
> 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
status-seamonkey2.17: affected → fixed
You need to log in before you can comment on or make changes to this bug.