1122740 - remove useless null checks after allocating memory with |new|

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

We have a fair number of:

  T* obj = new T(...)
  if (!obj) {
    return NS_ERROR_OUT_OF_MEMORY;
  }

blocks in xpcom/.  Unfortunately, many (all?) of them are useless, because our implementation of |new| never fails, so the |if| branch is never taken.  Removing them would slightly improve performance, code size, and readability.

(The same thing could be done for all the tree, but first things first; xpcom/ is a pretty small piece of code.)

The straightforward approach is just fixing the above; the more complete approach would be examining the call graph; if one had:

  T* obj = ThingThatAllocatesTWithNew();
  if (!obj) {
    return NS_ERROR_OUT_OF_MEMORY;

then you could remove the null-check here.

Comment 1

[Anirudh GP(:anirudhgp)]

Hi I'd like to work on this bug. How do i begin?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to anirudh.gp from comment #1)
> Hi I'd like to work on this bug. How do i begin?

Great!  The first step is to make sure that you can check out the source code and build Firefox on your own machine:

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

Once that's done, you can start making the modifications described in the first part of comment 0.

Comment 3

[Anirudh GP(:anirudhgp)]

I have already built firefox. In which file do i look for the code mentioned in comment 0 ?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to anirudh.gp from comment #3)
> I have already built firefox. In which file do i look for the code mentioned
> in comment 0 ?

Ah, ok!  You can search for it using:

hg grep NS_ERROR_OUT_OF_MEMORY xpcom/

at a shell prompt, or you can search for it on the web using our MXR tool:

http://mxr.mozilla.org/mozilla-central/search?find=%2Fxpcom%2F&string=NS_ERROR_OUT_OF_MEMORY

Then you'll need to go through the files and find instances of the pattern mentioned in comment 0.

Comment 5

[Anirudh GP(:anirudhgp)]

Ok so i just remove the if(!obj) blocks ?

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to anirudh.gp from comment #5)
> Ok so i just remove the if(!obj) blocks ?

Yes!

Comment 7

[Anirudh GP(:anirudhgp)]

Ok thanks! I'm trying to refine the search since it returned a lot of lines containing the keyword.

Comment 8

[Andrew McCreight [:mccr8] (PTO until 12-2)]

It'll probably be a little easier if you just deal with one subdirectory at a time, and make one patch for each.  So, you could do xpcom/base first, say:
  http://mxr.mozilla.org/mozilla-central/search?string=NS_ERROR_OUT_OF_MEMORY&find=%2Fxpcom%2Fbase%2F&findi=&filter=^%5B^\0%5D*%24&hitlimit=&tree=mozilla-central

Comment 9

[Anirudh GP(:anirudhgp)]

(In reply to Andrew McCreight [:mccr8] from comment #8)
> It'll probably be a little easier if you just deal with one subdirectory at
> a time, and make one patch for each.  So, you could do xpcom/base first, say:
>  
> http://mxr.mozilla.org/mozilla-central/
> search?string=NS_ERROR_OUT_OF_MEMORY&find=%2Fxpcom%2Fbase%2F&findi=&filter=^%
> 5B^\0%5D*%24&hitlimit=&tree=mozilla-central

Ok i'll submit a patch for each subdirectory!

Comment 10

[Avik Pal [:avikpal]]

I am a newbie to the XPCOM codebase and I am curious that how you are sure that the implementation of new() is not going to fail- there are several scenario in which the new() hasn't been overloaded with an implementation of allocation error handling like say internally handling a std::bad_alloc exception.

As far as I can recall sometimes JS scripts causing huge memory leaks would give the NS_ERROR_OUT_OF_MEMORY error and would leave an option for fallback. Though its not likely to happen with a regular script but it may happen if someone deviates from the good practice. Or are you handling the allocation related errors from a global wrapper class- though I am couldn't find one such.

The guide at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Avoiding_leaks_in_JavaScript_components states that it is outdated. If I am not correct in my perception of current working of Garbage Collection or Ref counting then do correct me.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Avik Pal [:avik] from comment #10)
> I am a newbie to the XPCOM codebase and I am curious that how you are sure
> that the implementation of new() is not going to fail- there are several
> scenario in which the new() hasn't been overloaded with an implementation of
> allocation error handling like say internally handling a std::bad_alloc
> exception.

Most of the code that lives in libxul uses what we call "infallible new", which is guaranteed to abort() if the requisite memory cannot be allocated:

http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.h#156
http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.cpp#50
http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc_oom.cpp#28

The JS engine needs to null-check allocations because it doesn't use mozalloc.h.  Actually, some of XPCOM probably can't use mozalloc.h:

http://mxr.mozilla.org/mozilla-central/source/config/gcc-stl-wrapper.template.h#35

so we'll probably need to be careful when removing null-checks.

Comment 12

[Avik Pal [:avikpal]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #11)
> (In reply to Avik Pal [:avik] from comment #10)
> > I am a newbie to the XPCOM codebase and I am curious that how you are sure
> > that the implementation of new() is not going to fail- there are several
> > scenario in which the new() hasn't been overloaded with an implementation of
> > allocation error handling like say internally handling a std::bad_alloc
> > exception.
> 
> Most of the code that lives in libxul uses what we call "infallible new",
> which is guaranteed to abort() if the requisite memory cannot be allocated:
> 
> http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.h#156
> http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.cpp#50
> http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc_oom.
> cpp#28
> 
> The JS engine needs to null-check allocations because it doesn't use
> mozalloc.h.  Actually, some of XPCOM probably can't use mozalloc.h:
> 
> http://mxr.mozilla.org/mozilla-central/source/config/gcc-stl-wrapper.
> template.h#35
> 
> so we'll probably need to be careful when removing null-checks.

This is exactly what I needed to know. Thanks.

Comment 13

[Andrew McCreight [:mccr8] (PTO until 12-2)]

In case anybody is curious, the idea behind infallible malloc is that in normal usage it is very unlikely that a small allocation is going to result in an out-of-memory error, so the code that runs then is probably not going to be very well tested.  If somebody clever does figure out how to get it to trigger, it could cause a security problem.  Therefore, we default to crashing when we fail to allocate.

If there is a place where we frequently allocate a large amount of memory at once, and thus hit the OOM condition regularly and crash with infallible new, then we get a crash report about that location.  If there are many such reports, then fallible new can be used instead.  One example of this is bug 950076.

Comment 14

[Avik Pal [:avikpal]]

Thanks Andrew for shedding some light on this. I was curious regarding how these corner cases are being handled.

Comment 15

[Naveen N]

I would like to work on this bug if Anirudh GP(:anirudhgp) isn't working on it anymore. I asked because i din't see it resolved in the bug description. This would be my first bug and so would like to do something easier. :-]

Comment 16

[Anirudh GP(:anirudhgp)]

(In reply to Naveen N from comment #15)
> I would like to work on this bug if Anirudh GP(:anirudhgp) isn't working on
> it anymore. I asked because i din't see it resolved in the bug description.
> This would be my first bug and so would like to do something easier. :-]

I had just started working on it but you can go ahead and do it :) Its an easy bug to fix so it'll be a good first one for you

Comment 17

[Naveen N]

:anirudhgp thanks a lot for allowing me finish it. I have just finished building firefox. I will start to make changes to the code by tomorrow. I will let you know when once i finish.

Comment 18

[shauryadhaka]

Can i work on this bug?? I just built the firefox os and would need a little help.

Comment 19

[TanmayAgrawal]

Hey I would like to work on this bug as it is my first bug and it is an easier.  The status says that the big is NEW and so if Anirudh or Naveen you guyz are not working on it, can i have the opportunity to contribute towards it?

Comment 20

[Naveen N]

the_decoder: i am working on it buddy :-). But feel free to choose another sub-directory inside xpcom to start working, as i am just about to finish working on xpcom/base/.

Comment 21

[Thomas Baquet]

Hi! I start to work on xpcom/io if it is okay for everyone

Comment 22

[Thomas Baquet]

There are some cases such as (xpcom/io/nsLocalFileWin.cpp:1055):

>mResolvedPath.SetLength(MAX_PATH);
>if (mResolvedPath.Length() != MAX_PATH) {
>  return NS_ERROR_OUT_OF_MEMORY;
>}

Should I suppose that the allocated space size will always be MAX_PATH?

Comment 23

[Benjamin Smedberg]

Yes. The "base" SetLength is now infallible, and if you want the fallible version you have to pass a fallible_t() to the method. See

Declaration of SetLength:
http://mxr.mozilla.org/mozilla-central/source/xpcom/string/nsTSubstring.h#706

Comment 24

[Thomas Baquet]

Attachment: #1122740 xpcom/io/

Comment 25

[Thomas Baquet]

Comment on attachment 8562013 [details] [diff] [review]
#1122740 xpcom/io/

>diff -r 3436787a82d0 xpcom/io/Base64.cpp
>--- a/xpcom/io/Base64.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/Base64.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -179,11 +179,7 @@
>   }
> 
>   uint32_t count = uint32_t(countlong);
>-
>   aDest.SetLength(count + aOffset);
>-  if (aDest.Length() != count + aOffset) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
> 
>   EncodeInputStream_State<T> state;
>   state.charsOnStack = 0;
>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	Tue Feb 10 11:36:04 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;
>   }
>   if (!nsCRT::strcmp(aProp, NS_APP_SEARCH_DIR_LIST)) {
>     static const char* keys[] = { nullptr, NS_APP_SEARCH_DIR, NS_APP_USER_SEARCH_DIR, nullptr };
>@@ -591,7 +591,7 @@
>     }
>     *aResult = new nsPathsDirectoryEnumerator(this, keys);
>     NS_IF_ADDREF(*aResult);
>-    rv = *aResult ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
>+    rv = NS_OK;
>   }
>   return rv;
> }
>diff -r 3436787a82d0 xpcom/io/nsBinaryStream.cpp
>--- a/xpcom/io/nsBinaryStream.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsBinaryStream.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -229,9 +229,6 @@
>     copy = temp;
>   } else {
>     copy = reinterpret_cast<char16_t*>(moz_malloc(byteCount));
>-    if (!copy) {
>-      return NS_ERROR_OUT_OF_MEMORY;
>-    }
>   }
>   NS_ASSERTION((uintptr_t(aString) & 0x1) == 0, "aString not properly aligned");
>   mozilla::NativeEndian::copyAndSwapToBigEndian(copy, aString, length);
>@@ -799,10 +796,6 @@
>   char* s;
> 
>   s = reinterpret_cast<char*>(moz_malloc(aLength));
>-  if (!s) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   rv = Read(s, aLength, &bytesRead);
>   if (NS_FAILED(rv)) {
>     moz_free(s);
>diff -r 3436787a82d0 xpcom/io/nsDirectoryService.cpp
>--- a/xpcom/io/nsDirectoryService.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsDirectoryService.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -91,9 +91,6 @@
>   }
> 
>   nsLocalFile* localFile = new nsLocalFile;
>-  if (!localFile) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>   NS_ADDREF(localFile);
> 
> #ifdef XP_WIN
>diff -r 3436787a82d0 xpcom/io/nsInputStreamTee.cpp
>--- a/xpcom/io/nsInputStreamTee.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsInputStreamTee.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -347,10 +347,6 @@
>   nsresult rv;
> 
>   nsCOMPtr<nsIInputStreamTee> tee = new nsInputStreamTee();
>-  if (!tee) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   rv = tee->SetSource(aSource);
>   if (NS_FAILED(rv)) {
>     return rv;
>diff -r 3436787a82d0 xpcom/io/nsLinebreakConverter.cpp
>--- a/xpcom/io/nsLinebreakConverter.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsLinebreakConverter.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -343,9 +343,6 @@
>       destBuffer = ConvertBreaks(*aIoBuffer, sourceLen, srcBreaks, dstBreaks);
>     }
> 
>-    if (!destBuffer) {
>-      return NS_ERROR_OUT_OF_MEMORY;
>-    }
>     *aIoBuffer = destBuffer;
>     if (aOutLen) {
>       *aOutLen = sourceLen;
>@@ -429,9 +426,6 @@
>       destBuffer = ConvertBreaks(*aIoBuffer, sourceLen, srcBreaks, dstBreaks);
>     }
> 
>-    if (!destBuffer) {
>-      return NS_ERROR_OUT_OF_MEMORY;
>-    }
>     *aIoBuffer = destBuffer;
>     if (aOutLen) {
>       *aOutLen = sourceLen;
>diff -r 3436787a82d0 xpcom/io/nsLocalFileUnix.cpp
>--- a/xpcom/io/nsLocalFileUnix.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsLocalFileUnix.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -828,10 +828,6 @@
> 
>     // actually create the file.
>     nsLocalFile* newFile = new nsLocalFile();
>-    if (!newFile) {
>-      return NS_ERROR_OUT_OF_MEMORY;
>-    }
>-
>     nsCOMPtr<nsIFile> fileRef(newFile); // release on exit
> 
>     rv = newFile->InitWithNativePath(newPathName);
>@@ -1771,10 +1767,6 @@
> 
>   int32_t size = (int32_t)symStat.st_size;
>   char* target = (char*)nsMemory::Alloc(size + 1);
>-  if (!target) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   if (readlink(mPath.get(), target, (size_t)size) < 0) {
>     nsMemory::Free(target);
>     return NSRESULT_FOR_ERRNO();
>@@ -1821,12 +1813,7 @@
> 
>     int32_t newSize = (int32_t)symStat.st_size;
>     if (newSize > size) {
>-      char* newTarget = (char*)nsMemory::Realloc(target, newSize + 1);
>-      if (!newTarget) {
>-        rv = NS_ERROR_OUT_OF_MEMORY;
>-        break;
>-      }
>-      target = newTarget;
>+      target = (char*)nsMemory::Realloc(target, newSize + 1);
>       size = newSize;
>     }
> 
>@@ -2327,9 +2314,6 @@
>   if (charsConverted == inStrLen) {
>     // all characters converted, do the actual conversion
>     aOutStr.SetLength(usedBufLen);
>-    if (aOutStr.Length() != (unsigned int)usedBufLen) {
>-      return NS_ERROR_OUT_OF_MEMORY;
>-    }
>     UInt8* buffer = (UInt8*)aOutStr.BeginWriting();
>     ::CFStringGetBytes(aInStrRef, CFRangeMake(0, inStrLen), kCFStringEncodingUTF8,
>                        0, false, buffer, usedBufLen, &usedBufLen);
>diff -r 3436787a82d0 xpcom/io/nsLocalFileWin.cpp
>--- a/xpcom/io/nsLocalFileWin.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsLocalFileWin.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -457,10 +457,6 @@
> NS_CreateShortcutResolver()
> {
>   gResolver = new ShortcutResolver();
>-  if (!gResolver) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   return gResolver->Init();
> }
> 
>@@ -765,10 +761,6 @@
>   }
> 
>   nsDir* d  = PR_NEW(nsDir);
>-  if (!d) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   nsAutoString filename(aName);
> 
>   // If |aName| ends in a slash or backslash, do not append another backslash.
>@@ -1018,10 +1010,6 @@
>   }
> 
>   nsLocalFile* inst = new nsLocalFile();
>-  if (!inst) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   nsresult rv = inst->QueryInterface(aIID, aInstancePtr);
>   if (NS_FAILED(rv)) {
>     delete inst;
>@@ -1195,10 +1183,6 @@
> {
>   // Just copy-construct ourselves
>   *aFile = new nsLocalFile(*this);
>-  if (!*aFile) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   NS_ADDREF(*aFile);
> 
>   return NS_OK;
>@@ -1733,10 +1717,6 @@
>   }
> 
>   void* ver = calloc(size, 1);
>-  if (!ver) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   if (::GetFileVersionInfoW(path, 0, size, ver)) {
>     LANGANDCODEPAGE* translate = nullptr;
>     UINT pageCount;
>@@ -2034,10 +2014,6 @@
>           newParentDir->GetTarget(target);
> 
>           nsCOMPtr<nsIFile> realDest = new nsLocalFile();
>-          if (!realDest) {
>-            return NS_ERROR_OUT_OF_MEMORY;
>-          }
>-
>           rv = realDest->InitWithPath(target);
> 
>           if (NS_FAILED(rv)) {
>@@ -3231,9 +3207,6 @@
>   *aEntries = nullptr;
>   if (mWorkingPath.EqualsLiteral("\\\\.")) {
>     nsDriveEnumerator* drives = new nsDriveEnumerator;
>-    if (!drives) {
>-      return NS_ERROR_OUT_OF_MEMORY;
>-    }
>     NS_ADDREF(drives);
>     rv = drives->Init();
>     if (NS_FAILED(rv)) {
>@@ -3381,9 +3354,6 @@
> NS_NewLocalFile(const nsAString& aPath, bool aFollowLinks, nsIFile** aResult)
> {
>   nsLocalFile* file = new nsLocalFile();
>-  if (!file) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>   NS_ADDREF(file);
> 
>   file->SetFollowLinks(aFollowLinks);
>diff -r 3436787a82d0 xpcom/io/nsMultiplexInputStream.cpp
>--- a/xpcom/io/nsMultiplexInputStream.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsMultiplexInputStream.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -152,7 +152,8 @@
> {
>   NS_ASSERTION(SeekableStreamAtBeginning(aStream),
>                "Appended stream not at beginning.");
>-  return mStreams.AppendElement(aStream) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
>+  mStreams.AppendElement(aStream);
>+  return NS_OK;
> }
> 
> /* void insertStream (in nsIInputStream stream, in unsigned long index); */
>@@ -685,9 +686,6 @@
>   }
> 
>   nsMultiplexInputStream* inst = new nsMultiplexInputStream();
>-  if (!inst) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
> 
>   NS_ADDREF(inst);
>   nsresult rv = inst->QueryInterface(aIID, aResult);
>diff -r 3436787a82d0 xpcom/io/nsPipe3.cpp
>--- a/xpcom/io/nsPipe3.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsPipe3.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -1331,10 +1331,6 @@
>   nsresult rv;
> 
>   nsPipe* pipe = new nsPipe();
>-  if (!pipe) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   rv = pipe->Init(aNonBlockingInput,
>                   aNonBlockingOutput,
>                   aSegmentSize,
>@@ -1357,9 +1353,6 @@
>     return NS_ERROR_NO_AGGREGATION;
>   }
>   nsPipe* pipe = new nsPipe();
>-  if (!pipe) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>   NS_ADDREF(pipe);
>   nsresult rv = pipe->QueryInterface(aIID, aResult);
>   NS_RELEASE(pipe);
>diff -r 3436787a82d0 xpcom/io/nsScriptableInputStream.cpp
>--- a/xpcom/io/nsScriptableInputStream.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsScriptableInputStream.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -59,9 +59,6 @@
>   uint32_t count =
>     XPCOM_MIN((uint32_t)XPCOM_MIN<uint64_t>(count64, aCount), UINT32_MAX - 1);
>   buffer = (char*)moz_malloc(count + 1);  // make room for '\0'
>-  if (!buffer) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
> 
>   rv = ReadHelper(buffer, count);
>   if (NS_FAILED(rv)) {
>@@ -82,9 +79,6 @@
>   }
> 
>   aResult.SetLength(aCount);
>-  if (aResult.Length() != aCount) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
> 
>   char* ptr = aResult.BeginWriting();
>   nsresult rv = ReadHelper(ptr, aCount);
>@@ -130,10 +124,6 @@
>   }
> 
>   nsScriptableInputStream* sis = new nsScriptableInputStream();
>-  if (!sis) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   NS_ADDREF(sis);
>   nsresult rv = sis->QueryInterface(aIID, aResult);
>   NS_RELEASE(sis);
>diff -r 3436787a82d0 xpcom/io/nsStorageStream.cpp
>--- a/xpcom/io/nsStorageStream.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsStorageStream.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -76,10 +76,6 @@
> nsStorageStream::Init(uint32_t aSegmentSize, uint32_t aMaxSize)
> {
>   mSegmentedBuffer = new nsSegmentedBuffer();
>-  if (!mSegmentedBuffer) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   mSegmentSize = aSegmentSize;
>   mSegmentSizeLog2 = mozilla::FloorLog2(aSegmentSize);
> 
>@@ -406,10 +402,6 @@
> 
>   nsStorageInputStream* inputStream =
>     new nsStorageInputStream(this, mSegmentSize);
>-  if (!inputStream) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   NS_ADDREF(inputStream);
> 
>   nsresult rv = inputStream->Seek(aStartingOffset);
>@@ -621,10 +613,6 @@
>                     nsIStorageStream** aResult)
> {
>   nsStorageStream* storageStream = new nsStorageStream();
>-  if (!storageStream) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   NS_ADDREF(storageStream);
>   nsresult rv = storageStream->Init(aSegmentSize, aMaxSize);
>   if (NS_FAILED(rv)) {
>diff -r 3436787a82d0 xpcom/io/nsStreamUtils.cpp
>--- a/xpcom/io/nsStreamUtils.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsStreamUtils.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -606,10 +606,6 @@
>     copier = new nsStreamCopierOB();
>   }
> 
>-  if (!copier) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   // Start() takes an owning ref to the copier...
>   NS_ADDREF(copier);
>   rv = copier->Start(aSource, aSink, aTarget, aCallback, aClosure, aChunkSize,
>@@ -665,9 +661,6 @@
>     }
> 
>     aResult.SetLength(length + avail);
>-    if (aResult.Length() != (length + avail)) {
>-      return NS_ERROR_OUT_OF_MEMORY;
>-    }
>     char* buf = aResult.BeginWriting() + length;
> 
>     uint32_t n;
>diff -r 3436787a82d0 xpcom/io/nsStringStream.cpp
>--- a/xpcom/io/nsStringStream.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsStringStream.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -345,10 +345,6 @@
>   NS_PRECONDITION(aStreamResult, "null out ptr");
> 
>   nsStringInputStream* stream = new nsStringInputStream();
>-  if (!stream) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   NS_ADDREF(stream);
> 
>   nsresult rv;
>@@ -391,10 +387,6 @@
>   NS_PRECONDITION(aStreamResult, "null out ptr");
> 
>   nsStringInputStream* stream = new nsStringInputStream();
>-  if (!stream) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   NS_ADDREF(stream);
> 
>   stream->SetData(aStringToRead);
>@@ -415,10 +407,6 @@
>   }
> 
>   nsStringInputStream* inst = new nsStringInputStream();
>-  if (!inst) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   NS_ADDREF(inst);
>   nsresult rv = inst->QueryInterface(aIID, aResult);
>   NS_RELEASE(inst);
>diff -r 3436787a82d0 xpcom/io/nsUnicharInputStream.cpp
>--- a/xpcom/io/nsUnicharInputStream.cpp	Sun Feb 08 17:40:44 2015 -0800
>+++ b/xpcom/io/nsUnicharInputStream.cpp	Tue Feb 10 11:36:04 2015 +0100
>@@ -413,9 +413,6 @@
>                                                        nsIUnicharInputStream** aResult)
> {
>   StringUnicharInputStream* it = new StringUnicharInputStream(aString);
>-  if (!it) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
> 
>   NS_ADDREF(*aResult = it);
>   return NS_OK;
>@@ -430,10 +427,6 @@
> 
>   // Create converter input stream
>   nsRefPtr<UTF8InputStream> it = new UTF8InputStream();
>-  if (!it) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>-
>   nsresult rv = it->Init(aStreamToWrap);
>   if (NS_FAILED(rv)) {
>     return rv;

Comment 26

[Andrew McCreight [:mccr8] (PTO until 12-2)]

NS_IF_ADDREF also does a null check, so you could replace these with NS_ADDREF when the argument is the result of a new.

When uploading a patch, you want it to be text/plain, and just check the box for "patch".

Comment 27

[Thomas Baquet]

Attachment: #1122740 xpcom/io/ fix removed 2 fallible cases;

Comment 28

[Andrew McCreight [:mccr8] (PTO until 12-2)]

To make it simpler to track things, I filed bug 1132078 for xpcom/io/ and assigned the bug to you, Thomas.  If you could reupload your patch there, I'd appreciate it.

Comment 29

[Thomas Baquet]

okay, starting from now, I do it there. Sorry for the previous disturbances -- I need a bit of time to handle Bugzilla

Comment 30

[Atif Iqbal Ahangar [:atifhans]]

Hi, I would like to work on this bug. I will attach the patch for the case of threads directory. Am a novice here, so might take some time to get familiar with this process.

Comment 31

[Atif Iqbal Ahangar [:atifhans]]

Attachment: Removes useless null checks from the files in xpcom/threads directory.

Removed the null checks for the case where "new" was used explicitly to allocate the memory.

Comment 32

[Nathan Froyd [:froydnj]]

Comment on attachment 8586941 [details] [diff] [review]
Removes useless null checks from the files in xpcom/threads directory.

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

Thanks for the patch!

This all looks good; skimming through the NS_ERROR_OUT_OF_MEMORY return codes in xpcom/threads/, it looks there's one more in nsEnvironment.cpp:

http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsEnvironment.cpp#30

Also, you might have noticed that I filed a new bug, bug 1150197, and assigned it to you.  Please upload future patches there (and don't forget to change the bug number in your commit message).

Comment 33

[Nicholas Parkanyi]

Attachment: Removes redundant memory allocation checks in xpcom/reflect

Patch for the files in xpcom/reflect.

Comment 34

[Nicholas Parkanyi]

Attachment: Removes redundant memory allocation checks in xpcom/ds

Patch for files in xpcom/ds.

Comment 35

[Nathan Froyd [:froydnj]]

Comment on attachment 8686430 [details] [diff] [review]
Removes redundant memory allocation checks in xpcom/reflect

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

Thanks for the patch!

Comment 36

[Nathan Froyd [:froydnj]]

Comment on attachment 8686431 [details] [diff] [review]
Removes redundant memory allocation checks in xpcom/ds

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

Thanks for the patch!  A few small things to fix up below.

::: xpcom/ds/nsSupportsArray.cpp
@@ +618,5 @@
>  
>    uint32_t count = 0;
>    Count(&count);
>    for (uint32_t i = 0; i < count; i++) {
> +    newArray->InsertElementAt(mArray[i], i);

Please make this MOZ_ALWAYS_TRUE(newArray->...), since the return value here is actually checking whether the index was within bounds, and that should always be the case here.

::: xpcom/ds/nsVariant.cpp
@@ -829,5 @@
>    }
>  
> -  if (!ptr) {
> -    return NS_ERROR_OUT_OF_MEMORY;
> -  }

Please put this check back, as this check is for PR_smprintf, which can still return nullptr.

::: xpcom/ds/nsWindowsRegKey.cpp
@@ -502,5 @@
>  
>    mWatchEvent = CreateEvent(nullptr, TRUE, FALSE, nullptr);
> -  if (!mWatchEvent) {
> -    return NS_ERROR_OUT_OF_MEMORY;
> -  }

Please put this check back, as CreateEvent can legitimately return null on failure:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms682396%28v=vs.85%29.aspx

Comment 37

[Nathan Froyd [:froydnj]]

Apologies for the review slowness!  I somehow missed these patches in my queue yesterday...

Comment 38

[Nicholas Parkanyi]

Attachment: Fixed patch for files in xpcom/ds.

Comment 39

[Nathan Froyd [:froydnj]]

Comment on attachment 8687502 [details] [diff] [review]
Fixed patch for files in xpcom/ds.

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

Thank you!

Comment 40

[kevin]

Hi is this still being worked on?

Comment 41

[Nicklas Boman [:smurfd]]

Attachment: Bug1122740_181201_2330.patch

Hey, so it seems alot of the not used NS_ERROR_OUT_OF_MEMORY checks has been removed.
Think this patch could remove the remaining in xpcom/

Comment 42

[Adrian Kaczmarek]

Hi,

I've found that CycleCollectedJSContext::InitializeNonPrimary uses JS_NewCooperativeContext:
  mJSContext = JS_NewCooperativeContext(aPrimaryContext->mJSContext);

The JS_NewCooperativeContext does just one thing:
  JS_PUBLIC_API JSContext*
  JS_NewCooperativeContext(JSContext* siblingContext)
  {
      MOZ_CRASH("Cooperative scheduling is unsupported");
  }

Then the result of this function is null-checked.

Doesn't MOZ_CRASH mean that nothing after this function will get executed and the rest of the JS_NewCooperativeConvext should be removed from the codebase?

Comment 43

[Adrian Kaczmarek]

Attachment: [mq]: 1122740

Removed one never changed bool variable and its usage in the ternary operators and if statements.

Comment 44

[Nathan Froyd [:froydnj]]

Comment on attachment 9029170 [details] [diff] [review]
Bug1122740_181201_2330.patch

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

::: xpcom/base/CycleCollectedJSContext.cpp
@@ -151,5 @@
>  
>    mozilla::dom::InitScriptSettings();
>    mJSContext = JS_NewContext(aMaxBytes, aMaxNurseryBytes, aParentRuntime);
> -  if (!mJSContext) {
> -    return NS_ERROR_OUT_OF_MEMORY;

I believe this is incorrect, as JS_NewContext (via js::NewContext) can return nullptr.

@@ -173,5 @@
>  
>    mozilla::dom::InitScriptSettings();
>    mJSContext = JS_NewCooperativeContext(aPrimaryContext->mJSContext);
> -  if (!mJSContext) {
> -    return NS_ERROR_OUT_OF_MEMORY;

This is technically correct, but I think we might as well keep the check there, in case JS_NewCooperativeContext ever is implemented correctly.  If we are going to remove this check, we should remove JS_NewCooperativeContext and friends, and that would be a much bigger, separate patch.

Comment 45

[Nathan Froyd [:froydnj]]

(In reply to Nicklas Boman [:smurfd] from comment #41)
> Hey, so it seems alot of the not used NS_ERROR_OUT_OF_MEMORY checks has been
> removed.
> Think this patch could remove the remaining in xpcom/

Besides comment 44, this patch seems reasonable.  Want to flag the patch for proper review once you've uploaded a new version?

Comment 46

[Nicklas Boman [:smurfd]]

Attachment: Bug1122740_181215_1705.patch

Addressed the issues mentioned in Comment #44

Comment 47

[Nathan Froyd [:froydnj]]

Comment on attachment 9031700 [details] [diff] [review]
Bug1122740_181215_1705.patch

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

The commit message looks truncated; did something go wrong when preparing the patch?  Also, it's customary to have a space between "Bug" and the bug number in the commit message.

Sorry I didn't catch the bit below previously; other than that, everything looks good!  Please upload a revised patch and flag me for review.

::: xpcom/io/nsPipe3.cpp
@@ -890,5 @@
>      // should never return nullptr here.
>      char* seg = mBuffer.AppendNewSegment();
> -    if (!seg) {
> -      return NS_ERROR_OUT_OF_MEMORY;
> -    }

This change doesn't look correct, as nsSegmentedBuffer::AppendNewSegment can return nullptr:

https://searchfox.org/mozilla-central/source/xpcom/io/nsSegmentedBuffer.cpp#25-27
https://searchfox.org/mozilla-central/source/xpcom/io/nsSegmentedBuffer.cpp#55-58

Please restore this check.

Comment 48

[Nicklas Boman [:smurfd]]

Attachment: Bug1122740_181218_2020.patch

Comment 49

[Nicklas Boman [:smurfd]]

> The commit message looks truncated; did something go wrong when preparing
> the patch?  Also, it's customary to have a space between "Bug" and the bug
> number in the commit message.
The reason was that i intended to have the original bug subject in the commit message, started to wonder wether the || would screw something up, so i removed that and had plan to add 'New', but missed that. :)
 
> Please restore this check.

Fixed.

Comment 50

[Nathan Froyd [:froydnj]]

Comment on attachment 9032220 [details] [diff] [review]
Bug1122740_181218_2020.patch

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

Thank you!  Nit: the commit message should read |new| (or `new`), since "New" isn't really a thing in standard C++. :)

::: xpcom/ds/nsVariant.cpp
@@ -37,5 @@
>  {
>    char* pChars = ToNewCString(aString);
> -  if (!pChars) {
> -    return NS_ERROR_OUT_OF_MEMORY;
> -  }

I was going to say that this removal looks incorrect, since ToNewCString can return nullptr:

https://searchfox.org/mozilla-central/source/xpcom/string/nsReadableUtils.cpp#39-41

But I see that on inspection, the allocation function it calls is AllocateStringCopy:

https://searchfox.org/mozilla-central/source/xpcom/string/nsReadableUtils.cpp#30-35

which calls moz_xmalloc, which can't actually fail.  Which is...bogus.  (Let's not talk about the weird ToCharT* argument in AllocateStringCopy.)

So, the upshot is that this code is fine, but would you please file a followup bug for fixing ToNewCString (and ToNewUnicode as well)?  You are welcome to fix that bug too if you like. :)

Comment 51

[Nicklas Boman [:smurfd]]

Attachment: Bug1122740_190127_2100.patch

fixed the nit in attachment 9032220 [details] [diff] [review] :)

Comment 52

[Eric Rahm [:erahm]]

Nathan, it looks like the changes here are complete. Can you help get it landed?

Comment 53

[hmrishavbandyopadhyay]

Hey i would like to work on this bug. Bit of a noobie here. How do i begin?

Comment 54

[Eric Rahm [:erahm]]

Shravan's going to take a look at rebasing this and getting it landed.

Comment 55

[Shravan Narayan]

Attachment: Bug 1122740 - remove useless null checks after allocating memory with |new|

Comment 56

[Shravan Narayan]

(In reply to Eric Rahm [:erahm] (Away until June 3rd) from comment #54)

Shravan's going to take a look at rebasing this and getting it landed.

Sorry still getting familiar with how mozilla's submission works... Does the above look OK?

Comment 57

[Andrew McCreight [:mccr8] (PTO until 12-2)]

(In reply to Shravan Narayan from comment #56)

Sorry still getting familiar with how mozilla's submission works... Does the above look OK?

The next step is to ask an XPCOM peer to review the patch (though I guess it has been reviewed a few times already), so you should mark Eric Rahm as a reviewer.

I looked at the patch, and I noticed a mistake (which was present in the previous versions). In AString2Double, ToNewCString is not infallible, so the null check for that return value should not be removed. Please fix that and update the patch. Thanks!

Comment 58

[Andrew McCreight [:mccr8] (PTO until 12-2)]

(In reply to Andrew McCreight [:mccr8] from comment #57)

I looked at the patch, and I noticed a mistake (which was present in the previous versions). In AString2Double, ToNewCString is not infallible, so the null check for that return value should not be removed. Please fix that and update the patch. Thanks!

Never mind, I was wrong about that. I didn't realize that moz_xmalloc is infallible.

Comment 59

[Eric Rahm [:erahm]]

(In reply to Andrew McCreight [:mccr8] from comment #58)

(In reply to Andrew McCreight [:mccr8] from comment #57)

I looked at the patch, and I noticed a mistake (which was present in the previous versions). In AString2Double, ToNewCString is not infallible, so the null check for that return value should not be removed. Please fix that and update the patch. Thanks!

Never mind, I was wrong about that. I didn't realize that moz_xmalloc is infallible.

To avoid confusion, we're fixing that in bug 1515419 where we'll convert calls that do a null check to an explicitly fallible version. We should leave it as-is until then.

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39063905a5b8
remove useless null checks after allocating memory with |new| r=erahm

Comment 61

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/39063905a5b8