leak of atom held by static nsCOMPtr

VERIFIED FIXED in M18

Status

SeaMonkey
MailNews: Account Configuration
P3
minor
VERIFIED FIXED
18 years ago
9 years ago

People

(Reporter: Chris Waterson, Assigned: Alec Flett)

Tracking

(Blocks: 1 bug, {memory-footprint, memory-leak})

Trunk
memory-footprint, memory-leak

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: Fix in Hand, URL)

(Reporter)

Description

18 years ago
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...
(Reporter)

Updated

18 years ago
Keywords: mlk
(Assignee)

Comment 1

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

I was just being lazy - I'll just use the bare pointer.
Status: NEW → ASSIGNED
Keywords: nsbeta3
Target Milestone: --- → M18

Updated

18 years ago
Keywords: footprint

Updated

18 years ago
Keywords: mail6

Comment 2

18 years ago
mail triage marking [nsbeta3-]
Whiteboard: [nsbeta3-]

Updated

18 years ago
Whiteboard: [nsbeta3-]

Comment 3

18 years ago
a leak is a leak is a leak. asking for reconsideration. shouldn't be hard.
(Assignee)

Comment 4

18 years ago
*** Bug 50399 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 5

18 years ago
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?
Whiteboard: Fix in Hand

Comment 6

18 years ago
Why not just say

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

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

Comment 7

18 years ago
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

18 years ago
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).
(Reporter)

Comment 9

18 years ago
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.
(Assignee)

Comment 10

18 years ago
Yup - also what chris said :)
fix is in
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Looked at the diff & code, seems good to me. Verifying.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
Blocks: 60697
You need to log in before you can comment on or make changes to this bug.