59212 - leaks with nsCOMPtr<nsIAtom> foo = NS_NewAtom(...)

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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>|".

Comment 1

[Peter "jag" Annema]

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

Comment 2

[Peter "jag" Annema]

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

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 5

[Peter "jag" Annema]

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

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to make NS_NewAtom return already_AddRefed<nsIAtom>

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 8

[Scott Collins]

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.

Comment 9

[Chris Waterson]

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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: changes I needed to make to get the above patch to build with gcc 2.91.66

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 14

[Simon Fraser [no longer active]]

I filed bug 78439 to implement do_GetAtom.

Comment 15

[rpotts (gone)]

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

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 17

[Scott Collins]

collecting under tracking bug # 178174

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[jag (Peter Annema)]

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).

Comment 20

[Boris Zbarsky [:bzbarsky]]

  nsIAtom* foo = do_GetAtom("foo").get();

is also leaky unless someone does something about it (like release the foo later).

Comment 21

[jag (Peter Annema)]

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 ...

Comment 22

[jag (Peter Annema)]

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.

Comment 23

[Benjamin Smedberg]

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

Comment 24

[Benjamin Smedberg]

Will be fixed by bug 859817

Comment 25

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

This was fixed by the patch from bug 859817:

https://hg.mozilla.org/mozilla-central/rev/57a0302a88b7