Last Comment Bug 831791 - possible memory leak in CompositorParent in SampleValue
: possible memory leak in CompositorParent in SampleValue
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: mozilla21
Assigned To: samuel.gallacier
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
tef+
wontfix
wontfix
fixed
fixed
fixed


Attachments
Correction proposal patch (608 bytes, patch)
2013-01-21 02:23 PST, samuel.gallacier
no flags Details | Diff | Review
Correction proposal patch (879 bytes, patch)
2013-01-21 08:17 PST, samuel.gallacier
dzbarsky: review+
Details | Diff | 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>();
functions->AppendElement(TransformMatrix(transform));
*aValue = *functions;

Perhaps it should be :
InfallibleTArray<TransformFunction> functions;
functions.AppendElement(TransformMatrix(transform));
*aValue = functions;
Comment 1 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 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 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 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 https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch (and specifically https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in). 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 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 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 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 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 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 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 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2013-01-21 13:21:15 PST
Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f92a3d2c1e5b
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2013-01-22 10:00:12 PST
https://hg.mozilla.org/mozilla-central/rev/f92a3d2c1e5b
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 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 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.