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...
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?
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.
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.
Priority: -- → P3
Target Milestone: --- → mozilla0.9.1
I see that we're leaking a boatload of these in the main threadview, in mailnews. Fix for those leaks coming up... Please review.
Hrm. I'm kind of suspicious to this patch though. Are you guys sure this is the right fix?
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.
thanks for pointing it out, I knew there was something strange with that patch..
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.
How about a do_GetAtom, like we have do_CreateInstance and do_GetService?
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.
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
getter_AddRefs(NS_NewAtom()) is fine
To clarify, you want this: nsCOMPtr<nsIAtom> atom = getter_AddRefs(NS_NewAtom()); (not nsIAtom* atom = ...).
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)
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
The trunk is the wave of the future!
Target Milestone: mozilla1.0.1 → mozilla1.1beta
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
[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".
Issue still applies XP.
OS: Mac System 8.5 → All
Hardware: Macintosh → All
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.