Last Comment Bug 684452 - Memory leak in nsEmlxHelperUtils::AddEmlxMessageToStream
: Memory leak in nsEmlxHelperUtils::AddEmlxMessageToStream
Status: RESOLVED FIXED
: mlk
Product: MailNews Core
Classification: Components
Component: Import (show other bugs)
: unspecified
: All All
: -- minor (vote)
: Thunderbird 13.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-02 23:45 PDT by Hiroyuki Ikezoe (:hiro)
Modified: 2012-02-13 13:01 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed fix (1.56 KB, patch)
2011-09-02 23:47 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Review
Revised patch (1.22 KB, patch)
2011-09-05 02:32 PDT, Hiroyuki Ikezoe (:hiro)
mozilla: review+
Details | Diff | Review
Rewrite with Adopt for X-Mozilla-Status2 header (1.45 KB, patch)
2011-09-05 02:33 PDT, Hiroyuki Ikezoe (:hiro)
mozilla: review-
Details | Diff | Review
Return NS_ERROR_OUT_OF_MEMORY instead of rv. (1.38 KB, patch)
2012-01-25 20:48 PST, Hiroyuki Ikezoe (:hiro)
mozilla: review+
Details | Diff | Review

Description Hiroyuki Ikezoe (:hiro) 2011-09-02 23:45:57 PDT
The return value of PR_smprintf should be freed by PR_smprintf_free.
Comment 1 Hiroyuki Ikezoe (:hiro) 2011-09-02 23:47:25 PDT
Created attachment 558043 [details] [diff] [review]
Proposed fix

I have no build environment for Mac OS X, so I don't build comm-central on Mac OS X with this patch.
Comment 2 Mark Banner (:standard8) 2011-09-05 01:22:14 PDT
Comment on attachment 558043 [details] [diff] [review]
Proposed fix

Fly-by comment: Doing this might be slightly nicer:

nsCAutoString buf;
buf.Adopt(PR_smprintf(X_MOZILLA_STATUS_FORMAT MSG_LINEBREAK, x_mozilla_flags));

Then you don't have to worry about what size the memory is.
Comment 3 Hiroyuki Ikezoe (:hiro) 2011-09-05 02:32:11 PDT
Created attachment 558248 [details] [diff] [review]
Revised patch

Follow Mark's comment.

> nsCAutoString buf;
> buf.Adopt(PR_smprintf(X_MOZILLA_STATUS_FORMAT MSG_LINEBREAK,
> x_mozilla_flags));
> 
> Then you don't have to worry about what size the memory is.

I did not know the Adopt method. Thanks for the advice!
Comment 4 Hiroyuki Ikezoe (:hiro) 2011-09-05 02:33:29 PDT
Created attachment 558249 [details] [diff] [review]
Rewrite with Adopt for X-Mozilla-Status2 header

Also use Adopt for X-Mozilla-Status2 header.
Comment 5 Hiroyuki Ikezoe (:hiro) 2011-09-05 02:38:25 PDT
Comment on attachment 558248 [details] [diff] [review]
Revised patch

Review of attachment 558248 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/import/applemail/src/nsEmlxHelperUtils.mm
@@ +206,5 @@
>    NS_ASSERTION(!buf.IsEmpty(), "printf error with X-Mozilla-Status header");
>    if (buf.IsEmpty()) {
>      [pool release];
>      return rv;
>    }

The return value of this failure case seems to be NS_OK. Should return error value? Should I open a new bug for it?
Comment 6 Wayne Mery (:wsmwk, NI for questions) 2012-01-05 06:46:12 PST
bienvenu?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> Comment on attachment 558248 [details] [diff] [review]
> Revised patch
> 
> Review of attachment 558248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/import/applemail/src/nsEmlxHelperUtils.mm
> @@ +206,5 @@
> >    NS_ASSERTION(!buf.IsEmpty(), "printf error with X-Mozilla-Status header");
> >    if (buf.IsEmpty()) {
> >      [pool release];
> >      return rv;
> >    }
> 
> The return value of this failure case seems to be NS_OK. Should return error
> value? Should I open a new bug for it?
Comment 7 David :Bienvenu 2012-01-05 09:13:02 PST
Comment on attachment 558249 [details] [diff] [review]
Rewrite with Adopt for X-Mozilla-Status2 header

sorry for the delay - I think in out of memory, you want to return NS_ERROR_OUT_OF_MEMORY, not rv. Other than that, it looks OK (except for the question of whether the allocator aborts the app in the case of out of memory, in which case this code wouldn't be hit).
Comment 8 David :Bienvenu 2012-01-05 09:14:56 PST
Comment on attachment 558248 [details] [diff] [review]
Revised patch

thx for the patch
Comment 9 Hiroyuki Ikezoe (:hiro) 2012-01-25 20:48:41 PST
Created attachment 591689 [details] [diff] [review]
Return NS_ERROR_OUT_OF_MEMORY instead of rv.
Comment 10 David :Bienvenu 2012-02-02 13:16:25 PST
Comment on attachment 591689 [details] [diff] [review]
Return NS_ERROR_OUT_OF_MEMORY instead of rv.

thx for the patch...
Comment 11 Mark Banner (:standard8) 2012-02-13 12:45:38 PST
Comment on attachment 558248 [details] [diff] [review]
Revised patch

I understand this patch isn't required, so marking obsolete.
Comment 12 Mark Banner (:standard8) 2012-02-13 13:01:42 PST
Checked in: http://hg.mozilla.org/comm-central/rev/1717874b0963

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