See bug 1122740.

NS_IF_ADDREF has been changed to NS_ADDREF when it is possible;

If the patch is ready for review or feedback, you can go to the "details" link for the patch, and set either flag for :froydnj and he'll look at it.

Comment on attachment 8562857 [details] [diff] [review] 1132078 Well I just have a little question first (and need to fix the patch at the below lines IMHO): >diff -r 3436787a82d0 xpcom/io/nsAppFileLocationProvider.cpp >--- a/xpcom/io/nsAppFileLocationProvider.cpp Sun Feb 08 17:40:44 2015 -0800 >+++ b/xpcom/io/nsAppFileLocationProvider.cpp Wed Feb 11 18:04:14 2015 +0100 >@@ -581,7 +581,7 @@ > *aResult = new nsPathsDirectoryEnumerator(this, keys); > #endif > NS_IF_ADDREF(*aResult); >- rv = *aResult ? NS_OK : NS_ERROR_OUT_OF_MEMORY; >+ rv = NS_OK; > } At this point, aResult can return an NS_ERROR_OUT_OF_MEMORY if MOZ_WIDGET_COCOA is not defined (preprocessor condition next to the line 555). Is that the correct error code in such a case?

Yeah, this is a problem I've run into when removing some of these checks. I'm not sure what the best error would be. You could look through this list and see if there's anything obvious that would work: http://mxr.mozilla.org/mozilla-central/source/xpcom/base/ErrorList.h Or you could change it to a generic error like NS_ERROR_FAILURE. In practice, the error type is mostly useful when debugging a particular failure, so it doesn't matter a huge amount what is returned.

Comment on attachment 8562857 [details] [diff] [review] 1132078 This patch also removes null-checks for after calls that are fallible: moz_malloc, nsMemory::Realloc and PR_NEW. That seems wrong. I'm also not convinced that some of these things *should* be infallible. For example, the "aDest.SetLength(count + aOffset)" in xpcom/io/Base64.cpp. It's used in the EncodeInputStream function. Assuming it base64-encodes a stream into a string then I think it makes sense to make that SetLength call fallible instead because we don't know what that stream is used for or how large the string will be. Perhaps it would be better to just fix null-checks after the infallible 'new' calls in this patch?

FYI, your patch lacks function names so it's hard to review, and it lacks Author data. You can setup your .hgrc to do that for you, see: https://developer.mozilla.org/en/docs/Installing_Mercurial#Basic_configuration

:mats: I'll correct that. For SetLength, I followed the comment of :bsmedberg ( https://bugzilla.mozilla.org/show_bug.cgi?id=1122740#c23 ). However, I will just remove the checks after infallible 'new' calls and (:mccr8) change NS_IF_ADDREF to NS_ADDREF under the given conditions. :mccr8: So, I'll change the error to NS_ERROR_FAILURE;

- remove check for unfallible 'new' calls; - change NS_IF_ADDREF to NS_ADDREF whenever possible

(filed bug 1133063 on Base64.cpp to follow-up on comment 5)

Comment on attachment 8563373 [details] [diff] [review] 1132078 There's still a PR_NEW call and a calloc call that you have removed the null-checks for. I'm guessing both of those allocations are small and could use infallible allocation instead. Or you could leave the code as is (with null-checks!) if you don't want to fix that in this patch.

I can fix the PR_NEW allocation, but about the calloc, I wanted to know if it would be a good practice to allocate a char array and cast it to void* ? About the base64 bug, I can leave the null checks in order to avoid further modification once bug 1133063 is resolved

I've replaced the mentioned PR_NEW called by a |new| infallible call + let the calloc allocation because didn't get answer on that point.

(In reply to Thomas Baquet from comment #12) > Created attachment 8567559 [details] [diff] [review] > 1132078 > > I've replaced the mentioned PR_NEW called by a |new| infallible call Thanks, but you should also change the corresponding "free" calls. There's a PR_Free a few lines down, and a PR_DELETE in CloseDir() -- you can remove the comment there, and add "aDir = nullptr;" after deleting it. > + let the calloc allocation because didn't get answer on that point. You can replace it with moz_xcalloc, see: http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.h#65 and replace the "free(ver)" with "moz_free(ver)". Also, there are a few unnecessary white-space changes, can you revert those please? xpcom/io/Base64.cpp xpcom/io/nsLinebreakConverter.cpp xpcom/io/nsLocalFileUnix.cpp

I've replaced the PR_FREE by delete, the calloc by moz_calloc (thus free by moz_free); Fix the empty lines problem.

(In reply to Thomas Baquet from comment #14) > I've replaced the PR_FREE by delete, the calloc by moz_calloc (thus free by > moz_free); Fix the empty lines problem. I think we might as well use infallible allocation for that calloc() call. So can you change it to moz_xcalloc and remove the null-check that follows please? Also, there's still a white-space change in xpcom/io/nsLinebreakConverter.cpp.

Comment on attachment 8569843 [details] [diff] [review] 1132078 Thanks, this looks good to me. I've pushed it to Try for testing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcb4dbd46335

Looks green to me, except for a few known (ignorable) intermittent failures. Please go ahead and ask for review from an XPCOM peer. Here's how to do that in case you don't already know: click on the "Details" link next to your patch attachment on this bug. click on "review" and set the value to "?" a text field is then presented, paste nfroyd@mozilla.com there and then click the Submit button.

Comment on attachment 8569843 [details] [diff] [review] 1132078 Review of attachment 8569843 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for doing this! This looks good, just one minor note below. Also, could you please generate your patch with author data and a commit message? That would be most helpful for committing your patch. Please see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for details on how to do that. ::: xpcom/io/nsAppFileLocationProvider.cpp @@ +580,5 @@ > } > *aResult = new nsPathsDirectoryEnumerator(this, keys); > #endif > NS_IF_ADDREF(*aResult); > + rv = *aResult ? NS_OK : NS_ERROR_FAILURE; This looks like this should be: NS_ADDREF(*aResult); rv = NS_OK; right? Since both of the above preprocessor paths assign *aResult with the result of a call to |new|.

I've fixed it.

Comment on attachment 8571274 [details] [diff] [review] 1132078 Review of attachment 8571274 [details] [diff] [review]: ----------------------------------------------------------------- Thank you! Apologies for the latency on reviewing your patch; I was out of town.