346061 - Favicons (site icons) disappear after a few days

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Bookmarks, defect)

Description

[Sven Bergvall]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.4) Gecko/20060613 Camino/1.0.2
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.4) Gecko/20060613 Camino/1.0.2

After not visiting a certain site for a few days the site's favicon disappear. The same is true after quitting the application, but this seems just ranodom. While not a great problem it is annoying as I use favicons only to save space in the bookmark bar.

Reproducible: Always

Steps to Reproduce:
1.Go to a site using favicons
2.Do not visit the site for a few days

Actual Results:  
The favicons disappear, leaving the default bookmark icon

Expected Results:  
The site favicon should remain indefinitely for bookmarks

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This is some sort of caching issue with the site icons; even though we cache them entirely separately, they seem to cache-out after a while.

Simon, is the site icon cache considered part of the main cache where cache size is concerned, and does it page out site icons when the cache needs files paged out?  Or are the two entirely separate?

Comment 2

[froodian (Ian Leue)]

see also bug 332230.

Comment 3

[Simon Fraser [no longer active]]

Site icons expire after 1 week. What we should do is not remove them from the cache until we actually hit the page again and check for a new icon.

Comment 4

[Håkan Waara]

Attachment: Possible patch

This patch simply bubbles up the expiration boolean, so the callees can decide what to do.  A cached favicon won't be removed unless it's updated on the fly.

I haven't tested it, so seeking some tester(s) before I get review.

Comment 5

[Simon Fraser [no longer active]]

I think we need to have something a little more sophisticated.

There are two pieces of data that can expire:
1. The images themselves, which we store on disk via the SiteIconCache
2. A mapping from page URL to icon URL for <link> favicons. This is stored in the Necko cache (code in SiteIconProvider).

Both have default expiries of 1 week.

We need to ensure that both pieces of data persist for longer than they do now, but that if the page is visited, we update them as appropriate.

For the images themselves, we can easily treat expiration as a hint to refetch the icon when visiting the page. For the necko cache, I'm not so sure what the eviction policy is. Someone needs to find out.

Comment 6

[Håkan Waara]

(In reply to comment #5)
> I think we need to have something a little more sophisticated.
> 
> There are two pieces of data that can expire:
> 1. The images themselves, which we store on disk via the SiteIconCache
> 2. A mapping from page URL to icon URL for <link> favicons. This is stored in
> the Necko cache (code in SiteIconProvider).
> 
> Both have default expiries of 1 week.
> 
> We need to ensure that both pieces of data persist for longer than they do now,
> but that if the page is visited, we update them as appropriate.
> 
> For the images themselves, we can easily treat expiration as a hint to refetch
> the icon when visiting the page. For the necko cache, I'm not so sure what the
> eviction policy is. Someone needs to find out.
> 

I think that this patch will fix this bug, in other words, favicons won't be randomly erased.

I haven't changed the behavior of the favicon *URL* cache, but as I see it, in this case it is irrelevant.

Expiring the URL might actually be a good thing, to make sure we try to refetch the image now and then, and see if the path has changed.

Comment 7

[Simon Fraser [no longer active]]

(In reply to comment #6)

> I think that this patch will fix this bug, in other words, favicons won't be
> randomly erased.
> 
> I haven't changed the behavior of the favicon *URL* cache, but as I see it, in
> this case it is irrelevant.

I don't think so. We use the necko cache to map from page URL to icon URL. If the page had a <link> favicon, and that bit of data expires, then you'll start getting the wrong favicon after the necko data expires.

Comment 8

[Stuart Morgan]

Simon, is there any reason not to just keep the favicon URL as part of the bookmark data? Then the bookmark favicon code could just bypass necko. We'd have to updated every time the page is loaded, obviously, but we are already poking bookmarks on pageload anyway.

Comment 9

[Simon Fraser [no longer active]]

(In reply to comment #8)
> Simon, is there any reason not to just keep the favicon URL as part of the
> bookmark data?

Favicons are also shown for history.

> Then the bookmark favicon code could just bypass necko.

But bypassing necko means not taking advantage of the necko caches, proxy settings etc. Its seems like Camino should try to funnel all of its network I/O through Necko.

> We'd
> have to updated every time the page is loaded, obviously, but we are already
> poking bookmarks on pageload anyway.

True. But what might be better is to figure out a way to pin entries in the necko cache if there is a bookmark for them.

Comment 10

[Stuart Morgan]

(In reply to comment #9)
> Favicons are also shown for history.

True, but the bookmark part drives me crazy on a regular basis, whereas I've never missed them on history, and the complaints we get in the forums and on feedback have always been about bookmarks as well.

> But bypassing necko means not taking advantage of the necko caches, proxy
> settings etc. Its seems like Camino should try to funnel all of its network I/O
> through Necko.

I phrased that badly; what I meant was that the bookmark favicon code could cut necko out of the cache lookup codepath. I.e.:
- Bookmark originally gets its favicon the way it does now.
- Bookmark stores the favicon URL.
- On all future lookups, it first goes directly to the cache with the URL it has stored, and if it finds the icon there, it's done.
- If not, the current code takes over again.
So no network access would be happening without necko.

Comment 11

[Simon Fraser [no longer active]]

I used the necko cache for two reasons:
1. it handles expiry for us
2. it's a place to store the page url -> icon url mapping for all sites (not just bookmarks).
I agree that the current expiry policy for bookmarks is wrong, but I'm not sure that the bookmarks file should be bloated with icon urls for everything.

Comment 12

[Stuart Morgan]

I agree the consistency is a win, and I'll definitely look into pinning.

Comment 14

[Chris Lawson (gone)]

Would it be at all useful, as an interim fix, to tie the expiration of site icons to the length of time history is kept? Or even to set it for a longer time period (say, 30 days instead of 7)?

Comment 15

[LH]

I think it would be useful but i mean temporary useful, to set it for a longer
time period (say, 30 days instead of 7); even 45 days would be cool since there are lot of chances to re-visit a website within the next 45 days and then, re-arm the time trigger for deleting favicon.

Comment 16

[Chris Lawson (gone)]

It also appears that local files' icons aren't being cached very long (if at all); see

http://forums.mozillazine.org/viewtopic.php?t=623265

I'm not sure if that's the same bug as this one or a different issue with the same code.

Comment 17

[Stuart Morgan]

The description sounds like a bug specific to the menu code, so probably neither.

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

See also bug 388649 and bug 410143; we don't want to re-introduce either of them when fixing this, either.