950953 - Images (PNG and GIF) served without a content-type header are not rendered properly

Status: RESOLVED WORKSFORME

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[PhobosK]

Attachment: The wrong rendering of the images

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131216203329

Steps to reproduce:

I found this while using TVHeadend (https://tvheadend.org/) web interface.

Before I updated to FF v26.0 everything worked fine.
 
After the update to FF v 26.0 most of the PNG and GIF images served by TVHeadend are not rendered properly (rendered as black - probably some transparency problem - see attached image). 



Actual results:

Here is what I found:

1. The PNG and GIF images affected are served by the TVHeadend without a content-type header:

http://_URL_/static/extjs/resources/images/default/qtip/tip-anchor-sprite.gif

GET /static/extjs/resources/images/default/qtip/tip-anchor-sprite.gif HTTP/1.1
Host: _URL_
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0
Accept: image/png,image/*;q=0.8,*/*;q=0.5
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
DNT: 1
Referer: http://_URL_/static/extjs/resources/css/xtheme-blue.css
Connection: keep-alive
If-Modified-Since: Mon, 16 Dec 2013 22:44:02 GMT
Cache-Control: max-age=0

HTTP/1.1 200 OK
Server: HTS/tvheadend
Last-Modified: Mon, 16 Dec 2013 22:45:37 GMT
Expires: Mon, 16 Dec 2013 22:45:47 GMT
Cache-Control: max-age=10
Connection: Keep-Alive
Content-Encoding: gzip
Content-Length: 229


2. Most of these images are not shown in the "More information" site button -> "Media" tab (see attached image) 

3. Using the url of the image directly, opens it and renders it without any problem.

4. This problem affects both Windows and *nix. Tested with FF v26.0 on Win8, Gentoo, Ubuntu with all the same results.

5. There is a similar (but not exactly the same) bug described here: https://bugzilla.mozilla.org/show_bug.cgi?id=423689

6. Is this a regression or an intended behavior?


Expected results:

These image types should be rendered properly as they did before the update to FF v26.0 on any platform.

Comment 1

[PhobosK]

Attachment: Missing images in media tab

Comment 2

[PhobosK]

Attachment: How it should look normally (in older FF versions it did)

Comment 3

[PhobosK]

Just to add that all the test are done on a clean FF install and clean(new) profile, without any addons or altering any default configuration of FF.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Lack of content-type shouldn't affect <img>: it ignores that header.

What this bug needs is a way to reproduce (so a link to a testcase or a testcase attached to the bug)....

Comment 5

[PhobosK]

I don't know if this is a valid testcase but one can reproduce this bug following these steps:

1. Get/Use Ubuntu Saucy

2. Add the TVHeadend beta repo:
deb http://apt.tvheadend.org/beta saucy main #TvHeadEnd (beta)

3. Install tvheadend-3.5.247~g098b7de~saucy

4. Start tvheadend service 

5. Point FF v26.0 (or above - I use 28.0 currently and it still has this problem) to:
http://localhost:9981

6. You get the results in the pics I've uploaded. (No need to have any TV adaptor)


Other facts:
1. Everything works ok (visually) in Opera 12.16 (*nix and Win8), Chrome 34.0.1847.132 (*nix and Win8) and Epiphany 3.10.3 (WebKit 2.2.5)
2. The problem is in FF above 26.0 on all platforms + rekonq v2.4.2 (Using KDE 4.13.0)

Thanks

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  I'll see if I can get Saucy set up, but in the mean time, would you be willing to try out http://mozilla.github.io/mozregression/ to narrow down when the problem appeared?

Comment 7

[PhobosK]

Sorry... it took some time :)

Last good revision: bb025b6949e8 (2013-08-20)
First bad revision: b2486721572e (2013-08-21)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bb025b6949e8&tochange=b2486721572e


After further bisection... this is the final result:

The first bad revision is:
changeset:   143234:4a5379f0776d
user:        Milan Sreckovic <[e-mail]>
date:        Thu Aug 15 17:06:01 2013 -0400
summary:     Bug 905793 - Send ImageUpdated() on the whole image in PostFrameStop. This may be an overkill in some cases, dirtying twice. r=seth
Regression found using mozcommitbuilder 0.4.10 on linux2 at 2014-05-10 21:19:09

