44626 - leak of atom held by static nsCOMPtr

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect, P3)

Description

[Chris Waterson]

There are two problems here:

1. You hold an atom in a static nsCOMPtr. You should never have C++ objects with
ctors and dtors declared as statics.

2. If you're gonna assign to an nsCOMPtr from NS_NewAtom(), use dont_AddRef()!

You should prolly fix this by using a bare pointer to the atom. I'll let you
decide...

Comment 1

[Alec Flett]

oh man, I forgot that nsCOMPtrs would have constructors/destructors.

I was just being lazy - I'll just use the bare pointer.

Comment 2

[scottputterman]

mail triage marking [nsbeta3-]

Comment 3

[Judson Valeski]

a leak is a leak is a leak. asking for reconsideration. shouldn't be hard.

Comment 4

[Alec Flett]

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

Comment 5

[Alec Flett]

ok ok I fixed this :)
I have it in my tree but I have fixes to bug 40343 in this file. 
I'm about to attach the patch to that bug.

Patrick/Waterson - care to review the kDefaultServerAtom part of that patch?

Comment 6

[Patrick C. Beard]

Why not just say

kDefaultAtom = dont_AddRef(NS_NewAtom("DefaultServer"));

rather than converting away from an nsCOMPtr? Seems like a step backward to me.

Comment 7

[Alec Flett]

Yes, it might seem that way... but in fact it was a /static/ nsCOMPtr, which is 
bad - static objects in general are bad because some platforms have broken 
library loaders which won't fire the constructors when the library is loaded.

(so for example a static nsCOMPtr would not be initialized to null, and it might 
try to NS_RELEASE() some bad pointer)

see chris's comments above in this bug

Comment 8

[Patrick C. Beard]

Hmm, even if the static constructor doesn't fire, it will still be cleared 
memory. All unitialized static data is at least guaranteed to be zeroed, on every 
platform I've programmed (except may be 68K code resources, but that doesn't 
count).

Comment 9

[Chris Waterson]

Don't hold an atom in a comptr. It will cause a bunch of assertions to fire 
("atom released after atom table destroyed!") and generally makes for noise on 
the bloat & leak reports.

Comment 10

[Alec Flett]

Yup - also what chris said :)
fix is in

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Looked at the diff & code, seems good to me. Verifying.