Last Comment Bug 856080 - memory leak in Layer with OMTC
: memory leak in Layer with OMTC
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Nicolas Silva [:nical]
:
:
Mentors:
: 861492 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-29 10:02 PDT by christophe.mouraud
Modified: 2013-04-30 05:39 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
tef+
wontfix
wontfix
fixed
fixed
wontfix
fixed


Attachments
layer.diff (1.29 KB, text/plain)
2013-03-29 10:02 PDT, christophe.mouraud
no flags Details
Fixes ComputedTimingFunction leak (reformated patch) (1.97 KB, patch)
2013-04-08 11:38 PDT, Nicolas Silva [:nical]
jmuizelaar: review+
Details | Diff | Splinter Review
Fixes ComputedTimingFunction leak (reformated patch) with >> template syntax fix. Carries jmuizelaar: review+ (1.98 KB, patch)
2013-04-08 12:23 PDT, Nicolas Silva [:nical]
no flags Details | Diff | Splinter Review

Description christophe.mouraud 2013-03-29 10:02:32 PDT
Created attachment 731205 [details]
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 2013-03-29 10:07:45 PDT
nsTArray does not own it manages pointers and there are not released in its destructor
Comment 2 Nicolas Silva [:nical] 2013-04-02 07:14:58 PDT
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] 2013-04-08 11:38:49 PDT
Created attachment 734745 [details] [diff] [review]
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] 2013-04-08 11:45:41 PDT
pushed to try severs https://tbpl.mozilla.org/?tree=Try&rev=52f7e9568509
Comment 5 Justin Lebar (not reading bugmail) 2013-04-08 12:10:03 PDT
> +    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] 2013-04-08 12:21:05 PDT
(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] 2013-04-08 12:23:04 PDT
Created attachment 734772 [details] [diff] [review]
Fixes ComputedTimingFunction leak (reformated patch) with >> template syntax fix. Carries jmuizelaar: review+
Comment 9 Ed Morley [:emorley] 2013-04-10 05:16:16 PDT
https://hg.mozilla.org/mozilla-central/rev/fdc97a5d6356
Comment 10 Justin Lebar (not reading bugmail) 2013-04-28 23:18:42 PDT
*** Bug 861492 has been marked as a duplicate of this bug. ***
Comment 11 Justin Lebar (not reading bugmail) 2013-04-28 23:19:45 PDT
tef? because of bug 861492.  Let's land this, please.

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