425878 - Bookmarks, History, and about:blank site icons (favicons) fail to load on first load of bookmarks/history/about:blank

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[davedit]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.13) Gecko/20080327 Camino/1.6b4 (like Firefox/2.0.0.13)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.13) Gecko/20080327 Camino/1.6b4 (like Firefox/2.0.0.13)

If you launch Camino with a blank homepage and the first thing you open is the bookmark manager or the history (or if you set one of these as your homepage), their favicons fail to display, instead showing globe_ico.

I noticed that when you launch Camino, it first shows globe_ico, then switches to smallDocument, then back to globe_ico! Perhaps this is at the root of the problem. Shouldn't smallDocument display in the first tab upon launch?

Subsequently, whenever you open a new tab, it always shows globe_ico first before switching to smallDocument.

Reproducible: Always

Steps to Reproduce:
1. Launch Camino.
2. Open the Bookmark Manager or History
Actual Results:  
Their favicon will display really quickly before switching back to globe_ico

Expected Results:  
Their favicons should display

Comment 1

[davedit]

Oh I forgot to mention that I see this behavior in both 1.5.5 and the 1.6 branch.

Comment 2

[Chris Lawson (gone)]

First mentioned here:

http://forums.mozillazine.org/viewtopic.php?p=3314795#3314795

just for posterity in case Dave forgot to capture something from that thread in comment 0. Smokey says he's seen this too.

Smokey said "I think it's the first time that you display either of them in a given app session, regardless of the previous page (or lack thereof)" and I can reproduce that behaviour in trunk as well, regardless of what tabs or windows are already open, so I'm going to go ahead and confirm this.

Comment 3

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

Note that currently we don't even get the "display really quickly before switching back" behavior; it's all-globe from the get-go.

I turned on smfr's VERBOSE_SITE_ICON_LOADING in SiteIconProvider, and, in a fresh profile, we get:

Sep  3 00:11:26 Qalaat-Samaan Camino[3680]: Site icon load about:bookmarks dispatched 1
Sep  3 00:11:26 Qalaat-Samaan Camino[3680]: Site icon load about:bookmarks done, status 0x00000000

for the first load, and

Sep  3 00:12:19 Qalaat-Samaan Camino[3680]: Site icon about:bookmarks known missing

for second and subsequent loads (when we see the right icon).

Other things I can tell you, while tilting at windmills:

1) We do successfully register the site icons in MainController in startup, before we start doing the "load stuff"

1a) We do indeed try to load site icons for all of the items in the Bookmarks Manager (menu, bar, address book, probably Bonjour) at launch (bug 351678): "site icon load fooIconURL dispatched 1", though we don't appear to actually fetch them into the cache until you visit the site and Necko caches them

2) We never cache these icons to disk, only to memory in the sharedSiteIconCache

3) I'm not going to figure this bug out

