Closed Bug 601470 Opened 11 years ago Closed 11 years ago

about:memory crash [@ mozilla::imagelib::RasterImage::GetSourceDataSize ] if you load an svg file as an image


(Core :: ImageLib, defect)

Not set



Tracking Status
blocking2.0 --- final+


(Reporter: cork, Assigned: dholbert)



(Keywords: crash, crashreportid)

Crash Data


(1 file, 2 obsolete files)

If you load an svg file either in an img tag or in css about:memory with mozilla::imagelib::RasterImage::GetSourceDataSize.

Crash Reports:

I'm testing this with testcase from bug 600574
Blocks: 231179
I tried and failed to reproduce this in FF b6 in my Ubuntu 10.4 VBox.
Confirmed on my desktop at home (running 32-bit Ubuntu 10.04)
blocking2.0: --- → ?
QA Contact: general → dholbert
Assignee: nobody → dholbert
QA Contact: dholbert → general
Component: SVG → ImageLib
QA Contact: general → imagelib
Blocks: 276431
No longer blocks: 231179
So, the issue is here:

228     RasterImage *image = static_cast<RasterImage*>(req->mImage.get());
229     if (!image)
230       return PL_DHASH_NEXT;
232     if (rtype & RAW_BIT) {
233       arg->value += image->GetSourceDataSize();
234     } else {
235       arg->value += image->GetDecodedDataSize();
236     }

We need to either
 (a) not assume that |image| is a RasterImage
 (b) either...
   (i)  skip the next chunk for vector-type images, or
   (ii) implement GetSourceDataSize & GetDecodedDataSize for VectorImage (and promote those methods to be declared in the "Image.h" superclass)
> We need to either

Sorry, I meant "We need to..." (do both (a) and part of (b))
Keywords: crash, crashreportid
Summary: about:memory crash with mozilla::imagelib::RasterImage::GetSourceDataSize if you load an svg file as an image → about:memory crash [@ mozilla::imagelib::RasterImage::GetSourceDataSize ] if you load an svg file as an image
Attached patch fix (obsolete) — Splinter Review
This patch:
 - Promotes GetSourceDataSize & GetDecodedDataSize from RasterImage to Image.
 - Promotes the (trivial) GetDataSize impl from RasterImage to Image.
 - Promotes mError from RasterImage to Image, so ^that impl can check mError.
 - Replaces the silly GetDataSize impl in VectorImage with a (correctly?) silly GetSourceDataSize impl and a XXXTODO silly GetDecodedDataSize impl. (to be fixed in bug 590790)
 - Changes the RasterImage* cast that caused this bug's crash to an Image* cast.
Attachment #480564 - Flags: review?
Attachment #480564 - Flags: review? → review?(joe)
Attachment #480564 - Flags: review?(joe) → review+
Whiteboard: [needs blocking approval][needs landing]
This definitely blocks final. However, I'll do you one better and a=b7 this too.
blocking2.0: ? → final+
Whiteboard: [needs blocking approval][needs landing] → [needs landing]
Attached patch fix with test (obsolete) — Splinter Review
Here's the fix, with a mochitest that just probes all of the entries in Components.interfaces.nsIMemoryReporterManager.  (based on aboutMemory.js)

Confirmed that this mochitest crashes @ mozilla::imagelib::RasterImage::GetSourceDataSize before-patch, and passes (with no crash) after-patch.
Attachment #480761 - Flags: review?(joe)
Attachment #480564 - Attachment is obsolete: true
Comment on attachment 480761 [details] [diff] [review]
fix with test

Add an extra, raster, image, and r=me. (Would prefer to load about:memory in a different tab, but I'm not terribly concerned either way.)
Attachment #480761 - Flags: review?(joe) → review+
damon.jpg to the rescue!
Attachment #480761 - Attachment is obsolete: true
Attachment #480766 - Flags: review+
Landed with a=joe per comment 6:
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Verified fixed in
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101007 Firefox/4.0b8pre
Crash Signature: [@ mozilla::imagelib::RasterImage::GetSourceDataSize ]
You need to log in before you can comment on or make changes to this bug.