682688 - Some icons missing first 3 pixels because FF6 moz-icon URIs sometimes generate bad icons

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Geoff Lankow (:darktrojan)]

Attachment: screenshot

The recent changes (bug 600556, or more likely bug 670466) have broken some Windows icons. It looks like they've had 4 bytes too many chopped off the beginning.

Comment 1

[Brian R. Bondy [:bbondy]]

Do you know a page or reference image I can reproduce with?

Comment 2

[Richard Marti (:Paenglab)]

I think Bug 682696 is related to this bug.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Proof that Firefox 6 generates invalid ICO files from moz-icon:file:///C:/Users/YOURUSERNAME/Downloads/?size=16

Comment 5

[pal-moz]

http://forums.mozillazine.org/viewtopic.php?p=11185685#p11185685

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Fixed Firefox 6 bug in moz-icon creating invalid ICOs sometimes

So it turns out that Bug 600556 didn't introduce a regression.
Instead Bug 600556 fixed 1 of 2 bugs that have existed since we had moz-icon URLs. 
Those 2 bugs worked nicely (or badly rather) together so that you didn't notice one another.

Bug 1 of 2 that existed forever:
Our old ICO decoder ignored the bitmap information header's compression value and always assumed there was no compression.
This is wrong, we should check the compression field and decode appropriately.
Bug 600556 fixed this.

Bug 2 of 2 that existed forever:
Our moz-icons in Windows (modules\libpr0n\decoders\icon\win\nsIconChannel.cpp) generates BI_BITFIELDS compressed bitmaps but omitted the 3 DWORD 
bitfield mask.  This is also wrong.
You can actually verify this if you grab moz-icon:file:///C:/Users/bbondy/Downloads/?size=16 with Firefox 3,4,5,6,7 and then save it to your desktop.
Then try to open it in Windows, Google Chrome, or Internet Explorer.  All of them will display a broken image.  Because it's invalid.
Firefox would of course display it perfectly because of Bug 1 of 2.

So Bug 600556 actually allows us to support ICOs that have BMP compression inside of them, whereas before Bug 600556 we did not support that.
This is nice because we'll support a wider range of favicons that exist on the web.
Bug 600556 was not related to moz-icons in any way.

So the fix here is simply to force windows to return to us a buffer that is no compression.
I could have also fixed this by not omitting the BI_BITFIELDS 3 DWORD bitmask, but that was a bigger change.

I did notice some additional existing problems with moz-icon code, but I will post for that separate since it's not related to the side effects of the fix in Bug 60056 for compression.

Comment 7

[Brian R. Bondy [:bbondy]]

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=ff00b183a230

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 556464 [details] [diff] [review]
Fixed Firefox 6 bug in moz-icon creating invalid ICOs sometimes

Review of attachment 556464 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/decoders/icon/win/nsIconChannel.cpp
@@ +452,5 @@
>    }
>    case 16:
>    case 32:
> +    // Even if we have BI_BITFIELDS, we don't need the 3 DWORDS
> +    // included because we will instead force to a normal BI_RGB bitmap

It looks sort of like this code is what's doing the forcing; is that the case? If so, this comment should be changed to say that. :)

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Fixed Firefox 6 bug in moz-icon creating invalid ICOs sometimes v2

Updated the comment to explain how we are forcing to BI_RGB data.

Comment 10

[Brian R. Bondy [:bbondy]]

Pushed to mozilla-inbound
http://hg.mozilla.org/integration/mozilla-inbound/rev/d48ac4bbb8e6

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 556758 [details] [diff] [review]
Fixed Firefox 6 bug in moz-icon creating invalid ICOs sometimes v2

>+    if (aHeader->biCompression == BI_BITFIELDS)
>+      aHeader->biCompression = BI_RGB;
>       colorTableSize = 0;

This indentation is really unclear.

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/d48ac4bbb8e6

btw, I guess you may want to address comment 11

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Formatting fix for previous push

No logic change, just fixed formatting

Comment 14

[Brian R. Bondy [:bbondy]]

Formatting change was pushed to mozilla-inbound by the way:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5117dd889921

Comment 15

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/5117dd889921

Comment 16

[pal-moz]

on Windows XP, no icon in download manager.
other bug ?

Comment 17

[Brian R. Bondy [:bbondy]]

same bug, but I think you're using an older build of Nightly.

Comment 18

[pal-moz]

Attachment: screenshot

