remove useless null checks after allocating memory with |new|

NEW
Unassigned

Status

()

Core
XPCOM
4 years ago
2 years ago

People

(Reporter: froydnj, Unassigned, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
Hi I'd like to work on this bug. How do i begin?
(Reporter)

Comment 2

4 years ago
(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.
I have already built firefox. In which file do i look for the code mentioned in comment 0 ?
(Reporter)

Comment 4

4 years ago
(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.
Ok so i just remove the if(!obj) blocks ?
(Reporter)

Comment 6

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

Yes!
Ok thanks! I'm trying to refine the search since it returned a lot of lines containing the keyword.
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
(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

4 years ago
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.
(Reporter)

Comment 11

4 years ago
(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

4 years ago
(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.
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

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

Comment 15

4 years ago
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. :-]
(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

4 years ago
: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

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

Comment 19

3 years ago
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

3 years ago
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

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

Comment 22

3 years ago
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

3 years ago
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

3 years ago
Created attachment 8562013 [details] [diff] [review]
#1122740 xpcom/io/

Comment 25

3 years ago
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;
Attachment #8562013 - Attachment is patch: true
Attachment #8562013 - Attachment mime type: text/x-patch → text/plain
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

3 years ago
Created attachment 8562805 [details] [diff] [review]
#1122740 xpcom/io/ fix removed 2 fallible cases;
Attachment #8562013 - Attachment is obsolete: true
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

3 years ago
okay, starting from now, I do it there. Sorry for the previous disturbances -- I need a bit of time to handle Bugzilla
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.
Created attachment 8586941 [details] [diff] [review]
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.
Attachment #8586941 - Flags: review?(nfroyd)
(Reporter)

Updated

3 years ago
Depends on: 1150197
(Reporter)

Comment 32

3 years ago
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).
Attachment #8586941 - Flags: review?(nfroyd) → feedback+

Comment 33

3 years ago
Created attachment 8686430 [details] [diff] [review]
Removes redundant memory allocation checks in xpcom/reflect

Patch for the files in xpcom/reflect.
Attachment #8686430 - Flags: review?(nfroyd)

Comment 34

3 years ago
Created attachment 8686431 [details] [diff] [review]
Removes redundant memory allocation checks in xpcom/ds

Patch for files in xpcom/ds.
Attachment #8686431 - Flags: review?(nfroyd)
(Reporter)

Comment 35

3 years ago
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!
Attachment #8686430 - Flags: review?(nfroyd) → review+
(Reporter)

Comment 36

3 years ago
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
Attachment #8686431 - Flags: review?(nfroyd) → feedback+
(Reporter)

Comment 37

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

Comment 38

3 years ago
Created attachment 8687502 [details] [diff] [review]
Fixed patch for files in xpcom/ds.
Attachment #8687502 - Flags: review?(nfroyd)
(Reporter)

Comment 39

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

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

Thank you!
Attachment #8687502 - Flags: review?(nfroyd) → review+

Comment 40

2 years ago
Hi is this still being worked on?
You need to log in before you can comment on or make changes to this bug.