1782854 - favicons.sqlite grows in size with no limit

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P2)

Description

[miker]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:103.0) Gecko/20100101 Firefox/103.0

Steps to reproduce:

I use Apache Guacamole for remote work. One of its features is to set the site's favicon to a tiny representation of the remote desktop, which updates every few seconds.

Actual results:

Over time, Firefox's favicons.sqlite database grows extremely large. After a couple of months, it's over 5GB, and causing noticeable slowdowns. This has been an issue with every version of Firefox I've used over the last couple of years (not a regression in v103).

I inquired with the Guacamole project, and they suggested filing a bug report here since it's not a bug per-se in Guacamole.

Expected results:

Firefox should limit the favicons.sqlite database to a reasonable (perhaps even user-configurable) size.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Layout' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Marco Bonardo [:mak]]

We had a similar report recently where an add-on was changing the favicon continuously and we were storing it.
Now, the server could just set the no-store header and we'd not store it... but most don't do that unfortunately.
We have code in place to ignore any favicon set after the pageshow event, that should prevent this from happening, but for some reason it doesn't...

I will try reproducing this with Apache Guacamole and see why it'd bypass our checks.

Comment 3

[Marco Bonardo [:mak]]

Ok, I followed a long procedure to install guacamole and I think I found the problem, these pages, similarly to the previous add-on case, set favicon using data uris.
When that happens the code takes a different path than network loaded images, and FaviconLoader assumes it can always load the favicon:
https://searchfox.org/mozilla-central/rev/9cd1e8cabf67ef5a47e95d70b7f40c9d3ad02ad0/browser/modules/FaviconLoader.jsm#545,563

So our beforePageShow check doesn't apply to this case https://searchfox.org/mozilla-central/rev/9cd1e8cabf67ef5a47e95d70b7f40c9d3ad02ad0/browser/modules/FaviconLoader.jsm#258

I'm setting this as an S2 because as a consequence one can easily hit a case where favicons.sqlite grows to multiple GB and that will definitely cause performance problems overall. I'm also assigning to myself.
I'm not sure there's an easy way to fix existing dbs that may be affected, though the icons should be bound to the page who loaded them and expired with it. It may be worth filing a follow-up to maybe check with Places Maintenance if a page has too many icons stored and expire the ones older than N days.

Comment 4

[Marco Bonardo [:mak]]

Attachment: Bug 1782854 - Don't store local uri favicons after pageShow. r=Mossop

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64f9bfbf800e
Don't store local uri favicons after pageShow. r=mossop

Comment 6

[Narcis Beleuzu [:NarcisB]]

Backed out for dt failure on browser_aboutdebugging_tab_favicons.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/030211be8d315e5baad478ca468b1fdda486f065
Log link: https://treeherder.mozilla.org/logviewer?job_id=388094087&repo=autoland&lineNumber=6875

Please also check this bc failure on browser_bug420605.js - https://treeherder.mozilla.org/logviewer?job_id=388089682&repo=autoland&lineNumber=9939

Comment 7

[Marco Bonardo [:mak]]

thanks, I used this.beforePageShow, but that's defined in another class, I should have used iconInfo.beforePageShow.
We don't have an explicit test about storing data uri icons thus I'll add one.

Comment 8

[Marco Bonardo [:mak]]

Attachment: Bug 1782854 - Don't store local uri favicons after pageShow. r=Mossop

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78fc9399cf70
Don't store local uri favicons after pageShow. r=mossop

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/78fc9399cf70