My guess is that either the bug is in |fetchFavoriteIconForPage| (we seem to show the right values for |registerFaviconImage| and |favoriteIconForPage|) or that we don't update site icons in the location bar/tabs with the image we've cached (maybe because there's not a network load for bookmarks/history or for their site icons, like there are for real sites?), which is probably the gunk in BrowserWrapper's onLocationChange or updateSiteIconImage…

Comment 4

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

What's interesting is that I just used a profile (with my debug build) that had about:bookmarks open in the saved session, and when the session restored, the site icon was actually present.

Comment 5

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

The icons also seem to fall out of memory cache after some time, because I've been running this Camino since the sixth, have visited history either daily or every-other-day, and just opened history tonight to no site icon.

Comment 6

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

I suspect bug 391864 regressed this (landed for 1.5.2), and that a fix for bug 414272 might fix this.

Comment 7

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

(In reply to comment #6)
> I suspect bug 391864 regressed this (landed for 1.5.2)

Nope.

Comment 8

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

OK, I've figured this out.  (about:blank is also affected, at least in certain cases.)

First, at launch, MainController registers custom icon images with SiteIconProvider (hereinafter SIP) for about:blank/bookmarks/history (and missing local files).

In BrowserWrapper's |onLocationChange|, we ask SIP for the default URI for the page's site icon.  For about: pages, it's always themselves.

We then check to see if that URI is equal to the URI of the current site icon.  For the first load of about:blank in a window after launch (i.e., about:blank as home page), the URI of the current site icon is null; for history/bookmarks, the URI of the current site icon is the site icon URI of whatever is currently showing.  So in all three cases we fail.

We then see if we have a cached icon for the page (in this case, we have the ones we registered in MainController during startup) and display that (if this gets displayed, it's wiped away instantaneously by the next step), but immediately also ask SIP to go to the network and get |fetchFavoriteIconForPage|.  As you might expect, Necko's (via RemoteDataProvider, who SIP calls) is not returning an icon for any of these three about: URIs.  SIP does note that the site icon is missing, though.

When SIP's |fetchFavoriteIconForPage| is done, BW's |imageLoadedNotification| gets called, and it does all its stuff with a null image (thanks, SIP/RDP/Necko!), and resets the site icon URI (which long ago was about:foo) to "".  It calls BW's |updateSiteIconImage| with the null image and the "" site icon URI.

|updateSiteIconImage| then checks to see if the new site icon URI matches the old one; if they don't, then it wants to set the new icon.  In the case here, "" and about:foo don't match, so we want to update to the new icon.  Except we have no new icon (null image), so |updateSiteIconImage| looks at its own code to determine if it knows what icon to set.

If the page is not a blocked site or page load error (we special-case these based on eRequestStatus enum, and |imageLoadedNotification| calls |updateSiteIconImage| with eRequestSucceeded), fall back to the globe icon.  We don't meet any of the non-success status conditions, so we get the globe :P


The second load for about:blank/history/bookmarks, we go all the way through to SIP's |fetchFavoriteIconForPage|, which notes that the site icon is missing, so it returns early (doesn't do a network load again, and doesn't trigger BW's |imageLoadedNotification|), which means we don't update the site icon that we set correctly to begin with in |onLocationChange| :P

There are a couple of ways to fix this.

1) Don't ever do stupid network lookups for things we know will fail anyway.  I.e., check for about:blank|about:bookmarks|about:history here: http://mxr.mozilla.org/camino/source/camino/src/browser/BrowserWrapper.mm#749

This has the advantage of avoiding three network hits over the life of the browser session (or until the "missing site icon list" is purged, which is in a week).  But it also means that we have three icons that are only semi-managed by SIP.

2) Set the right icon (again!) once we get back to BW's |updateSiteIconImage|.

This has the advantage of being in the same place where we handle other special-case icons, so it's pretty easy to find.  We'll occasionally do the useless network lookups, but overall this feels cleaner to me, I think.  We keep SIP in the loop here like with all other site icons.

(Either way, a comment in MainController at the place where it registers these special icons would be useful, so that any future icons we add there get handled appropriately.)

Comment 9

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

Attachment: Fix via |updateSiteIconImage|

This implements the change in |updateSiteIconImage|, to group these with all of our other special-casings.

Stuart, of the people around, I think you're most familiar with the fun that is SiteIconProvider and the site icon setting/caching/etc. behavior, so r? to you.  See comment 8 where I outlined what's going on, to the best of my ability to comprehend.

Comment 10

[Stuart Morgan]

Comment on attachment 487288 [details] [diff] [review]
Fix via |updateSiteIconImage|

Nice detective work! We should never hard-code the same thing in two places though; it's asking for trouble.

SIP should manage this correctly; there are a two options that come to mind based on looking at the code and your description:
1) have SIP keep an NSSet of the explicitly registered icons in registerFaviconImage:, and then in the load method just return early if the request is for one of those.
2) use the fact that SIC already has this information in the form of the expiration being NSDistantFuture, and add a SIC method to ask if the given URL's favicon never expires, and return early if that's the case.

2 might be a tad cleaner, but 1 would be easier and would have less runtime overhead, so I would lean slightly toward 2.

Comment 11

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

Will either of those help (or be better) with the problem of the icon (apparently) being thrown out of memory cache (comment 5)?  Or as long as we return early, it'll be irrelevant (I think, just now, I saw the right icon just before it was replaced by the globe; if I didn't open History today, it's been only 24 hours since I last opened it :P ).

