953060 - IBM II4C authored GIF file does not display in browser

Status: NEW

(Core :: Graphics: ImageLib, defect, P3)

Description

[Cory]

Attachment: 08-30-50.gif

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Steps to reproduce:

To reproduce this issue simply attempt to open the GIF file using File | Open File...


Actual results:

When the browser attempts to display the GIF file it cannot.  In it's place you are simply presented the image of a broken file.  Using inspect element you can sometime get the alt text to read "The image “file:///C:/xxxx/08-30-50.gif” cannot be displayed because it contains errors.""
This GIF file was created using the IBM II4C libraries.  These GIF file work in all Firefox browsers up to Firefox 21.


Expected results:

The GIF image should simply display in the browser.  The sample GIF is attached.

Comment 1

[Cory]

Attachment: GIF with FF24.jpg

image of FF24 displaying the GIF file.

Comment 2

[Cory]

Attachment: GIF with MS Office PIcture Manager.jpg

image of MS Office Picture Manager opening the GIF file.

Comment 3

[Cory]

Attachment: GIF with IE8.jpg

GIF opening with IE8.

Comment 4

[Cory]

Further details.
-  these GIF files have been working with FF14 for about 1.5 years now.
-  All the FF browsers were tested up to 26.  It seems that GIF displays in FF21, but not FF22.
-  the GIF files are generated on the fly using the IBM II4C libraries.  The GIFs are intended to show a small rendered version of a larger document.
-  IE8 and I think IE10 display the GIF fine.

Comment 5

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c9c29cc97af0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130320 Firefox/22.0 ID:20130320062640
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1407288d820b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130320 Firefox/22.0 ID:20130320074043
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c9c29cc97af0&tochange=1407288d820b

Regresssed by Bug 716140

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Milan, can you find someone to fix this regression?

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Seth has been looking at the regressions in this area.

Comment 8

[Joel Wheeler]

Any updates with this? Having the exact same issue, with files from another source (not IBM II4C).

Comment 9

[Joel Wheeler]

I traced this to the nsGIFDecoder2::DoLzw() function in image/decoders/nsGIFDecoder2.cpp...

There is a check in that function which does the following:

      // Check for explicit end-of-stream code
      if (code == (clear_code + 1)) {
        // end-of-stream should only appear after all image data
        return (mGIFStruct.rows_remaining == 0);
      }

In the case of my problem image, and the 08-30-50.gif attached here, rows_remaining==1, which causes DoLzw() to return false, which causes the caller of DoLzw() to think that there was some error reading the image.

However, all online documentation that I have found referring to the LZW/GIF "EndOfInformation" code (aka "ClearCode+1"), states simply that when you hit this code, you're done. No auditing of the remaining data appears to be necessary.

I changed this block of code to:

      // Check for explicit end-of-information (EOI) code
      if (code == (clear_code + 1)) {
        // EOI code indicates end of data stream
        return true;
      }

and that fixed the image viewing problem, for both my image and the 08-30.gif image attached to this bug.

Does this seem like something that can rolled out? What is the process for getting a fix committed and distributed? I'm looking to get a fix specifically for FF24, asap.

Thanks.

Comment 10

[Seth Fowler [:seth] [:s2h]]

