Closed
Bug 395542
Opened 17 years ago
Closed 17 years ago
Crash in nsPNGEncoder::AddImageFrame()
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9beta1
People
(Reporter: asmith16, Assigned: MatsPalmgren_bugz)
Details
(Keywords: crash)
Attachments
(4 files)
525 bytes,
application/octet-stream
|
Details | |
3.71 KB,
text/plain
|
Details | |
3.80 KB,
text/plain
|
Details | |
3.00 KB,
patch
|
pavlov
:
review+
pavlov
:
superreview+
pavlov
:
approval1.9+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•17 years ago
|
||
Reporter | ||
Comment 2•17 years ago
|
||
Reporter | ||
Comment 3•17 years ago
|
||
The extension itself is too big to attach, You can get it from here: http://littlesvr.ca/apng/svg2png/svg2png.xpi
Comment 4•17 years ago
|
||
If you try this with a debug build, you should see libpng complain about something on the console (stderr).
Reporter | ||
Comment 5•17 years ago
|
||
Nope, I don't see any libpng output.
Comment 6•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Assignee | ||
Comment 7•17 years ago
|
||
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.)
Assignee | ||
Comment 8•17 years ago
|
||
Attachment #280258 -
Flags: review?(dolske)
Comment 9•17 years ago
|
||
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 10•17 years ago
|
||
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)
Assignee | ||
Comment 11•17 years ago
|
||
(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.
Assignee | ||
Updated•17 years ago
|
Attachment #280258 -
Flags: superreview?(pavlov)
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → mats.palmgren
Updated•17 years ago
|
Attachment #280258 -
Flags: superreview?(pavlov)
Attachment #280258 -
Flags: superreview+
Attachment #280258 -
Flags: review?(pavlov)
Attachment #280258 -
Flags: review+
Attachment #280258 -
Flags: approval1.9+
Assignee | ||
Comment 12•17 years ago
|
||
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.
Description
•