Last Comment Bug 794240 - Signaling doesn't clean up message queues in /tmp (seen on linux+mac)
: Signaling doesn't clean up message queues in /tmp (seen on linux+mac)
Status: RESOLVED FIXED
[WebRTC], [blocking-webrtc-] [qa-]
:
Product: Core
Classification: Components
Component: WebRTC: Signaling (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla20
Assigned To: Jan-Ivar Bruaroey [:jib]
: Jason Smith [:jsmith]
:
Mentors:
: 740254 (view as bug list)
Depends on:
Blocks: 822978 821812
  Show dependency treegraph
 
Reported: 2012-09-25 14:19 PDT by Randell Jesup [:jesup]
Modified: 2013-02-01 11:36 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Prelim patch fixing the SIP* messages only (3.65 KB, patch)
2012-12-07 14:00 PST, Jan-Ivar Bruaroey [:jib]
no flags Details | Diff | Splinter Review
bug--fix: Closes sockets + cleans up tmp-files on shutdown + file permissions (14.37 KB, patch)
2012-12-12 09:26 PST, Jan-Ivar Bruaroey [:jib]
no flags Details | Diff | Splinter Review
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions (14.28 KB, patch)
2012-12-12 09:42 PST, Jan-Ivar Bruaroey [:jib]
no flags Details | Diff | Splinter Review
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions (14.71 KB, patch)
2012-12-13 20:30 PST, Jan-Ivar Bruaroey [:jib]
no flags Details | Diff | Splinter Review
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions (17.09 KB, patch)
2012-12-14 10:49 PST, Jan-Ivar Bruaroey [:jib]
rjesup: review+
Details | Diff | Splinter Review
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions. (16.82 KB, patch)
2012-12-14 12:36 PST, Jan-Ivar Bruaroey [:jib]
no flags Details | Diff | Splinter Review
Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions. (16.81 KB, patch)
2012-12-14 13:17 PST, Jan-Ivar Bruaroey [:jib]
jib: review+
rjesup: checkin+
Details | Diff | Splinter Review
bustage (10.66 KB, text/plain)
2012-12-18 21:02 PST, Nicholas Nethercote [:njn]
no flags Details
Followup: Fixes implicit function warning that broke linkage + symmetric fixes (10.44 KB, patch)
2012-12-19 10:17 PST, Jan-Ivar Bruaroey [:jib]
rjesup: review+
ethanhugg: checkin+
Details | Diff | Splinter Review

Description Randell Jesup [:jesup] 2012-09-25 14:19:26 PDT
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] 2012-10-03 11:06:24 PDT
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] 2012-11-29 12:10:49 PST
Jan-Ivar - this may be a good next bug; it's in the signaling code
Comment 3 Jan-Ivar Bruaroey [:jib] 2012-12-07 14:00:40 PST
Created attachment 689908 [details] [diff] [review]
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] 2012-12-07 14:58:27 PST
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] 2012-12-07 15:15:24 PST
ehugg and crypt think no-one uses the cpr timers, so I'll test commenting them out.
Comment 6 Jan-Ivar Bruaroey [:jib] 2012-12-12 09:26:30 PST
Created attachment 691395 [details] [diff] [review]
bug--fix: Closes sockets + cleans up tmp-files on shutdown + file permissions
Comment 7 Jan-Ivar Bruaroey [:jib] 2012-12-12 09:42:47 PST
Created attachment 691401 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions
Comment 8 Jan-Ivar Bruaroey [:jib] 2012-12-13 20:30:52 PST
Created attachment 692166 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions
Comment 9 Jan-Ivar Bruaroey [:jib] 2012-12-14 10:49:19 PST
Created attachment 692372 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions
Comment 10 Randell Jesup [:jesup] 2012-12-14 12:06:08 PST
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] 2012-12-14 12:36:32 PST
Created attachment 692423 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions.
Comment 13 Jan-Ivar Bruaroey [:jib] 2012-12-14 13:17:23 PST
Created attachment 692437 [details] [diff] [review]
Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions.
Comment 14 Jan-Ivar Bruaroey [:jib] 2012-12-14 13:20:40 PST
Comment on attachment 692437 [details] [diff] [review]
Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions.

Carrying forward r+ from Randell.
Comment 16 Nicholas Nethercote [:njn] 2012-12-18 21:02:34 PST
Created attachment 693747 [details]
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 [:njn] 2012-12-18 21:55:30 PST
The problem doesn't happen unless --enable-tests is specified, so it's a problem with the unit tests.
Comment 18 Nicholas Nethercote [:njn] 2012-12-18 22:03:27 PST
Actually, it might be --enable-debug that's causing the problem... not sure yet.
Comment 19 Mike Hommey [:glandium] 2012-12-19 00:21:49 PST
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] 2012-12-19 06:54:11 PST
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] 2012-12-19 07:06:35 PST
(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] 2012-12-19 10:17:57 PST
Created attachment 693939 [details] [diff] [review]
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] 2012-12-19 10:20:12 PST
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] 2012-12-19 11:40:24 PST
https://hg.mozilla.org/mozilla-central/rev/5c073841902c
Comment 25 Ethan Hugg [:ehugg] 2012-12-19 14:25:31 PST
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] 2012-12-20 13:47:48 PST
https://hg.mozilla.org/mozilla-central/rev/bafbc8d9c6b8
Comment 27 Ethan Hugg [:ehugg] 2013-02-01 11:36:25 PST
*** Bug 740254 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.