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

RESOLVED FIXED

Status

Camino Graveyard
General
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: davedit, Assigned: Smokey Ardisson (offline for a while; not following bugs - do not email))

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

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

Comment 2

10 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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…
Summary: Bookmark Manager and History favicons fail to load immediately after launch → Bookmarks and History favicons fail to load on first load of bookmarks and history
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.
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.
I suspect bug 391864 regressed this (landed for 1.5.2), and that a fix for bug 414272 might fix this.
(In reply to comment #6)
> I suspect bug 391864 regressed this (landed for 1.5.2)

Nope.
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.)
Assignee: nobody → alqahira
Status: NEW → ASSIGNED
Summary: Bookmarks and History favicons fail to load on first load of bookmarks and history → Bookmarks, History, and about:blank site icons (favicons) fail to load on first load of bookmarks/history/about:blank
Created attachment 487288 [details] [diff] [review]
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.
Attachment #487288 - Flags: review?(stuart.morgan+bugzilla)

Comment 10

7 years ago
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.
Attachment #487288 - Flags: review?(stuart.morgan+bugzilla) → review-
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

7 years ago
> 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 :)
Created attachment 488393 [details] [diff] [review]
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)
Attachment #487288 - Attachment is obsolete: true
Attachment #488393 - Flags: review?(stuart.morgan+bugzilla)

Comment 14

7 years ago
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]
Attachment #488393 - Flags: review?(stuart.morgan+bugzilla) → review-
Created attachment 488425 [details] [diff] [review]
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 :)
Attachment #488425 - Flags: review?(stuart.morgan+bugzilla)
Attachment #488393 - Attachment is obsolete: true

Comment 16

7 years ago
(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.
OS: Mac OS X → Windows 7

Comment 17

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

r=me :)
Attachment #488425 - Flags: superreview?(mikepinkerton)
Attachment #488425 - Flags: review?(stuart.morgan+bugzilla)
Attachment #488425 - Flags: review+

Updated

7 years ago
OS: Windows 7 → Mac OS X
Hardware: PowerPC → All
Comment on attachment 488425 [details] [diff] [review]
Fix via NSSet record-keeping, with better small stuff

sr=pink
Attachment #488425 - Flags: superreview?(mikepinkerton) → superreview+
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).
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(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 :)
You need to log in before you can comment on or make changes to this bug.