1240665 - Make image loading error log messages more accurate

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Starke]

Attachment: example.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.111 Safari/537.36

Steps to reproduce:

I did open a large PNG file like the one attached with Firefox ESR 38.5.2.


Actual results:

Firefox only shows a black page with "The image ... cannot be displayed because it contains errors."


Expected results:

Chrome, Internet Explorer, Photoshop or IrfanView for example show no issue in opening this file.
As the file itself is totally fine, Firefox should at least display a different error than "file is broken". This is really confusing.
The best solution would be to make Firefox support such PNG files.

Comment 1

[Glenn Randers-Pehrson]

To get more useful information, run firefox from a console window, with
NSPR_LOG_MODULES=PNGDecoder:3 in the environment.  Then besides the
"image cannot be displayed" screen you also get the following in the
console window:

[0x7f02ee2603e0]: W/PNGDecoder libpng warning: Image width exceeds user limit in IHDR
[0x7f02ee2603e0]: E/PNGDecoder libpng error: Invalid IHDR data

Comment 2

[Glenn Randers-Pehrson]

See bug #408568 which is the same issue, but with JPEG.

Comment 3

[Glenn Randers-Pehrson]

To get more useful information, run firefox from a console window, with
NSPR_LOG_MODULES=PNGDecoder:3 in the environment.  Then besides the
"image cannot be displayed" screen you also get the following in the
console window:

[0x7f02ee2603e0]: W/PNGDecoder libpng warning: Image width exceeds user limit in IHDR
[0x7f02ee2603e0]: E/PNGDecoder libpng error: Invalid IHDR data

Comment 4

[Daniel Starke]

I did not see such a message within the console windows of my Firefox.
And the JPEG bug is quite old and still unsolved.
But anyway, will there be a bugfix for this or is there a workaround available?
Maybe some magic setting in about:config?

Comment 5

[comethope]

In recent months I have been finding a lot of jpg and png images on Imgur which 'contain errors' 
on ff42 vista, but render fine on everything else.

Here is a 2160 x 3840 px image of a pepsi bottle which firefox wont render
  http://i.imgur.com/jmxTwAp.jpg

I flick through imgur gallery randomly 4 images until i get another which firefox wont render.
  http://i.imgur.com/Efon9ni.jpg  2546 x 1738px - a slightly crude meme 

I flick randomly again, this see 17 successful images before another broken jpg
  http://i.imgur.com/San5GYN.jpg  2895 x 2895 - a celeb mugshot

Once more, just 5 images before the next broken
  http://i.imgur.com/p5K3PWc.jpg 2650 x 1600px - nature satelite photo or such

I think ive noticed this since around ff39, it looks like a serious issue at large
with the new image decoding.

Comment 6

[comethope]

However after a restart, they all load in fine. This issue's example.png does not however.
Sorry if this isnt the right place to have noted my intermittent problem.

Comment 7

[Glenn Randers-Pehrson]

Attachment: v00-1240665-log-all-png-errors

Some errors in image/decoders/nsPNGDecoder.cpp call png_longjmp() instead of png_error(), which bypasses the MOZ_LOG() call.  This patch changes these png_longjmp() calls to png_error() calls.

Comment 8

[Glenn Randers-Pehrson]

Note that the two "SuccessfulEarlyFinish" calls to png_longjmp() are left intact by the v00 patch, because it's appropriate for these to not emit an error message.

Comment 9

[Glenn Randers-Pehrson]

Attachment: v01-1240665-log-all-png-errors

Rebase, fix typo in one error message

Comment 10

[Renato Alves]

The following link is a bug affecting imlib2. I don't know if Firefox uses or shares any of the same code but Firefox has similar symptoms (without the segfault).

https://github.com/muennich/sxiv/issues/236#issuecomment-220143248

It seems that if either width or height is bigger than 32768 pixels rendering of the image fails.

Comment 11

[Glenn Randers-Pehrson]

Attachment: v02-1240665-log-all-png-errors

Fix bit rot due to landing of bug #1255107

Comment 12

[Masatoshi Kimura [:emk]]

Why is more logging needed?

>#  define MOZ_PNG_MAX_DIMENSION 32767
>   if (width > MOZ_PNG_MAX_DIMENSION || height > MOZ_PNG_MAX_DIMENSION) {
>-    png_longjmp(decoder->mPNG, 1);
>+    png_error(decoder->mPNG," Image dimensions exceed Mozilla's limits");
>   }

Obviously this is the cause of the bug. The comment #0 image has 93765 px wide.

Comment 14

[Glenn Randers-Pehrson]

In my opinion, "Image dimensions exceed Mozilla's limits" is more obvious (and accurate) than "The image ... cannot be displayed because it contains errors."

Comment 15

[Masatoshi Kimura [:emk]]

Oh, is this message user-visible? I see.

Comment 16

[Glenn Randers-Pehrson]

It only makes the message user-visible when the user runs Firefox from a terminal/console window.  It would be nice to put the libpng message in the image window, where the "The image ... cannot be displayed because it contains errors" is currently displayed, but the patch doesn't do that.

