Closed Bug 395542 Opened 17 years ago Closed 17 years ago

Crash in nsPNGEncoder::AddImageFrame()

Categories

(Core :: Graphics: ImageLib, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9beta1

People

(Reporter: asmith16, Assigned: MatsPalmgren_bugz)

Details

(Keywords: crash)

Attachments

(4 files)

While playing with an extension that uses the PNG encoder to create APNGs I got firefox to crash.

I haven't tried to find the cause. Justin probably already knows what it is.

I will attache the extension, the SVG file I used, and the stack trace.

To reproduce: install the extension, check 'Render after loading' on the left, and open the svg file. After the progress bar fills up firefox crashes.
Attached file the stack trace
The extension itself is too big to attach, You can get it from here: http://littlesvr.ca/apng/svg2png/svg2png.xpi
If you try this with a debug build, you should see libpng complain about something on the console (stderr).
Nope, I don't see any libpng output.
Huh, the last frame being _setjmp() usually, iirc, means libpng ran into a warning/error, and was returning control to this point... That usually was accompanied by it printing what the error was [well, technically, by calling our nsPNGEncoder::ErrorCallback which did the output].

This might be an interesting opportunity to play with roc's Chronomancer.
Severity: normal → critical
Component: GFX → ImageLib
Keywords: crash
QA Contact: general → imagelib
Attached file stack
There seems to be some race condition in the extension code that
leads to nsPNGEncoder::AddImageFrame being called *after*
nsPNGEncoder::EndImageEncode was called (and EndImageEncode will have
deallocated mPNG at that point.)
Attached patch fix?Splinter Review
Attachment #280258 - Flags: review?(dolske)
this error only occurs when calling the extension via:
window.open('chrome://svg2png/content/main.xul', 'svg2png', '')
which i'm forced to do because of Bug 235877  .
if i run the extension in its own window with
window.open('chrome://svg2png/content/main.xul', 'svg2png', 'chrome')
there is no crash.
Comment on attachment 280258 [details] [diff] [review]
fix?


>+  // don't leak if EndImageEncode wasn't called
>+  if (mPNG) {
>+    png_destroy_write_struct(&mPNG, &mPNGinfo);
>+  }

Nit: no braces needed

>+  if (!mPNG)
>+    return NS_BASE_STREAM_CLOSED;

This should be fine; the other way to do this would be to make sure mImageBuffer is always null when mPNG is null... That would avoid the need for adding this check, but it looks like the cost would be more headaches elsewhere to make sure that's an invariant.

[I'm not a peer here, so in lieu of a r+ I'll bounce it over to Pav]
Attachment #280258 - Flags: review?(dolske) → review?(pavlov)
(In reply to comment #10)
> Nit: no braces needed

Will fix before checkin.

> This should be fine; the other way to do this would be to make sure
> mImageBuffer is always null when mPNG is null...

We still need these mPNG null-checks since EndImageEncode() deallocates it
when it's successful.
Your suggestion would make Available() fail in case there was a previous
error though, for example a few successful AddImageFrame() and then one that
fails.  I don't know this code well enough to say what is desired in that
case, but it shouldn't be too hard to implement if desired.
Attachment #280258 - Flags: superreview?(pavlov)
Assignee: nobody → mats.palmgren
Attachment #280258 - Flags: superreview?(pavlov)
Attachment #280258 - Flags: superreview+
Attachment #280258 - Flags: review?(pavlov)
Attachment #280258 - Flags: review+
Attachment #280258 - Flags: approval1.9+
With brace nit fixed:
mozilla/modules/libpr0n/encoders/png/nsPNGEncoder.cpp 	1.6 

-> FIXED
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: