480873 - Favicons missing from the places UI after cache has been cleared

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dão Gottwald [:dao]]

As the favicons are expired when the Web cache is cleared, they should be re-fetched actively when they are expected to be displayed. Otherwise the favicon "cache" is more than just a cache and shouldn't be cleared together with the Web cache.

Comment 1

[John]

So as a result of bug 476636, are the favicons actually lost or just not being displayed?

Also, clearing the cache OR offline data causes blank favicons on bookmarks. Should a pref be added to clear favicon data/ will they be stored elsewhere?

Comment 2

[Michael Kohler [:mkohler]]

thanks for CCing Noah.

IMO load them when they need to be displayed is a good idea. Store them elsewhere (places.sqlite?) would be a good alternative.

We could load them 1. all at once when starting Firefox or 2. just what we need to display correctly (that would mean that favicons of the bookmarks toolbar would be loaded at startup, all other when opening the bookmarks menu the first time).

This is a huge usability regression for users who want to clear their chache at FF shutdown.

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #1)
> So as a result of bug 476636, are the favicons actually lost or just not being
> displayed?

They aren't on your computer anymore. They are likely still on the corresponding server, so they aren't lost.

Comment 4

[Michael Kohler [:mkohler]]

at the moment the only solution is to load every bookmark new so the favicon gets cached till the user closes firefox again.

(or just uncheck the delete cache when closing firefox box, but this solution is neither comfortable nor what the most the most user want)

maybe I'll write a script that loads every bookmark new when starting minefield, but that's not good for performance..

Comment 5

[Ria Klaassen (not reading all bugmail)]

Also custom icons (once manually created e.g. with Favicon Picker) on bookmarklets and javascript URLs are gone. When they will be re-fetched the wrong icons will be bound to the bookmarks.

Comment 6

[Marco Bonardo [:mak]]

just to clarify, the behavior is expected, and the idea of that changed in bug 460300 (bug 476636 is an additional fix to that since was originally not working). The idea is exactly that if you clear the cache you want to get rid of all known files and images.
If that ends up being more annoying than needed we could rethink such a behavior clearly, we'll discuss about this.

Comment 7

[Dão Gottwald [:dao]]

Although it's expected based on the code, you can tell by the comments in bug 476636 that users don't expect this. Throwing away the memory without loading the images when they are expected to be displayed doesn't even fit the definition of the word "cache". Either it should be refilled as needed, making it a proper cache, or it shouldn't be cleared in the first place, making it a proper primary memory.

Comment 8

[Wesley Johnston (:wesj)]

"The idea is exactly that if you clear the cache you want to get rid of all known files and images."

Bookmarks aren't considered part of that though, right? I don't think anyone would care if history favicons were gone (or notice, since their history is probably gone too). Its just the bookmarked ones that need to be preserved.

Comment 9

[Michael Kohler [:mkohler]]

so why not just making an own checkbox for deleting favicons of the bookmarks?

Comment 10

[John]

^Store the favicons in places.sqlite and have another GUI checkbox to clear them also

Comment 11

[IU]

My 2 cents (for whatever it's worth); I agree with the stated notions of (both):

1. Having a separate favicons cache; and
2. Having a separate "checkbox for deleting favicons"

However, I do not believe the idea of fetching favicons remotely, when they need to be displayed, is a good idea, because it's going to cause a performance hit (especially with slow or intermittent connections) and impact UI responsiveness.

John sums up the best solution nicely, though whether places.sqlite is the best store I am not qualified to judge. :-)

Comment 12

[u88484]

Having a seperate checkbox for favicons would be just clutter the UI and confuse end-users.  I would bet most people have no clue or care what a favicon is and having a checkbox there for them would make them scratch their heads.

Comment 13

[Marco Bonardo [:mak]]

we discussed about this in our meeting, we are aware of the problem and on the causes, situation is quite clear actually, we don't need further reports or guesses on the issue, thanks.
We have a couple possible solutions in mind, and will investigate on them.

Comment 14

[Marco Bonardo [:mak]]

clear cache cases i found so far:
- with sanitize (cpd)
- removeDatafromDomain
- clear offline cache
- add-ons that clear cache

Comment 15

[Benjamin Xiao]

Is clearing icons supposed to help privacy in anyway? I don't think so. I would think that the text reveals more than the icon itself. Its a useless tactic. It is not a correct way of thinking, no matter how you put it. Bookmarks are not cleared for a reason. The icons should be considered part of bookmarks.

Comment 16

[Dão Gottwald [:dao]]

Shawn, Benjamin made a good point that hasn't been mentioned before...

Comment 17

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Asking a first look at this to evaluate the approach and the hook in browser code.

Includes a dedicated browser-chrome test, it is slow due to lazy timer but there isn't a way around that.

