Status

()

18 years ago
11 years ago

People

(Reporter: sfraser_bugs, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [code])

Attachments

(1 obsolete attachment)

(Reporter)

Description

18 years ago
We have a bunch of places in the code that do this:

  nsCOMPtr<nsIAtom> tagAtom = NS_NewAtom(tagName);

which leaks the atom. It should be:

  nsCOMPtr<nsIAtom> tagAtom = getter_AddRefs(NS_NewAtom(tagName));

Here they are, and there may be others.

/editor/base/nsHTMLEditRules.cpp, line 1332 -- nsCOMPtr<nsIAtom> embeddingLevel = 
NS_NewAtom("EmbeddingLevel"); 
/editor/base/nsHTMLEditRules.cpp, line 1333 -- nsCOMPtr<nsIAtom> baseLevel = 
NS_NewAtom("BaseLevel"); 
/editor/base/nsHTMLEditor.cpp, line 451 -- nsCOMPtr<nsIAtom> tagAtom = 
NS_NewAtom(tagName);
/editor/base/nsTextEditRules.cpp, line 785 -- nsCOMPtr<nsIAtom> embeddingLevel = 
NS_NewAtom("EmbeddingLevel");

This happens outside of editor code at:
/docshell/base/nsDocShell.cpp, line 3306 -- nsCOMPtr<nsIAtom> method = NS_NewAtom 
("POST");
/embedding/browser/webBrowser/nsWebBrowserPersist.cpp, line 154 -- nsCOMPtr<
nsIAtom> method = NS_NewAtom("POST");
/xpfe/components/xfer/src/nsStreamTransfer.cpp, line 100 -- nsCOMPtr<nsIAtom> 
atom = NS_NewAtom( "content-disposition" );
/xpfe/components/xfer/src/nsStreamTransfer.cpp, line 205 -- nsCOMPtr<nsIAtom> 
method = NS_NewAtom ("POST");
/mailnews/absync/src/nsAbSyncPostEngine.cpp, line 701 -- nsCOMPtr<nsIAtom> method 
= NS_NewAtom ("POST");
/uriloader/exthandler/nsExternalHelperAppService.cpp, line 615 -- nsCOMPtr<
nsIAtom> atom = NS_NewAtom( "content-disposition" );
See bug 59212.  Maybe I should stop being scared of it and just get that patch
reviewed and checked in...

Comment 2

18 years ago
the third example you list i siumply can't find in the source.  The others are 
all ibmbidi stuff that i've never looked at before (seems like I woulda had to 
review that checkin, wonder what happened there?).  Is IBMBIDI turned on?

Comment 3

18 years ago
IBMBIDI isn't turned on as far as I know.

It looks like the only nsIAtom leak left in nsHTMLEditor is in the method 
GetNextElementByTagName(). tagAtom is never released.

Comment 4

18 years ago
The one Joe can't find is probably the one I checked in yesterday then removed
last night in an attempt to see whether I was the culprit on the Tinderbox leaks.

Comment 5

18 years ago
moz0.9.1
Priority: -- → P3
Target Milestone: --- → mozilla0.9.1

Updated

18 years ago
Target Milestone: mozilla0.9.1 → mozilla0.9.2

Comment 6

18 years ago
I see that we're leaking a boatload of these in the main threadview, in
mailnews. Fix for those leaks coming up... Please review.

Comment 7

18 years ago
Created attachment 32598 [details] [diff] [review]
fix for mailnews

Comment 8

18 years ago
Hrm. I'm kind of suspicious to this patch though. Are you guys sure this is the
right fix?

Comment 9

18 years ago
no, that's wrong. Our nsIAtoms are not stored in nsCOMPtr's, whereas the others
were. The fix is only correct if the atoms are stored in nsCOMPtrs. In fact,
there's no evidence that the mailnews atoms leak at all.

Comment 10

18 years ago
thanks for pointing it out, I knew there was something strange with that patch..

Comment 11

18 years ago
Is dont_AddRef or getter_AddRefs the correct macro to fix this problem? I know 
both are functionally identical but it seems to me that dont_AddRef is clearer.

Personally I believe the best solution would be to create a new form of 
NS_NewAtom that returns the nsIAtom as an out parameter so we avoid this 
confusion in future.
(Reporter)

Comment 12

18 years ago
How about a do_GetAtom, like we have do_CreateInstance and do_GetService?

Comment 13

18 years ago
is it true that all the remaining occurances of this are in IBMBIDI code?  If so 
we should assign to any IBMBIDI'ers that have a bugzilla account, so that they 
don't duplicate the problem elsewhere.
(Reporter)

Comment 14

18 years ago
I filed bug 78439 for do_GetAtom.
FYI, the IBMBIDI checkin was bug 71314. What I originally wrote was

 nsIAtom* embeddingLevel = NS_NewAtom("EmbeddingLevel");
etc..

which was changed after review to 
 
 nsCOMPtr<nsIAtom> embeddingLevel = NS_NewAtom("EmbeddingLevel");

If I understand correctly the comments here, it should be changed to EITHER

 nsIAtom* embeddingLevel = getter_AddRefs(NS_NewAtom("EmbeddingLevel"));

which could go in now, OR

 nsCOMPtr<nsIAtom> embeddingLevel = do_GetAtom("EmbeddingLevel");

which would be dependent on bug 78439 being checked in. Let me know which is 
preferable and I'll attach diffs.

Per adam locks comment:

Should I replace this with getter_AddRefs or dont_AddRef?

Thanks
(Reporter)

Comment 17

18 years ago
getter_AddRefs(NS_NewAtom()) is fine
(Reporter)

Comment 18

18 years ago
To clarify, you want this:

nsCOMPtr<nsIAtom> atom = getter_AddRefs(NS_NewAtom());
(not nsIAtom* atom = ...).

Updated

18 years ago
Keywords: correctness
Whiteboard: [code]

Updated

18 years ago
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Updated

18 years ago
Target Milestone: mozilla0.9.3 → mozilla1.0
Is this bug still valid? The Bidi code referenced above no longer exists. (It
was moved to nsTextEditRulesBIDI.cpp, and the leaks were fixed along the way.
See bug 80550)

Comment 20

17 years ago
Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 21

17 years ago
The trunk is the wave of the future!
Target Milestone: mozilla1.0.1 → mozilla1.1beta

Comment 22

17 years ago
The days of having a half dozen milestones out in front of us to divide bugs 
between seem to be gone, though I dont know why.  Lumping everything together as 
far out as I can.  I'll pull back things that I am working on as I go.
Target Milestone: mozilla1.1beta → mozilla1.2beta

Comment 23

16 years ago
[ushing these out as far as bugzilla will let me.  I'll pull them back as I work
on them.
Target Milestone: mozilla1.2beta → mozilla1.4beta
This bug is targeted at a Mac classic platform/OS, which is no longer supported
by mozilla.org. Please re-target it to another platform/OS if this bug applies
there as well or resolve this bug.

I will resolve this bug as WONTFIX in four weeks if no action has been taken.
To filter this and similar messages out, please filter for "mac_cla_reorg".
(Reporter)

Comment 25

15 years ago
Issue still applies XP.
OS: Mac System 8.5 → All
Hardware: Macintosh → All
QA Contact: sujay → editor
Assignee: mozeditor → nobody

Updated

11 years ago
Priority: P3 → --
Target Milestone: mozilla1.4beta → ---
Comment on attachment 32598 [details] [diff] [review]
fix for mailnews

Obsolete per comment #9
Attachment #32598 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.