Closed Bug 1435098 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Safe Browsing, defect, P1)

Product:
Toolkit
Component:
Safe Browsing
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: francois, Assigned: francois)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1435098 - Gate flashinfobar list on the plugins.show_infobar.
59 bytes, text/x-review-board-request
bytesized
: review+
gcp
: review+
ryanvm
: approval-mozilla-beta+
Details 
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
Kirk, are there any concerns with gating this functionality on the flashblock.enabled pref?
Flags: needinfo?(ksteuber)
The work-around to disable all flash-related lists is to set both of these prefs:

plugins.flashBlock.enabled = false
urlclassifier.flashInfobarTable = ""
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?
Flags: needinfo?(ksteuber)
(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.
Flags: needinfo?(ksteuber)
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.
Flags: needinfo?(ksteuber)
(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.
status-firefox58: --- → affected
status-firefox59: --- → affected
status-firefox60: --- → affected
status-firefox-esr52: --- → unaffected
Summary: The flashinfobar list is not gated by plugins.flashBlock.enabled → The flashinfobar list is not gated by any of the existing feature prefs
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.
(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?
Flags: needinfo?(ksteuber)
That idea seems fine to me.
Flags: needinfo?(ksteuber)
Assignee: nobody → francois
Status: NEW → ASSIGNED
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?
Attachment #8948039 - Flags: review?(gpascutto)
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.
Attachment #8948039 - Flags: review?(ksteuber) → review+
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.
(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?
Flags: needinfo?(gpascutto)
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.
Attachment #8948039 - Flags: review+
>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.
Flags: needinfo?(gpascutto)
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3884f0f9f316
Gate flashinfobar list on the plugins.show_infobar. r=bytesized,gcp
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d62e9bfeac1c
Backed out changeset 3884f0f9f316 for xpcshell failures on /test_bug1274685_unowned_list.js. CLOSED TREE
(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.
Flags: needinfo?(francois)
Kirk, does my fix work for you given what I found in comment 21?

https://reviewboard.mozilla.org/r/217682/diff/1-2/
Flags: needinfo?(ksteuber)
See Also: → 1436213
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
lgtm
Flags: needinfo?(ksteuber)
I went with Felipe's solution from comment 23 since it's much simpler: https://reviewboard.mozilla.org/r/217682/diff/1-3/
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40f74605367e
Gate flashinfobar list on the plugins.show_infobar. r=bytesized,gcp
https://hg.mozilla.org/mozilla-central/rev/40f74605367e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Please request Beta approval on this when you get a chance.
Flags: needinfo?(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
Flags: needinfo?(francois)
Attachment #8948039 - Flags: approval-mozilla-beta?
Comment on attachment 8948039 [details]
Bug 1435098 - Gate flashinfobar list on the plugins.show_infobar.

Taking for 59b8.
Attachment #8948039 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.