i don't think we can go the road of "invalidate only non bookmarked entries", mainly due to removeDataForDomain (i clear a domain and all my history favicons are wiped), and add-ons (that could do the same removeDataForDomain does, since they can't remove only part of the cache, they'll remove everything).
So unless clearing cache does not support selective clears (that we can listen on), current approach is simply too boring for the end user.

Original reason to clear with cache was to allow seeing an updated favicon for a site after cache is manually cleared, this would reach the same scope requiring a slightly different action from the user, a forced reload (usually that's shift+reload), and acting only on the current page favicon.

Comment 18

[Marco Bonardo [:mak]]

Attachment: patch v1.0

ops, i attached the wrong patch

Comment 19

[John P Baker]

Just to clarify. This will revert to only bulk clearing icons - through EraseFavicons - if there are no history or bookmark items that currently refer to them?

Looks like a good approach to me - it is much the same as I had to take in my extension. [and it saves me filing the bug, that we mentioned on IRC a week or two ago, to allow null as an argument to setFaviconUrlForPage :-]

>  // Expire favicon for current uri, so we will try to load a new one.
>  PlacesUtils.favicons.setFaviconUrlForPage(getWebNavigation().currentURI, null);

I am wondering, but can't decide, if this needs a check against the browser.chrome.site_icons preference here.

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #17)
> i don't think we can go the road of "invalidate only non bookmarked entries"

If you did, I guess my next bug report would be "Favicons missing from the urlbar autocomplete popup after cache has been cleared" :)
Certainly less annoying than with the bookmarks, but still slightly unexpected, depending on a user's intention when clearing the cache.

Comment 21

[Marco Bonardo [:mak]]

(In reply to comment #19)
> Just to clarify. This will revert to only bulk clearing icons - through
> EraseFavicons - if there are no history or bookmark items that currently refer
> to them?

yes, plus timed expiration for each icon, same as before the cache change.

> I am wondering, but can't decide, if this needs a check against the
> browser.chrome.site_icons preference here.

Well, i don't think it applies, since the pref could have been changed later, and this could still unset an old icon. On the other side would save a select query. So both options are probably worth considering.

Comment 22

[John P Baker]

(In reply to comment #21)
> (In reply to comment #19)
> > I am wondering, but can't decide, if this needs a check against the
> > browser.chrome.site_icons preference here.
> 
> Well, i don't think it applies, since the pref could have been changed later,
> and this could still unset an old icon. On the other side would save a select
> query. So both options are probably worth considering.

I was trying to consider the case when the icon has been set by someone else - probably an extension - this code will clear it but, maybe, it won't be set again by loading the page.

In any case I don't now think that that line of code is useful - esp. for bug 418144.
a) There wasn't data for an icon but is now - favicon.ico will still be in the failed icon cache (still needs restart).
b) The data for the icon at the URL has changed - if something else shares the icon then the place will re-link to the existing entry which still won't refresh until 'expiration' (usually reasonable as we are now using supplied Expires header).
c) The URL of the favicon has changed - this case already works.
d) There was an icon but it has gone away - OK it will now handle this, but it is an expensive solution for just this case.

Comment 23

[Marco Bonardo [:mak]]

we could handle a) removing the icon from the failed favicon cache as well in expireFaviconForPage.
we could handle b) setting the expiration time of the icon to now.

Comment 24

[Marco Bonardo [:mak]]

Attachment: patch v1.1

i don't want to mixup sync and async in the same method, so i added a new expireFaviconForPage method.

This should handle all known cases, for custom icons i'm using an annotation (actually "Favicons/isCustomIcon", every add-on that allows to set a custom icon on a page should add this anno to the page, so we won't unlink nor expire the icon.
ExpireFavicon will unlink the icon from the page (done async, no ui locking), remove it from failed cache and expire it (sets expiration to now, so next request will reload it).

Comment 25

[Marco Bonardo [:mak]]

oh i removed the browser-chrome test because was too slow (lazyness), and we should have tested at lest 4 or 5 cases... we could introduce a test later if me move away from lazy_add.

Comment 26

[Ria Klaassen (not reading all bugmail)]

So you can't manually change icons anymore?
The simplest way is to go to a site with a nice icon, bookmark it, right click > Properties and change name and address. This is is easiest way to color bookmarklets.

Comment 27

[Ria Klaassen (not reading all bugmail)]

Ok, I see that this only worked in bookmarks.html. You can change icons in Firefox 2 and import the bookmarks.html and then they stick. Forgot about that.

Comment 28

[John P Baker]

This is still wrong for both (a) and (b) in comment #22; The suggestions in comment #23 are bogus.

In case (a) the favicon_id will already be null so you have no idea which entry to remove from the failed favicon cache.
In case (b) you are expiring the favicon that the page had last time rather than the one that it should have after the reload - admittedly the chance of the URL changing is quite low but then so is the chance of the previous ones content changing. Also, even if you force an 'expiry', you will probably still fetch the icon from the browser cache so it still won't be updated from the site. Although, as I said, it doesn't seem unreasonable to not handle this case at all as we now honour the expiration sent with the response.

