Last Comment Bug 467356 - Eliminate static nsCOMPtr<> uses in mailnews
: Eliminate static nsCOMPtr<> uses in mailnews
Status: RESOLVED FIXED
[needs comment 11 answer]
: mlk
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Mark Banner (:standard8) (afk until 26th July)
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 290678 720354
Blocks: 60697
  Show dependency treegraph
 
Reported: 2008-12-01 07:11 PST by Mark Banner (:standard8) (afk until 26th July)
Modified: 2012-08-16 15:44 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Part 1 - mimemoz2.cpp [Checkin: Comment 2] (1.77 KB, patch)
2009-06-20 14:03 PDT, Mark Banner (:standard8) (afk until 26th July)
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
(Bv1) mimevcrd.*: remove bug 290678 remnants, and a bunch of includes [Checkin: Comment 6] (3.05 KB, patch)
2009-07-24 17:45 PDT, Serge Gautherie (:sgautherie)
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
(Cv1) nsSMIMEStub.cpp: improve and port mimemoz2.cpp code, ++ [Checkin: Comment 8] (7.46 KB, patch)
2009-07-29 03:44 PDT, Serge Gautherie (:sgautherie)
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
(Dv1) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++ (6.63 KB, patch)
2009-08-01 03:40 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Dv1a) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++ (8.29 KB, patch)
2009-08-01 04:02 PDT, Serge Gautherie (:sgautherie)
standard8: review-
Details | Diff | Splinter Review
(Dv2) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++ [Checkin: See comment 17] (8.42 KB, patch)
2009-09-17 10:04 PDT, Serge Gautherie (:sgautherie)
standard8: review+
standard8: superreview+
standard8: approval‑thunderbird3+
Details | Diff | Splinter Review

Description Mark Banner (:standard8) (afk until 26th July) 2008-12-01 07:11:38 PST
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.
Comment 1 Mark Banner (:standard8) (afk until 26th July) 2009-06-20 14:03:07 PDT
Created attachment 384245 [details] [diff] [review]
Part 1 - mimemoz2.cpp
[Checkin: Comment 2]

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 2 Mark Banner (:standard8) (afk until 26th July) 2009-06-21 09:58:56 PDT
Comment on attachment 384245 [details] [diff] [review]
Part 1 - mimemoz2.cpp
[Checkin: Comment 2]

Checked in: http://hg.mozilla.org/comm-central/rev/93854d46f3a9
Comment 3 Serge Gautherie (:sgautherie) 2009-07-24 17:45:43 PDT
Created attachment 390595 [details] [diff] [review]
(Bv1) mimevcrd.*: remove bug 290678 remnants, and a bunch of includes
[Checkin: Comment 6]

[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
Comment 4 David :Bienvenu 2009-07-28 14:41:35 PDT
if this string bundle is never used, then it's certainly not causing any leaks...
Comment 5 David :Bienvenu 2009-07-28 14:44:17 PDT
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 6 Serge Gautherie (:sgautherie) 2009-07-29 01:30:14 PDT
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 7 Serge Gautherie (:sgautherie) 2009-07-29 03:44:42 PDT
Created attachment 391308 [details] [diff] [review]
(Cv1) nsSMIMEStub.cpp: improve and port mimemoz2.cpp code, ++
[Checkin: Comment 8]
Comment 8 Serge Gautherie (:sgautherie) 2009-08-01 02:07:45 PDT
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
Comment 9 Serge Gautherie (:sgautherie) 2009-08-01 03:40:04 PDT
Created attachment 392070 [details] [diff] [review]
(Dv1) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++
Comment 10 Serge Gautherie (:sgautherie) 2009-08-01 04:02:02 PDT
Created attachment 392071 [details] [diff] [review]
(Dv1a) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++

Dv1, with #include removals.
Comment 11 Serge Gautherie (:sgautherie) 2009-08-01 04:23:44 PDT
(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 12 Serge Gautherie (:sgautherie) 2009-08-14 09:07:45 PDT
Comment on attachment 392071 [details] [diff] [review]
(Dv1a) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++


Ping for r+sr?
Comment 13 Serge Gautherie (:sgautherie) 2009-08-19 20:00:32 PDT
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 14 Mark Banner (:standard8) (afk until 26th July) 2009-09-17 04:39:05 PDT
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.
Comment 15 Serge Gautherie (:sgautherie) 2009-09-17 10:04:30 PDT
Created attachment 401228 [details] [diff] [review]
(Dv2) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++
[Checkin: See comment 17]

Dv1a, with comment 14 suggestion(s).
Comment 16 Mark Banner (:standard8) (afk until 26th July) 2009-09-21 05:45:00 PDT
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 17 Serge Gautherie (:sgautherie) 2009-09-21 11:43:24 PDT
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).
Comment 18 Serge Gautherie (:sgautherie) 2009-09-21 11:55:26 PDT
Mark, what about my comment 11 questions?
Comment 19 Mark Banner (:standard8) (afk until 26th July) 2012-08-16 15:44:03 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.