Comment 17

[Glenn Randers-Pehrson]

Attachment: v03-1240665-log-all-png-errors

Rebased after landing bug #1282566.  Please "try".

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49eb610788df525cc2a00d07544477750e1704f0

Comment 19

[Glenn Randers-Pehrson]

Comment on attachment 8766838 [details] [diff] [review]
v03-1240665-log-all-png-errors

"try" result is very green.  R?

Comment 20

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

Comment on attachment 8766838 [details] [diff] [review]
v03-1240665-log-all-png-errors

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

Thanks Glenn! r+, but please fix the formatting issues below.

::: image/decoders/nsPNGDecoder.cpp
@@ +550,5 @@
>                 &interlace_type, &compression_type, &filter_type);
>  
>    // Are we too big?
>    if (width > MOZ_PNG_MAX_DIMENSION || height > MOZ_PNG_MAX_DIMENSION) {
> +    png_error(decoder->mPNG," Image dimensions exceed Mozilla's limits");

Nit: |mPNG," Image| should be |mPNG, "Image| - in other words, the position of the quote and the space are reversed.

@@ +559,5 @@
>    // Post our size to the superclass
>    decoder->PostSize(frameRect.width, frameRect.height);
>    if (decoder->HasError()) {
>      // Setting the size led to an error.
> +    png_error(decoder->mPNG," Sizing error");

Same as above.

@@ +672,5 @@
>  
>      if (decoder->mDownscaler && !decoder->IsFirstFrameDecode()) {
>        MOZ_ASSERT_UNREACHABLE("Doing downscale-during-decode "
>                               "for an animated image?");
> +      png_error(decoder->mPNG,"Invalid downscale attempt"); // Abort the decode.

Nit: there's a missing space between the comma and the quote.

@@ +709,2 @@
>      if (NS_FAILED(rv)) {
> +      png_error(decoder->mPNG,"CreateFrame failed");

Same as above.

@@ +718,5 @@
>      uint32_t bpp[] = { 0, 3, 4, 3, 4 };
>      decoder->mCMSLine =
>        static_cast<uint8_t*>(malloc(bpp[channels] * frameRect.width));
>      if (!decoder->mCMSLine) {
> +      png_error(decoder->mPNG,"malloc of mCMSLine failed");

Same as above.

Comment 21

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

(In reply to Masatoshi Kimura [:emk] from comment #12)
> Why is more logging needed?
> 
> >#  define MOZ_PNG_MAX_DIMENSION 32767
> >   if (width > MOZ_PNG_MAX_DIMENSION || height > MOZ_PNG_MAX_DIMENSION) {
> >-    png_longjmp(decoder->mPNG, 1);
> >+    png_error(decoder->mPNG," Image dimensions exceed Mozilla's limits");
> >   }
> 
> Obviously this is the cause of the bug. The comment #0 image has 93765 px
> wide.

Yep, that's true. A 32767 x 32767 image is 4GB, and allocating that much memory is either dangerous or impossible on many platforms that we have to support. (We could probably relax those limits on some platforms, but it requires some careful thought; this isn't the bug to do it in.) However, we don't need to forbid images that are over 32767 in one dimension but whose total size may be much lower. The image in comment 0 is 93765px wide, but only 640px high, which works to 240MB; that's no problem. This is probably a pretty quick fix, though I need to take a look at the code and make sure we're not in danger of overflow anywhere if we naively make this change.

Comment 22

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

Since this bug is about improving the logging messages, let's just deal with that here. We'll make the image in comment 0 successfully decode in bug 1283961.

Comment 23

[Glenn Randers-Pehrson]

Attachment: v04-1240665-log-all-png-errors

Nits fixed.

Comment 24

[Glenn Randers-Pehrson]

Attachment: v05-1240665-log-all-png-errors.

Rebased.  Please transfer the r+ from the v03 patch to v05.

Comment 25

[Glenn Randers-Pehrson]

Attachment: v06-1240665-log-all-png-errors

Rebased.

Comment 26

[Glenn Randers-Pehrson]

Attachment: v07-1240665-log-all-png-errors

Rebased

Comment 27

[Glenn Randers-Pehrson]

Attachment: v08-1240665-log-all-png-errors

Rebased again, and destroy png structs before returning from error handler.  Please run this through "try".

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c065d3613add13685c92cff220c68e4202150637

Comment 29

[Glenn Randers-Pehrson]

Attachment: v09-1240665-log-all-png-errors

Removed incorrect line added to isValidICO().

Comment 30

[Glenn Randers-Pehrson]

Please retry

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18b21945f8d6

Comment 32

[Glenn Randers-Pehrson]

Comment on attachment 8774807 [details] [diff] [review]
v09-1240665-log-all-png-errors

Try B are green.  R?

Comment 33

[Glenn Randers-Pehrson]

Attachment: v10-1240665-log-all-png-errors

Rebased after landing Bug #1283961

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4581a6d4a620
Log all error returns from the PNG decoder. r=tnikkel

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4581a6d4a620