Comment 29

[Marco Bonardo [:mak]]

(In reply to comment #28)
> In case (a) the favicon_id will already be null so you have no idea which entry
> to remove from the failed favicon cache.

Have you read the patch? I catch the favicon BEFORE nulling it.

> In case (b) you are expiring the favicon that the page had last time rather
> than the one that it should have after the reload - admittedly the chance of
> the URL changing is quite low but then so is the chance of the previous ones
> content changing. Also, even if you force an 'expiry', you will probably still
> fetch the icon from the browser cache so it still won't be updated from the
> site. Although, as I said, it doesn't seem unreasonable to not handle this case
> at all as we now honour the expiration sent with the response.

There's no point in expiring the future favicon it will catch, and the user could still hard refresh it again in case this rare case would happen.

Comment 30

[John P Baker]

(In reply to comment #29)
> (In reply to comment #28)
> > In case (a) the favicon_id will already be null so you have no idea which entry
> > to remove from the failed favicon cache.
> 
> Have you read the patch? I catch the favicon BEFORE nulling it.

If the favicon is in the failed icon cache then there is no favicon data, no entry in the moz_favicons table and, hence, nothing for favicon_id to be pointing to.

See early returns from FaviconLoadListener::OnStopRequest and
http://hg.mozilla.org/mozilla-central/file/2d6e148e8122/toolkit/components/places/src/nsFaviconService.cpp#l636

Comment 31

[Marco Bonardo [:mak]]

oh, right, thanks for pointing that

Comment 32

[Dietrich Ayala (:dietrich)]

Comment on attachment 365910 [details] [diff] [review]
patch v1.1

removing review requests, needs to address comment #30.

Comment 33

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Added a ForceReloadIconPages hash, when a page icon is expired we add the page to this hash, and when we ask for the page icon we force reload if the page is in the cache. Unified and fixed some logic with FailedFavicons hash.

Comment 34

[Marco Bonardo [:mak]]

new trybuilds, to test sidebar and Library.
https://build.mozilla.org/tryserver-builds/2009-03-12_09:[e-mail]-try-69d31316094/

Comment 35

[Marco Bonardo [:mak]]

oops, sorry, comment 34 is not related to this bug!

Comment 37

[John P Baker]

(In reply to comment #33)
> Added a ForceReloadIconPages hash, when a page icon is expired we add the page
> to this hash, and when we ask for the page icon we force reload if the page is
> in the cache.

I am not sure that this hash is the correct way to go - it adds unnecessary complexity but still fails in some edge case, in particular with IsFailedFavicon.

I suspect that it would be better to change tabbrowser to remember the  aForceReload state for the current page and to just call setAndLoadFaviconForPage with true when it is set. [The call to isFailedIcon in useDefaultIcon would also need to be suplemented with a check of this flag]

Comment 38

[Marco Bonardo [:mak]]

(In reply to comment #37)
> (In reply to comment #33)
> I am not sure that this hash is the correct way to go - it adds unnecessary
> complexity but still fails in some edge case, in particular with
> IsFailedFavicon.

Not completely fail, a user can hard refresh its icons at request, removing the icon from isFailedFavicon is not something doable atm, unless we remove them all, since we don't know what page a failed favicon was associated to. But force reload removes the new icon from the failed favicon cache (in case icon is the same).

> I suspect that it would be better to change tabbrowser to remember the 
> aForceReload state for the current page and to just call
> setAndLoadFaviconForPage with true when it is set. [The call to isFailedIcon in
> useDefaultIcon would also need to be suplemented with a check of this flag]

that would be doable, more than a property of the tab it is a property of the current page loaded in such a tab, so we should also ensure every time the location of the tab changes that flag is unset.

Btw, remember that the fix is not Firefox only, but should be easily useable by all places implementers, that's why on first site i tried to implement a backend way to easily expire an icon for a single uri and not a browser-only fix.

Comment 39

[John P Baker]

I suspect people are fed up with dogfooding this.

I would recommend that we go with previous patch 365910 (v1.1) which behaves in a well-defined way and fixes _this_ bug and its dependant.

The problems, that we are trying to fix with the extra complex and unpredictable code, are only mild irritations (one case refreshes when icon expires and the other on browser restart) and would be better addressed in a separate bug (probably bug 418144)

Comment 40

[Marco Bonardo [:mak]]

Attachment: patch v2.0

stop clearing with cache and split patch.

Comment 41

[Dietrich Ayala (:dietrich)]

Comment on attachment 370489 [details] [diff] [review]
patch v2.0

r=me, thanks

Comment 42

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/37d76d13964e

Comment 43

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv