1435098 - The flashinfobar list is not gated by any of the existing feature prefs

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[François Marier [:francois]]

It used to be possible to prevent all flash-blocking related lists from being downloaded by setting plugins.flashBlock.enabled = false.

This is no longer possible since bug 1369160 introduced the flashinfobar list without checking that pref:

https://hg.mozilla.org/mozilla-central/rev/2322c37ad902#l2.45

Comment 1

[François Marier [:francois]]

Kirk, are there any concerns with gating this functionality on the flashblock.enabled pref?

Comment 2

[François Marier [:francois]]

The work-around to disable all flash-related lists is to set both of these prefs:

plugins.flashBlock.enabled = false
urlclassifier.flashInfobarTable = ""

Comment 3

[Kirk Steuber (he/him) [:bytesized]]

I do not think that the intention was for the Flash infobar exception list to be a part of Flash Blocking at all. It came about because Flash blocking caused users to see these infobars frequently, when most users would never have seen them before. But I don't believe that it is fundamentally related to Flash blocking.

This seems to be backed up by :bsmedberg when he said in Bug 1369160 Comment 3, "Don't pref this on plugins.flashBlock.enabled, it can be on by default."

I don't see any functional problem with gating the infobar exception list behind the |plugins.flashblock.enabled| pref, but I also don't really see why it should be. Is there a good reason for making this change?

Comment 4

[François Marier [:francois]]

