Closed
Bug 1435098
Opened 7 years ago
Closed 7 years ago
The flashinfobar list is not gated by any of the existing feature prefs
Categories
(Toolkit :: Safe Browsing, defect, P1)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox58 | --- | wontfix |
firefox59 | --- | fixed |
firefox60 | --- | fixed |
People
(Reporter: francois, Assigned: francois)
References
Details
(Keywords: regression)
Attachments
(1 file)
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
Assignee | ||
Comment 1•7 years ago
|
||
Kirk, are there any concerns with gating this functionality on the flashblock.enabled pref?
Flags: needinfo?(ksteuber)
Assignee | ||
Comment 2•7 years ago
|
||
The work-around to disable all flash-related lists is to set both of these prefs:
plugins.flashBlock.enabled = false
urlclassifier.flashInfobarTable = ""
Comment 3•7 years ago
|
||
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)
Assignee | ||
Comment 4•7 years ago
|
||
(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)
Comment 5•7 years ago
|
||
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)
Assignee | ||
Comment 6•7 years ago
|
||
(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
Assignee | ||
Updated•7 years ago
|
Summary: The flashinfobar list is not gated by plugins.flashBlock.enabled → The flashinfobar list is not gated by any of the existing feature prefs
Comment 7•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
(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)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → francois
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
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 12•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-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.
Assignee | ||
Comment 14•7 years ago
|
||
(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 15•7 years ago
|
||
mozreview-review |
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+
Comment 16•7 years ago
|
||
>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)
Comment 17•7 years ago
|
||
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3884f0f9f316
Gate flashinfobar list on the plugins.show_infobar. r=bytesized,gcp
Comment 18•7 years ago
|
||
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
Flags: needinfo?(francois)
Comment 19•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
(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)
Assignee | ||
Comment 22•7 years ago
|
||
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)
Comment 23•7 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 26•7 years ago
|
||
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•7 years ago
|
||
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40f74605367e
Gate flashinfobar list on the plugins.show_infobar. r=bytesized,gcp
Comment 28•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•7 years ago
|
Comment 29•7 years ago
|
||
Please request Beta approval on this when you get a chance.
Flags: needinfo?(francois)
Assignee | ||
Comment 30•7 years ago
|
||
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 31•7 years ago
|
||
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+
Comment 32•7 years ago
|
||
bugherder uplift |
Comment 33•7 years ago
|
||
(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.
Description
•