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

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Underpass]

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

Comment 1

[Marco Bonardo [:mak]]

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?

Comment 2

[Ciprian Muresan [:cmuresan], Ecosystem QA]

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?

Comment 3

[Andrei Oprea [:andreio]]

bug 1421306 got uplifted to 58 and handles favicons.

Comment 4

[Underpass]

(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.

Comment 5

[Ciprian Muresan [:cmuresan], Ecosystem QA]

Attachment: 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?

Comment 6

[Underpass]

(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.

Comment 7

[Ed Lee :Mardak]

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.)

Comment 8

[Ed Lee :Mardak]

Underpass, does setting browser.newtabpage.activity-stream.tippyTop.service.endpoint to empty "" string and clearing favicons.sqlite avoid the favicons from showing up?

Comment 9

[Ed Lee :Mardak]

Oh, and when you delete `favicons.sqlite`, also delete `activity-stream.tippytop.json`

Comment 10

[Underpass]

Hello, apparently, when you set browser.newtabpage.activity-stream.tippyTop.service.endpoint to empty and delete the files, the icons aren't created anymore.

Comment 11

[Ed Lee :Mardak]

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?

Comment 12

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/3898

Comment 13

[Marco Bonardo [:mak]]

(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.

Comment 14

[[github robot]]

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

Comment 15

[Ed Lee :Mardak]

[Tracking Requested - why for this release]: This is a regression in 58 from bug 1421306 for an about:config pref

Comment 16

[Ed Lee :Mardak]

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

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

Comment 17

[Kate Hudson :k88hudson]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b41233cc2b97
Tippytop does not honor preference browser.chrome.site_icons. r=k88hudson

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b41233cc2b97

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Ed Lee :Mardak]

Can somebody verify the fix on Nightly? Most likely will need to set site_icons pref to false and also delete favicons.sqlite

Comment 22

[Julien Cristau [:jcristau]]

(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?

Comment 23

[Underpass]

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

Comment 24

[Ed Lee :Mardak]

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.)

Comment 25

[Underpass]

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.

Comment 26

[Ed Lee :Mardak]

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?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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).