Closed Bug 811375 Opened 7 years ago Closed 7 years ago

setting extensions.blocklist.enabled to false doesn't disable click-to-play blocklisting

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox18 + verified
firefox19 + verified
firefox-esr17 18+ verified

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(2 files, 1 obsolete file)

STR:
1. Somehow get a plugin to be click-to-play blocklisted (either roll your own blocklist.xml or install an old plugin that's currently blocked by default, etc.)
2. set extensions.blocklist.enabled to false
3. visit a page with the above plugin

Expected results: the plugin plays
Actual results: the plugin is click-to-play (non-security version)

Right now, the clicktoplay flag in nsPluginTags is set by nsBlocklistService when a plugin is ctp-blocklisted. When nsBlocklistService is disabled, however, this flag isn't unset. This has two problems. The first is that nsPluginHost will see that flag and say a plugin is ctp when it shouldn't be. The second is that the clicktoplay flag can't be used if/when we move from all-or-nothing user-set click-to-play to plugin-by-plugin user-set click-to-play.

I think this flag is ill-suited for this purpose. Instead of using the flag, nsPluginHost should just ask nsBlocklistService the blocklist state of a plugin in IsPluginClickToPlayForType. If it's one of the click-to-play blocklist states or if plugins.click_to_play is true, it will return true.
Attached patch patch (obsolete) — Splinter Review
Here's what I think we should do. Let me know if this doesn't make sense or will cause problems. In terms of priority, this isn't huge, but it would probably be a good idea to fix before we really let this loose on the world.
Attachment #681596 - Flags: review?(joshmoz)
Attachment #681596 - Flags: feedback?(jschoenick)
As it turns out, when switching channels (e.g. current -> aurora or nightly -> current), a similar thing happens where a plugin will retain its click-to-play status regardless of whether or not the plugin blocklist entry applies to that version of firefox. So, this is a bit more than just a good idea - we should really fix this sooner rather than later.
Clearly a potential candidate for uplift to Fx 18.
Attachment #681596 - Flags: review?(joshmoz) → review+
Attached patch patch v1.1Splinter Review
Here's the try run: https://tbpl.mozilla.org/?tree=Try&rev=d847655597ac

I added another check to test_pluginBlocklistCtp.js, but it's minor, so I'm carrying over the r+.

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/41a0c591a156
Attachment #681596 - Attachment is obsolete: true
Attachment #681596 - Flags: feedback?(jschoenick)
Attachment #683309 - Flags: review+
Comment on attachment 683309 [details] [diff] [review]
patch v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play blocklisting plugins
User impact if declined: essentially, we won't be confident in deploying any click-to-play blocks
Testing completed (on m-c, etc.): see try run, above
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: I added a method to nsIPluginHost - I can work around this by adding nsIPluginHost2, if that's the way we think we should go
Attachment #683309 - Flags: approval-mozilla-beta?
Attachment #683309 - Flags: approval-mozilla-aurora?
Comment on attachment 683309 [details] [diff] [review]
patch v1.1

[Triage Comment]
Approving for Aurora/Beta. The merge will be complete in a few minutes, you can land at that time.
Attachment #683309 - Flags: approval-mozilla-beta?
Attachment #683309 - Flags: approval-mozilla-beta+
Attachment #683309 - Flags: approval-mozilla-aurora?
Attachment #683309 - Flags: approval-mozilla-aurora+
Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/5b7a7ab98fca
Pushed to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/09e13b703139
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whoops - shouldn't have closed this until it hit central.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla20 → ---
https://hg.mozilla.org/mozilla-central/rev/41a0c591a156
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Verified fixed on Nightly 20.0a1 (2012-11-21) Win 7 x64.
Thanks Paul. Can you please check this on that latest Firefox 18 Beta and 19 Aurora?
Status: RESOLVED → VERIFIED
Keywords: verifyme
Verified fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0b1
Aurora 19.0a2 (2012-11-21).
No longer blocks: 812927
Duplicate of this bug: 812927
Paul, it would be great if you could also check that the scenarios in https://bugzilla.mozilla.org/show_bug.cgi?id=810984#c10 work as expected.
Thanks!
1. Start FF 17 with a new profile, ping -> Flash not blocked, Java softblocked
2. Start FF 18 with the same profile, ping -> Flash CTP blocked, Java CTP blocked 
This works fine as expected.

But trying vice-versa:
1. Start FF 18 with a new profile, ping -> Flash CTP blocked, Java CTP blocked
2. Start FF 17 with the same profile, ping -> Flash not blocked, Java NOT softblocked
So here is the problem - Java is not softblocked when downgrading from FF 18 to 17
Whatever the final resolution is here, it will need to be uplifted to the ESR as well (which also has CTP).
(In reply to Paul Silaghi [QA] from comment #15)
> 1. Start FF 17 with a new profile, ping -> Flash not blocked, Java
> softblocked
> 2. Start FF 18 with the same profile, ping -> Flash CTP blocked, Java CTP
> blocked 
> This works fine as expected.

Awesome - thanks for checking that.

> But trying vice-versa:
> 1. Start FF 18 with a new profile, ping -> Flash CTP blocked, Java CTP
> blocked
> 2. Start FF 17 with the same profile, ping -> Flash not blocked, Java NOT
> softblocked
> So here is the problem - Java is not softblocked when downgrading from FF 18
> to 17

I figured that might be the case. I believe this is a bug that has been in the blocklist code since it was written, basically. Since we're moving to click-to-play (and it's almost done, for real this time, I swear), I'm not sure how important it is we fix that.

I'll start work on getting this fix to ESR tomorrow.
Attached patch esr17 patchSplinter Review
My understanding is that IID changes can't happen on ESR, so I had to add nsIPluginHost2. Josh - I'd appreciate if you would have a look to make sure I did that properly. (The other option is to leave nsIPluginHost as it is and basically not test some aspects of plugin click-to-play blocklisting, but I'm assuming we don't want to do that.)
Attachment #686251 - Flags: review?(joshmoz)
Attachment #686251 - Flags: review?(joshmoz) → review+
Comment on attachment 686251 [details] [diff] [review]
esr17 patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: see user impact
User impact if declined: click-to-play blocklisting plugins won't work properly on (esr)17 without this (it all depends on when users upgrade, but this is a bad situation to be in, so we should just fix it)
Fix Landed on Version: 18
Risk to taking this patch (and alternatives if risky): lowish (it's possible I broke nsPluginHost, but I ran this through try (https://tbpl.mozilla.org/?tree=Try&rev=786b11645825), and despite android not working, the other platforms were mostly green)
String or UUID changes made by this patch: no changes - I did add nsIPluginHost2

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #686251 - Flags: approval-mozilla-esr17?
Attachment #686251 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
Setting extensions.blocklist.enabled to false disable click-to-play blocklisting. Verified fixed FF 17.0.2 ESR Win 7 x64.
David, what about comment 17? Should be fixed in FF 18 and 17.0.2 ESR ?
Well, this will probably never work when downgrading to 17, if that's what you're asking. But yes, this should be fixed in FF 18 and whatever ESR corresponds to 18.
You need to log in before you can comment on or make changes to this bug.