Closed Bug 1242778 Opened 4 years ago Closed 4 years ago

Add MOZ_COUNT_CTOR/DTOR calls to FrameAnimator


(Core :: ImageLib, defect)

Not set



Tracking Status
firefox47 --- fixed


(Reporter: dholbert, Assigned: dholbert)



(1 file, 1 obsolete file)

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.)
Transferring ni=seth from bug 1237709 (particularly for a testcase that causes us to invoke RasterImage::OnError().)
Flags: needinfo?(seth)
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.
Assignee: nobody → dholbert
Flags: needinfo?(seth)
Attached patch fix v1 (obsolete) — Splinter Review
Here's the fix and a crashtest.
Attachment #8712006 - Flags: review?(tnikkel)
Comment on attachment 8712006 [details] [diff] [review]
fix v1

Yay, thanks for tracking that file down.
Attachment #8712006 - Flags: review?(tnikkel) → review+
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 == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 15631
 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 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.
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+.
Attachment #8712006 - Attachment is obsolete: true
Attachment #8712008 - Flags: review+
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.