Closed Bug 1359436 Opened 7 years ago Closed 7 years ago

Add leak checking to CycleCollectedJSContext and friends

Categories

(Core :: XPCOM, enhancement)

Product:
Core
Component:
XPCOM
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mccr8, Assigned: kedziorski.lukasz, Mentored)

Details

(Whiteboard: [lang=c++][good first bug])

Attachments

(1 file, 2 obsolete files)

Add leak checking to CycleCollectedJSContext and related classes.
6.76 KB, patch
mccr8
: review+
Details | Diff | Splinter Review 
While looking at bug 1359245, I noticed that CycleCollectedJSContext, its two subclasses, and CycleCollectedJSRuntime don't have any leak checking. We should add that. They just need a MOZ_COUNT_CTOR/DTOR{_INHERITED} pair. This isn't too critical, as I'd think it would hard to leak one of these without leaking a lot of other stuff.
Mentor: erahm
Whiteboard: [lang=c++][good first bug]
Attached patch 1359436_v1.patch (obsolete) — — Splinter Review 
Eric could you assign me to this bug?

I've just added MOZ_COUNT_{CTOR, DTOR} to CycleCollectedJS{Context, Runtime} and MOZ_COUNT_{CTOR, DTOR}_INHERITED to XPCJS{Context, Runtime}.
Attachment #8865255 - Flags: review?(erahm)
Assignee: nobody → kedziorski.lukasz
Comment on attachment 8865255 [details] [diff] [review]
1359436_v1.patch

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

Looks good to me, Andrew's a bit more familiar with leak checking so lets check with him as well.
Attachment #8865255 - Flags: review?(erahm)
Attachment #8865255 - Flags: review?(continuation)
Attachment #8865255 - Flags: review+
Comment on attachment 8865255 [details] [diff] [review]
1359436_v1.patch

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

This looks pretty good. You just need to do the same thing for WorkerJSRuntime and WorkerJSContext that you did for the XPConnect classes.
Attachment #8865255 - Flags: review?(continuation)
Attached patch 1359436_v2.patch (obsolete) — — Splinter Review 
Added leak checking to WorkerJSRuntime and WorkerJSContext.
Attachment #8865255 - Attachment is obsolete: true
Attachment #8865638 - Flags: review?(continuation)
Comment on attachment 8865638 [details] [diff] [review]
1359436_v2.patch

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

Thanks. I can do a try run for this patch.
Attachment #8865638 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #5)
> Comment on attachment 8865638 [details] [diff] [review]
> 1359436_v2.patch
> 
> Review of attachment 8865638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks. I can do a try run for this patch.

That would be great.
I forgot to mention in my review comment that your patch should have a descriptive commit message, including the bug number and reviewer. I have updated that here.
Attachment #8866051 - Flags: review+
Attachment #8865638 - Attachment is obsolete: true
The try run looked good. I will set the checkin-needed flag. Somebody should land your patch in the next day or two in mozilla-inbound, then it will be merged to mozilla-central a day or two after that, and the bug will be marked FIXED.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb6ed5e7e4b
Add leak checking to CycleCollectedJSContext and related classes. r=mccr8
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7cb6ed5e7e4b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: