Closed Bug 1122740 Opened 10 years ago Closed 5 years ago

remove useless null checks after allocating memory with |new|

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: froydnj, Assigned: shravanrn, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(8 files, 4 obsolete files)

#1122740 xpcom/io/ fix removed 2 fallible cases;
(deleted), patch
Details | Diff | Splinter Review
Removes useless null checks from the files in xpcom/threads directory.
(deleted), patch
froydnj
: feedback+
Details | Diff | Splinter Review
Removes redundant memory allocation checks in xpcom/reflect
(deleted), patch
froydnj
: review+
Details | Diff | Splinter Review
Removes redundant memory allocation checks in xpcom/ds
(deleted), patch
froydnj
: feedback+
Details | Diff | Splinter Review
Fixed patch for files in xpcom/ds.
(deleted), patch
froydnj
: review+
Details | Diff | Splinter Review
[mq]: 1122740
(deleted), text/x-phabricator-request
Details
Bug1122740_190127_2100.patch
(deleted), patch
Details | Diff | Splinter Review
Bug 1122740 - remove useless null checks after allocating memory with |new|
(deleted), text/x-phabricator-request
Details
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?
(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 ?
(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 ?
(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!
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.
(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.
(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.
Thanks Andrew for shedding some light on this. I was curious regarding how these corner cases are being handled.
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
: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.
Can i work on this bug?? I just built the firefox os and would need a little help.
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?
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/.
Hi! I start to work on xpcom/io if it is okay for everyone
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?
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
Attached patch #1122740 xpcom/io/ (obsolete) (deleted) — — Splinter Review
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".
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.
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.
Removed the null checks for the case where "new" was used explicitly to allocate the memory.
Attachment #8586941 - Flags: review?(nfroyd)
Depends on: 1150197
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+
Patch for the files in xpcom/reflect.
Attachment #8686430 - Flags: review?(nfroyd)
Patch for files in xpcom/ds.
Attachment #8686431 - Flags: review?(nfroyd)
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+
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+
Apologies for the review slowness! I somehow missed these patches in my queue yesterday...
Attachment #8687502 - Flags: review?(nfroyd)
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+
Hi is this still being worked on?
Attached patch Bug1122740_181201_2330.patch (obsolete) (deleted) — — Splinter Review
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/
Flags: needinfo?(nfroyd)
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?
Attached file [mq]: 1122740 (deleted) —
Removed one never changed bool variable and its usage in the ternary operators and if statements.
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.
(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?
Flags: needinfo?(nfroyd)
Attached patch Bug1122740_181215_1705.patch (obsolete) (deleted) — — Splinter Review
Addressed the issues mentioned in Comment #44
Attachment #9029170 - Attachment is obsolete: true
Attachment #9031700 - Flags: review?(nfroyd)
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.
Attachment #9031700 - Flags: review?(nfroyd) → feedback+
Attached patch Bug1122740_181218_2020.patch (obsolete) (deleted) — — Splinter Review
Attachment #9031700 - Attachment is obsolete: true
Attachment #9032220 - Flags: review?(nfroyd)
> 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 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. :)
Attachment #9032220 - Flags: review?(nfroyd) → review+
Attached patch Bug1122740_190127_2100.patch (deleted) — — Splinter Review

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

Attachment #9032220 - Attachment is obsolete: true

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

Flags: needinfo?(nfroyd)

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

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

Assignee: nobody → shravanrn
Flags: needinfo?(nfroyd)

(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?

(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!

(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.

(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.

Pushed by erahm@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/39063905a5b8 remove useless null checks after allocating memory with |new| r=erahm

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

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox69: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: