Missing favicon on audible.de

RESOLVED FIXED in Firefox 63

Status

()

defect
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: Tobias.Marty, Assigned: standard8)

Tracking

61 Branch
mozilla63
Points:
---

Firefox Tracking Flags

(firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
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.
Component: General → Places
Product: Firefox → Toolkit
(Assignee)

Comment 1

8 months ago
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?
Flags: needinfo?(mak77)
(Assignee)

Comment 2

8 months ago
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.
we should be ignoring icons without an href and indeed fallback to the root icon, I suspect the problem is in ContentLinkHandler though.
Flags: needinfo?(mak77)
(Assignee)

Comment 4

8 months ago
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.
Assignee: nobody → standard8
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 5

8 months ago
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.
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.
(Assignee)

Comment 8

8 months ago
(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 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.
Attachment #9004695 - Flags: review+

Comment 10

8 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/527b48f62d15
Correctly handle empty (invalid) href attributes in <link> elements (e.g. favicons). r=mak

Comment 11

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/527b48f62d15
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.