A common leak pattern is to assign to an nsCOMPtr with NS_NewAtom, which addrefs, without using dont_AddRef(). This lxr search shows a number (but probably not nearly all) of these leaks: http://lxr.mozilla.org/seamonkey/search?string=nsCOMPtr.*%3D+NS_NewAto&regexp=on We should either fix these leaks (and perhaps find a better way to find them), or (as scc suggested on IRC) "make |NS_NewAtom| return |already_addrefed<nsIAtom>|".

Let's see if I understand this stuff... nsCOMPtr<nsIAtom> name (NS_NewAtom("foo")); // (1) nsCOMPtr<nsIAtom> name = NS_NewAtom("foo"); // (2) and nsCOMPtr<nsIAtom> name; // (3) name = NS_NewAtom("foo"); all leak because NS_NewAtom AddRefs and the copy constructor and assignment also AddRef? That's gonna be fun to find all those. dbaron's url gets (1), a query for (2) shows they all dont_AddRef, so that leaves us with (3). Apparanty, we've got some of these: nsIAtom* foo; foo = NS_NewAtom(foo); And there are a few #define's: http://lxr.mozilla.org/seamonkey/search?string=%5E.*%5C%23define.*NS_NewAtom&regexp=on

Heh, that should be the other way around. dbaron's url gets (2), a query shows (1) all dont_AddRef.

We don't need to worry about the |#define|s in jag's query above, since those are for massive collections of atoms that we know don't leak.

BTW, a better way (than LXR searches) to fix these might be either to make the leaky construct either not leak or not compile.

Cool. I'm all for already_AddRefed, if that doesn't break anything.

I built with the above patch today. It seems to work (although I haven't done much leak testing yet, it certainly should fix the leaks, including those not found by the LXR query). We should probably try to figure out if this patch causes any significant increase in code size or slower performance. If it doesn't, I'm in favor of doing this for NS_NewAtom (and, if it works well, for other functions that return addrefed pointers).

Cool. This is exactly what I made |already_AddRefed| for. See my comment at http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsCOMPtr.h#220 and my checkin comment for version 1.63 ... but of course, you probably already knew that :-) It really makes me happy to see this.

Looks cool. It'd be nice to see somebody try compiling this on Win32, gcc-2.7.2.3, not to mention AIX, HP-UX, etc.

This actually doesn't compile on gcc 2.91.66 (egcs 1.1.2). It fails on the line: nsIAtom* prefixAtom = ((0 < prefix.Length()) ? NS_NewAtom(prefix) : nsnull); with the errors: /home/david/builds/seamonkey/mozilla/layout/xml/document/src/nsXMLContentSink.cpp: In method `void nsXMLContentSink::PushNameSpacesFrom(const class nsIParserNode &)': /home/david/builds/seamonkey/mozilla/layout/xml/document/src/nsXMLContentSink.cpp:523: ambiguous overload for `bool ? already_AddRefed<nsIAtom> : int' /home/david/builds/seamonkey/mozilla/layout/xml/document/src/nsXMLContentSink.cpp:523: candidates are: operator ?:(bool, already_AddRefed<nsIAtom>, already_AddRefed<nsIAtom>) <builtin> /home/david/builds/seamonkey/mozilla/layout/xml/document/src/nsXMLContentSink.cpp:523: operator ?:(bool, nsIAtom *, nsIAtom *) <builtin> gmake: *** [nsXMLContentSink.o] Error 1 (IIRC, waterson tried this on gcc 2.72.3 and it worked. At least I thought he did. Oh well...) scc, any ideas?

The combination of these 2 patches built and ran for sfraser on Mac with no changes. He said the size of Layout was 6.7MB debug before and after, although he didn't get more accurate numbers.

bryner built this on windows. He wrote: Total disk space for the components directory increased from 21,772,790 bytes before applying the patch to 22,184,946 bytes afterwards. Runs fine (I'm sending this email with it). I guess I"d like to test this with an optimized build on Windows to make sure there's no size increase there before checking in...

I filed bug 78439 to implement do_GetAtom.

*** Bug 59393 has been marked as a duplicate of this bug. ***

Considering the recent changes to already_AddRefed, we should probably just fix the ones we've found in LXR and perhaps convert them to do_GetAtom if we ever get around to implementing it... But moving off to 0.9.2 unless someone else wants to do this in the next few days...

collecting under tracking bug # 178174

As a note, do_GetAtom exists. I think we can just eliminate NS_NewAtom usage, except from inside do_GetAtom...

See bug 213601 for converting some users of NS_GetAtom over to do_GetAtom. What's left after this are the ones that are of the form nsIAtom* foo = NS_NewAtom("foo"); which would have to become nsIAtom* foo = do_GetAtom("foo").get(); Not exactly pretty, since we discourage a very similar looking pattern elsewhere |const char* foo = NS_LITERAL_CTRING("foo").get()| because it's leaky (unlike the above pattern).

nsIAtom* foo = do_GetAtom("foo").get(); is also leaky unless someone does something about it (like release the foo later).

Erh, that should be memory access errors, not leaks. And NS_LITERAL_CSTRING is a rather bad example, since that'll actually happen to work (no internal buffer that'll go away when the containing object goes away). Anyway ...

And yeah, do_GetAtom().get() is "leaky" in the same way NS_NewAtom() is leaky. Except the former notation might suggest it's not leaky and the release automagically taken care of.

Seems to me that NS_NewAtom could just return a already_AddRefed<nsIAtom> instead of a bare pointer...

Will be fixed by bug 859817

This was fixed by the patch from bug 859817: https://hg.mozilla.org/mozilla-central/rev/57a0302a88b7