Closed Bug 1237709 Opened 4 years ago Closed 4 years ago

Stop leaking mAnim

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(2 files)

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
Attachment #8705278 - Attachment is patch: true
Attachment #8705278 - Attachment mime type: text/x-patch → text/plain
Attachment #8705278 - Flags: review?(seth)
Assignee: nobody → jmuizelaar
See Also: → 1237712
(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?
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 % %
Review ping?
Flags: needinfo?(seth)
Can you use MOZ_COUNT_CTOR/DTOR on FrameAnimator and include an image that triggers this so it gets test coverage?
(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?
Review ping.
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.
Attachment #8705278 - Flags: review?(seth) → review+
(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.
Flags: needinfo?(seth)
Presumably one of the images from the try push in comment 0 is such an image. Otherwise the push would have been green.
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.
Why does it have to be a low memory state?
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.
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).
(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.)
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
(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.
https://hg.mozilla.org/mozilla-central/rev/bf5be8700b2c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.