319486 - Empty Cache and Reset Camino don't clear site icon cache

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Preferences, defect)

Description

[Chris Lawson (gone)]

Our "Empty Cache..." button in History prefs doesn't clear the recently implemented site icon cache. It probably should.

Comment 1

[Simon Fraser [no longer active]]

Hrm, should it? Does clearing the cache in FF remove the bookmark icons (which are actually stored in the bookmarks file)?

Comment 2

[Josh Aas]

This is basically a vague history of your browsing, even more so if you look at modification dates on the files. Especially since they are not in with the bookmarks, I think it is important to delete them. Precisely since they are *not* part of bookmarks is what makes this a big problem. It is a record of visits to places you did not bookmark.

Comment 3

[Josh Aas]

*** Bug 328143 has been marked as a duplicate of this bug. ***

Comment 4

[Chris Lawson (gone)]

(In reply to comment #1)
> Hrm, should it? Does clearing the cache in FF remove the bookmark icons (which
> are actually stored in the bookmarks file)?

No, it doesn't. IMO, this is a bug in Fx, and I agree with Josh's comment 2.

cl

Comment 5

[Josh Aas]

I'm saying this is *not* a bug in Firefox. If they bookmarked the site, who cares if they keep the favicon around? Bookmarks don't get cleared in a reset of the cache.

Comment 6

[Samuel Sidler (old account; do not CC)]

Requesting approval for Camino 1.0.1.

Comment 7

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

Tweaking summary so we remember to fix both places, and removing "polish" since privacy issues are more than just "polish" ;)

Comment 8

[Håkan Waara]

Attachment: Make us clear the icon cache

Comment 9

[Håkan Waara]

Comment on attachment 214719 [details] [diff] [review]
Make us clear the icon cache

Forgot one thing:  There's also the obvious change to resetBrowser: and emptyCache: in MainController:

+    // clear the icon cache.
+    [[SiteIconProvider sharedFavoriteIconProvider] clearCache];

Comment 10

[Mark Mentovai]

Comment on attachment 214719 [details] [diff] [review]
Make us clear the icon cache

It seems a little evil to dump favicons associated with bookmarks and then have to fetch them again.  How hard would it be to add a key to IconCacheIndex.plist that contains a count of bookmarks that refer to an icon?

Comment 11

[Simon Fraser [no longer active]]

