1280297 - Make it easier to fix memory leaks in gfx code

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

If a LayerTransactionParent gets leaked, you can't use the refcount tracing stuff on it, because it extends from its own super-special AtomicRefCountedWithFinalize class which doesn't support refcount logging. We should fix that.

There's also all sorts of inconsistent use of MOZ_COUNT_CTOR/MOZ_COUNT_DTOR which we should fix eventually. I might do some of that in this bug.

Comment 1

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

Attachment: WIP

WIP. This compiles and does what it's intended to, but doesn't take it far enough. With this patch applied things like TextureHostOGL and TextureHostBasic will both report as "TextureHost" type but might have different sizes, and that throws an assertion in the tracing code. We have to push the names down all the way to the leaf classes.

Comment 2

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

Attachment: Patch

Here's one that avoids the assertion.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8762964 [details] [diff] [review]
Patch

Review of attachment 8762964 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with some modifications, particularly around the comment called out below.

::: gfx/layers/AtomicRefCountedWithFinalize.h
@@ +113,5 @@
>        ++mRefCount;
> +      // Lie and use 100 as the size of this class. If we want to do this properly
> +      // each leaf class in the inheritance tree of this class needs to have
> +      // a unique name in mName.
> +      NS_LOG_ADDREF(this, mRefCount, mName, 100);

I think you can use:

DebugOnly<WhateverType> refcount = ++mRefCount;

and then use |refcount| in the NS_LOG_ADDREF call for a small efficiency win.

The comment reads a little funny to me, because the first sentence states that we're lying (i.e. not doing things properly), and then the second sentence purports to explain how we might do things properly.  That is, to do this properly, we should pass in a unique name.  But we already pass in a unique name to this class, so there's some dissonance.

If we're already passing in a unique name, is there any problem with passing sizeof(this) from subclasses?  Or is the concern that if we have:

AtomicRefCountedWithFinalize -> Subclass1 -> Subclass2

that passing Subclass2's size up requires an amount of code that we don't think is worthwhile?  I can get behind that argument, but I think the comment should be rewritten for clarity.  Maybe the comment should read something like:

"We lie to the leak detector and pass in a size of 100 for the class here because getting sizes from subclasses of AtomicRefCountedWithFinalize and passing them into this point require too much work for too little gain."

Or, since the size of the class doesn't really matter, you can legitimately pass |sizeof(*this)| here, with a similar comment.

@@ +129,5 @@
>          gfxCriticalError() << "Invalid reference count release" << currCount;
>          ++mRefCount;
>          return;
>        }
> +      NS_LOG_RELEASE(this, mRefCount, mName);

I think you want to use currCount instead of mRefCount here?

Comment 4

[Nicolas Silva [:nical]]

Comment on attachment 8762964 [details] [diff] [review]
Patch

Review of attachment 8762964 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with Nathan about the AddRef stuff, the rest looks good.

Comment 5

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

(In reply to Nathan Froyd [:froydnj] from comment #3)
> I think you can use:
> 
> DebugOnly<WhateverType> refcount = ++mRefCount;
> 
> and then use |refcount| in the NS_LOG_ADDREF call for a small efficiency win.

Done

> Or, since the size of the class doesn't really matter, you can legitimately
> pass |sizeof(*this)| here, with a similar comment.

I originally hadn't done that because I was getting assertions about mismatching sizes being reported to the refcounting code. However I think that was in an earlier version of the patch where I hadn't removed the MOZ_COUNT_CTOR/DTOR things, and so the final version doesn't have that problem. So I put back sizeof(*this) and removed the comment entirely, as the size being reported is now correct.

> @@ +129,5 @@
> >          gfxCriticalError() << "Invalid reference count release" << currCount;
> >          ++mRefCount;
> >          return;
> >        }
> > +      NS_LOG_RELEASE(this, mRefCount, mName);
> 
> I think you want to use currCount instead of mRefCount here?

Done.

Try push with the above changes [1] was green, except for the missing explicit keyword on the constructor. I put that in as well in my local version, will land it.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce4c9ba4c8a0&group_state=expanded

Comment 6

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

Here's another try push which has a green static analysis build. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eb39c2e288f&group_state=expanded

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/537e6ba21ea9
Add refcount logging support for AtomicRefCountedWithFinalize. r=nical,froydnj

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/537e6ba21ea9