790504 - Signaling code should directly use malloc/free

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Ethan Hugg [:ehugg]]

The signaling code currently has a bunch of memory handling code which should all be replaced by a FF standard like moz_xmalloc.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - Signaling code changed to use moz_xmalloc

Comment 2

[Ethan Hugg [:ehugg]]

Comment on attachment 660304 [details] [diff] [review]
Patch 1 - Signaling code changed to use moz_xmalloc


Everything in signaling uses cpr_malloc which I def'd to moz_xmalloc, same with the calloc, realloc and free.

I will make a second patch with the removal of all of the unused memory management code.

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 660304 [details] [diff] [review]
Patch 1 - Signaling code changed to use moz_xmalloc

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

This all looks basically fine.

The changes to VcmSIPCSBinding.cpp look to be cosmetic only (spaces, etc.)
Maybe unnecessary?

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 660304 [details] [diff] [review]
Patch 1 - Signaling code changed to use moz_xmalloc

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

This all looks basically fine.

The changes to VcmSIPCSBinding.cpp look to be cosmetic only (spaces, etc.)
Maybe unnecessary?

Comment 5

[Ethan Hugg [:ehugg]]

>The changes to VcmSIPCSBinding.cpp look to be cosmetic only (spaces, etc.)
>Maybe unnecessary?

The VcmSIPCCBinding.cpp changes are changing malloc() to cpr_malloc().  I considered changing them directly to moz_xmalloc(), but I'm trying to keep it consistent with the rest of the signaling code so that the allocators could all be changed in one place if necessary.

Comment 6

[Eric Rescorla (:ekr)]

Wow, my brain just seems to see cpr_malloc == malloc now.

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - Signaling code changed to use moz_xmalloc f=ekr

Comment 8

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - Signaling code changed to use moz_xmalloc f=ekr

Comment 9

[Ethan Hugg [:ehugg]]

moz_xmalloc is the wrong thing to use.  Changing the patch to use malloc/calloc/realloc/free.  Next patch will remove all memory management code from signaling.

Comment 10

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - Signaling code changed to directly use malloc/free

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - Signaling code changed to directly use malloc/free

Comment 12

[Ethan Hugg [:ehugg]]

Comment on attachment 660617 [details] [diff] [review]
Patch 1 - Signaling code changed to directly use malloc/free

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

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +1478,5 @@
>          result = sdp_attr_get_ice_attribute (sdp_p, level, 0, sdp_attr, (uint16_t) (i + 1),
>            &ice_attrib);
>          if (result != SDP_SUCCESS) {
>      		GSM_ERR_MSG("Failed to retrieve ICE attribute\n");
> +    		cpr_free(*ice_attribs);

Switching to free() and having -Werror made the Try builders pop out this little gem.  Trying to free the function param.

Comment 13

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - Signaling code changed to directly use malloc/free

Comment 14

[Ethan Hugg [:ehugg]]

Attachment: Patch 2 - remove unused memory management code from signaling.

Comment 15

[Ethan Hugg [:ehugg]]

Attachment: Patch 2 - remove unused memory management code from signaling.

Comment 16

[Ethan Hugg [:ehugg]]

Attachment: Patch 2 - remove unused memory management code from signaling.

Comment 17

[Ethan Hugg [:ehugg]]

Comment on attachment 661081 [details] [diff] [review]
Patch 2 - remove unused memory management code from signaling.


Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/8b21741e7037

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Oh good, this gets rid of cpr_crashdump(), which I was about to flag.

Comment 19

[Ethan Hugg [:ehugg]]

Re-opening this one.  We need to go back to trying moz_xmalloc which really is infallible.

Comment 20

[Ethan Hugg [:ehugg]]

Attachment: Change to use moz_xmalloc

Comment 21

[Ethan Hugg [:ehugg]]

Comment on attachment 667112 [details] [diff] [review]
Change to use moz_xmalloc


moz_xmalloc patch pushed to Alder - https://hg.mozilla.org/projects/alder/rev/0a74f8743068