109672 - site icon for iframe content is shown as proxy icon

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Jesse Ruderman]

http://mozilla.org/projects/ui/menus/shortcut/ has an iframe with src= 
http://damowmow.com/.  http://damowmow.com/ has a favicon, a drawing of a cat.  
Mozilla shows Hixie's cat as the icon for 
http://mozilla.org/projects/ui/menus/shortcut/, misleading users into thinking 
Hixie wrote the spec.

Comment 1

[R.K.Aa.]

Looking at the same page in NS4.77 i see no less than 6 cats - all over the
page. Misleading me to think the CAT wrote the spec..

I see no cat as favicon in moz though, when i load
http://mozilla.org/projects/ui/menus/shortcut/ - only the iframe cat(s) further
down on page. Testing with a linux CVS build from yesterday.

Comment 2

[Jesse Ruderman]

rkaa sees the cat on damowmow.com but not at the context menu spec.  I still 
see it at both pages using 2001 111203 on Win98.  I guess this bug is platform-
specific.

Comment 3

[Jesse Ruderman]

Attachment: testcase

Comment 4

[Jesse Ruderman]

Attachment: testcase, hopefully with correct mime type this time

Comment 5

[John Morrison]

I had earlier seen the cat as the in the url proxy icon's place. But I don't 
see it now, with either the UI spec page, or with the testcase (on win32). 
Jesse, can you try this again.

Note to claudius: you should add this to your testcases for this feature (I 
assume since sairuh is sending all the favicon love your way, you "won" this
feature :-]). This is something we absolutely must not do for performance 
reasons. The reason is that ad banners, which are typically iframes, use a 
multitude of server names to dish the adverts. We do not want to be pinging
iframes (or frames) for their favicon.ico, or for [not that they would have 
one] a <link rel="icon" ...>. I'll set you up with a way to check for this if 
you need it.

Comment 6

[Jesse Ruderman]

I still see this bug on 2001 120703 Win98.

jrgm: we shouldn't be asking sites for favicon.ico unless they have a <link> 
pointing to it...

Comment 7

[John Morrison]

Well, "should" and "are" are not the same thing. We are GETting /favicon.ico by 
default in the current implementation.

At any rate, in a testcase at http://cowtools/bugs/109672 I can see that we
are not getting /favicon.ico for a child frame, which was the condition that
I wanted to be sure to avoid. But, as the URL for this bug shows, and the 
testcase on cowtools, the presence of a <link rel> in a child iframe causes the 
proxy icon for the urlbar to be set to the icon of the child frame, even 
overriding an icon set for the containing document by a <link rel> (or from
a default /favicon.ico).

Comment 8

[John Morrison]

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

Comment 9

[Matthias Versen [:Matti]]

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

Comment 10

[Sébastien Delahaye]

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

Comment 11

[Sébastien Delahaye]

The favicon changes when there is a tag like that <meta http-equiv=Refresh
Content="20; URL=http://example.com/"> in the Iframe. 
It changes on refresh only.
There is a testcase here: http://musique.delahaye.net/favicon-iframe.html

Comment 12

[André Dahlqvist]

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

Comment 13

[Matthias Versen [:Matti]]

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

Comment 14

[Jesse Ruderman]

I still see this bug at http://mozilla.org/projects/ui/menus/shortcut/ in 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6a) Gecko/20031028

I can't repro in Firebird, probably because of the Firebird bug that prevents it
from showing http://www.damowmow.com/'s site icon in the address bar when you
visit http://www.damowmow.com/.

Comment 15

[Alfonso Martinez]

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

Comment 16

[Alex Hirzel]

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

Comment 17

[Rudy Zeinoun]

Bug is in Firefox too. Whenever an iframe refreshes (whether using meta
http-equiv refresh or javascript location.reload), the favicon of the tab
changes, which is undesirable.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

Comment 18

[William Price]

Attachment: Tarball of testcases for IFRAME meta refresh & favicon bug.

I also see this bug when an IFRAME's content uses a meta refresh. 
Additionally, I can confirm on Firefox 1.0 official for WinXP **and Linux** so
this isn't a Win32-only bug.

See attached testcases.

Comment 19

[Christian Schmidt]

Attachment: Patch for Firefox

This is a patch for Firefox. I don't know if this is appropriate in this bug,
but if this patch get reviews, I'll make an xpfe patch too.

Some of the logic concerning icons was duplicated in browser.js and
tabbrowser.xml. This is now (almost) collected in tabbrowser.xml.

mIcon is removed from the listener. Instead the icon URL is stored as a
property of the browser object. This property is now cleared on locationChange
instead of on STATE_START, so that the icon will persist if we cancel the load.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 185899 [details] [diff] [review]
Patch for Firefox

r=vladimir

I /think/ this looks OK, and my quick 5-minute testing thinks it's OK, too. 
There may yet be some odd cases that are broken (I remember things being fairly
fragile when I poked at this code for 1.0), but I don't have very good tests
for those.

Comment 21

[Christian Schmidt]

Vladimir, thanks for the review.

Would you please check this in for me? Thanks.

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

checked in

Comment 23

[Simon Bünzli]

The patch to this bug might disable SessionSaver. It fails in addTab with the
following exception (line number is from build 2005061506):

Error: this.mProgressListeners has no properties
Source File: chrome://global/content/bindings/tabbrowser.xml
Line: 430

Discussion and work-around:
http://forums.mozillazine.org/viewtopic.php?p=1543893#1543893

Comment 24

[Christian Schmidt]

Attachment: Patch that fixes the issue with sessionsaver

This patch makes sure that mProgressListeners is always an array. In also moves
the constructor-like code from addProgressListener to the constructor.

I'd like to test this a bit more before requesting reviews.

Comment 25

[Worcester12345]

(In reply to comment #24)
> Created an attachment (id=186478) [edit]
> Patch that fixes the issue with sessionsaver
> 
> This patch makes sure that mProgressListeners is always an array. In also moves
> the constructor-like code from addProgressListener to the constructor.
> 
> I'd like to test this a bit more before requesting reviews.

So, is this bug fixed and that a new one? If so, what is the bug number for that
patch? If not, shouldn't THIS bug be reopened?

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This bug may have caused the regression in bug 297945 -- cs, can you check if
that's the case?

Comment 27

[Simon Bünzli]

One other problem I stumbled upon while trying to fix SessionSaver is that in
the following code from attachment 185899 [details] [diff] [review], p could be null, leading to an
"invalid 'in' operand null" exception. In all other places in tabbrowser.xml,
the check is just "if (p)":

434             for (var i = 0; i < this.mProgressListeners.length; i++) {
435               var p = this.mProgressListeners[i];
436               if ('onLinkIconAvailable' in p)
437                 p.onLinkIconAvailable(browser);
438             }

(p is null, if the listener has been removed; cf.
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#1311)

Comment 28

[Frej Bjon]

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

Comment 29

[Christian Schmidt]

Follow-up bug filed regarding the issue with session saver: bug 305828