1237709 - Stop leaking mAnim

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Stop leaking mAnim

Calling release() on a unique ptr leaks it instead of freeing like it looks like the caller intended. catlee ran into this with this change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc31768a4a07

Comment 1

[Timothy Nikkel (:tnikkel)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #0)
> catlee ran into this with this change:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc31768a4a07

What does that change do? It's not clear to me.

Can we make sure that this is tested in the tree?

Comment 2

[Chris AtLee [:catlee]]

I ran zopflipng with various flags [1] on all the png's in-tree.

[1] find -name '*.png' -not -path '*test*' | xargs -n1 -P4 -I % zopflipng -m -y --keepchunks=IHDR,IDAT,IEND,PLTE,tEXt,pHYs,vpAg,bKGD,zTXt,sBIT,gAMA,sRGB,tIME,iCCP,tRNS,cHRM,iTXt,mkBT,mkBF,mkTS,prVW,fdAT,fcTL,acTL,iDOT % %

Comment 3

[Chris AtLee [:catlee]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1234008

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Review ping?

Comment 5

[Timothy Nikkel (:tnikkel)]

Can you use MOZ_COUNT_CTOR/DTOR on FrameAnimator and include an image that triggers this so it gets test coverage?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to Timothy Nikkel (:tnikkel) from comment #5)
> Can you use MOZ_COUNT_CTOR/DTOR on FrameAnimator and include an image that
> triggers this so it gets test coverage?

Can I let seth do that?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Review ping.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8705278 [details] [diff] [review]
Stop leaking mAnim

Review of attachment 8705278 [details] [diff] [review]:
-----------------------------------------------------------------

I'll drive-by r+ this -- it seems very clear that your fix matches the intent of the code here, and this was just a typo.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Timothy Nikkel (:tnikkel) from comment #5)
> Can you use MOZ_COUNT_CTOR/DTOR on FrameAnimator

I spun off bug 1242778 for this, BTW.

> and include an image that
> triggers this so it gets test coverage?

That seems like it could be difficult, since the image would have to be large enough that it makes us OOM, basically.  If this is even possible to do reliably, I agree with jrmuizel that seth is probably in the best position to come up with that sort of testcase.  Given the simplicity/obviousness of the patch here, I don't think we should block this patch on wwaiting for a testcase that is able to trigger just the right type of low-memory conditions.

Comment 10

[Timothy Nikkel (:tnikkel)]

Presumably one of the images from the try push in comment 0 is such an image. Otherwise the push would have been green.

Comment 11

[Daniel Holbert [:dholbert]]

Hmm, I suppose.  It may depend on the particular constellation of images/content have been loaded so far, though (as a way of getting us towards a low-memory state), so still may be a bit fragile.

Seems like we'll still be just as able to work towards a test, even after jrmuizel's patch has landed, by layering comment 0's try-patch on top of a backout of jrmuizel's patch.

Comment 12

[Timothy Nikkel (:tnikkel)]

Why does it have to be a low memory state?

Comment 13

[Daniel Holbert [:dholbert]]

I was assuming this line was getting triggered in a low-memory condition, based on the summary of bug 1155332 (the bug that introduced the faulty line here).

But I suppose you're right -- we could simply have a testcase that triggers RasterImage::OnError() for some other error -- not necessarily for a low-memory condition.  And it does seem likely that one of the images in comment 0's Try run could be corrupted in just the right way to do that.  So, maybe a testcase isn't as hard as I was thinking.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: file that triggers this leak

I checked out the source that was used in comment 0's try run, and I added some logging to report whenever the leaky code was tripped.

This revealed that this attached image triggers this bug (toolkit/themes/linux/global/icons/loading_16.png from comment 0's try run's source).

Comment 15

[Daniel Holbert [:dholbert]]

(As a side note, this means the "zopflipng" tool apparently turns perfectly-good animated PNG files like http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/icons/loading_16.png into corrupted images.)

Comment 16

[Daniel Holbert [:dholbert]]

Landed this on Jeff's behalf (together with my patch for bug 1242778, which includes the attached testcase). 
 https://hg.mozilla.org/integration/mozilla-inbound/rev/bf5be8700b2c

Comment 17

[Jeff Muizelaar [:jrmuizel]]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> Landed this on Jeff's behalf (together with my patch for bug 1242778, which
> includes the attached testcase). 
>  https://hg.mozilla.org/integration/mozilla-inbound/rev/bf5be8700b2c

Thanks Daniel.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf5be8700b2c