(In reply to Joel Wheeler from comment #9)
> Does this seem like something that can rolled out? What is the process for
> getting a fix committed and distributed? I'm looking to get a fix
> specifically for FF24, asap.

Thanks for identifying the issue!

I'd be happy to take care of the process of landing a patch for this. Given that the fix seems fairly simple and well-contained, we could probably uplift it to the current ESR (31). However, unfortunately, I don't think that we can uplift it to the previous ESR (24).

Lukas, is my understanding correct?

Comment 11

[Seth Fowler [:seth] [:s2h]]

So regarding the End of Information code, the GIF89a spec says:

> 2. An End of Information code is defined that explicitly indicates the end of
> the image data stream. LZW processing terminates when this code is encountered.
> It must be the last code output by the encoder for an image. The value of this
> code is <Clear code>+1.

The GIF87a spec has identical language.

My reading of this language is that the original code is correct - if these images have data after the End of Information code, they're invalid. If this wasn't a regression, based upon the spec I'd be hesitant to make this change. However, it seems that we did support the GIF file attached to this bug at one time.

I think it's worth trying to determine what we changed in bug 716140 that made us reject this previously accepted file. I seem to recall that we became more strict about errors when decoding images in that bug, and if that's the only reason we now reject this image, the change Joel proposes in comment 9 seems reasonable.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Alright. I just took a look at the changes in bug 716140 and it does indeed look like we stopped ignoring some kinds of decoder errors in that bug. Since the error in this case is encountered after the entire image is decoded and written to the imgFrame, it's not surprising that in the past, when we ignored those errors, the image displayed successfully even though the decoder bailed at the very end. Now, since we mark the image as having an error, we don't even try to display it.

I think this is OK for us to support, but I'm going to allow as narrow as possible a deviation from the spec. It sounds like allowing rows_remaining to be either 0 or 1 is enough to get these images working.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: Be more flexible about GIF End of Information code

Here's the patch. We allow rows_remaining to be 0 or 1 without reporting an error. For any other value, we print an NS_WARNING and report an error. I've confirmed that this is enough to fix the image attached to this bug, and according to comment 9 it should be enough to fix Joel's image too.

Comment 14

[Joel Wheeler]

Thanks for looking into this some more.

My interpretation of the following:

> 2. An End of Information code is defined that explicitly indicates the end of
> the image data stream. LZW processing terminates when this code is encountered.
> It must be the last code output by the encoder for an image. The value of this
> code is <Clear code>+1.

is a little looser. I came across this same definition. But, to me it states that while it must be the last "code" output by the encoder, it doesn't explicitly exclude the possibility that there could be extra "garbage" bytes after that last code. Or, stated another way, these garbage bytes are not "codes" (almost by definition, because we just encountered the last possible code), they are "garbage", and are not excluded.

But, I can see both sides... Either way, if it works for my images I'm happy. I just don't want to have to revisit this when the images I'm getting have rows_remaining == 2 :)

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

[Tracking Requested - why for this release]:

Correct, we no longer support esr 24 and there will be no more builds for that branch.  ESR 31 will ship in two weeks so if it can be nominated for uplift before that, great.

Comment 16

[Joel Wheeler]

Do I have to nominate it? Or Seth? Anything more I need to do on my part? Thanks.

Comment 17

[Seth Fowler [:seth] [:s2h]]

(In reply to Joel Wheeler from comment #16)
> Do I have to nominate it? Or Seth? Anything more I need to do on my part?
> Thanks.

I'll take it from here. Nominating it for uplift comes a little latter; it needs to get reviewed and the code needs to land first.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8539645 [details] [diff] [review]
Be more flexible about GIF End of Information code

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

I'm not sure this is the right approach. It seems like we should just ignore the error like we did before. i.e. we should make a best effort attempt to display the data that we have and not throw it away. I'd be surprised if other browsers only special cased the last row.

Comment 19

[Seth Fowler [:seth] [:s2h]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #18)
> I'm not sure this is the right approach. It seems like we should just ignore
> the error like we did before. i.e. we should make a best effort attempt to
> display the data that we have and not throw it away. I'd be surprised if
> other browsers only special cased the last row.

I agree that other browsers probably don't special case the last row, but I still think this is the way to go. We detect errors during decode asynchronously, and we have no way of reliably telling if the error occurred at the very end of the image or at some other time. Standards require us to display the image as broken if we encounter an error in the data during decoding, and there are security implications if the decoder doesn't write to its entire buffer, so we can't just ignore arbitrary errors. So the only path I see forward is to special case this specific error, since ignoring it is safe. And that's exactly what the patch does.

Comment 20

[Seth Fowler [:seth] [:s2h]]

Needinfo'ing because I was hoping to uplift this.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

(In reply to Seth Fowler [:seth] from comment #19)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #18)
> > I'm not sure this is the right approach. It seems like we should just ignore
> > the error like we did before. i.e. we should make a best effort attempt to
> > display the data that we have and not throw it away. I'd be surprised if
> > other browsers only special cased the last row.
> 
> I agree that other browsers probably don't special case the last row, but I
> still think this is the way to go. We detect errors during decode
> asynchronously, and we have no way of reliably telling if the error occurred
> at the very end of the image or at some other time. 

I'm suggesting that we shouldn't treat the error specially regardless of when it happens. We should always make a best effort to display images.

> Standards require us to
> display the image as broken if we encounter an error in the data during
> decoding,

Which standards are those? If all the other browsers don't comply with the standards then perhaps the standards should be changed.

> and there are security implications if the decoder doesn't write
> to its entire buffer, so we can't just ignore arbitrary errors. So the only
> path I see forward is to special case this specific error, since ignoring it
> is safe. And that's exactly what the patch does.

I believe this gif is missing the last pixel so we're still not writing the entire buffer. Also, if you just chop some bytes off the attached gif it displays fine.

Comment 22

[Seth Fowler [:seth] [:s2h]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #21)
> Which standards are those? If all the other browsers don't comply with the
> standards then perhaps the standards should be changed.

Hmm.. I just checked, and apparently my memory of the WHATWG image loading spec is incorrect. You're right; as long as we can obtain the dimensions, we should attempt to display whatever we can do, rather than display a broken image icon. And since it seems that Chrome and Safari implement image loading in this way, web compatibility also suggests that we should do the same.

Joel, Cory, my apologies, but I have to agree with Jeff that this isn't the right fix. I'll look into changing our decoding code to conform with the spec, but unfortunately doing so will require some thought about security and will increase the scope of this bug substantially. Therefore I'm sorry to say that there is no chance a fix will be uplifted to the next ESR, and indeed it won't make FF 37 either.

Comment 23

[Joel Wheeler]

I'm not sure that I follow... 

Isn't Jeff's suggestion that we just remove the special case "if (mGIFStruct.rows_remaining == 0 || mGIFStruct.rows_remaining == 1) {..." ? 
(when he states "I'm suggesting that we shouldn't treat the error specially regardless of when it happens. We should always make a best effort to display images.")

I also still contend that this does not need to be treated as an error. The spec states that the end of image code is the end of the data stream and that no other codes follow. It doesn't exclude the possibility of additional non-code bytes after the end of image code.

Respectfully, the bug has sat for a year with no action and now there is a simple one line fix which corrects the issue. I don't know what the WHATWG spec is, but it seems like the current fix corrects the issue within the quidelines of "as long as we can obtain the dimensions, we should attempt to display whatever we can do, rather than display a broken image icon. And since it seems that Chrome and Safari implement image loading in this way, web compatibility also suggests that we should do the same."

I respect whatever decision is made, I'm just a little confused why the current fix doesn't fix image viewing in the "right" way?

Comment 24

[Seth Fowler [:seth] [:s2h]]

(In reply to Joel Wheeler from comment #23)
> I'm not sure that I follow... 

I think that what's perhaps not obvious to someone not familiar with the code is that we cannot distinguish between different kinds of errors that happen inside the GIF decoder, so we can't easily ignore *just this error*. If we don't want to add the hack that the patch currently in this bug implements (and the consensus is that we don't, because it doesn't fix the root of the problem), then what we have to do *is* to fix the root of the problem, which is wrong error handling behavior with respect to the spec. That is more complicated, unfortunately, and it will take some time to get done.

I know this is pretty frustrating, and I'm sorry for the delay. Fixing this the right way will be worth it, though, as it will fix many other similar issues as well, and not just this specific one.

Comment 25

[Joel Wheeler]

Thanks for the clarification.

The delay is moot now as we've worked around the issue by deploying a different browser.

Comment 26

[u279076]

This bug still reproduces.