Closed Bug 1009685 Opened 6 years ago Closed 6 years ago

Path2D leak

Categories

(Core :: Canvas: 2D, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox30 --- unaffected
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: jruderman, Assigned: mccr8)

References

(Blocks 2 open bugs)

Details

(Keywords: memory-leak, testcase, Whiteboard: [MemShrink:P2] [qa-])

Attachments

(3 files)

Attached file 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
Whiteboard: [MemShrink]
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)
Flags: needinfo?(bzbarsky)
I can take a look.
Assignee: nobody → continuation
Flags: needinfo?(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?
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.
Whiteboard: [MemShrink] → [MemShrink:P2]
Keywords: checkin-needed
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
Attachment #8422227 - Flags: review+
Keywords: checkin-needed
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.
Keywords: checkin-needed
(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
Attachment #8422227 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/967ee3262d27
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attachment #8422227 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(continuation)
Oops, right, the fancy variadic macros are not in Aurora.  I'll fix that up later today.  Thanks!
Flags: needinfo?(continuation)
I confirmed locally that this compiles on Aurora.
Whiteboard: [MemShrink:P2] → [MemShrink:P2] [qa-]
You need to log in before you can comment on or make changes to this bug.