Mike Hommey [:glandium] Assignee
	Description • 10 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 1 • 10 years ago

Didn't we make malloc() infallible?

	Mike Hommey [:glandium] Assignee
	Comment 2 • 10 years ago

Infallible is moz_xmalloc.

	Mike Hommey [:glandium] Assignee
	Comment 4 • 10 years ago

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.

	Nicholas Nethercote [inactive]
	Comment 5 • 10 years ago

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

	Mike Hommey [:glandium] Assignee
	Comment 6 • 10 years ago

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

	Nicholas Nethercote [inactive]
	Comment 7 • 10 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 8 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d54a547bdc
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c533ee4071

	Mike Hommey [:glandium] Assignee
	Comment 9 • 10 years ago

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.

	Carsten Book [:Tomcat]
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/b5d54a547bdc
https://hg.mozilla.org/mozilla-central/rev/f2c533ee4071

	(Away)
	Comment 11 • 10 years ago

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

	(Away)
	Comment 12 • 10 years ago

rather, "eventually infallible"

	Mike Hommey [:glandium] Assignee
	Comment 13 • 10 years ago

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

	(Away)
	Comment 14 • 8 years ago

MDN looks fine now