1. Run with XPCOM_MEM_LEAK_LOG=2 2. Load the testcase 3. Quit Firefox Result: trace-refcnt reports leaked nsGlobalWindow and nsDocument objects

I'm unsure why this is happening or what I can do about this. Could it be an issue with the defines: NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(CanvasPath, AddRef) NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(CanvasPath, Release) NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(CanvasPath)

I can take a look.

> Could it be an issue with the defines: Yes. CanvasPath has a strong ref to mParent but isn't telling the cycle collector about it, no?

And in fact, the cycle in that testcase is, I expect, Window JS object -> CanvasPath JS object -> CanvasPath C++ object -> Window C++ object (mParent) -> Window JS object via preserved wrapper. Also, why are the CanvasPath constructors taking an nsCOMPtr argument, not a raw pointer? And why is the one-arg constructor not explicit?

Yeah, I noticed the missing mPath. Adding that to the CC macros fixes the leak.

(In reply to Andrew McCreight [:mccr8] from comment #5) > Yeah, I noticed the missing mPath. Adding that to the CC macros fixes the > leak. Will you land that fix? If not, post the line that should change and I will do it.

Yeah, it is a very simple fix. I have a patch that fixes that and the other things bz pointed out. I'll upload it here assuming the try run is green, or you can just go look at the try thing if you curious. try run: https://tbpl.mozilla.org/?tree=Try&rev=93e68741cd27

I tried changing the RefPtr<PathBuilder> in the ctor arg to a raw pointer, but it looks like there's no implicit conversion, which scared me, so I just left it alone.

I assume bz reviewed this patch since it says "r=bz"

You assume incorrectly. It's common to put the planned reviewer in the commit message while developing, so the patch doesn't need more updates if it gets review+.

Comment on attachment 8422227 [details] [diff] [review] Fix for memory leak in Path2D + added test case That said, r=me

The patch was already landed, it doesn't need the checkin-needed keyword any more. Thanks for uploading the patch, though I was going to just do that myself this morning. :) > so the patch doesn't need more updates if it gets review+. It is also quite handy because git-bz now uses that info to pre-populate the review? field when uploading a patch. Anyways, if you don't see anything about a patch being r+ in the bug, you should assume it isn't.

(In reply to Andrew McCreight [:mccr8] from comment #14) > The patch was already landed, it doesn't need the checkin-needed keyword any > more. > > Thanks for uploading the patch, though I was going to just do that myself > this morning. :) ah, I reread the thread and I can see now that I misunderstood. :-) > > so the patch doesn't need more updates if it gets review+. > It is also quite handy because git-bz now uses that info to pre-populate the > review? field when uploading a patch. > > Anyways, if you don't see anything about a patch being r+ in the bug, you > should assume it isn't. OK. Good to know.

Comment on attachment 8422227 [details] [diff] [review] Fix for memory leak in Path2D + added test case [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 830734 User impact if declined: severe memory leaks with a new feature Testing completed (on m-c, etc.): landed on inbound Risk to taking this patch (and alternatives if risky): very low String or IDL/UUID changes made by this patch: none

Backed out for bustage. https://hg.mozilla.org/releases/mozilla-aurora/rev/c3c8afe1aa39 https://tbpl.mozilla.org/php/getParsedLog.php?id=39730862&tree=Mozilla-Aurora

Oops, right, the fancy variadic macros are not in Aurora. I'll fix that up later today. Thanks!

I confirmed locally that this compiles on Aurora.