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

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: cork, Assigned: dholbert)

Tracking

({crash, crashreportid})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

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

Crash Reports:
bp-85247612-f38f-4f8a-a5a5-7916f2101003
bp-675a88ff-2d01-4cb4-afd2-fa1172101003
bp-4e3879d0-d576-4adf-b301-7914e2101003
bp-4263a542-8323-47d4-8ed5-f003c2101003
bp-df776738-c39f-4576-b526-9c9b32101003
bp-01fd1bfd-9bc9-4f06-86d9-181d32101003

I'm testing this with testcase from bug 600574 https://bugzilla.mozilla.org/attachment.cgi?id=479420
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)
bp-5af4fb5e-883b-4c7e-a31b-eb95a2101003
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:
http://hg.mozilla.org/mozilla-central/annotate/dabe4204f1c1/modules/libpr0n/src/imgLoader.cpp#l228

228     RasterImage *image = static_cast<RasterImage*>(req->mImage.get());
229     if (!image)
230       return PL_DHASH_NEXT;
231 
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
Posted 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)
...and...
 - 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]
Posted 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:
 http://hg.mozilla.org/mozilla-central/rev/d262762e4f87
Status: NEW → RESOLVED
Closed: 9 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
Status: RESOLVED → VERIFIED
Crash Signature: [@ mozilla::imagelib::RasterImage::GetSourceDataSize ]
You need to log in before you can comment on or make changes to this bug.