Tippytop does not honor preference browser.chrome.site_icons

RESOLVED FIXED in Firefox 59

Status

()

Firefox
Activity Streams: Newtab
P1
normal
RESOLVED FIXED
a month ago
12 days ago

People

(Reporter: Underpass, Assigned: Mardak)

Tracking

(Blocks: 1 bug, {regression})

58 Branch
Firefox 59
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58+ wontfix, firefox59 fixed)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

(Reporter)

Description

a month ago
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171204150510

Steps to reproduce:

Hello, I keep the preference browser.chrome.favicons  set to false in about:config and until version 58b8 no favicon was shown in the bookmarks toolbar

This is what I found updating Firefox from 58b8 to 58b9:




Actual results:

- Updated to Firefox 58b9: since then, I see some favicons in the bookmark toolbar
- Tried to empty browser cache = no effect
- Tried to close Firefox and delete favicons.sqlite from profile folder = icons are re-created upon restart
- Tried to delete favicons.sqlite and downgrade Firefox to version 58b8 = favicons are no more present and the preference is honored
- Tried to upgrade Firefox again to 58b9 = favicons are shown again


Expected results:

Like in previous version, I expect that the preference browser.chrome.favicons is honored and no favicon is shown in the bookmarks toolbar.

Thanks for your attention

Updated

a month ago
Component: Untriaged → Bookmarks & History
Maybe bug 1421306 is storing some favicons. I can't find a good list of changesets between b8 and b9 yet and I can't think of icon patches we recently uplifted.
Mardak, do you think that's a possiblity?
Component: Bookmarks & History → Activity Streams: Newtab
Flags: needinfo?(edilee)
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID 	20171130160223

I've tried to reproduce this issue on Windows 10 and 7 using 58.0b7, 58.0b8 and 58.0b9. And in all of these builds I can't make the favicons from the browser toolbar disappear using the "browser.chrome.favicons" pref. They always show up, so there might have been some extra options/prefs that were changed.

@Underpass, could you try to reproduce the issue on 58.0b8 using a new profile and post your results?
Flags: needinfo?(underpass_bugzilla)
bug 1421306 got uplifted to 58 and handles favicons.
(Reporter)

Comment 4

a month ago
(In reply to Ciprian Muresan [:cmuresan], Desktop Engineering QA from comment #2)
> User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0)
> Gecko/20100101 Firefox/58.0
> Build ID 	20171130160223
> 
> I've tried to reproduce this issue on Windows 10 and 7 using 58.0b7, 58.0b8
> and 58.0b9. And in all of these builds I can't make the favicons from the
> browser toolbar disappear using the "browser.chrome.favicons" pref. They
> always show up, so there might have been some extra options/prefs that were
> changed.
> 
> @Underpass, could you try to reproduce the issue on 58.0b8 using a new
> profile and post your results?

Hello, sorry then probably the preference is browser.chrome.site_icons. I managed to make them disappear setting the preference to false, closing Firefox and deleting the favicons.sqlite file.
Flags: needinfo?(underpass_bugzilla)
Created attachment 8934978 [details]
IMG_06122017_163236_0.png

I've set browser.chrome.site_icons and browser.chrome.favicons to false, deleted the favicons.sqlite file from my profile, opened the browser, navigated to a few websites and my tabs no longer had favicons, but the bookmarks that I've made still had a "generic" favicon. 
I've then updated to 58.0b9 and I still don't have favicons in tabs and the generic ones persisted in the bookmarks toolbar.

Were the generic favicons supposed to not be displayed as well?
Flags: needinfo?(underpass_bugzilla)
(Reporter)

Comment 6

