I'll try to do some null-arg check in files of that directory. Also there are some possibilities to optimize 'if (!unlikely problem) return ERR' to 'NS_ENSURE_TRUE(unlikely problem, ERR)'. Also, I'll add these from bug 413548: --- a/mailnews/mime/src/nsMimeConverter.cpp Tue Jan 22 16:02:08 2008 -0500 +++ b/mailnews/mime/src/nsMimeConverter.cpp Tue Jan 22 16:31:14 2008 -0500 @@ -136,7 +136,8 @@ nsMimeConverter::EncodeMimePartIIStr(con PRInt32 encodedWordSize, char **encodedString) { - + NS_ENSURE_ARG_POINTER(header); + NS_ENSURE_ARG_POINTER(mailCharset); // Encoder needs utf-8 string. nsAutoString tempUnicodeString; nsresult rv = ConvertToUnicode(mailCharset, header, tempUnicodeString);
Created attachment 589647 [details] [diff] [review] null checks, whitespace cleanup and NS_ENSUREs
Comment on attachment 589647 [details] [diff] [review] null checks, whitespace cleanup and NS_ENSUREs >+ NS_ENSURE_ARG_POINTER(mailCharset); Are you sure mailCharset can't be null? (I think it just defaults to ASCII.) >- char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1); >+ char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1); [This should actually use NS_Alloc. Sadly all_headers isn't null-terminated, so we can't simply clone it.] >+ NS_ASSERTION (allHeaders, "nsMimeHeaders - out of memory"); >+ NS_ENSURE_TRUE(allHeaders, NS_ERROR_OUT_OF_MEMORY); Not sure we need both checks here. > static int > Initialize(MimeObject *obj) So, the rest of the file has a 4 space indent, which is fair enough, except that you changed this function from a 2 space indent to a weird 4+2 indent...
(In reply to neil@parkwaycc.co.uk from comment #2) > >+ NS_ENSURE_ARG_POINTER(mailCharset); > Are you sure mailCharset can't be null? (I think it just defaults to ASCII.) No idea, I took this from Joey Minta's patch in bug 413548. I can remove it. > >- char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1); > >+ char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1); > [This should actually use NS_Alloc. Sadly all_headers isn't null-terminated, > so we can't simply clone it.] I assume I should not do anything about this. > >+ NS_ASSERTION (allHeaders, "nsMimeHeaders - out of memory"); > >+ NS_ENSURE_TRUE(allHeaders, NS_ERROR_OUT_OF_MEMORY); > Not sure we need both checks here. Who will decide? :)
(In reply to aceman from comment #3) > (In reply to comment #2) > > >+ NS_ENSURE_ARG_POINTER(mailCharset); > > Are you sure mailCharset can't be null? (I think it just defaults to ASCII.) > No idea, I took this from Joey Minta's patch in bug 413548. Ah, well not all null in arguments are invalid. This was the only one that I spotted though; I guess if you mistakenly trap other null in arguments then exceptions will get thrown... > > >- char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1); > > >+ char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1); > > [This should actually use NS_Alloc. Sadly all_headers isn't null-terminated, > > so we can't simply clone it.] > I assume I should not do anything about this. Well, I would appreciate it if you switched from PR_MALLOC to NS_Alloc. > > >+ NS_ASSERTION (allHeaders, "nsMimeHeaders - out of memory"); > > >+ NS_ENSURE_TRUE(allHeaders, NS_ERROR_OUT_OF_MEMORY); > > Not sure we need both checks here. > Who will decide? :) Well, unlike the previous code, NS_ENSURE_TRUE does log an error to stdout, which might make the assertion less useful.
(In reply to neil@parkwaycc.co.uk from comment #4) > (In reply to aceman from comment #3) > > (In reply to comment #2) > > > >+ NS_ENSURE_ARG_POINTER(mailCharset); > > > Are you sure mailCharset can't be null? (I think it just defaults to ASCII.) > > No idea, I took this from Joey Minta's patch in bug 413548. > Ah, well not all null in arguments are invalid. This was the only one that I > spotted though; I guess if you mistakenly trap other null in arguments then > exceptions will get thrown... Yes, I try to only check OUT parameters that are dereferenced and would cause a crash. only this 'mailCharset' and 'header' IN parameters are checked because Joey had it that was. I can drop those 2. > > > >- char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1); > > > >+ char *allHeaders = (char *) PR_MALLOC(mHeaders->all_headers_fp + 1); > > > [This should actually use NS_Alloc. Sadly all_headers isn't null-terminated, > > > so we can't simply clone it.] > > I assume I should not do anything about this. > Well, I would appreciate it if you switched from PR_MALLOC to NS_Alloc. If it is just replacing the function name I can do that. > > > >+ NS_ASSERTION (allHeaders, "nsMimeHeaders - out of memory"); > > > >+ NS_ENSURE_TRUE(allHeaders, NS_ERROR_OUT_OF_MEMORY); > > > Not sure we need both checks here. > > Who will decide? :) > Well, unlike the previous code, NS_ENSURE_TRUE does log an error to stdout, > which might make the assertion less useful. Ok, I'll remove it.
Created attachment 603387 [details] [diff] [review] patch v2
http://hg.mozilla.org/comm-central/rev/1bb9b9a58eea BTW Neil, the Modules wiki doesn't list you as a peer for MailNews Core::MIME. You should probably update that. https://wiki.mozilla.org/Modules/MailNews_Core
or
