1242778 - Add MOZ_COUNT_CTOR/DTOR calls to FrameAnimator

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

I'm spinning this off of bug 1237709 (where we were accidentally leaking a FrameAnimator object).

We need to add MOZ_COUNT_CTOR/DTOR calls to FrameAnimator.

(Also, ideally, it'd be nice to add an image to our testsuite that requires more memory than we have available, so we can exercise the ::OnError cleanup code that was causing the leak in bug 1237709, to make sure we can detect this sort of leak correctly.  Not sure how easy it is to generate an image like that, though.)

Comment 1

[Daniel Holbert [:dholbert]]

Transferring ni=seth from bug 1237709 (particularly for a testcase that causes us to invoke RasterImage::OnError().)

Comment 2

[Daniel Holbert [:dholbert]]

Actually, coming up with a testcase wasn't as difficult as I thought -- I've got a patch & a testcase now, so I'll just take this & post them for review shortly.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Here's the fix and a crashtest.

Comment 4

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8712006 [details] [diff] [review]
fix v1

Yay, thanks for tracking that file down.

Comment 5

[Daniel Holbert [:dholbert]]

I've verified the following, in a local mozilla-inbound debug build, running "./mach crashtest image/test/crashtests/" with various configurations:
 (1) With just the crashtest piece of this patch, nothing is obviously amiss.  (the crashteest run passes)
 (2) If I add the code piece of this patch (the MOZ_COUNT_CTOR/DTOR parts), then the crashtest run fails (at the end) with the following error output:
 1:18.30 
 1:18.30 == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 15631
 1:18.30 
 1:18.30      |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
 1:18.30      |                                      | Per-Inst   Leaked|   Total      Rem|
 1:18.30    0 |TOTAL                                 |       26      320|19333637        4|
 1:18.31  177 |FrameAnimator                         |       80      320|      11        4|
 1:18.31 
 1:18.31 nsTraceRefcnt::DumpStatistics: 1122 entries
 1:18.31 TEST-INFO | leakcheck | default process: leaked 4 FrameAnimator
 1:18.31 TEST-UNEXPECTED-FAIL | leakcheck | default process: 320 bytes leaked (FrameAnimator)

(3) If I add bug 1237709's fix, that error output goes away. (i.e. the leak is indeed fixed)

I think this is all good & well.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (just renamed crashtest)

Thanks for the review! Here's the patch again, with one tweak -- I renamed the crashtest to use the bug number from this bug here (instead of naming it after bug 1237709).

(I originally named it after bug 1237709, since it's kind of a test for that but.  But it's also sorta weird that a patch on this bug adds a test with a different bug number.  So I'm just naming after this bug here for sanity.)

Carrying forward r+.

Comment 7

[Daniel Holbert [:dholbert]]

Try run, with this stacked on top of bug 1237709's fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6128c741757

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35cdeaef79ab

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35cdeaef79ab