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

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Robin Steuber (they/them) [:bytesized]]

Documents in which Flash is blacklisted should not advertise Flash's existence in navigator.plugins.

Comment 1

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document

Review commit: https://reviewboard.mozilla.org/r/108568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108568/

Comment 2

[Robin Steuber (they/them) [:bytesized]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0519c7676696

Comment 3

[Robin Steuber (they/them) [:bytesized]]

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

Comment 4

[Benjamin Smedberg]

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.

Comment 5

[Robin Steuber (they/them) [:bytesized]]

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?

Comment 6

[Benjamin Smedberg]

Is there any difference is user-visible behavior? Or does this affect telemetry at all? Not having PLUGIN_SUPPRESSED seems fine to me.

Comment 7

[Robin Steuber (they/them) [:bytesized]]

I believe the user-visible behavior is the same. I am not sure how it affects telemetry.

Comment 8

[Benjamin Smedberg]

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 9

[Robin Steuber (they/them) [:bytesized]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108568/diff/1-2/

Comment 10

[Benjamin Smedberg]

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!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8649cbdc0969
Remove Flash from navigator.plugins when Flash is blacklisted in the document r=bsmedberg

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8649cbdc0969

Comment 13

[Robin Steuber (they/them) [:bytesized]]

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

Comment 14

[Gerry Chang [:gchang]]

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

For shield study purpose. Aurora53+.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/920aa8178562

Comment 16

[Julien Cristau [:jcristau]]

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