Closed Bug 435853 Opened 14 years ago Closed 14 years ago
Running xpcshell tests involving Mac components leaks memory due to not having an NSAutorelease
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+
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+
Duplicate of this bug: 432189
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.
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.
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.
(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.
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
Closed: 14 years ago
Resolution: --- → FIXED
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.
Comment on attachment 322776 [details] [diff] [review] The fix v2 Moving nomination to new 188.8.131.52. 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? → approval184.108.40.206?
Attachment #322776 - Flags: approval220.127.116.11? → approval18.104.22.168?
Comment on attachment 322776 [details] [diff] [review] The fix v2 Approved for 22.214.171.124. Please land in CVS. a=ss
Attachment #322776 - Flags: approval126.96.36.199? → approval188.8.131.52+
Patch checked into cvs for 184.108.40.206 (note, this only affects test code).
You need to log in before you can comment on or make changes to this bug.