Closed Bug 1350381 Opened 7 years ago Closed 7 years ago

Change Flash Blocking to use the user setting in about:addons for CTA

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 fixed, firefox54 fixed, firefox55 fixed)

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: bytesized, Assigned: bytesized)

References

Details

(Whiteboard: [fce-active-legacy])

Attachments

(1 file)

Bug 1350381 - Change Flash blocking to allow the setting "Ask to Activate" to control CTA of unknown documents.
59 bytes, text/x-review-board-request
qdot
: review+
lizzard
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
Details 
The Flash blocking blacklists should be usable even when Flash blocking is disabled. This behavior will be accessible via a pref.

This feature will be needed for eventual deployment of Flash blocking, but will not be needed for the Shield study.
No longer depends on: flashstudy
Blocks: 1348089
Whiteboard: [fce-active]
Attachment #8851698 - Flags: review?(kyle)
Is it possible to change the behavior of the existing pref, rather than introduce a new one? In particular, the UI we want still doesn't map very well to the available pref options here:

Whitelist contents                    WHITELISTED     BLACKLISTED     OTHER
about:addons
"never activate"                      deny            deny            deny
"ask to activate"
* "use mozilla lists" checked/default allow           deny            ask
* "use mozilla lists" unchecked       ask             ask             ask
"always activate"
* "use mozilla lists" checked/default allow           deny            allow
* "use mozilla lists" unchecked       allow           allow           allow

I think you could get the desired behavior here by alternately enabling plugins.flashBlock.blacklist.forceEnable for some cases and plugins.flashBlock.enabled for other cases. However it's not a clean mapping.

Why though can't we just have the plugins.flashBlock.enabled pref turn on the whitelist and blacklist behavior without changing the default behavior at all?
Flags: needinfo?(ksteuber)
In the current implementation (which I feel is the best way to do this), the Flash blocking system introduces additional security, but never detracts from existing security. This means that I never implemented a whitelist mechanism that "breaks through" other security to allow automatic activation of whitelisted content despite security settings.

That is to say, all the whitelist does is prevents the site from being CTA'ed by the Flash blocking system itself. It does not prevent it from being CTA'ed by the "Ask to Activate" setting.
Flags: needinfo?(ksteuber)
ok, but... in the addon manager UI, when this is deployed to users, the setting that is displayed to users will be "ask to activate". And in reality, what's happening is that this feature is tuning ask-to-activate to stop asking in cases where we don't think the user 1) needs to make a choice or 2) can make an informed choice.

As long as the UI ends up in comment 2, the underlying prefs are an engineering choice, but I still find these prefs hard to read and reason about when structured this way.
Attachment #8851698 - Flags: review?(kyle)
Summary: Allow Flash blacklist to be activated without Flash Blocking enabled → Change Flash Blocking to use the user setting in about:addons for CTA
Attachment #8851698 - Flags: feedback?(benjamin)
Attachment #8851698 - Flags: feedback?(benjamin)
Attachment #8851698 - Flags: feedback?(benjamin)
Attachment #8851698 - Flags: feedback?(benjamin)
Comment on attachment 8851698 [details]
Bug 1350381 - Change Flash blocking to allow the setting "Ask to Activate" to control CTA of unknown documents.

This is a great commit message!
Attachment #8851698 - Flags: feedback?(benjamin) → feedback+
It looks like this patch is causing failures in browser/base/content/test/plugins/browser_CTP_context_menu.js. Once I fix that I will request review for this patch.
Attachment #8851698 - Flags: review?(kyle)
Comment on attachment 8851698 [details]
Bug 1350381 - Change Flash blocking to allow the setting "Ask to Activate" to control CTA of unknown documents.

https://reviewboard.mozilla.org/r/123944/#review127348
Attachment #8851698 - Flags: review?(kyle) → review+
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da5282f55afb
Change Flash blocking to allow the setting "Ask to Activate" to control CTA of unknown documents. r=qdot
Comment on attachment 8851698 [details]
Bug 1350381 - Change Flash blocking to allow the setting "Ask to Activate" to control CTA of unknown documents.

Approval Request Comment
[Feature/Bug causing the regression]: This feature represents a significant change to the mechanism underlying Flash blocking, which will be undergoing a Shield study on Release 52 (Bug 1335232).
[User impact if declined]: The Shield study will test the current mechanism instead of the new one
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Manual testing available at itisatrap.org
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal Risk
[Why is the change risky/not risky?]: Code changes are pref'ed off.
[String changes made/needed]: none
Attachment #8851698 - Flags: approval-mozilla-beta?
Attachment #8851698 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/da5282f55afb
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8851698 [details]
Bug 1350381 - Change Flash blocking to allow the setting "Ask to Activate" to control CTA of unknown documents.

As I understand it this should change behavior only for the shield study, for a study on 53 (not for 52)
Attachment #8851698 - Flags: approval-mozilla-beta?
Attachment #8851698 - Flags: approval-mozilla-beta+
Attachment #8851698 - Flags: approval-mozilla-aurora?
Attachment #8851698 - Flags: approval-mozilla-aurora+
Sorry about that! It should have read:

[Feature/Bug causing the regression]: This feature represents a significant change to the mechanism underlying Flash blocking, which will be undergoing a Shield study on Release 53 (Bug 1335232).
Note: to uplift this to beta without conflicts we gotta uplift the _test-only_ bug 1340448 (no need to request approval for that)
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43983c66642e
Define this pref so that the test doesn't fail. r=me a=test-fix
Note: this made it to the Nightly on Mar 31, and the build ID I got for that on Mac is 20170331030216 (this will need to be set as the minBuildID for bug 1352224)
This bug has been verified on Windows 7/10, Ubuntu 16.04 and Mac OS X 10.11 on fixed versions. When changing the options in about:addons to "always activate" or "ask to activate" it shows flash as expected.
Status: RESOLVED → VERIFIED
Whiteboard: [fce-active] → [fce-active-legacy]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: