In mailnews we have a number of cases where we do: static nsCOMPtr<...> varname; These cause reference leaks on shutdown and also mean we don't clean up memory properly. From the link there are four I'm really worried about, 3 in mime, 1 in compose. The nsMsgAccountManagerDS.h ones I'm not worried about because that should be going away soon with the rdf removal.

Fixes the static nsCOMPtr usage in mimemoz2.cpp. This also fixes some reported leaks in the test_mime_emitter.js once we've got a tail file added to do the cleanup. I think this is safe to do, as that function doesn't appear to be called a lot, and string bundles are cached. So we've just got the added overhead of getting a reference to the string bundle service and getting the string bundle from the cache.

Comment on attachment 384245 [details] [diff] [review] Part 1 - mimemoz2.cpp [Checkin: Comment 2] Checked in: http://hg.mozilla.org/comm-central/rev/93854d46f3a9

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090725 SeaMonkey/2.1a1pre] (home, optim default) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/b8c752d9bc65 +http://hg.mozilla.org/comm-central/rev/413ab70abcfb + bug 467356 patch Bv1) Initial code was added by http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mailnews/mime/cthandlers/vcard/mimevcrd.cpp&rev=HEAD&mark=1.37#1.38 then was (mostly) removed by http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mailnews/mime/cthandlers/vcard/mimevcrd.cpp&rev=HEAD&mark=1.103#1.104

if this string bundle is never used, then it's certainly not causing any leaks...

Comment on attachment 390595 [details] [diff] [review] (Bv1) mimevcrd.*: remove bug 290678 remnants, and a bunch of includes [Checkin: Comment 6] builds fine, thx for the patch.

Comment on attachment 390595 [details] [diff] [review] (Bv1) mimevcrd.*: remove bug 290678 remnants, and a bunch of includes [Checkin: Comment 6] http://hg.mozilla.org/comm-central/rev/9f41c009a2ca

Comment on attachment 391308 [details] [diff] [review] (Cv1) nsSMIMEStub.cpp: improve and port mimemoz2.cpp code, ++ [Checkin: Comment 8] http://hg.mozilla.org/comm-central/rev/f9dada77416b

Dv1, with #include removals.

(In reply to comment #0) nsMsgAccountManager.h: 2 functions. That's fine. > The nsMsgAccountManagerDS.h ones I'm not worried about because that should be > going away soon with the rdf removal. nsMsgAccountManagerDS.h: 2 vars. nsMsgDBFolder.h: 1 var. Anything that can be done here? Separate/blocking bugs filed?

Comment on attachment 392071 [details] [diff] [review] (Dv1a) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++ Ping for r+sr?

Comment on attachment 392071 [details] [diff] [review] (Dv1a) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++ Mark, maybe you could do the (s)reviews?

Comment on attachment 392071 [details] [diff] [review] (Dv1a) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++ >+ nsresult rv = mSMIMEBundle->GetStringFromName(name, outString); >+ return NS_SUCCEEDED(rv) ? NS_OK : rv; Just return the result direct from GetStringFromName. Having the additional check doesn't add any value. >+ nsCOMPtr<nsIStringBundleService> bundleService = >+ do_GetService(NS_STRINGBUNDLE_CONTRACTID); > bundleService->CreateBundle(SMIME_STRBUNDLE_URL, > getter_AddRefs(mSMIMEBundle)); >+ return !!mSMIMEBundle; If you do: rv = ...CreateBundle... NS_ENSURE_SUCCESS(rv, PR_FALSE); return PR_TRUE; Then we'll get a warning on the console if something goes wrong (e.g. in the rare case of a missing file or something) and we'll still return the correct value.

Dv1a, with comment 14 suggestion(s).

Comment on attachment 401228 [details] [diff] [review] (Dv2) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++ [Checkin: See comment 17] > nsMsgComposeSecure::nsMsgComposeSecure() >+ : mSMIMEBundle(nsnull) You don't need to initialise nsCOMPtr<> - they initialise themselves. > nsMsgComposeSecure:: > SMIMEBundleFormatStringFromName(const PRUnichar *name, > const PRUnichar **params, > PRUint32 numParams, > PRUnichar **outString) > { > >- nsresult rv = NS_ERROR_FAILURE; >+ NS_ENSURE_ARG_POINTER(name); nit: drop the blank line before this one as well please.

Comment on attachment 401228 [details] [diff] [review] (Dv2) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++ [Checkin: See comment 17] http://hg.mozilla.org/comm-central/rev/2f0a82173cc1 Dv2, with comment 16 suggestion(s).

Mark, what about my comment 11 questions?

So the other two cases are in RDF, that I suspect we're not going to do anything about, as we'd still like to remove RDF. I'm going to call this fixed for now.