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

VERIFIED FIXED in Firefox 53

Status

()

VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: bytesized, Assigned: bytesized)

Tracking

(Depends on: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [fce-active-legacy])

Attachments

(1 attachment)

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

Updated

2 years ago
No longer depends on: 1335232
Blocks: 1348089
Whiteboard: [fce-active]
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8851698 - Flags: review?(kyle)

Comment 2

2 years ago
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?

Updated

2 years ago
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)

Comment 4

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8851698 - Flags: review?(kyle)
(Assignee)

Updated

2 years ago
Summary: Allow Flash blacklist to be activated without Flash Blocking enabled → Change Flash Blocking to use the user setting in about:addons for CTA
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8851698 - Flags: feedback?(benjamin)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8851698 - Flags: feedback?(benjamin)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8851698 - Flags: feedback?(benjamin)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8851698 - Flags: feedback?(benjamin)

Comment 9

2 years ago
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.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
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?

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/da5282f55afb
Status: NEW → RESOLVED
Last Resolved: 2 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)

Comment 22

2 years ago
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]
You need to log in before you can comment on or make changes to this bug.