1486226 - Missing favicon on audible.de

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[TMart]

Favicon for www.audible.de is missing in Firefox. 

Browser console reports: 
[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIContentSniffer.getMIMETypeFromContent]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource:///modules/FaviconLoader.jsm :: onStopRequest :: line 181"  data: no]

This affects 61.0.2, 62 Beta 20 and current Nightly.

Comment 1

[Mark Banner (:standard8)]

This also reproduces on www.audible.co.uk.

My initial thoughts are that this is mainly audible's fault, in their HTML, they have:

<link rel="icon" sizes="192x192" href="" />
<link rel="apple-touch-icon-precomposed" sizes="180x180" href="" />
<link rel="apple-touch-icon-precomposed" sizes="120x120" href="" />
<link rel="apple-touch-icon-precomposed" href="" />

According to the spec, this is invalid HTML: "The destination of the link(s) is given by the href attribute, which must be present and must contain a valid non-empty URL potentially surrounded by spaces. If the href attribute is absent, then the element does not define a link."

So really, this is audible's issue - I'll see if I can submit something to their customer support about it.

Interestingly Google Chrome and Edge fall back to the sites default /favicon.ico which is specified. So I guess we could do the same, because the link values are invalid - Marco, what do you think?

Comment 2

[Mark Banner (:standard8)]

Just for general info, I found a customer support email for Audible, and I've sent details of this to them. Even if we change how we're handling this, it isn't clear if they're intending to specify other sizes but failing, or this is just invalid/useless html.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

we should be ignoring icons without an href and indeed fallback to the root icon, I suspect the problem is in ContentLinkHandler though.

Comment 4

[Mark Banner (:standard8)]

Thanks for the confirmation, seeing as I was looking at the code to see what was going on, I've got a quick patch for this.

Comment 5

[Mark Banner (:standard8)]

I was just about to submit a patch, then I took another look, it seems the problem is actually here:

https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/browser/actors/LinkHandlerChild.jsm#93

The `!link.href` appears to be checking for an empty href, however the node that is effectively `event.target` and `event.target.href` has the href of the document when the href attribute is empty.

Therefore, I think we should also be additionally checking `!link.getAttribute("href")`.

I did some looking back, and further back than Firefox 3, we've been using some variant of the `link.href` check, but never .getAttribute. So I don't know if this is something that has always been broken, or something that got broken along the way - perhaps if DOM nodes changed how they handled the "href" field.

In any case, it seems best to append the getAttribute variant of the check onto the existing check, to ensure we don't miss anything if link.href ever happens to be empty.

Comment 6

[Mark Banner (:standard8)]

Attachment: Bug 1486226 - Correctly handle empty (invalid) href attributes in <link> elements (e.g. favicons).

Comment 7

[Dave Townsend [:mossop]]

There's an argument to be made here that href attributes are relative to the document URL and so we're actually doing the right thing per spec (see what happens with <img src=""> as a similar example). But this is pretty dumb.

I have been wondering if we should be trying to load /favicon.ico (or other less-preferred icons) if the specified icon fails which would be another way to fix this.

Comment 8

[Mark Banner (:standard8)]

(In reply to Dave Townsend [:mossop] from comment #7)
> There's an argument to be made here that href attributes are relative to the
> document URL and so we're actually doing the right thing per spec (see what
> happens with <img src=""> as a similar example). But this is pretty dumb.

I think link.href is acting correctly, but we're not acting correctly according to how href is handled the <link> spec aiui. That's probably what you're saying as well.

> I have been wondering if we should be trying to load /favicon.ico (or other
> less-preferred icons) if the specified icon fails which would be another way
> to fix this.

I think that's a good fallback, but I'm still thinking we shouldn't even try to process these invalid link elements anyway.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 9004695 [details]
Bug 1486226 - Correctly handle empty (invalid) href attributes in <link> elements (e.g. favicons).

Marco Bonardo [::mak] has approved the revision.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/527b48f62d15
Correctly handle empty (invalid) href attributes in <link> elements (e.g. favicons). r=mak

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/527b48f62d15