1009685 - Path2D leak

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Run with XPCOM_MEM_LEAK_LOG=2
2. Load the testcase
3. Quit Firefox

Result: trace-refcnt reports leaked nsGlobalWindow and nsDocument objects

Comment 1

[Rik Cabanier]

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)

Comment 2

[Andrew McCreight [:mccr8]]

I can take a look.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Andrew McCreight [:mccr8]]

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

Comment 6

[Rik Cabanier]

(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.

Comment 7

[Andrew McCreight [:mccr8]]

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

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Rik Cabanier]

Attachment: Fix for memory leak in Path2D + added test case

Comment 10

[Rik Cabanier]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8422227 [details] [diff] [review]
Fix for memory leak in Path2D + added test case

That said, r=me

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/967ee3262d27

Comment 14

[Andrew McCreight [:mccr8]]

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.

Comment 15

[Rik Cabanier]

(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 16

[Andrew McCreight [:mccr8]]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/967ee3262d27

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/67d0b27a43d9

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Andrew McCreight [:mccr8]]

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

Comment 21

[Andrew McCreight [:mccr8]]

Attachment: Tell the cycle collector about the CanvasPath::mParent. (for Aurora)

Comment 22

[Andrew McCreight [:mccr8]]

I confirmed locally that this compiles on Aurora.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7edb60d00b59