831791 - possible memory leak in CompositorParent in SampleValue

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[samuel.gallacier]

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

[Kartikaya Gupta (email:[e-mail])]

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)]

Yep, your suggestion seems correct.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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]

Attachment: Correction proposal patch

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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]

Attachment: Correction proposal patch

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 704556 [details] [diff] [review]
Correction proposal patch

Thanks! :)

Comment 8

[David Zbarsky (:dzbarsky)]

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

[Kartikaya Gupta (email:[e-mail])]

The person landing it can do that. Inbound is closed currently so marking as checkin-needed.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f92a3d2c1e5b

Comment 11

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/f92a3d2c1e5b

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

Should we consider uplift this to b2g18? Leaking a object for each animation composition seems pretty bad.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Sounds like a good idea to me. It's a simple enough patch.

Comment 14

[Andrew Overholt [:overholt]]

I spoke about this with kats via IRC and he re-iterated his thinking that this was low risk and worth the reward.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/6fecf538bfcb
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/7cb579ad7aed

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

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]

scratch the previous comment - it got uplifted yesterday to the right branches - still fixed.