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

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Cork]

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

Comment 1

[Will Pittenger]

I tried and failed to reproduce this in FF b6 in my Ubuntu 10.4 VBox.

Comment 2

[Daniel Holbert [:dholbert]]

Confirmed on my desktop at home (running 32-bit Ubuntu 10.04)
bp-5af4fb5e-883b-4c7e-a31b-eb95a2101003

Comment 3

[Daniel Holbert [:dholbert]]

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)

Comment 4

[Daniel Holbert [:dholbert]]

> We need to either

Sorry, I meant "We need to..." (do both (a) and part of (b))

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix

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.

Comment 6

[Joe Drew (not getting mail)]

This definitely blocks final. However, I'll do you one better and a=b7 this too.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix with test

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.

Comment 8

[Joe Drew (not getting mail)]

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.)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix with test, with raster image too [r=joe]

damon.jpg to the rescue!

Comment 10

[Daniel Holbert [:dholbert]]

Landed with a=joe per comment 6:
 http://hg.mozilla.org/mozilla-central/rev/d262762e4f87

Comment 11

[Cork]

Verified fixed in
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101007 Firefox/4.0b8pre