Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Description • 12 years ago

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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 1 • 12 years ago

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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 2 • 12 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 3 • 12 years ago

Clearly a potential candidate for uplift to Fx 18.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 4 • 12 years ago

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

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 5 • 12 years ago

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

	Alex Keybl [:akeybl]
	Comment 6 • 12 years ago

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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 7 • 12 years ago

Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/5b7a7ab98fca
Pushed to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/09e13b703139

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 8 • 12 years ago

Whoops - shouldn't have closed this until it hit central.

	Ed Morley [:emorley]
	Comment 9 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/41a0c591a156

	Paul Silaghi, QA [:pauly]
	Comment 10 • 12 years ago

Verified fixed on Nightly 20.0a1 (2012-11-21) Win 7 x64.

	u279076
	Comment 11 • 12 years ago

Thanks Paul. Can you please check this on that latest Firefox 18 Beta and 19 Aurora?

	Paul Silaghi, QA [:pauly]
	Comment 12 • 12 years ago

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

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 14 • 12 years ago

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!

	Paul Silaghi, QA [:pauly]
	Comment 15 • 12 years ago

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

	Alex Keybl [:akeybl]
	Comment 16 • 12 years ago

Whatever the final resolution is here, it will need to be uplifted to the ESR as well (which also has CTP).

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 17 • 12 years ago

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

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 18 • 12 years ago

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

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 19 • 12 years ago

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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 20 • 12 years ago

https://hg.mozilla.org/releases/mozilla-esr17/rev/b86662e8e65a

	Paul Silaghi, QA [:pauly]
	Comment 21 • 12 years ago

Setting extensions.blocklist.enabled to false disable click-to-play blocklisting. Verified fixed FF 17.0.2 ESR Win 7 x64.

	Paul Silaghi, QA [:pauly]
	Comment 22 • 12 years ago

David, what about comment 17? Should be fixed in FF 18 and 17.0.2 ESR ?

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 23 • 12 years ago

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.