Closed Bug 1132078 Opened 9 years ago Closed 9 years ago

Remove useless null checks after allocating memory with |new| from xpcom/io/

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: mccr8, Assigned: thomas, Mentored)

Details

Attachments

(1 file, 5 obsolete files)

See bug 1122740.
Attached patch 1132078 (obsolete) — Splinter Review
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;
Attached patch 1132078 (obsolete) — Splinter Review
- remove check for unfallible 'new' calls;
- change NS_IF_ADDREF to NS_ADDREF whenever possible
Attachment #8562857 - Attachment is obsolete: true
(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.
Flags: needinfo?(thomas)
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
Flags: needinfo?(thomas)
Attached patch 1132078 (obsolete) — Splinter Review
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.
Attachment #8563373 - Attachment is obsolete: true
(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
Flags: needinfo?(thomas)
Attached patch 1132078 (obsolete) — Splinter Review
I've replaced the PR_FREE by delete, the calloc by moz_calloc (thus free by moz_free); Fix the empty lines problem.
Attachment #8567559 - Attachment is obsolete: true
Flags: needinfo?(thomas)
(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.
Flags: needinfo?(thomas)
Attached patch 1132078 (obsolete) — Splinter Review
Attachment #8567626 - Attachment is obsolete: true
Flags: needinfo?(thomas)
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
Attachment #8569843 - Flags: feedback+
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.
Attachment #8569843 - Flags: review?(nfroyd)
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|.
Attachment #8569843 - Flags: review?(nfroyd) → feedback+
Attached patch 1132078Splinter Review
I've fixed it.
Attachment #8569843 - Attachment is obsolete: true
Flags: needinfo?(nfroyd)
Attachment #8571274 - Flags: review?(nfroyd)
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.
Attachment #8571274 - Flags: review?(nfroyd) → review+
Flags: needinfo?(nfroyd)
https://hg.mozilla.org/mozilla-central/rev/2a019a588c26
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.