The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla9

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: darktrojan, Assigned: bbondy)

Tracking

({regression})

Trunk
mozilla9
x86
Windows Vista
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 8 obsolete attachments)

1.12 KB, image/vnd.microsoft.icon
Details
1.51 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
1.22 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
4.19 KB, image/vnd.microsoft.icon
Details
7.17 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
Keywords: regression
(Assignee)

Updated

6 years ago
Assignee: nobody → netzen
(Assignee)

Comment 1

6 years ago
Do you know a page or reference image I can reproduce with?
I think Bug 682696 is related to this bug.
(Assignee)

Comment 3

6 years ago
Created attachment 556418 [details]
Proof that Firefox 6 generates invalid ICO files from  moz-icon:file:///C:/Users/YOURUSERNAME/Downloads/?size=16
(Assignee)

Updated

6 years ago
Duplicate of this bug: 682696

Comment 5

6 years ago
http://forums.mozillazine.org/viewtopic.php?p=11185685#p11185685
(Assignee)

Comment 6

6 years ago
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.
Attachment #556464 - Flags: review?(joe)
(Assignee)

Updated

6 years ago
Attachment #556418 - Attachment description: Reference ICO file with the problem → Proof that Firefox 6 generates invalid ICO files from moz-icon:file:///C:/Users/YOURUSERNAME/Downloads/?size=16
(Assignee)

Updated

6 years ago
Summary: Some icons missing first 4 pixels since recent changes → Some icons missing first 3 pixels because FF6 moz-icon URIs sometimes generate bad icons
(Assignee)

Comment 7

6 years ago
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=ff00b183a230
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. :)
Attachment #556464 - Flags: review?(joe) → review+
(Assignee)

Comment 9

6 years ago
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.
Attachment #556464 - Attachment is obsolete: true
Attachment #556758 - Flags: review+
(Assignee)

Comment 10

6 years ago
Pushed to mozilla-inbound
http://hg.mozilla.org/integration/mozilla-inbound/rev/d48ac4bbb8e6
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.
http://hg.mozilla.org/mozilla-central/rev/d48ac4bbb8e6

btw, I guess you may want to address comment 11
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
(Assignee)

Comment 13

6 years ago
Created attachment 557127 [details] [diff] [review]
Formatting fix for previous push

No logic change, just fixed formatting
Attachment #557127 - Flags: review?(joe)
Attachment #557127 - Flags: review?(joe) → review+
(Assignee)

Comment 14

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

Comment 16

6 years ago
on Windows XP, no icon in download manager.
other bug ?
(Assignee)

Comment 17

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

Comment 18

6 years ago
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.
(Assignee)

Comment 19

6 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

6 years ago
Created attachment 558027 [details] [diff] [review]
Should always use no compression for moz-icons
Attachment #558027 - Flags: review?(joe)
(Assignee)

Comment 21

6 years ago
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?
(Assignee)

Updated

6 years ago
Attachment #558027 - Flags: review?(joe)
(Assignee)

Comment 22

6 years ago
Holding off on review until I get more info.
(Assignee)

Comment 23

6 years ago
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

6 years ago
(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)
(Assignee)

Comment 25

6 years ago
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

6 years ago
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

6 years ago
Created attachment 558063 [details]
screenshot

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

Comment 28

6 years ago
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!
(Assignee)

Comment 29

6 years ago
Example URI:
moz-icon:file:///c:/?size=32
(Assignee)

Comment 30

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #558027 - Attachment is obsolete: true

Comment 31

6 years ago
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 ?
(Assignee)

Comment 32

6 years ago
pal-moz: I got what I need now but thanks for following up. I'm working on a fix now.
(Assignee)

Updated

6 years ago
Attachment #558025 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #558061 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #558063 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #558105 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #556376 - Attachment is obsolete: true
(Assignee)

Comment 33

6 years ago
Created attachment 558143 [details]
Reference ICO image with the regression on transparency
(Assignee)

Comment 34

6 years ago
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.
Attachment #558145 - Flags: review?(joe)
(Assignee)

Comment 35

6 years ago
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 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.
(Assignee)

Comment 37

6 years ago
>-        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.
(Assignee)

Comment 38

6 years ago
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.
Attachment #558145 - Attachment is obsolete: true
Attachment #558145 - Flags: review?(joe)
Attachment #558455 - Flags: review?(joe)
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"
Attachment #558455 - Flags: review?(joe) → review+
(Assignee)

Comment 40

6 years ago
> 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.
(Assignee)

Comment 41

6 years ago
> "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.
(Assignee)

Comment 42

6 years ago
(Unless they have negative height specified in which case it's top down, but it's more common to have bottom up)
(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.
(Assignee)

Comment 44

6 years ago
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ec179e716216
(Assignee)

Comment 45

6 years ago
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/72bace03b6ce
(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
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 47

6 years ago
(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.
Depends on: 775793
You need to log in before you can comment on or make changes to this bug.