Comment 8

[Boris Zbarsky [:bzbarsky]]

Thank you for bisecting that (and sorry for the lag; I was offline for a few days).

Milan, Seth, could you take a look?

Comment 9

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

I can run this on Friday and try to confirm that backing out this change gets the problem to go away, don't have access to Linux until then.

Comment 10

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

STR from comment 5 shows the problem for me on 12.04 as well (with 28).  Let me try latest trunk and then with the backout.

Comment 11

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

Yes, backing out the patch from bug 905793 does make the problem go away.

Comment 12

[PhobosK]

It is not distro related at all... It is just all FF versions above (including) official v26

As I said the bug is valid for Windows and Linux (Ubuntu and Gentoo tested) and just out of curiosity I tried the Android version too - latest FF 29.0.1 on Android.... It is affected too...

Comment 13

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

At the same time, backing out the patch from bug 905793 regresses bug 899861.

Comment 14

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

Nothing weird about the image itself; just using background-position to switch between click/over/etc. states.

Comment 15

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

nsGIFDecoder2::EndImageFrame eventually calls Decoder::PostFrameStop. This method was modified in bug 905793 to also call ImageUpdated method on the current frame. Removing this call gets rid of the issue described here, but regresses bug 905793. If the ImageUpdated() is (eventually) called on > 1 decoded images, then both bugs are "fixed".  Or, more likely, wallpapered over. I'll put a patch together, ask for feedback, maybe it will trigger thoughts in people that actually know imagelib.

Comment 16

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

Attachment: Call image update only on subsequent frames, not on the first one.

This basically reverts the fix to bug 905793 (which fixes this one), then introduces a different fix for bug 905973 that does not break the STR in this bug.

Comment 17

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

Comment on attachment 8428857 [details] [diff] [review]
Call image update only on subsequent frames, not on the first one.

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

::: image/src/Decoder.cpp
@@ -354,5 @@
>  
>    mCurrentFrame->SetFrameDisposalMethod(aDisposalMethod);
>    mCurrentFrame->SetRawTimeout(aTimeout);
>    mCurrentFrame->SetBlendMethod(aBlendMethod);
> -  mCurrentFrame->ImageUpdated(mCurrentFrame->GetRect());

What I don't get is, why was this a problem?

I need to look into this a little more.

Comment 18

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

NI ping for Seth...

Comment 19

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

Is there a damage to doing this patch, to basically undo the damage of bug 905793, but keep the benefits?  In other words, just consider this patch as the original fix to bug 905793.
Of course, if there are issues, we shouldn't do it, but I've broken things, I wouldn't mind making it better again.

Comment 20

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

(In reply to Milan Sreckovic [:milan] from comment #19)
> Is there a damage to doing this patch, to basically undo the damage of bug
> 905793, but keep the benefits?  In other words, just consider this patch as
> the original fix to bug 905793.
> Of course, if there are issues, we shouldn't do it, but I've broken things,
> I wouldn't mind making it better again.

I think your original patch in bug 905793 was correct. I don't think that the patch in this bug is correct. You said in comment 15:

> If the ImageUpdated() is (eventually) called on > 1
> decoded images, then both bugs are "fixed".  Or, more likely, wallpapered
> over.

I think you were 100% right, and this patch just wallpapers over whatever the real problem is.

Also problematic at this point is that virtually everything related to this has been completely rewritten. I definitely would not be comfortable just landing this patch at this point, which was written against a *very* different version of Gecko.

I think there's a good chance, given how much has changed between when this bug was filed and now, that we've already fixed this bug. We really need to retest and see if it's still reproducible on Nightly. If it is, then we can look again at trying to find the underlying issue.

Comment 21

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

FWIW we would probably have fixed this a long time ago if we had STR that didn't involve TVHeadend. If we *can* still reproduce, it'd be really nice to get simpler STR.

Comment 22

[Michelle Funches - QA]

@Reporter: Is this still a concern?
If yes provide simplified STR & current Firefox version.

Comment 23

[PhobosK]

It was a regression fixed long ago.... Now on FF 43.0 this still works ok, so you may change to resolved fixed...