Last Comment Bug 831791 - possible memory leak in CompositorParent in SampleValue
: possible memory leak in CompositorParent in SampleValue
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: mozilla21
Assigned To: samuel.gallacier
: Milan Sreckovic [:milan]
Depends on:
Blocks: 783835 938334
  Show dependency treegraph
Reported: 2013-01-17 09:08 PST by samuel.gallacier
Modified: 2013-11-14 01:00 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Correction proposal patch (608 bytes, patch)
2013-01-21 02:23 PST, samuel.gallacier
no flags Details | Diff | Splinter Review
Correction proposal patch (879 bytes, patch)
2013-01-21 08:17 PST, samuel.gallacier
dzbarsky: review+
Details | Diff | Splinter Review

Description samuel.gallacier 2013-01-17 09:08:19 PST
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20130115 Firefox/20.0
Build ID: 20130115042018

Steps to reproduce:

Using css transform (translateX) of a <p> element (scrolling text)

Actual results:

In method SampleValue in CompositorParent.cpp (gfx/layers/ipc/), a InfallibleTArray<TransformFunction> object is constructed, and copied in a Animatable. The object seem never been destructed.
The code is like:

InfallibleTArray<TransformFunction>* functions = new InfallibleTArray<TransformFunction>();
*aValue = *functions;

Perhaps it should be :
InfallibleTArray<TransformFunction> functions;
*aValue = functions;
Comment 1 away[Nov24,Dec5) Kartikaya Gupta ( 2013-01-17 11:03:24 PST
Not sure what this code is supposed to be doing but it does look like there's a memory leak. CC'ing :dzbarksy who added it and :roc who r+'d it.
Comment 2 David Zbarsky (:dzbarsky) 2013-01-17 12:34:04 PST
Yep, your suggestion seems correct.
Comment 3 away[Nov24,Dec5) Kartikaya Gupta ( 2013-01-18 08:28:16 PST
Samuel, would you be willing to create a patch for review with your suggested change? There are instructions at (and specifically If not let me know and I can do it.
Comment 4 samuel.gallacier 2013-01-21 02:23:37 PST
Created attachment 704493 [details] [diff] [review]
Correction proposal patch
Comment 5 away[Nov24,Dec5) Kartikaya Gupta ( 2013-01-21 06:44:50 PST
Comment on attachment 704493 [details] [diff] [review]
Correction proposal patch

Samuel, the patch should really include a commit message and your author info (name + email) as well.
Comment 6 samuel.gallacier 2013-01-21 08:17:50 PST
Created attachment 704556 [details] [diff] [review]
Correction proposal patch
Comment 7 away[Nov24,Dec5) Kartikaya Gupta ( 2013-01-21 09:37:50 PST
Comment on attachment 704556 [details] [diff] [review]
Correction proposal patch

Thanks! :)
Comment 8 David Zbarsky (:dzbarsky) 2013-01-21 11:12:42 PST
Comment on attachment 704556 [details] [diff] [review]
Correction proposal patch

Looks good to me.  Make sure you put "r=dzbarsky" in your commit message.
Comment 9 away[Nov24,Dec5) Kartikaya Gupta ( 2013-01-21 11:25:54 PST
The person landing it can do that. Inbound is closed currently so marking as checkin-needed.
Comment 10 away[Nov24,Dec5) Kartikaya Gupta ( 2013-01-21 13:21:15 PST
Landed on inbound:
Comment 12 Kan-Ru Chen [:kanru] (UTC+8) 2013-01-27 19:38:54 PST
Should we consider uplift this to b2g18? Leaking a object for each animation composition seems pretty bad.
Comment 13 away[Nov24,Dec5) Kartikaya Gupta ( 2013-01-27 20:34:19 PST
Sounds like a good idea to me. It's a simple enough patch.
Comment 14 Andrew Overholt [:overholt] 2013-01-28 12:21:18 PST
I spoke about this with kats via IRC and he re-iterated his thinking that this was low risk and worth the reward.
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2013-01-30 15:48:06 PST
Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.
Comment 17 Lukas Blakk [:lsblakk] use ?needinfo 2013-01-30 15:48:36 PST
scratch the previous comment - it got uplifted yesterday to the right branches - still fixed.

Note You need to log in before you can comment on or make changes to this bug.