1229720 - Some ICO images aren't show anymore in FF 43

Status: RESOLVED WONTFIX

(Core :: Graphics: ImageLib, defect)

Description

[Fernando Hartmann]

Attachment: The site favicon.ico

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151126120800

Steps to reproduce:

Opening some sites like https://app.taticview.com/


Actual results:

The favicon of the site aren't showed


Expected results:

The favicon should be showed.

This started on FF 43, until that (FF 42 and before) it worked.

Comment 1

[Gingerbread Man]

Push log:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4313752f69956ae248bd4e7ff3913c8dd4252698&tochange=ccd6b5f5e544c1d708849144943a776941bd3794

Suspect:
Bug 1201796

The icon includes the following sizes:
16×16, 24×24, 32×32, 57×57, 72×72, 96×96, 128×128, 195×195

Firefox 42 displays the 16×16 size. Firefox 43 displays a large gray square and shows “256×256” in the tab title.

Comment 2

[Peter Oliver]

Sounds like a duplicate of #1220021?

Comment 3

[Gingerbread Man]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0
20160101030330

The issue persists in the latest Nightly, therefore not a duplicate of bug 1220021.

Comment 4

[Timothy Nikkel (:tnikkel)]

Hmm, Chrome doesn't seem to show the favicon either. The OS X preview app can view it though.

Comment 5

[Timothy Nikkel (:tnikkel)]

The dir entry for the resource we select from the .ico file has 0,0 for the width height, in ico speak 0 means 256. So the dir entry says the size of the contained png is 256x256. When we actually decode the contained png we find it to be 72x72. This makes us hit this failure case

http://mxr.mozilla.org/mozilla-central/source/image/decoders/nsICODecoder.cpp?rev=e79cd09c87da#591

which puts the decoder in error state, so we don't draw anything.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Jet, can you help get movement here?

Comment 7

[Nicholas Nethercote [inactive]]

ICO files can be tricky because they can contain inconsistent size data, as
comment 5 mentions. Basically, the header claims the image is one size, and
then the actual image embedded within claims a different size, and the latter
is usually correct. So ideally we'd use the latter, but the difficulty is that
the latter is not within the "metadata decode" which is the cheap initial scan
of the file.

tnikkel has suggested we give up on the metadata decode being cheap for ICOs.
From an email:

> I think we should ignore the performance concerns and do a proper parse of
> the sizes of all contained resources during the metadata decode, and keep
> that information around for each resource (so when we do a full decode we
> already know not to believe the dir entry).
> 
> I don't think the perf concerns are that big of a deal because:
>
> 1) we are talking about icos, very few pages should be using icos, if they
> are, they are probably giving them a specified size, so it shouldn't be
> blocking the proper visual layout of the page,
> 2) where we use icos in chrome code we give them a specified size, and we
> aren't loading a new document, we already have the chrome document and are
> just putting an ico into it,
> 3) we have to wait until the file is complete from the network anyways, so
> the only extra overhead is jumping around in memory a bit to the resources.

I'm inclined to agree. Unfortunately, ICO has been extended so many times that
you can't decode it correctly in general without looking at the whole file.

As for the priority of this, it doesn't seem that high. The number of files
affected still seems to be low.

Comment 8

[Jet Villegas (inactive)]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> As for the priority of this, it doesn't seem that high. The number of files
> affected still seems to be low.

The current behavior is as designed, per the first patch from bug 1229720:
(Part 1) Treat ICOs with wrong widths and heights as corrupt 

If we ever fix this, I'd want to do it as part of bug 1255102.

Comment 9

[Timothy Nikkel (:tnikkel)]

We landed patches to display ico files like this in another bug.