299270 - Linked favicon (site icon) sometimes doesn't show on first page load

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[Wevah (Nate Weaver)]

These steps allow me to reproduce this 100%:

1. Make sure the URL isn't currently loaded in a window/tab.
2. Clear the browser cache.
3. Quit and relaunch Camino.
4. Go to the above URL (http://www.derailer.org/temp/favicontest.html).

The linked favicon ("H9") will not load the first time, and Camino instead shows
the default "dp" favicon. The correct favicon will show on a reload. This does
*not* happen to me on, say, Hyatt's weblog (maybe because it takes a lot more
time to load than my test page and some other pages I've noticed it on).

Comment 1

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

I actually saw the dp icon load initially and then be replaced by the H9 icon,
like a wierd flash.  Bizarre.

Comment 2

[Wevah (Nate Weaver)]

That I'm (personally) okay with, but on pretty much every page on
http://www.barebones.com/ it uses the ICO on the first go, and then the PNG on a
refresh. However *both* icons are linked in the HTML source; the PNG's link just
comes after the ICO's.

Maybe Camino should always use the last icon it knows about? (What does Firefox do?)

Comment 3

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

(In reply to comment #2)
 
> Maybe Camino should always use the last icon it knows about? (What does
Firefox do?)

Fx does that; it gets the PNG exclusively at barebones.  

On your site, the favicon is blank for a long time after the page has apparently
finished loading and then the H9 icon finally appears in my DeerPark from 7-1
(generic in URL bar/spinner on the tab > blank > H9)

Comment 4

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

We keep talking about this bug (in IRC), but for the life of me I can't remember if we're any closer to fixing it... Is this something we can get by 1.0 with our current knowledge of the issue (or lack there of)?

Comment 5

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

We want Wevah to read the Fx code to see what they do to always only load the last-linked item (if one is present) instead of the default /favicon.ico :-)

Comment 6

[Simon Fraser [no longer active]]

We probably just need to save a bit more state to be able to ignore the /favicon.ico load when it comes back.

Comment 7

[Wevah (Nate Weaver)]

Or keep track of the last "I got a link element!" event so we only use the resource specified there.

Comment 8

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

Attachment: Missing favicon in bugzilla

And when there's no favicon.ico, you get just the globe instead.  I captured this the other night in Bugzilla and meant to post it, but I'm pretty sure it wasn't the first load of any Bugzilla url, so I'm not sure what was up....

(I think I was trying to convince people this was bug 282647 earlier, but it's really this bug.)

Comment 9

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

-> 1.1

Comment 10

[Håkan Waara]

Wevah, the site case doesn't exist on your server anymore.
Does anyone have a testcase around still?

Comment 11

[Wevah (Nate Weaver)]

It's still there; my host was just down for a good chunk of time today. :(

Comment 12

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

http://lxr.mozilla.org/mozilla/source/camino/src/browser/SiteIconProvider.mm#474

474     // we should deal with the case of a page with a link element asking
475     // for the site icon, then a subsequent request with a nil inIconURI.
476     // however, we normally try to load the "default" favicon.ico before
477     // we see a link element, so the right thing happens.

Sounds like we're currently doing this on purpose, and maybe we should rework that logic to delay loading favicon.ico (and favicons in general) until we're sure the link element has a first shot at displaying if it's really valid?

Comment 13

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

Wevah, can you take a look at the Fx code and come up with a way to re-work our code to accomplish this?

Comment 14

[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 15

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

"/temp/favicontest.html ist nicht hier!" so adding the page Lisa Thompson set up at .Mac: http://homepage.mac.com/lthompson.22/favicon-test/

Re-reading this bug, and playing with Lisa's testcase, I wonder if this might be related to bug 434266, particularly if one is coming in late?

Comment 16

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

Attachment: Old site icon load logging patch

(In reply to comment #12)
> http://lxr.mozilla.org/mozilla/source/camino/src/browser/SiteIconProvider.
> mm#474
> 
> 474     // we should deal with the case of a page with a link element asking
> 475     // for the site icon, then a subsequent request with a nil inIconURI.
> 476     // however, we normally try to load the "default" favicon.ico before
> 477     // we see a link element, so the right thing happens.
> 
> Sounds like we're currently doing this on purpose, and maybe we should
> rework that logic to delay loading favicon.ico (and favicons in general)
> until we're sure the link element has a first shot at displaying if it's
> really valid?

In particular, I believe what happens is the BrowserWrapper's |onLocationChange:| (which in turn calls SiteIconProvider's |defaultFaviconLocationStringFromURI:|, which is what fetches favicon.ico) happens before BrowserWrapper's |onFoundShortcutIcon:| (which lets us know we have a <link rel="shortcut icon" href="foo"> element) gets called.  And, although we keep trying in a couple of places to reset the site icon to the latest discovered thing, we don't always succeed (witness bug 425878).

I don't so much mind us fetching the favicon.ico first (so we get something in the location bar before the entire pageload/subresource loads are done) if we'd reliably do the right thing in cases where we did find a <link> icon--and if we'd reliably save the right icon for future use.

(I really like Lisa's testcase, BTW; on the initial pageload of the initial page, I see the Apple->GreenDot transition, on the initial pageload of the second page, I just get the Apple icon, and on the initial pageload of the third page, I just get the GreenDot.  On the second pageload of each, I then got the GreenDot universally.  …And then in a new browser session, I got something completely different, such that it took three pageloads of each page to get them all to use the GreenDot :P )

I think someone first needs to sit down and log what actually happens in several of these cases, perhaps with SIP's VERBOSE_SITE_ICON_LOADING set and some custom logging.  I've attached the (partial) logging patch that I used for bug 425878 comment 8, but it's lacking some useful logging inside of several SIP methods, including (for this bug) at least |favoriteIconURLFromPageURL:|, |doneRemoteLoad:forTarget:withUserData:data:status:usingNetwork:|, |fetchFavoriteIconForPage:withIconLocation:allowNetwork:notifyingClient:|, and |defaultFaviconLocationStringFromURI|, and probably BW's |onFoundShortcutIcon:| and CHBL's |CHBrowserListener::HandleFaviconLink|.  (I also can't guarantee that this patch will still apply right now, but at least it's a start for anyone wanting to look at this).