Closed
Bug 854867
Opened 12 years ago
Closed 12 years ago
Click-to-play no longer respects per-site permissions
Categories
(SeaMonkey :: Passwords & Permissions, defect)
Tracking
(seamonkey2.17 fixed, seamonkey2.18 fixed, seamonkey2.19 fixed)
RESOLVED
FIXED
seamonkey2.20
People
(Reporter: zart, Assigned: neil)
References
()
Details
(Keywords: regression, relnote)
Attachments
(2 files, 2 obsolete files)
|
10.25 KB,
patch
|
iannbugzilla
:
review+
philip.chee
:
feedback+
iannbugzilla
:
approval-comm-aurora+
iannbugzilla
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
|
10.62 KB,
patch
|
iannbugzilla
:
review+
Callek
:
approval-comm-release+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Keywords: regression
|
Assignee | |
Comment 1•12 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 3•12 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•12 years ago
|
||
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•12 years ago
|
||
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 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•12 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•12 years ago
|
||
Small nit, shouldn't "var nsIObjectLoadingContent = Components.interfaces.nsIObjectLoadingContent;" be const, not var?
|
|
Assignee | |
Updated•12 years ago
|
status-seamonkey2.17:
--- → affected
status-seamonkey2.18:
--- → affected
status-seamonkey2.19:
--- → affected
Keywords: relnote
|
|
Assignee | |
Comment 9•12 years ago
|
||
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•12 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•12 years ago
|
Attachment #731623 -
Flags: review?(iann_bugzilla) → review+
|
|
Assignee | |
Comment 11•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.20
|
|
Assignee | |
Comment 12•12 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•12 years ago
|
||
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 14•12 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)
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•12 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•12 years ago
|
Attachment #733617 -
Flags: approval-comm-release? → approval-comm-release+
|
|
Assignee | |
Comment 16•12 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...
|
|
||
Comment 17•12 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
You need to log in
before you can comment on or make changes to this bug.
Description
•