794240 - Signaling doesn't clean up message queues in /tmp (seen on linux+mac)

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

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

We're leaking SIPCC message queues:

$ ls -l /tmp | grep SIP-MsgQ | wc
     78     702    6144

Note that it goes up even if I exit "cleanly" via close.

Comment 1

[Ethan Hugg [:ehugg]]

Adding to this that EKR has requested the tmp files be attributed 700 instead of 755 per m-c landing review.

Comment 2

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

Jan-Ivar - this may be a good next bug; it's in the signaling code

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Prelim patch fixing the SIP* messages only

I've fixed the SIP-* sockets. It's now closing and deleting those cleanly.

Moving on to:
2. the CprTmr* sockets. Same problem, but cpr_timer_de_init() is never called.
3. Fix permissions in /tmp on all four sockets. I plan to chmod 700 on a mkstemp() dir and put sockets inside so they'll inherit the right permissions.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I have a shutdown question: cpr_timer_de_init() @ http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c#935 isn't called anywhere. Do we think it was meant to be called on the timer thread itself or on the main thread after the timer thread is gone? Should I pass an unload msg, or is that overkill for a timer thread?

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

ehugg and crypt think no-one uses the cpr timers, so I'll test commenting them out.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: bug--fix: Closes sockets + cleans up tmp-files on shutdown + file permissions

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions

Comment 10

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

Comment on attachment 692372 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions

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

r+ with nits fixed

::: media/webrtc/signaling/signaling.gyp
@@ +583,5 @@
>        
>        'defines' : [
> +      # CPR timers are needed by SIP, but are disabled for now
> +      # to avoid the extra timer thread and stale cleanup code
> +      #    'CPR_TIMERS_ENABLED', 

trailing whitespace

::: media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_socket.c
@@ +773,5 @@
>  {
>      /* Bind to the local socket */
>      memset(addr, 0, sizeof(cpr_sockaddr_un_t));
>      addr->sun_family = AF_UNIX;
> +    PR_snprintf(addr->sun_path, sizeof(addr->sun_path), name, pid);

snprintf - this is mac-only code where it exists and is safe

::: media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c
@@ +226,5 @@
>   */
>  static cprRC_t addTimerToList (cpr_timer_t *cprTimerPtr, uint32_t duration, void *data)
>  {
> +    static const char fname[] = "addTimerToList";
> +

remove

@@ +270,5 @@
>          API_RETURN(tmr_rsp.u.result);
>      }
> +#else
> +    cprAssert(FALSE, CPR_FAILURE);
> +    CPR_ERROR("CPR Timers are disabled! %s\n", fname);

__FUNCTION__

::: media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_socket.c
@@ +748,5 @@
>  {
>      /* Bind to the local socket */
>      memset(addr, 0, sizeof(cpr_sockaddr_un_t));
>      addr->sun_family = AF_LOCAL;
> +    PR_snprintf(addr->sun_path, sizeof(addr->sun_path), name, pid);

snprintf - exists and is safe in linux

::: media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_socket.c
@@ +741,5 @@
>      char hostAndPort[HOST_AND_PORT_SIZE];
>  
>      cprAssert(host != NULL, CPR_FAILURE);
>      cprAssert(localPort != NULL, CPR_FAILURE);
> +    PR_snprintf(hostAndPort, HOST_AND_PORT_SIZE, "%s:%d", host, port);

snprintf - defined somewhere in signaling as safe

@@ +1083,5 @@
>    /* COPIED FROM OTHER PLATFORM AS A PLACE HOLDER */
>    /* Bind to the local socket */
>    memset(addr, 0, sizeof(cpr_sockaddr_un_t));
>    addr->sun_family = AF_INET;
> +  PR_snprintf(addr->sun_path, sizeof(addr->sun_path), name, pid);

ditto

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions.

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions.

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 692437 [details] [diff] [review]
Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions.

Carrying forward r+ from Randell.

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c073841902c

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: bustage

I'm getting link bustage.  I bisected it to this patch.  The exact output is attached.  The problem seems to be with PR_snprintf and/or mkdir.

I'm on Ubuntu 12.04, using clang 3.2 and gold 1.11.

Comment 17

[Nicholas Nethercote [inactive]]

The problem doesn't happen unless --enable-tests is specified, so it's a problem with the unit tests.

Comment 18

[Nicholas Nethercote [inactive]]

Actually, it might be --enable-debug that's causing the problem... not sure yet.

Comment 19

[Mike Hommey [:glandium]]

Missing includes in media/webrtc/signaling/src/sipcc/core/sipstack/sip_platform_task.c are the reason for Nicholas' build failure.
  #include <sys/stat.h>
  #include "prprf.h"

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Thanks Nick and Mike! Sorry, I'm a newbie. I'll add another patch to this bug and get it reviewed and checked in asap. Odd, inbound is still showing as green and it doesn't appear to have been backed out. Did this break the tree?

Mike, I see you added a dependency, do I need to hold off making a fix?

Comment 21

[Mike Hommey [:glandium]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #20)
> Thanks Nick and Mike! Sorry, I'm a newbie. I'll add another patch to this
> bug and get it reviewed and checked in asap. Odd, inbound is still showing
> as green and it doesn't appear to have been backed out. Did this break the
> tree?

It breaks the tree for some people, not on build bots.
 
> Mike, I see you added a dependency, do I need to hold off making a fix?

No, the dependency is just not in the right direction.

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Followup: Fixes implicit function warning that broke linkage + symmetric fixes

This adds the missing includes that broke linkage + also, dmose pointed out that not all changes were applied symmetrically (to android, darwin, linux and win32), now fixed.

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Note that the last patch is a follow-up, to be applied on-top of the first patch that has already landed on inbound.

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5c073841902c

Comment 25

[Ethan Hugg [:ehugg]]

Comment on attachment 693939 [details] [diff] [review]
Followup: Fixes implicit function warning that broke linkage + symmetric fixes


http://hg.mozilla.org/integration/mozilla-inbound/rev/bafbc8d9c6b8

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bafbc8d9c6b8