(In reply to comment #10)
> 2 might be a tad cleaner, but 1 would be easier and would have less runtime
> overhead, so I would lean slightly toward 2.

Prefer the cleaner one over the "easier and less runtime" one, or did you mean to lean slightly towards 1?

Comment 12

[Stuart Morgan]

> Will either of those help (or be better) with the problem of the icon
> (apparently) being thrown out of memory cache (comment 5)?

Hm, I missed that. Is there any chance you force-reloaded? That would cause us to lose the icon, which is clearly a bug. That's fixable under either scenario; we just add a similar check in removal at the same layer we make the other fix.

> or did you mean to lean slightly towards 1?

That one :)

Comment 13

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

Attachment: Fix via NSSet record-keeping

This uses the NSSet storing/comparison to return early from fetchFavoriteIconForPage for explicitly-registered icons.

[7:52pm] sauron: (also, i think the expiry-after-a-while problem is that the "known missin" bit expires, so once again we have to request it and show the wrong thing once to re-set the "known missing" bit)
[7:54pm] sauron: (so this fix, returning early, will prevent the reset part, and as long as the icon itself truly does not expire from memory, we should be OK)

Comment 14

[Stuart Morgan]

Comment on attachment 488393 [details] [diff] [review]
Fix via NSSet record-keeping

Looks good structurally, just some small stuff:

>+  // dict of favicon url -> request url

d->D, . at end.

>+  // Set of URIs for which we've explicitly registered images to be icons

. at end

>+    mURIsWithRegisteredIcons = [[NSMutableSet alloc] initWithCapacity:1];

Just do init instead of initWithCapacity:.

>+  // If this is a URI for which we've explicitly regsitered an image, don't
>+  // check the network for an icon, because checking will always fail to find
>+  // an icon and will cause us to display the generic icon initially (and
>+  // whenever the URI has been expired from the MissedIconsCache).

regsitered->registered. I don't think this actually needs a comment though; it's pretty self-explanatory that we don't want to do a lookup for an explicitly-registered icon.

>+  if([[NSSet setWithObject:inPageURI] isSubsetOfSet:mURIsWithRegisteredIcons])

This should just be [mURIsWithRegisteredIcons containsObject:inPageURI]

Comment 15

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

Attachment: Fix via NSSet record-keeping, with better small stuff

(In reply to comment #14)
> >+    mURIsWithRegisteredIcons = [[NSMutableSet alloc] initWithCapacity:1];
> 
> Just do init instead of initWithCapacity:.

What's the difference, practically speaking?  (Also, why do the NSSet/NSMutableSet docs not list a plain init?)

> regsitered->registered. I don't think this actually needs a comment though;
> it's pretty self-explanatory that we don't want to do a lookup for an
> explicitly-registered icon.

Agreed. I put it there in case someone was confused (unfamiliar with what we mean by "register", or otherwise as swimming in the code as I was when I started debugging), but I realized there's the comment I added in  registerFaviconImage:forPage: that's trivially accessible if you follow "mURIsWithRegisteredIcons" (and more succinct, too).  So big confusing verbose comment deleted :)

Comment 16

[Stuart Morgan]

(In reply to comment #15)
> What's the difference, practically speaking?

Not much, but it's weird to use initWithCapacity: when you don't have a meaningful number to put there.

> (Also, why do the NSSet/NSMutableSet docs not list a plain init?)

Everything inherits init from NSObject, and inherited methods aren't re-listed.

Comment 17

[Stuart Morgan]

Comment on attachment 488425 [details] [diff] [review]
Fix via NSSet record-keeping, with better small stuff

r=me :)

Comment 18

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 488425 [details] [diff] [review]
Fix via NSSet record-keeping, with better small stuff

sr=pink

Comment 19

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

http://hg.mozilla.org/camino/rev/3ceef7504941

I'll keep an eye out for disappearing history/bookmarks/blank icons after this, which would indicate I was wrong in comment 13 and that we'd need to do something to stop memory cache eviction, too (comment 12).

Comment 20

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

(In reply to comment #19)
> I'll keep an eye out for disappearing history/bookmarks/blank icons after this,
> which would indicate I was wrong in comment 13 and that we'd need to do
> something to stop memory cache eviction, too (comment 12).

I haven't seen this at all since running a build containing the fix :)