Closed Bug 1237709 Opened 4 years ago Closed 4 years ago
Stop leaking m
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
(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  on all the png's in-tree.  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 % %
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?
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.
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.
(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.
You need to log in before you can comment on or make changes to this bug.