Closed Bug 59212 Opened 20 years ago Closed 7 years ago

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

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Unassigned)

References

Details

(Keywords: memory-leak, Whiteboard: [xptest])

Attachments

(2 files)

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>|".
Status: NEW → ASSIGNED
Keywords: mlk
Target Milestone: --- → mozilla0.9.1
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?
Whiteboard: [xptest]
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...
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Blocks: 92580
Target Milestone: mozilla0.9.5 → mozilla0.9.6
No longer blocks: 92580
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Target Milestone: mozilla0.9.7 → Future
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...
QA Contact: kandrot → scc
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.
QA Contact: scc → xpcom
Seems to me that NS_NewAtom could just return a already_AddRefed<nsIAtom> instead of a bare pointer...
Assignee: dbaron → nobody
Priority: P3 → P2
Target Milestone: Future → mozilla1.9.3
Status: ASSIGNED → NEW
Target Milestone: mozilla1.9.3 → mozilla2.0
Will be fixed by bug 859817
Depends on: 859817
Priority: P2 → P3
Target Milestone: mozilla2.0 → ---
This was fixed by the patch from bug 859817:

https://hg.mozilla.org/mozilla-central/rev/57a0302a88b7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.