(In reply to Brian R. Bondy [:bbondy] from comment #17)
> same bug, but I think you're using an older build of Nightly.

no.
using latest NIGHTLY(9.0a1).
see screenshot.

Comment 19

[Brian R. Bondy [:bbondy]]

I guess the last patch was only a partial fix.  I think the problem is that we should be requesting a plain bitmap with no compression in all cases not just for 32bpp and 16bpp.

Comment 20

[Brian R. Bondy [:bbondy]]

Attachment: Should always use no compression for moz-icons

Comment 21

[Brian R. Bondy [:bbondy]]

pal-moz: Just to confirm, you can see that icon in Firefox 6 right?  Which program is your default zip handler that the icon comes from?

Comment 22

[Brian R. Bondy [:bbondy]]

Holding off on review until I get more info.

Comment 23

[Brian R. Bondy [:bbondy]]

I have no reasoning for needing to put all compression methods to none like I did before for 16bpp and 32bpp so I'd rather see the reference image and confirm the icon works in ff6 first.

Comment 24

[pal-moz]

(In reply to Brian R. Bondy [:bbondy] from comment #21)
> pal-moz: Just to confirm, you can see that icon in Firefox 6 right?  Which
> program is your default zip handler that the icon comes from?

no problem with Firefox 6.0.*

and my zip handler is 
Explzh : http://www.ponsoftware.com/ (Japanese) ::: http://www.ponsoftware.com/en/ (English)

Comment 25

[Brian R. Bondy [:bbondy]]

I tried extracting all 36 icons from the English version and viewing them in the download manager, but they display correctly for me with Nightly.

Could you take a screenshot of the icon that should be displayed (as is in FF6)?
Do other icons display correctly in the download manager? Or is it just .zip file ones that don't display correctly?

Comment 26

[pal-moz]

Attachment: screenshot (Nightly and 6.0.2)

(In reply to Brian R. Bondy [:bbondy] from comment #25)
> I tried extracting all 36 icons from the English version and viewing them in
> the download manager, but they display correctly for me with Nightly.
> 
> Could you take a screenshot of the icon that should be displayed (as is in
> FF6)?
> Do other icons display correctly in the download manager? Or is it just .zip
> file ones that don't display correctly?

all icons are not displayed on NIGHTLY.

Comment 27

[pal-moz]

Attachment: screenshot

and irc/ircs/mailto icons in Options>Applications are not displayed.

Comment 28

[Brian R. Bondy [:bbondy]]

Thanks for all the info so far.  One more thing, can you type any file:/// url into your browser, then prepend moz-icon:

Then save the bad icon it shows and attach.
Thanks!

Comment 29

[Brian R. Bondy [:bbondy]]

Example URI:
moz-icon:file:///c:/?size=32

Comment 30

[Brian R. Bondy [:bbondy]]

Reproduced an invisible icon on Nightly on a Win XP VM with moz-icon://.eml?size=16.  Disregard my previous request for sending a moz-icon.  Thanks for your help, I'll update once I know more.  

This is different than the previous task because this time the moz-icon is being generated correctly (I think) but we aren't displaying it, so this fix will probably be in the ICO decoder code.  I'll do the fix in the context of this same task though.

Comment 31

[pal-moz]

Attachment: screenshot

(In reply to Brian R. Bondy [:bbondy] from comment #28)
> Thanks for all the info so far.  One more thing, can you type any file:///
> url into your browser, then prepend moz-icon:
> 
> Then save the bad icon it shows and attach.
> Thanks!

(In reply to Brian R. Bondy [:bbondy] from comment #29)
> Example URI:
> moz-icon:file:///c:/?size=32

this ?

Comment 32

[Brian R. Bondy [:bbondy]]

pal-moz: I got what I need now but thanks for following up. I'm working on a fix now.

Comment 33

[Brian R. Bondy [:bbondy]]

Attachment: Reference ICO image with the regression on transparency

Comment 34

[Brian R. Bondy [:bbondy]]

Attachment: Fixed regression on transparency for 32BPP ICOs which don't use the alpha data byte.

Fixed. 

Attachment 558143 [details] shows a regressed ICO.  The problem was with 32bpp ICOs which don't use the 4th byte for alpha data (i.e. when all of them are 0).  This caused the ICO to be fully transparent. 

To fix I just restored the fix that was there before I did Bug 600556.  The regression was introduced as of Bug 60056 Comment 30.

Comment 35

[Brian R. Bondy [:bbondy]]

By the way I tested the patch with all previous reftests, and also tried transparent bitmaps with and without the AND mask.  And with and without using the 4th byte of alpha data. The reproduced moz-icon on XP also works once I use a build with this patch.

Comment 36

[[e-mail]]

Comment on attachment 558145 [details] [diff] [review]
Fixed regression on transparency for 32BPP ICOs which don't use the alpha data byte.

>-        else if (mBIH.compression != BI_BITFIELDS && mBIH.bpp == 32) {
>-          // Use default 8-8-8 format
>-          mBitFields.red   = 0xFF0000;
>-          mBitFields.green = 0x00FF00;
>-          mBitFields.blue  = 0x0000FF;
>-          CalcBitShift();
>-        }
>+        else if (mBIH.compression != BI_BITFIELDS && mBIH.bpp == 32) {
>+          // Use default 8-8-8 format
>+          mBitFields.red   = 0xFF0000;
>+          mBitFields.green = 0x00FF00;
>+          mBitFields.blue  = 0x0000FF;
>+          CalcBitShift();
>+        }
???

>+                            SetPixel(d,red, green, blue, mHaveAlphaData ? p[3] : 0xFF);
Nit: space after first comma got lost.

Comment 37

[Brian R. Bondy [:bbondy]]

>-        else if (mBIH.compression != BI_BITFIELDS && mBIH.bpp == 32) {
>-          // Use default 8-8-8 format
>-          mBitFields.red   = 0xFF0000;
>-          mBitFields.green = 0x00FF00;
>-          mBitFields.blue  = 0x0000FF;
>-          CalcBitShift();
>-        }
>+        else if (mBIH.compression != BI_BITFIELDS && mBIH.bpp == 32) {
>+          // Use default 8-8-8 format
>+          mBitFields.red   = 0xFF0000;
>+          mBitFields.green = 0x00FF00;
>+          mBitFields.blue  = 0x0000FF;
>+          CalcBitShift();
>+        }
> ???

I had a patch somewhere in my patch queue with wrong line endings there.  This will not be there when I rebase the patch to push it.

Comment 38

[Brian R. Bondy [:bbondy]]

Attachment: Fixed regression on transparency for 32BPP ICOs which don't use the alpha data byte. v2

Fixed Neil's nit and re-based to tip.

Comment 39

[Joe Drew (not getting mail)]

Comment on attachment 558455 [details] [diff] [review]
Fixed regression on transparency for 32BPP ICOs which don't use the alpha data byte. v2

Review of attachment 558455 [details] [diff] [review]:
-----------------------------------------------------------------

r+ because I think the code existed before, but I really don't understand why it works. Some explanation is required before this can be committed, I think!

::: modules/libpr0n/decoders/nsBMPDecoder.cpp
@@ +499,5 @@
>                            if (mUseAlphaData) {
> +                            if (!mHaveAlphaData && p[3]) {
> +                              // Non-zero alpha byte detected! Clear previous 
> +                              // pixels from current row until the end of the 
> +                              // bitmap. This works because we know that if we 

Shouldn't we actually clear from the beginning of the bitmap (not just the row) to where we currently are? Why does this work?

@@ +504,5 @@
> +                              // are reaching here then the alpha data in byte 
> +                              // 4 has been right all along.  And we know it
> +                              // has been set to 0 the whole time, so that 
> +                              // means that everything is transparent so far.
> +                              memset(mImageData + (mCurLine -1) * GetWidth(), 0, 

put a space between - and 1 here

::: modules/libpr0n/decoders/nsBMPDecoder.h
@@ +118,5 @@
>      /** Set mBIH from the raw data in mRawBuf, converting from little-endian
>       * data to native data as necessary */
>      void ProcessInfoHeader();
>  
> +    // Stores whether the image data may stores alpha data, or if

"may stores"

Comment 40

[Brian R. Bondy [:bbondy]]

> Shouldn't we actually clear from the beginning of the bitmap (not just the row) to where we currently are? Why does this work?

It's doing what you think it should be doing. 

This took me a while to figure out originally too because the first part of the comment which always existed sounds like it's only for the row.  I tried to clarify more that it IS for the whole bitmap though after it.  I'll clarify this comment more to say it is clearing the whole bitmap processed so far before pushing.

Comment 41

[Brian R. Bondy [:bbondy]]

> "Clear previous pixels from current row until the end of the bitmap" 

The reason the original author probably put it this way is because bitmaps are stored bottom up.

Comment 42

[Brian R. Bondy [:bbondy]]

(Unless they have negative height specified in which case it's top down, but it's more common to have bottom up)

Comment 43

[Joe Drew (not getting mail)]

(In reply to Brian R. Bondy [:bbondy] from comment #41)
> > "Clear previous pixels from current row until the end of the bitmap" 
> 
> The reason the original author probably put it this way is because bitmaps
> are stored bottom up.

Oh, cripes. Yeah.

Comment 44

[Brian R. Bondy [:bbondy]]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ec179e716216

Comment 45

[Brian R. Bondy [:bbondy]]

Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/72bace03b6ce

Comment 46

[Justin Wood (:Callek)]

(In reply to Brian R. Bondy [:bbondy] from comment #45)
> Pushed to mozilla-inbound:
> http://hg.mozilla.org/integration/mozilla-inbound/rev/72bace03b6ce

And now on m-c

Comment 47

[pal-moz]

(In reply to pal-moz from comment #16)
> on Windows XP, no icon in download manager.
> other bug ?

(In reply to Justin Wood (:Callek) from comment #46)
> (In reply to Brian R. Bondy [:bbondy] from comment #45)
> > Pushed to mozilla-inbound:
> > http://hg.mozilla.org/integration/mozilla-inbound/rev/72bace03b6ce
> 
> And now on m-c

fixed on XP with latest 0910 NIGHTLY.
thanks.