Make it easier to fix memory leaks in gfx code

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: kats, Assigned: kats)

Tracking

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 8762948 [details] [diff] [review]
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.
Created attachment 8762964 [details] [diff] [review]
Patch

Here's one that avoids the assertion.
Attachment #8762948 - Attachment is obsolete: true
Attachment #8762964 - Flags: review?(nical.bugzilla)
Attachment #8762964 - Flags: review?(nfroyd)
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?
Attachment #8762964 - Flags: review?(nfroyd) → review+
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.
Attachment #8762964 - Flags: review?(nical.bugzilla) → review+
(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
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

2 years ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/537e6ba21ea9
Add refcount logging support for AtomicRefCountedWithFinalize. r=nical,froydnj

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/537e6ba21ea9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

2 years ago
Depends on: 1287155

Updated

2 years ago
Depends on: 1306945
No longer depends on: 1306945
Depends on: 1387845
You need to log in before you can comment on or make changes to this bug.