Closed Bug 1138293 Opened 5 years ago Closed 5 years ago

Replace moz_malloc/moz_free with malloc/free

Categories

(Core :: Memory Allocator, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files)

The distinction between moz_malloc/moz_free and malloc/free is not interesting. We are inconsistent in our use of one or the other, and I wouldn't be surprised if we are mixing them anyways.

One difficulty is that things withing mozglue itself can't really use malloc/free verbatim because of how it's linked. See bug 868814 comment 10.
Didn't we make malloc() infallible?
Infallible is moz_xmalloc.
The problem from bug 868814 comment 10 still exists, but is more or less controlled with the changes in mozalloc.h. It won't prevent someone using malloc somehow in mozglue.dll, but I don't want to block this on waiting for a complete solution. And those should be easy to spot, as mozglue is a rather small surface, especially on Windows.
Attachment #8583708 - Flags: review?(n.nethercote)
Comment on attachment 8583708 [details] [diff] [review]
Remove moz_malloc/moz_free/moz_realloc/moz_calloc

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

If you haven't done so already, please grep for any remaining mentions of the removed moz_ functions.

::: memory/mozalloc/mozalloc.h
@@ +67,5 @@
> + * Each declaration below is analogous to a "standard" allocation
> + * function, except that the out-of-memory handling is made explicit.
> + * The |moz_x| versions will never return a NULL pointer; if memory
> + * is exhausted, they abort.  The |moz_| versions may return NULL
> + * pointers if memory is exhausted: their return value must be checked.

The second half of this comment should be modified or removed, because the moz_ versions no longer exist. Maybe change it to this:

"Each declaration below is analogous to a "standard" allocation function, except that it will never return a NULL pointer; if memory is exhausted, it will abort."
Attachment #8583708 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #5)
> Comment on attachment 8583708 [details] [diff] [review]
> Remove moz_malloc/moz_free/moz_realloc/moz_calloc
> 
> Review of attachment 8583708 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If you haven't done so already, please grep for any remaining mentions of
> the removed moz_ functions.

Did this every time I rebased this patch, and surprisingly, nothing piled up.

> ::: memory/mozalloc/mozalloc.h
> @@ +67,5 @@
> > + * Each declaration below is analogous to a "standard" allocation
> > + * function, except that the out-of-memory handling is made explicit.
> > + * The |moz_x| versions will never return a NULL pointer; if memory
> > + * is exhausted, they abort.  The |moz_| versions may return NULL
> > + * pointers if memory is exhausted: their return value must be checked.
> 
> The second half of this comment should be modified or removed, because the
> moz_ versions no longer exist. Maybe change it to this:

Actually, I left it on purpose, because there /still/ are a few moz_ functions.
Comment on attachment 8583707 [details] [diff] [review]
Use malloc/free/realloc/calloc instead of moz_malloc/moz_free/moz_realloc/moz_calloc

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

Please do a full try server run before landing these patches :)

::: dom/base/nsTextFragment.cpp
@@ +348,5 @@
>    if (first16bit != -1) { // aBuffer contains no non-8bit character
>      // The old data was 1-byte, but the new is not so we have to expand it
>      // all to 2-byte
> +    char16_t* buff = (char16_t*)malloc((mState.mLength + aLength) *
> +                                       sizeof(char16_t));

I'd put the linebreak after the '=' and get the rest on a single line.

@@ +383,1 @@
>                                      (mState.mLength + aLength) * sizeof(char));

Adjust indentation of second line.

::: dom/media/webrtc/AudioOutputObserver.h
@@ +51,5 @@
>    nsAutoPtr<webrtc::SingleRwFifo> mPlayoutFifo;
>    uint32_t mChunkSize;
>  
>    // chunking to 10ms support
> +  FarEndAudioChunk *mSaved; // can't be nsAutoPtr since we need to use free()

Pre-existing, but can you add ", not delete" to the comment? Because I had to look at the definition of nsAutoPtr to understand this comment.

::: gfx/graphite2/src/MozGrMalloc.h
@@ +5,5 @@
>  
>  #ifndef MOZ_GR_MALLOC_H
>  #define MOZ_GR_MALLOC_H
>  
> +// Override malloc() and friends to call moz_xmalloc() etc, so that we get

Good catch!

::: gfx/skia/trunk/src/ports/SkMemory_mozalloc.cpp
@@ +39,1 @@
>  }

It's weird that there aren't equivalent versions of this function for calloc and realloc. Hmm.

::: intl/uconv/nsScriptableUConv.cpp
@@ +150,5 @@
>    rv = mDecoder->GetMaxLength(reinterpret_cast<const char*>(aData),
>                                inLength, &outLength);
>    if (NS_SUCCEEDED(rv))
>    {
> +    char16_t* buf = (char16_t*)malloc((outLength+1)*sizeof(char16_t));

Might as well add whitespace around the '*' while you're here.
Attachment #8583707 - Flags: review?(n.nethercote) → review+
Depends on: 1142434
I know at least those need an update:
https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation
https://developer.mozilla.org/en-US/docs/Choosing_the_right_memory_allocator

But maybe they should be consolidated instead. Also, there is going to be more changes (like bug 1134920 and bug 1134923) that will need doc updates as well. I expect those to be landed during this cycle (40). Maybe it's worth waiting for documentation updates. Please contact me to discuss this.
Keywords: dev-doc-needed
Depends on: 1149416
https://hg.mozilla.org/mozilla-central/rev/b5d54a547bdc
https://hg.mozilla.org/mozilla-central/rev/f2c533ee4071
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Blocks: 1150898
A reviewer cited MDN as a reason for me to use moz_malloc -- which I couldn't do.

Can you please update these docs:
https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation
https://developer.mozilla.org/en-US/docs/Choosing_the_right_memory_allocator
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIMemory
and possibly others.

In addition to changing the function names, I think we need to re-visit the prose as well. For example, is |malloc| still expected to become "eventually fallible"?
Flags: needinfo?(mh+mozilla)
rather, "eventually infallible"
(In reply to David Major [:dmajor] from comment #11)
> Can you please update these docs:
> https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation
> https://developer.mozilla.org/en-US/docs/Choosing_the_right_memory_allocator
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIMemory
> and possibly others.

See comment 9.
Flags: needinfo?(mh+mozilla) → needinfo?(eshepherd)
MDN looks fine now
Flags: needinfo?(eshepherd)
You need to log in before you can comment on or make changes to this bug.