(In reply to comment #10)
> (From update of attachment 214719 [details] [diff] [review] [edit])
> It seems a little evil to dump favicons associated with bookmarks and then have
> to fetch them again.  How hard would it be to add a key to IconCacheIndex.plist
> that contains a count of bookmarks that refer to an icon?

The BookmarksManager keeps a hash of site icon url -> array of bookmarks, so it would be easier to use that.

Comment 12

[Mark Mentovai]

(In reply to comment #11)
> The BookmarksManager keeps a hash of site icon url -> array of bookmarks, so it
> would be easier to use that.

I think it'd be cleaner to have the BM push the data into the cache than to have the cache go poking around in the BM, but if the data's already there and easily usable, why not?

Comment 13

[Håkan Waara]

I'm a little worried that this can significantly slow down this operation, validating a few hundred icons against the bookmarks manager, but I will give it a try.

So I'll implement a hasBookmarkForFavIcon: method on BookmarkManager, and just validate all favicons against that.

Comment 14

[Håkan Waara]

Attachment: Only clear favicons the bm doesn't use

So, I just spent the last three hours of my night debugging a cache regression (thinking I introduced it), coming out of two threads. Turns out Darin already fixed it two days ago.   Oh well, this works fine now. :-)

This patch is much nicer than the last.

Comment 15

[Håkan Waara]

Attachment: Bookmarks part

Sorry for the spam, I'm tired.   Obviously there's this change to bookmark manager as well.

Comment 16

[Simon Fraser [no longer active]]

Comment on attachment 215089 [details] [diff] [review]
Bookmarks part


>+- (BOOL)hasBookmarkForFavicon:(NSString*)inFaviconURI
>+{
>+  return ([mBookmarkFaviconURLMap objectForKey:inFaviconURI] != nil);
>+}

You should test the count. It could be an empty array.

Comment 17

[Mark Mentovai]

Comment on attachment 215088 [details] [diff] [review]
Only clear favicons the bm doesn't use

Patch doesn't apply:

mark@crack bash$ patch --dry-run -p0<~/Desktop/cachediff.txt 
patching file camino/src/application/MainController.mm
patch: **** malformed patch at line 32: @@ -1723,17 +1728,20 @@ Otherwise, we return the URL we original

Note that the failed hunk begins with:

@@ -1682,20 +1683,24 @@

indicating that the new version should have four more lines than the old for, but there are only three new lines introduced.  Smells like a hand-edited diff to me.

Comment 18

[Mark Mentovai]

This whole thing is kinda-sorta halfway broken anyway, because the necko cache is blown away when the icon cache is cleared, making everything seem expired and eliminating the icons from bookmarks that don't use the default (like http://server/favicon.ico).  I guess that's OK, since most things use the default icon and removing the dependency on the necko cache (NeckoCacheHelper) is a larger job better left for another day, if at all.  As long as phantom non-default icons won't be hanging around in the IconCache, I suppose it's fine for now.

Comment 19

[Håkan Waara]

(In reply to comment #18)
> This whole thing is kinda-sorta halfway broken anyway, because the necko cache
> is blown away when the icon cache is cleared, making everything seem expired
> and eliminating the icons from bookmarks that don't use the default (like
> http://server/favicon.ico).  I guess that's OK, since most things use the
> default icon and removing the dependency on the necko cache (NeckoCacheHelper)
> is a larger job better left for another day, if at all.  As long as phantom
> non-default icons won't be hanging around in the IconCache, I suppose it's fine
> for now.
> 


I'm not sure what you mean about it being half-broken.  The site icons are stored and handled 100% by us. Only the actual loading part is done by necko. We take care of all storage, both memory and disk.

The necko-cache-helper is used for loads, and also seems to be used for remembering missed icons (so we don't keep hitting the same ones), and edge cases like that.

Perhaps we can remove the necko dependency, and make it less fat, if that's what you mean. :)

Anyway, just re-tested the patch, and it from 52 favicons it removed all of them except for 13 that were indeed in my bookmarks. Restarted Camino, and they were still there on the toolbar.

Sorry for the hand-edited patch, I think I've resolved that problem today -- been working on making a few local camino-forks. :)

Comment 20

[Håkan Waara]

Attachment: Patch that should apply cleanly.

Clean patch, and also addressed Simon's comment about checking for count.

Comment 21

[Mark Mentovai]

(In reply to comment #19)
> I'm not sure what you mean about it being half-broken.  The site icons are
> stored and handled 100% by us. Only the actual loading part is done by necko.
> We take care of all storage, both memory and disk.
>
> The necko-cache-helper is used for loads, and also seems to be used for
> remembering missed icons (so we don't keep hitting the same ones), and edge
> cases like that.

The necko cache helper is responsible for mapping a URL to the URL of its site icon.  It also handles expiration and negative caching.  None of these, and certainly not the first one, is an "edge case."

See [(SiteIconProvider) favoriteIconURLFromPageURL:].  If you toss the necko cache, you lose the icon unless it's at the default location (like http://server/favicon.ico), even if you were careful to not delete the icon during the icon-cache-cleaning operation.  That's why I say that it's "half-broken."  Fixing the broken half fully would mean removing the reliance on the Necko cache.  I don't think that we need to do that right now, but I want to make sure that bad things won't happen to us because we're still hanging on to icons for bookmarks when those icons are unreachable because they're not the default site icons.

Comment 22

[Håkan Waara]

(In reply to comment #21)
> See [(SiteIconProvider) favoriteIconURLFromPageURL:].  If you toss the necko
> cache, you lose the icon unless it's at the default location (like
> http://server/favicon.ico), even if you were careful to not delete the icon
> during the icon-cache-cleaning operation.  That's why I say that it's
> "half-broken."  Fixing the broken half fully would mean removing the reliance
> on the Necko cache.  I don't think that we need to do that right now, but I
> want to make sure that bad things won't happen to us because we're still
> hanging on to icons for bookmarks when those icons are unreachable because
> they're not the default site icons.

We can do this:  Iterate through the cache dooming each entry. I'd have to make our SiteIconCache call the SiteIconProvider; and I hope that dependency then won't be a problem.

Removing the necko dependency here requires work that feels like it isn't worth the time right now.

Comment 23

[Håkan Waara]

Comment on attachment 215173 [details] [diff] [review]
Patch that should apply cleanly.

I'll take bull by the horns, and remove NeckoCacheHelper (unify it with SiteIconProvider, and remove the necko dependency).

Comment 24

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

No patch, no 1.0.2.  We should still try to get this in 1.0.x, since it is a "privacy regression" we introduced for 1.0; any timeline for a new patch?

Comment 25

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

Missed 1.0.3, but we can try again for 1.0.4 if there's a patch.

Comment 26

[Håkan Waara]

Not realistic that I'll get to this anytime soon.

Comment 27

[froodian (Ian Leue)]

Mark, you seem to imply in comment 18 that the fix given by the most recent patch would be ok to review and land, given a followup bug.  Am I interpreting it correctly?

Comment 28

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

We can consider this for 1.0.5 if it has a patch (since it's a regression we shipped in 1.0.x), but that seems increasingly unlikely.

Comment 29

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

Not 1.0.5, either.  Please nominate for 1.5.x when there's a patch landed on trunk/1.6.

Comment 30

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

Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.

Comment 31

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

FWIW, I tend to think that for the Reset Camino case, we should throw away *all* icons, since we also remove the other auto-generated bookmarks metadata (visit count) when resetting.