856080 - memory leak in Layer with OMTC

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[christophe.mouraud]

Attachment: layer.diff

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130320062118

Steps to reproduce:

usign CSS opacity transition  with OMTC


Actual results:

Memroy leak (finding with trace-malloc)

Comment 1

[christophe.mouraud]

nsTArray does not own it manages pointers and there are not released in its destructor

Comment 2

[Nicolas Silva [:nical]]

Hi Christophe, thanks for your patch, can you please format it with the "unified" format, as shown here https://developer.mozilla.org/en-US/docs/Creating_a_patch
and submit it again?

Please add me as reviewer when you do, patches without review request tend to not get much attention.

Comment 3

[Nicolas Silva [:nical]]

Attachment: Fixes ComputedTimingFunction leak (reformated patch)

This fix shouldn't wait longer so I reformated the patch (Christophe, I made sure to put your name and not mine in the patch's header, I don't mean to steal your work).

It's probably worth uplifting asap, I wonder if this could be the cause of the b2g camera app leak.

Comment 4

[Nicolas Silva [:nical]]

pushed to try severs https://tbpl.mozilla.org/?tree=Try&rev=52f7e9568509

Comment 5

[Justin Lebar (not reading bugmail)]

> +    InfallibleTArray<nsAutoPtr<css::ComputedTimingFunction>>& functions = data->mFunctions;

Shouldn't this be "> >"?  I'd be surprised if all the compilers we support work with ">>" closing the nested template.

How do you think this might cause leaks of the objects identified in bug 846903 comment 64?  I don't see the ComputedTimingFunction class holding on to OGL objects (or any objects at all, actually).

Comment 6

[Nicolas Silva [:nical]]

(In reply to Justin Lebar [:jlebar] from comment #5)
> > +    InfallibleTArray<nsAutoPtr<css::ComputedTimingFunction>>& functions = data->mFunctions;
> 
> Shouldn't this be "> >"?  I'd be surprised if all the compilers we support
> work with ">>" closing the nested template.

Oh, right. here is a fix

> 
> How do you think this might cause leaks of the objects identified in bug
> 846903 comment 64?  I don't see the ComputedTimingFunction class holding on
> to OGL objects (or any objects at all, actually).

This leak happens in SetAnimation which you mentioned in Comment 47 of the b2g bug:
> from mozilla::layers::Layer::SetAnimations(), which may or may not be related.

I suppose this won't fix the leak in the camera entirely though.

Comment 7

[Nicolas Silva [:nical]]

Attachment: Fixes ComputedTimingFunction leak (reformated patch) with >> template syntax fix. Carries jmuizelaar: review+

Comment 8

[Nicolas Silva [:nical]]

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

Comment 9

[Ed Morley [:emorley]]

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

Comment 11

[Justin Lebar (not reading bugmail)]

tef? because of bug 861492.  Let's land this, please.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/eec81d0b7e26
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/77bdee6d53aa