Last Comment Bug 682688 - Some icons missing first 3 pixels because FF6 moz-icon URIs sometimes generate bad icons
: Some icons missing first 3 pixels because FF6 moz-icon URIs sometimes generat...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
: 682696 (view as bug list)
Depends on: 775793
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-28 02:30 PDT by Geoff Lankow (:darktrojan)
Modified: 2012-07-20 00:28 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot (4.56 KB, image/png)
2011-08-28 02:30 PDT, Geoff Lankow (:darktrojan)
no flags Details
Proof that Firefox 6 generates invalid ICO files from moz-icon:file:///C:/Users/YOURUSERNAME/Downloads/?size=16 (1.12 KB, image/vnd.microsoft.icon)
2011-08-28 12:10 PDT, Brian R. Bondy [:bbondy]
no flags Details
Fixed Firefox 6 bug in moz-icon creating invalid ICOs sometimes (1.25 KB, patch)
2011-08-28 20:53 PDT, Brian R. Bondy [:bbondy]
joe: review+
Details | Diff | Splinter Review
Fixed Firefox 6 bug in moz-icon creating invalid ICOs sometimes v2 (1.51 KB, patch)
2011-08-29 22:42 PDT, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review
Formatting fix for previous push (1.22 KB, patch)
2011-08-31 05:33 PDT, Brian R. Bondy [:bbondy]
joe: review+
Details | Diff | Splinter Review
screenshot (176.12 KB, image/jpeg)
2011-09-02 20:14 PDT, pal-moz
no flags Details
Should always use no compression for moz-icons (2.47 KB, patch)
2011-09-02 21:03 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
screenshot (Nightly and 6.0.2) (177.97 KB, image/jpeg)
2011-09-03 06:03 PDT, pal-moz
no flags Details
screenshot (159.86 KB, image/jpeg)
2011-09-03 07:09 PDT, pal-moz
no flags Details
screenshot (11.02 KB, image/jpeg)
2011-09-03 15:46 PDT, pal-moz
no flags Details
Reference ICO image with the regression on transparency (4.19 KB, image/vnd.microsoft.icon)
2011-09-04 06:45 PDT, Brian R. Bondy [:bbondy]
no flags Details
Fixed regression on transparency for 32BPP ICOs which don't use the alpha data byte. (8.51 KB, patch)
2011-09-04 07:10 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Fixed regression on transparency for 32BPP ICOs which don't use the alpha data byte. v2 (7.17 KB, patch)
2011-09-06 06:10 PDT, Brian R. Bondy [:bbondy]
joe: review+
Details | Diff | Splinter Review

Description Geoff Lankow (:darktrojan) 2011-08-28 02:30:24 PDT
Created attachment 556376 [details]
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] 2011-08-28 04:36:19 PDT
Do you know a page or reference image I can reproduce with?
Comment 2 Richard Marti (:Paenglab) 2011-08-28 06:11:12 PDT
I think Bug 682696 is related to this bug.
Comment 3 Brian R. Bondy [:bbondy] 2011-08-28 12:10:02 PDT
Created attachment 556418 [details]
Proof that Firefox 6 generates invalid ICO files from  moz-icon:file:///C:/Users/YOURUSERNAME/Downloads/?size=16
Comment 4 Brian R. Bondy [:bbondy] 2011-08-28 12:11:06 PDT
*** Bug 682696 has been marked as a duplicate of this bug. ***
Comment 6 Brian R. Bondy [:bbondy] 2011-08-28 20:53:22 PDT
Created attachment 556464 [details] [diff] [review]
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] 2011-08-28 21:20:26 PDT
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=ff00b183a230
Comment 8 Joe Drew (not getting mail) 2011-08-29 13:14:53 PDT
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] 2011-08-29 22:42:00 PDT
Created attachment 556758 [details] [diff] [review]
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] 2011-08-29 22:46:19 PDT
Pushed to mozilla-inbound
http://hg.mozilla.org/integration/mozilla-inbound/rev/d48ac4bbb8e6
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2011-08-31 01:52:52 PDT
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] 2011-08-31 01:57:30 PDT
http://hg.mozilla.org/mozilla-central/rev/d48ac4bbb8e6

btw, I guess you may want to address comment 11
Comment 13 Brian R. Bondy [:bbondy] 2011-08-31 05:33:37 PDT
Created attachment 557127 [details] [diff] [review]
Formatting fix for previous push

No logic change, just fixed formatting
Comment 14 Brian R. Bondy [:bbondy] 2011-08-31 10:59:35 PDT
Formatting change was pushed to mozilla-inbound by the way:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5117dd889921
Comment 15 Ed Morley [:emorley] 2011-09-01 01:46:36 PDT
http://hg.mozilla.org/mozilla-central/rev/5117dd889921
Comment 16 pal-moz 2011-09-02 18:55:14 PDT
on Windows XP, no icon in download manager.
other bug ?
Comment 17 Brian R. Bondy [:bbondy] 2011-09-02 18:56:13 PDT
same bug, but I think you're using an older build of Nightly.
Comment 18 pal-moz 2011-09-02 20:14:54 PDT
Created attachment 558025 [details]
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] 2011-09-02 20:42:03 PDT
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] 2011-09-02 21:03:12 PDT
Created attachment 558027 [details] [diff] [review]
Should always use no compression for moz-icons
Comment 21 Brian R. Bondy [:bbondy] 2011-09-02 21:04:17 PDT
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] 2011-09-02 21:16:00 PDT
Holding off on review until I get more info.
Comment 23 Brian R. Bondy [:bbondy] 2011-09-02 21:25:36 PDT
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 2011-09-02 21:42:31 PDT
(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] 2011-09-03 05:12:20 PDT
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 2011-09-03 06:03:08 PDT
Created attachment 558061 [details]
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 2011-09-03 07:09:05 PDT
Created attachment 558063 [details]
screenshot

and irc/ircs/mailto icons in Options>Applications are not displayed.
Comment 28 Brian R. Bondy [:bbondy] 2011-09-03 10:49:18 PDT
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] 2011-09-03 11:41:52 PDT
Example URI:
moz-icon:file:///c:/?size=32
Comment 30 Brian R. Bondy [:bbondy] 2011-09-03 13:29:56 PDT
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 2011-09-03 15:46:56 PDT
Created attachment 558105 [details]
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] 2011-09-03 16:44:41 PDT
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] 2011-09-04 06:45:54 PDT
Created attachment 558143 [details]
Reference ICO image with the regression on transparency
Comment 34 Brian R. Bondy [:bbondy] 2011-09-04 07:10:35 PDT
Created attachment 558145 [details] [diff] [review]
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] 2011-09-04 07:16:17 PDT
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 neil@parkwaycc.co.uk 2011-09-06 00:43:05 PDT
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] 2011-09-06 04:10:54 PDT
>-        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] 2011-09-06 06:10:15 PDT
Created attachment 558455 [details] [diff] [review]
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) 2011-09-08 12:27:56 PDT
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] 2011-09-08 12:31:02 PDT
> 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] 2011-09-08 12:32:10 PDT
> "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] 2011-09-08 12:32:56 PDT
(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) 2011-09-08 12:34:42 PDT
(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] 2011-09-08 13:37:04 PDT
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ec179e716216
Comment 45 Brian R. Bondy [:bbondy] 2011-09-09 08:28:34 PDT
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/72bace03b6ce
Comment 46 Justin Wood (:Callek) (Away until Aug 29) 2011-09-09 22:39:31 PDT
(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 2011-09-10 06:28:25 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.