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

RESOLVED FIXED in mozilla20

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jesup, Assigned: jib)

Tracking

(Blocks: 1 bug)

Trunk
mozilla20
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC], [blocking-webrtc-] [qa-])

Attachments

(3 attachments, 6 obsolete attachments)

16.81 KB, patch
jib
: review+
jesup
: checkin+
Details | Diff | Splinter Review
10.66 KB, text/plain
Details
10.44 KB, patch
jesup
: review+
ehugg
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Whiteboard: [WebRTC], [blocking-webrtc-]

Comment 1

5 years ago
Adding to this that EKR has requested the tmp files be attributed 700 instead of 755 per m-c landing review.
(Reporter)

Comment 2

5 years ago
Jan-Ivar - this may be a good next bug; it's in the signaling code
Assignee: nobody → jib
Status: NEW → ASSIGNED
Summary: Signaling doesn't clean up message queues in /tmp on linux → Signaling doesn't clean up message queues in /tmp on linux+mac
Summary: Signaling doesn't clean up message queues in /tmp on linux+mac → Signaling doesn't clean up message queues in /tmp (seen on linux+mac)
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.
Attachment #689908 - Attachment is patch: true
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?
ehugg and crypt think no-one uses the cpr timers, so I'll test commenting them out.
Created attachment 691395 [details] [diff] [review]
bug--fix: Closes sockets + cleans up tmp-files on shutdown + file permissions
Created attachment 691401 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions
Attachment #691395 - Attachment is obsolete: true
Created attachment 692166 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions
Attachment #691401 - Attachment is obsolete: true
Created attachment 692372 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions
Attachment #692166 - Attachment is obsolete: true
Attachment #692372 - Flags: review?(rjesup)
(Reporter)

Comment 10

5 years ago
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
Attachment #692372 - Flags: review?(rjesup) → review+

Updated

5 years ago
Blocks: 821812
Created attachment 692423 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions.
Attachment #692372 - Attachment is obsolete: true
Created attachment 692437 [details] [diff] [review]
Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions.
Attachment #692423 - Attachment is obsolete: true
Comment on attachment 692437 [details] [diff] [review]
Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions.

Carrying forward r+ from Randell.
Attachment #692437 - Flags: review+
Attachment #692437 - Flags: checkin?(rjesup)
(Reporter)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c073841902c
Target Milestone: --- → mozilla20
(Reporter)

Updated

5 years ago
Attachment #692437 - Flags: checkin?(rjesup) → checkin+
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.
The problem doesn't happen unless --enable-tests is specified, so it's a problem with the unit tests.
Actually, it might be --enable-debug that's causing the problem... not sure yet.
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"
Depends on: 822978
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?
(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.
Blocks: 822978
No longer depends on: 822978
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.
Attachment #693939 - Flags: review?(rjesup)
Note that the last patch is a follow-up, to be applied on-top of the first patch that has already landed on inbound.
Attachment #689908 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #693939 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/5c073841902c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #693939 - Flags: checkin?(ethanhugg)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 25

5 years ago
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
Attachment #693939 - Flags: checkin?(ethanhugg) → checkin+
https://hg.mozilla.org/mozilla-central/rev/bafbc8d9c6b8
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [WebRTC], [blocking-webrtc-] → [WebRTC], [blocking-webrtc-] [qa-]

Updated

5 years ago
Duplicate of this bug: 740254
You need to log in before you can comment on or make changes to this bug.