(In reply to Kirk Steuber [:bytesized] from comment #3)
> I do not think that the intention was for the Flash infobar exception list
> to be a part of Flash Blocking at all. It came about because Flash blocking
> caused users to see these infobars frequently, when most users would never
> have seen them before. But I don't believe that it is fundamentally related
> to Flash blocking.

It's possible I've misunderstood this feature, but it seems like it is related to flash blocking in that unless flash blocking is enabled, you don't need the infobar suppression list, right?

> This seems to be backed up by :bsmedberg when he said in Bug 1369160 Comment
> 3, "Don't pref this on plugins.flashBlock.enabled, it can be on by default."

Given that plugins.flashblocked.enabled is now on by default, I guess that comment is no longer important.

> I don't see any functional problem with gating the infobar exception list
> behind the |plugins.flashblock.enabled| pref, but I also don't really see
> why it should be. Is there a good reason for making this change?

To make it easy to disable all network connections to the shavar server that are related to flash "blocking". For example for automated tests that don't want to have any network traffic.

Comment 5

[Kirk Steuber (he/him) [:bytesized]]

When Flash blocking is not enabled the Flash infobar suppression will still be active for any users that use the option: "CTA Flash by Default".

This is why I say that it is not part of Flash blocking.

Shavar serves other lists to Firefox besides those used by Flash blocking. Perhaps all of them ought to be controlled by a single pref if this is a problem.

Comment 6

[François Marier [:francois]]

(In reply to Kirk Steuber [:bytesized] from comment #5)
> When Flash blocking is not enabled the Flash infobar suppression will still
> be active for any users that use the option: "CTA Flash by Default".
> 
> This is why I say that it is not part of Flash blocking.

I see, thanks for the clarification.

Perhaps what we need then is a separate pref to control that feature. How about plugins.suppressFlashInfoBars = true ?

> Perhaps all of them ought to be controlled by a single pref if this is a problem.

Every other shavar-using feature already has a pref to toggle it, so adding a new one is probably the simplest thing.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

The infobar is fully disabled right now, by the pref plugins.show_infobar

So maybe we can gate it on !plugins.show_infobar

But I don't see this infobar ever coming back, so actually we should probably remove all of the infobar UI code + flashinfobar list code. But that will be more work.

Comment 8

[François Marier [:francois]]

(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> The infobar is fully disabled right now, by the pref plugins.show_infobar
> 
> So maybe we can gate it on !plugins.show_infobar

Thanks for the extra info Felipe. That sounds like a good plan.

> But I don't see this infobar ever coming back, so actually we should
> probably remove all of the infobar UI code + flashinfobar list code. But
> that will be more work.

I'll create a follow-up bug. Given this I assume we are currently downloading that list unnecessarily?

Kirk are you ok with gating the list download on the above pref and therefore not downloading the list anymore?

Comment 9

[Kirk Steuber (he/him) [:bytesized]]

That idea seems fine to me.

Comment 10

[François Marier [:francois]]

Attachment: Bug 1435098 - Gate flashinfobar list on the plugins.show_infobar.

The list of sites to suppress flash infobars on should not be
downloaded from shavar unless the infobar feature is enabled.

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

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8948039 [details]
Bug 1435098 - Gate flashinfobar list on the plugins.show_infobar.

https://reviewboard.mozilla.org/r/217682/#review223564

::: toolkit/components/url-classifier/SafeBrowsing.jsm:235
(Diff revision 1)
>      this.passwordsEnabled = Services.prefs.getBoolPref("browser.safebrowsing.passwords.enabled");
>      this.trackingEnabled = Services.prefs.getBoolPref("privacy.trackingprotection.enabled") || Services.prefs.getBoolPref("privacy.trackingprotection.pbmode.enabled");
>      this.blockedEnabled = Services.prefs.getBoolPref("browser.safebrowsing.blockedURIs.enabled");
>      this.trackingAnnotations = Services.prefs.getBoolPref("privacy.trackingprotection.annotate_channels");
>      this.flashBlockEnabled = Services.prefs.getBoolPref("plugins.flashBlock.enabled");
> +    this.flashInfobarListEnabled = Services.prefs.getBoolPref("plugins.show_infobar");

plugins.show_infobar seems like bad naming given that it is Flash specific?

Comment 12

[Kirk Steuber (he/him) [:bytesized]]

Comment on attachment 8948039 [details]
Bug 1435098 - Gate flashinfobar list on the plugins.show_infobar.

https://reviewboard.mozilla.org/r/217682/#review223712

This looks good to me.

Comment 13

[François Marier [:francois]]

Comment on attachment 8948039 [details]
Bug 1435098 - Gate flashinfobar list on the plugins.show_infobar.

https://reviewboard.mozilla.org/r/217682/#review223738

::: toolkit/components/url-classifier/SafeBrowsing.jsm:235
(Diff revision 1)
>      this.passwordsEnabled = Services.prefs.getBoolPref("browser.safebrowsing.passwords.enabled");
>      this.trackingEnabled = Services.prefs.getBoolPref("privacy.trackingprotection.enabled") || Services.prefs.getBoolPref("privacy.trackingprotection.pbmode.enabled");
>      this.blockedEnabled = Services.prefs.getBoolPref("browser.safebrowsing.blockedURIs.enabled");
>      this.trackingAnnotations = Services.prefs.getBoolPref("privacy.trackingprotection.annotate_channels");
>      this.flashBlockEnabled = Services.prefs.getBoolPref("plugins.flashBlock.enabled");
> +    this.flashInfobarListEnabled = Services.prefs.getBoolPref("plugins.show_infobar");

Yeah it would have been nice to get "flash" in the name, but this is not a new pref I'm introducing, it's the existing pref that controls the infobar feature.

Probably not worth changing since there is talk of removing the infobars entirely later.

::: toolkit/components/url-classifier/SafeBrowsing.jsm:235
(Diff revision 1)
>      this.passwordsEnabled = Services.prefs.getBoolPref("browser.safebrowsing.passwords.enabled");
>      this.trackingEnabled = Services.prefs.getBoolPref("privacy.trackingprotection.enabled") || Services.prefs.getBoolPref("privacy.trackingprotection.pbmode.enabled");
>      this.blockedEnabled = Services.prefs.getBoolPref("browser.safebrowsing.blockedURIs.enabled");
>      this.trackingAnnotations = Services.prefs.getBoolPref("privacy.trackingprotection.annotate_channels");
>      this.flashBlockEnabled = Services.prefs.getBoolPref("plugins.flashBlock.enabled");
> +    this.flashInfobarListEnabled = Services.prefs.getBoolPref("plugins.show_infobar");

Yeah it would have been nice to get "flash" in the name, but this is not a new pref I'm introducing, it's the existing pref that controls the infobar feature.

Probably not worth changing since there is talk of removing the infobars entirely later.

Comment 14

[François Marier [:francois]]

(In reply to François Marier [:francois] from comment #13)
> Yeah it would have been nice to get "flash" in the name, but this is not a
> new pref I'm introducing, it's the existing pref that controls the infobar
> feature.
> 
> Probably not worth changing since there is talk of removing the infobars
> entirely later.

GCP, do you agree with the above?

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8948039 [details]
Bug 1435098 - Gate flashinfobar list on the plugins.show_infobar.

https://reviewboard.mozilla.org/r/217682/#review223940

No point in cleaning up the unfortunate pref naming in this patch.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

>GCP, do you agree with the above?

Sure, if it's not a new preference it's clearly out of scope to fix this now.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3884f0f9f316
Gate flashinfobar list on the plugins.show_infobar. r=bytesized,gcp

Comment 18

[Narcis Beleuzu [:NarcisB]]

Backed out for xpcshell failures on /test_bug1274685_unowned_list.js

Push: https://hg.mozilla.org/integration/autoland/rev/3884f0f9f316613ba0e60845e40f166c58b20d3b
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=160632449&repo=autoland&lineNumber=8067

Comment 19

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d62e9bfeac1c
Backed out changeset 3884f0f9f316 for xpcshell failures on /test_bug1274685_unowned_list.js. CLOSED TREE

Comment 20

[François Marier [:francois]]

Comment on attachment 8948039 [details]
Bug 1435098 - Gate flashinfobar list on the plugins.show_infobar.

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

Comment 21

[François Marier [:francois]]

(In reply to Narcis Beleuzu [:NarcisB] from comment #18)
> Backed out for xpcshell failures on /test_bug1274685_unowned_list.js

This is the problematic line:

NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
readPrefs@resource://gre/modules/SafeBrowsing.jsm:235:36
init@resource://gre/modules/SafeBrowsing.jsm:71:5
run_test@/home/francois/devel/mozilla-unified/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/url-classifier/tests/unit/test_bug1274685_unowned_list.js:10:3
_execute_test@/home/francois/devel/mozilla-unified/testing/xpcshell/head.js:540:7
@-e:1:1

The problem was that the plugins.show_infobar pref introduced in bug 1377036
is only defined in firefox.js:
https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/browser/app/profile/firefox.js#706

It needs to exist everywhere if we want to be able to gate the list on it.

Comment 22

[François Marier [:francois]]

Kirk, does my fix work for you given what I found in comment 21?

https://reviewboard.mozilla.org/r/217682/diff/1-2/

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Alternatively, the get*Pref functions now support a second parameter to define a default value in case it exists. That would avoid having to set that pref in all.js

Comment 24

[Kirk Steuber (he/him) [:bytesized]]

lgtm

Comment 25

[François Marier [:francois]]

Comment on attachment 8948039 [details]
Bug 1435098 - Gate flashinfobar list on the plugins.show_infobar.

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

Comment 26

[François Marier [:francois]]

I went with Felipe's solution from comment 23 since it's much simpler: https://reviewboard.mozilla.org/r/217682/diff/1-3/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40f74605367e
Gate flashinfobar list on the plugins.show_infobar. r=bytesized,gcp

Comment 28

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/40f74605367e

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 30

[François Marier [:francois]]

Comment on attachment 8948039 [details]
Bug 1435098 - Gate flashinfobar list on the plugins.show_infobar.

Approval Request Comment
[Feature/Bug causing the regression]: 8948039
[User impact if declined]: Unnecessary list downloaded to user's browsers.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, manually.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not much.
[Why is the change risky/not risky?]: Very small patch, just querying an extra pref.
[String changes made/needed]: None

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8948039 [details]
Bug 1435098 - Gate flashinfobar list on the plugins.show_infobar.

Taking for 59b8.

Comment 32

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/536f8eeb6485

Comment 33

[Andrei Vaida [:avaida]]

(In reply to François Marier [:francois] from comment #30)
> [Is this code covered by automated tests?]: No
> [Has the fix been verified in Nightly?]: Yes, manually.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on François's assessment on manual testing needs.