Eliminate static nsCOMPtr<> uses in mailnews

RESOLVED FIXED

Status

MailNews Core
Backend
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks: 1 bug, {mlk})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs comment 11 answer], URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

8 years ago
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.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #384245 - Flags: superreview?(bienvenu)
Attachment #384245 - Flags: review?(bienvenu)

Updated

8 years ago
Attachment #384245 - Flags: superreview?(bienvenu)
Attachment #384245 - Flags: superreview+
Attachment #384245 - Flags: review?(bienvenu)
Attachment #384245 - Flags: review+
(Assignee)

Comment 2

8 years ago
Comment on attachment 384245 [details] [diff] [review]
Part 1 - mimemoz2.cpp
[Checkin: Comment 2]

Checked in: http://hg.mozilla.org/comm-central/rev/93854d46f3a9
Attachment #384245 - Attachment description: Part 1 - mimemoz2.cpp → [checked in] Part 1 - mimemoz2.cpp
Blocks: 60697
Depends on: 290678
Attachment #384245 - Attachment description: [checked in] Part 1 - mimemoz2.cpp → Part 1 - mimemoz2.cpp [Checkin: Comment 2]
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
Attachment #390595 - Flags: superreview?(bienvenu)
Attachment #390595 - Flags: review?(bienvenu)

Comment 4

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

Comment 5

8 years ago
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.
Attachment #390595 - Flags: superreview?(bienvenu)
Attachment #390595 - Flags: superreview+
Attachment #390595 - Flags: review?(bienvenu)
Attachment #390595 - Flags: review+
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
Attachment #390595 - Attachment description: (Bv1) mimevcrd.*: remove bug 290678 remnants, and a bunch of includes → (Bv1) mimevcrd.*: remove bug 290678 remnants, and a bunch of includes [Checkin: Comment 6]
Created attachment 391308 [details] [diff] [review]
(Cv1) nsSMIMEStub.cpp: improve and port mimemoz2.cpp code, ++
[Checkin: Comment 8]
Attachment #391308 - Flags: superreview?(bienvenu)
Attachment #391308 - Flags: review?(bienvenu)

Updated

8 years ago
Attachment #391308 - Flags: superreview?(bienvenu)
Attachment #391308 - Flags: superreview+
Attachment #391308 - Flags: review?(bienvenu)
Attachment #391308 - Flags: review+
Attachment #391308 - Attachment description: (Cv1) nsSMIMEStub.cpp: improve and port mimemoz2.cpp code, ++ → (Cv1) nsSMIMEStub.cpp: improve and port mimemoz2.cpp code, ++ [Checkin: Comment 8]
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
Created attachment 392070 [details] [diff] [review]
(Dv1) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++
Attachment #392070 - Flags: superreview?(bienvenu)
Attachment #392070 - Flags: review?(bienvenu)
Created attachment 392071 [details] [diff] [review]
(Dv1a) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++

Dv1, with #include removals.
Attachment #392071 - Flags: superreview?(bienvenu)
Attachment #392071 - Flags: review?(bienvenu)
Attachment #392070 - Attachment is obsolete: true
Attachment #392070 - Flags: superreview?(bienvenu)
Attachment #392070 - Flags: review?(bienvenu)
(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?
Attachment #392071 - Flags: review?(bugzilla)
Flags: wanted-thunderbird3?
Whiteboard: [Dv1a: needs r+sr, bienvenu or standard8] [needs comment 11 answer]
(Assignee)

Comment 14

8 years ago
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.
Attachment #392071 - Flags: review?(bugzilla)
Attachment #392071 - Flags: review?(bienvenu)
Attachment #392071 - Flags: review-
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).
Attachment #392071 - Attachment is obsolete: true
Attachment #401228 - Flags: superreview?(bugzilla)
Attachment #401228 - Flags: review?(bugzilla)
Attachment #401228 - Flags: approval-thunderbird3?
Attachment #392071 - Flags: superreview?(bienvenu)
(Assignee)

Comment 16

8 years ago
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.
Attachment #401228 - Flags: superreview?(bugzilla)
Attachment #401228 - Flags: superreview+
Attachment #401228 - Flags: review?(bugzilla)
Attachment #401228 - Flags: review+
Attachment #401228 - Flags: approval-thunderbird3?
Attachment #401228 - Flags: approval-thunderbird3+
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).
Attachment #401228 - Attachment description: (Dv2) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++ → (Dv2) nsMsgComposeSecure.cpp: remove 'static' and improve the code, ++ [Checkin: See comment 17]
Mark, what about my comment 11 questions?
Whiteboard: [Dv1a: needs r+sr, bienvenu or standard8] [needs comment 11 answer] → [needs comment 11 answer]
Flags: wanted-thunderbird3?
(Assignee)

Updated

6 years ago
Depends on: 720354
(Assignee)

Comment 19

5 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.