Running xpcshell tests involving Mac components leaks memory due to not having an NSAutoreleasePool

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({fixed1.9.0.2})

Trunk
x86
Mac OS X
fixed1.9.0.2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

5.87 KB, patch
standard8
: review+
standard8
: superreview+
samuel.sidler+old
: approval1.9.0.2+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
Created attachment 322618 [details] [diff] [review]
The fix

In Bug 397811 I am enabling (by default) an interface to the Mac OS X Address book. When I run xpcshell unit tests on code that activates the interface I get lots of warnings along the lines of:

2008-05-27 11:35:58.299 xpcshell[47392:10b] *** _NSAutoreleaseNoPool(): Object 0x5561e0 of class NSCFArray autoreleased with no pool in place - just leaking
Stack: (0x9070312f 0x9060fec2 0x93c36138 0x92aaa114 0x92aa9d53 0x92aa9997 0x92aceab9 0x134219f 0x133c3af 0x1302885 0x1302eb8 0x1303a54 0x12c18d4 0x33bd5b 0x6e230e 0x6f0ef1 0x6ecbe2 0x1c594f 0x1c5c84 0x1c5ed0 0x1da836 0x1db6b0 0x1b45f2 0x1c6234 0x165964 0x32ab 0x1c594f 0x1b8a21 0x1c6234 0x165964 0x3d14 0x404b 0x46a5 0x58fd 0x2670 0x259d)

on the console. Not only does this show we're leaking memory (at xpcshell test level only), but it also makes debugging the unit tests very hard as it fills up the terminal window.

This is easily fixed by defining an NSAutoreleasePool for xpcshell on mac.

Attaching patch, if I've not got the right reviewer, please point me in the right direction.
Attachment #322618 - Flags: review?(shaver)
Comment on attachment 322618 [details] [diff] [review]
The fix

r=shaver, as long as those are really infallible functions and thus don't give you a way to do error checking and reporting.
Attachment #322618 - Flags: review?(shaver) → review+
(Assignee)

Comment 2

11 years ago
Comment on attachment 322618 [details] [diff] [review]
The fix

(In reply to comment #1)
> (From update of attachment 322618 [details] [diff] [review])
> r=shaver, as long as those are really infallible functions and thus don't give
> you a way to do error checking and reporting.
> 

I couldn't find anything relating to error checking of NSAutoreleasePool in docs or in various searches on the internet. Our own code doesn't seem to error check these either (which may or may not be good, of course).
Attachment #322618 - Flags: superreview?(brendan)
Comment on attachment 322618 [details] [diff] [review]
The fix

Sure (we usually call the antonym of "init" "finish", not "uninit" -- ob. nit).

/be
Attachment #322618 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 5

11 years ago
Created attachment 322776 [details] [diff] [review]
The fix v2

Changed "UnInit" to "Finish" as suggested by Brendan.

Requesting approval for 1.9, although I'm not sure if I really need to - This is a change to the xpcshell code (basically mac only) to fix some memory leaks that only happen on Mac/xpcshell.

MailNews apps need this due to enabling of the Mac OS X directory be default, without this tests are extremely hard to debug on mac (due to console noise). As the MailNews decision for 1.9.0/1.9.1/trunk hasn't been decided yet, I'd like to get this in on the 1.9 cvs.
Attachment #322618 - Attachment is obsolete: true
Attachment #322776 - Flags: superreview+
Attachment #322776 - Flags: review+
Attachment #322776 - Flags: approval1.9?
Just to add to what Mark wrote in favor of getting this in 1.9 cvs, the same console spam plagues Mochitest (see the duped bug), which we'd like to get up and running for Camino; our next release will be from 1.9.0.

Comment 7

11 years ago
Drive-by comment: perhaps the init/uninit might make sense as a ctor/dtor pair on an AutoAutoReleasePool?  (Gotta love that name!)  I don't really care, but avoiding forgetfulness is good in my book.
(Assignee)

Comment 8

11 years ago
(In reply to comment #7)
> Drive-by comment: perhaps the init/uninit might make sense as a ctor/dtor pair
> on an AutoAutoReleasePool?  (Gotta love that name!)  I don't really care, but
> avoiding forgetfulness is good in my book.
> 
Given this is done in a similar style all throughout the code for Mac (though normally just calling the functions direct) I'm not too fussed about this, esp as these functions are only really intended for use in xpcshell.
(Assignee)

Comment 9

11 years ago
Checked into mozilla-central, so marking fixed, still requesting approval 1.9 as the final decision where TB/SM is going not made. Also Camino still wants it.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Duplicate of this bug: 439241

Comment 11

11 years ago
To whomever is doing the approval1.9 decision:

This should really go into CVS as well, if we want OS X unit tests to run cleanly. The patch is simple and as it stands, all autoreleased objects just plain leak in unit tests, which IMO is not acceptable. 
(Assignee)

Comment 12

11 years ago
Comment on attachment 322776 [details] [diff] [review]
The fix v2

Moving nomination to new 1.9.0.1. Drivers please see comment 5, comment 6 and comment 11 for original approval request comments and other folks wanting this.
Attachment #322776 - Flags: approval1.9? → approval1.9.0.1?
Attachment #322776 - Flags: approval1.9.0.1? → approval1.9.0.2?
Comment on attachment 322776 [details] [diff] [review]
The fix v2

Approved for 1.9.0.2. Please land in CVS. a=ss
Attachment #322776 - Flags: approval1.9.0.2? → approval1.9.0.2+
(Assignee)

Comment 14

10 years ago
Patch checked into cvs for 1.9.0.2 (note, this only affects test code).
Keywords: fixed1.9.0.2
You need to log in before you can comment on or make changes to this bug.