578357 - Use fallible allocation for image frames

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bobby Holley (:bholley)]

Now that we're using infallible malloc, we should switch image frames to fallible allocation, since it's pretty trivial to cause the allocation of a huge frame with a very small compressed image file.

Comment 1

[Bobby Holley (:bholley)]

Working up a patch for this - A lot of the relevant allocations here end up happening in cairo, which should probably be using a globally fallible allocator.

cjones - can you confirm whether cairo uses fallible malloc by default? If not, I think it probably should.

Comment 2

[Bobby Holley (:bholley)]

Attachment: patch - v1

Attached a patch to do fallible allocation for image surfaces in imagelib and thebes. Note that the low-memory checking we do in imgFrame.cpp is a no-op everywhere but windows and maemo, so fallible allocation is still very important there. Please speak up if there's another spot I missed.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 468232 [details] [diff] [review]
patch - v1

Flagging joe and cjones for review.

Joe, are you an unofficial thebes peer? The only people listed on despot are vlad stuart and roc, and I'm guessing they're all on the busy side. Then again, vlad might like to look at it anyway...

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 468232 [details] [diff] [review]
patch - v1

Yeah, I'm an unofficial gfx peer.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 468232 [details] [diff] [review]
patch - v1

Looks OK to me, except for the C-style casts, but your gfx peer has already ruled on those ;).

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, missed this.

(In reply to comment #1)
> Working up a patch for this - A lot of the relevant allocations here end up
> happening in cairo, which should probably be using a globally fallible
> allocator.
> 
> cjones - can you confirm whether cairo uses fallible malloc by default? If not,
> I think it probably should.

If it's using bare malloc/calloc/realloc, then yes, that's fallible.  I don't know the cairo code, though.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #6)
> Sorry, missed this.
> 
> (In reply to comment #1)
> > Working up a patch for this - A lot of the relevant allocations here end up
> > happening in cairo, which should probably be using a globally fallible
> > allocator.
> > 
> > cjones - can you confirm whether cairo uses fallible malloc by default? If not,
> > I think it probably should.
> 
> If it's using bare malloc/calloc/realloc, then yes, that's fallible.

It's fallible and unaffected by mozalloc, I should add.

Comment 8

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/mozilla-central/rev/7b35f8891b85

Comment 9

[[e-mail]]

Comment on attachment 468232 [details] [diff] [review]
patch - v1

>-    mPalettedImageData = (PRUint8*)PR_MALLOC(PaletteDataLength() + GetImageDataLength());
>+    // Use the fallible allocator here
>+    mPalettedImageData = (PRUint8*)moz_malloc(PaletteDataLength() + GetImageDataLength());
Nit: should switch the deallocator from PR_FREEIF to free too.