Remove Flash from navigator.plugins when Flash is blacklisted in the document

RESOLVED FIXED in Firefox 53

Status

()

Core
Plug-ins
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: bytesized, Assigned: bytesized)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox53 fixed, firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Documents in which Flash is blacklisted should not advertise Flash's existence in navigator.plugins.
(Assignee)

Updated

10 months ago
Depends on: 1335475
Comment hidden (mozreview-request)
(Assignee)

Comment 2

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0519c7676696
(Assignee)

Updated

10 months ago
Attachment #8832220 - Flags: review?(benjamin)
(Assignee)

Comment 3

10 months ago
@bsmedberg Let me know if I should request additional/different reviewers.

Comment 4

10 months ago
mozreview-review
Comment on attachment 8832220 [details]
Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document

https://reviewboard.mozilla.org/r/108568/#review109950

This is not the right way to do this. We can safely assume that Flash is the only plugin, so we don't need to check for Flash in particular any more.

With that, this should all happen through nsPluginArray::AllowPlugins() -> nsDocShell::PluginsAllowedInCurrentDoc() -> nsDocument::GetAllowPlugins and you should just add a DocumentFlashClassification check in nsDocument::GetAllowPlugins.

::: dom/base/nsPluginArray.cpp:334
(Diff revision 1)
>    // This only supports one hidden plugin
>    return Preferences::GetCString("plugins.navigator.hidden_ctp_plugin").Equals(aName);
>  }
>  
> +static bool
> +PluginIsFlash(nsPluginTag* aPlugin) {

Formatting nit, opening brace of a function should be in column 1 of the next line.
Attachment #8832220 - Flags: review?(benjamin) → review-
(Assignee)

Comment 5

10 months ago
mozreview-review-reply
Comment on attachment 8832220 [details]
Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document

https://reviewboard.mozilla.org/r/108568/#review109950

Having nsDocument::GetAllowPlugins return False causes Flash to be blocked via a different code path resulting in a Plugin Fallback Type of PLUGIN_USER_DISABLED instead of PLUGIN_SUPPRESSED. Is that ok?
(Assignee)

Updated

10 months ago
Flags: needinfo?(benjamin)

Comment 6

10 months ago
Is there any difference is user-visible behavior? Or does this affect telemetry at all? Not having PLUGIN_SUPPRESSED seems fine to me.
Flags: needinfo?(benjamin) → needinfo?(ksteuber)
(Assignee)

Comment 7

10 months ago
I believe the user-visible behavior is the same. I am not sure how it affects telemetry.
Flags: needinfo?(ksteuber)

Comment 8

10 months ago
I mean the telemetry you and Felipe are specifically adding for this shield study. I'm confident it doesn't affect any other telemetry.
Comment hidden (mozreview-request)

Comment 10

10 months ago
mozreview-review
Comment on attachment 8832220 [details]
Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document

https://reviewboard.mozilla.org/r/108568/#review110388

LGTM. The outparam in nsDocument::GetAllowPlugins is ugly but let's not fix that here!
Attachment #8832220 - Flags: review?(benjamin) → review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 11

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8649cbdc0969
Remove Flash from navigator.plugins when Flash is blacklisted in the document r=bsmedberg
Keywords: checkin-needed

Comment 12

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8649cbdc0969
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Updated

10 months ago
No longer depends on: 1335475
(Assignee)

Updated

10 months ago
Blocks: 1335232, 1277346
(Assignee)

Comment 13

10 months ago
Comment on attachment 8832220 [details]
Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document

Approval Request Comment
[Feature/Bug causing the regression]: This feature is needed for the Shield study to be run on Release 52 (bug 1335232), which we'll use to study the effect of making flash click-to-play by default.
[User impact if declined]: Flash will be present in navigator.plugins on pages that are strictly denied from loading Flash.
[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]: Not for the feature independently. We'll do QE on the study as a whole to make sure all pieces work as expected
[List of other uplifts needed for the feature/fix]: bug 1307604
[Is the change risky?]: No
[Why is the change risky/not risky?]: Outside of test code, only adds a check to nsDocument::GetAllowPlugins. This check will only change behavior when Flash blocking is turned on and the page is denied from loading Flash.
[String changes made/needed]: none
Attachment #8832220 - Flags: approval-mozilla-beta?
Attachment #8832220 - Flags: approval-mozilla-aurora?

Updated

10 months ago
status-firefox52: --- → affected
status-firefox53: --- → affected

Comment 14

10 months ago
Comment on attachment 8832220 [details]
Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document

For shield study purpose. Aurora53+.
Attachment #8832220 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 15

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/920aa8178562
status-firefox53: affected → fixed
Comment on attachment 8832220 [details]
Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document

this was deemed too risky for beta
Attachment #8832220 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox52: affected → wontfix
You need to log in before you can comment on or make changes to this bug.