Closed Bug 854867 Opened 11 years ago Closed 11 years ago

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

Tracking

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

Status:

RESOLVED FIXED

Milestone:

seamonkey2.20

Tracking Flags:

Tracking

Status

seamonkey2.17

---

fixed

seamonkey2.18

---

fixed

seamonkey2.19

---

fixed

People

(Reporter: zart, Assigned: neil)

References

(
URL
)

Details

(Keywords: regression, relnote)

Attachments

(2 files, 2 obsolete files)

2.17 branch patch 11 years ago neil@parkwaycc.co.uk 11.23 KB, patch		Details \| Diff \| Splinter Review
2.18 branch patch 11 years ago neil@parkwaycc.co.uk 10.70 KB, patch		Details \| Diff \| Splinter Review
Addressed review comments 11 years ago neil@parkwaycc.co.uk 10.25 KB, patch	iannbugzilla : review+ philip.chee : feedback+ iannbugzilla : approval-comm-aurora+ iannbugzilla : approval-comm-beta+	Details \| Diff \| Splinter Review
2.17 branch patch 11 years ago neil@parkwaycc.co.uk 10.62 KB, patch	iannbugzilla : review+ Callek : approval-comm-release+	Details \| Diff \| Splinter Review

Konstantin Zemlyak

Reporter

Description

•

11 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.

Konstantin Zemlyak

Reporter

Updated

•

11 years ago

Keywords: regression

neil@parkwaycc.co.uk

Assignee

Comment 1

•

11 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

Ian Neal

Comment 2

•

11 years ago

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

Robert Kaiser

Comment 3

•

11 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.

neil@parkwaycc.co.uk

Assignee

Comment 4

•

11 years ago

Attached patch 2.17 branch patch (obsolete) — Details — Splinter Review

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.

neil@parkwaycc.co.uk

Assignee

Comment 5

•

11 years ago

Attached patch 2.18 branch patch (obsolete) — Details — Splinter Review

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)

Ian Neal

Comment 6

•

11 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?

neil@parkwaycc.co.uk

Assignee

Comment 7

•

11 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.

Konstantin Zemlyak

Reporter

Comment 8

•

11 years ago

Small nit, shouldn't "var nsIObjectLoadingContent = Components.interfaces.nsIObjectLoadingContent;" be const, not var?

neil@parkwaycc.co.uk

Assignee

Updated

•

11 years ago

status-seamonkey2.17: --- → affected

status-seamonkey2.18: --- → affected

status-seamonkey2.19: --- → affected

Keywords: relnote

neil@parkwaycc.co.uk

Assignee

Comment 9

•

11 years ago

Attached patch Addressed review comments — Details — Splinter Review

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)

Philip Chee

Comment 10

•

11 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+

Philip Chee

Updated

•

11 years ago

URL: http://mozqa.com/data/firefox/plugins...

Ian Neal

Updated

•

11 years ago

Attachment #731623 - Flags: review?(iann_bugzilla) → review+

neil@parkwaycc.co.uk

Assignee

Comment 11

•

11 years ago

Pushed comm-central changeset 3e75505e7166.

Status: NEW → RESOLVED

Closed: 11 years ago

Resolution: --- → FIXED

Target Milestone: --- → seamonkey2.20

neil@parkwaycc.co.uk

Assignee

Comment 12

•

11 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?

neil@parkwaycc.co.uk

Assignee

Comment 13

•

11 years ago

Attached patch 2.17 branch patch — Details — Splinter Review

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?

Jens Hatlak (:InvisibleSmiley)

Comment 14

•

11 years ago

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)

Ian Neal

Updated

•

11 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+

Ian Neal

Comment 15

•

11 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+

Justin Wood (:Callek)

Updated

•

11 years ago

Attachment #733617 - Flags: approval-comm-release? → approval-comm-release+

neil@parkwaycc.co.uk

Assignee

Comment 16

•

11 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

Philip Chee

Comment 17

•

11 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.