a month ago
(In reply to Ciprian Muresan [:cmuresan], Desktop Engineering QA from comment #5)
 
> Were the generic favicons supposed to not be displayed as well?

No, the generic favicon (the globe) should be present.
Flags: needinfo?(underpass_bugzilla)
(Assignee)

Comment 7

a month ago
Bug 1421306 did introduce a new way that favicons get stored, and I see that ContentLinkHandler does skip adding an icon if `.site_icons` is `false`:

https://searchfox.org/mozilla-central/source/browser/modules/ContentLinkHandler.jsm#336

Bug 1421306 did not have logic checking for that pref, so there could indeed be a bug assuming we rely on all potential sources to *not* add favicons based on the pref vs additionally having consumers check the pref to *not* show site icons. (Where the latter means once favicons get stored, they will be shown independent of the pref.)
Flags: needinfo?(edilee)
Keywords: regression
Summary: Upgrading to 58b9 the preference browser.chrome.favicons is no more honored → Upgrading to 58b9 the preference browser.chrome.site_icons is no more honored
(Assignee)

Comment 8

a month ago
Underpass, does setting browser.newtabpage.activity-stream.tippyTop.service.endpoint to empty "" string and clearing favicons.sqlite avoid the favicons from showing up?
Flags: needinfo?(underpass_bugzilla)
(Assignee)

Comment 9

a month ago
Oh, and when you delete `favicons.sqlite`, also delete `activity-stream.tippytop.json`
(Assignee)

Updated

a month ago
Blocks: 1421306
status-firefox58: --- → affected
status-firefox59: --- → affected
(Assignee)

Updated

a month ago
See Also: → bug 1423668
(Reporter)

Comment 10

a month ago
Hello, apparently, when you set browser.newtabpage.activity-stream.tippyTop.service.endpoint to empty and delete the files, the icons aren't created anymore.
Flags: needinfo?(underpass_bugzilla)
(Assignee)

Comment 11

a month ago
mak, any thoughts on fixing this to uplift to 58? Necessary for this about:config pref? Approach of checking pref to not add vs checking pref to not show? Or maybe whether to keep maintaining the pref?
Flags: needinfo?(mak77)
(Assignee)

Updated

a month ago
Summary: Upgrading to 58b9 the preference browser.chrome.site_icons is no more honored → Tippytop does not honor preference browser.chrome.site_icons
(In reply to Ed Lee :Mardak from comment #11)
> mak, any thoughts on fixing this to uplift to 58? Necessary for this
> about:config pref? Approach of checking pref to not add vs checking pref to
> not show? Or maybe whether to keep maintaining the pref?

I'm not sure I understand the question, but since it's a regression I think we should uplift it to 58 if possible.

Note the full check is ShouldLoadFavicon, that checks both prefs and the scheme, it seems to make sense to me, maybe it should be shared more broadly, or even done directly by Places?
https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/browser/base/content/tabbrowser.xml#1057
https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/browser/base/content/tabbrowser.xml#1093

The prefs situations looks as bad as it could be.
I cannot understand the difference between browser.chrome.site_icons and browser.chrome.favicons, why do we have 2?
Imo browser.chrome.favicons should be migrated to browser.chrome.site_icons and removed.
Flags: needinfo?(mak77)

Comment 14

a month ago
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/f0e1304ce05e16a8a0592f029b7d9462d5a43222
fix(favicons): Check endpoint and site_icons prefs before fetching icons (#3898)

Fix Bug 1423506 - Tippytop does not honor preference browser.chrome.site_icons

Updated

a month ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
(Assignee)

Updated

a month ago
Assignee: nobody → edilee
Iteration: --- → 1.25
Priority: -- → P1
(Assignee)

Updated

a month ago
Blocks: 1423993
(Assignee)

Updated

a month ago
See Also: → bug 1421902
(Assignee)

Updated

a month ago
Blocks: 1418130
(Assignee)

Comment 15

a month ago
[Tracking Requested - why for this release]: This is a regression in 58 from bug 1421306 for an about:config pref
tracking-firefox58: --- → ?
Comment hidden (mozreview-request)

Comment 17

a month ago
mozreview-review
Comment on attachment 8935881 [details]
Bug 1423506 - Tippytop does not honor preference browser.chrome.site_icons.

https://reviewboard.mozilla.org/r/206744/#review212558

Looks good
Attachment #8935881 - Flags: review?(khudson) → review+

Comment 18

a month ago
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b41233cc2b97
Tippytop does not honor preference browser.chrome.site_icons. r=k88hudson
(Assignee)

Updated

a month ago
status-firefox59: affected → fixed
Target Milestone: --- → Firefox 59
Please request Beta approval on this when you get a chance.
status-firefox57: --- → unaffected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(edilee)
(Assignee)

Comment 21

a month ago
Can somebody verify the fix on Nightly? Most likely will need to set site_icons pref to false and also delete favicons.sqlite
(In reply to Ed Lee :Mardak from comment #21)
> Can somebody verify the fix on Nightly? Most likely will need to set
> site_icons pref to false and also delete favicons.sqlite

Underpass, can you verify the fix?
Flags: needinfo?(underpass_bugzilla)
tracking-firefox58: ? → +
(Reporter)

Comment 23

a month ago
Hello, I've updated Firefox to the last beta, reset the "Tippytop" preference, set the browser.chrome.site_icons preference to false and deleted the favicons.sqlite.
Upon restarting the problems seems to be solved. Thanks a lot.

(In reply to Marco Bonardo [::mak] (Away 16 Dec - 2 Jan) from comment #13)
> I cannot understand the difference between browser.chrome.site_icons and
> browser.chrome.favicons, why do we have 2?
> Imo browser.chrome.favicons should be migrated to browser.chrome.site_icons
> and removed.

About this, I've found these two pages on the MozillaZine KB

http://kb.mozillazine.org/Browser.chrome.favicons
http://kb.mozillazine.org/Browser.chrome.site_icons
Flags: needinfo?(underpass_bugzilla)
(Assignee)

Comment 24

a month ago
The fix hasn't actually made it to beta. Could you try restarting another time to see if it causes the icons to appear, i.e., run into this bug. (I believe your prefs and empty favicons are in the correct state, so no need to change / delete them, and just restart.)
Flags: needinfo?(underpass_bugzilla)
(Reporter)

Comment 25

a month ago
Hello, I've restarted the browser and the icons don't appear (saved a couple of bookmarks to see them).

Then I picked the favicons.sqlite file and opened it with the SQLite browser and didn't find any reference to these bookmarked pages.
Flags: needinfo?(underpass_bugzilla)
(Assignee)

Comment 26

a month ago
From comment 23, if this is no longer a problem on latest beta, perhaps there's no more that needs to be done here for 58?
Flags: needinfo?(edilee)
We don't have a worksforme status flag, so I'm calling Fx58 wontfix based on the recent comments in the bug (because we're not intending to uplift any fixes to Beta at this point).
status-firefox58: affected → wontfix
You need to log in before you can comment on or make changes to this bug.