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

RESOLVED WORKSFORME

Status

()

Core
Layout: Images
RESOLVED WORKSFORME
4 years ago
2 years ago

People

(Reporter: PhobosK, Assigned: milan)

Tracking

({qawanted, testcase-wanted})

26 Branch
x86_64
Linux
qawanted, testcase-wanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
Created attachment 8348381 [details]
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.
(Reporter)

Comment 1

4 years ago
Created attachment 8348383 [details]
Missing images in media tab
(Reporter)

Comment 2

4 years ago
Created attachment 8348385 [details]
How it should look normally (in older FF versions it did)
(Reporter)

Comment 3

4 years ago
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.

Updated

4 years ago
Component: Untriaged → Layout: Images
Product: Firefox → Core
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)....
Keywords: qawanted, testcase-wanted
(Reporter)

Comment 5

4 years ago
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
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?
(Reporter)

Comment 7

4 years ago
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 <milan@mozilla.com>
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
Thank you for bisecting that (and sorry for the lag; I was offline for a few days).

Milan, Seth, could you take a look?
Blocks: 905793
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(seth)
Flags: needinfo?(milan)
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.
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.
Yes, backing out the patch from bug 905793 does make the problem go away.
(Reporter)

Comment 12

4 years ago
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...
At the same time, backing out the patch from bug 905793 regresses bug 899861.
Flags: needinfo?(milan)
Nothing weird about the image itself; just using background-position to switch between click/over/etc. states.
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.
Created attachment 8428857 [details] [diff] [review]
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.
Attachment #8428857 - Flags: review?(seth)
Assignee: nobody → milan
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.
NI ping for Seth...
Flags: needinfo?(seth)
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.
Flags: needinfo?(seth)
(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.
Flags: needinfo?(seth)
Attachment #8428857 - Flags: review?(seth)
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.
@Reporter: Is this still a concern?
If yes provide simplified STR & current Firefox version.
Flags: needinfo?(phobosk)
(Reporter)

Comment 23

2 years ago
It was a regression fixed long ago.... Now on FF 43.0 this still works ok, so you may change to resolved fixed...
Flags: needinfo?(phobosk)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.